Subject: [IPsec] My review of draft-nir-ipsecme-curve25519

From: Tero Kivinen <[email-masked]>
Date: Tue, 25 Aug 2015 15:19:15 +0300

Firstly the name of the draft is bit misleading, as this defines both
curve25519 and Curve448 keys not only curve25519.

This document is bit confusing as some of the values are defined as
"strings of 32 octets", some are defined as 255/448-bit numbers having
certain properties (high bit set, n lowest bits unset), but then later
it mixes these.

For example the public key is defined of "string of 32 octets", but
then section 3.1 says that it is "32 octets encoded as an array of
bytes in little-endian order...". As we are talking about the string
of 32 octets, talking about it being litt-endian is confusing.

I would assume that section 8 of [CFRG-Curves] would define how to
convert the internal number to 32-octet string, so leaving out the
"little-endian order" from this draft would be clearer.

In the section 3.2 recipient tests there is discussion about checking
that the public key 32-octet string will have last byte in such way
that high-order bit of it is not set.

If this is property of the func(d, G) for curve25519, and curve448,
how can we ever get public values having that bit set? So why it is
only SHOULD to reject that public key? I mean if we receive such
public key, that clearly means that other end is either attacker, or
working incorrectly, and we MUST always reject it.

Or if there is no security issues accepting such public keys, then why
not just say that no checks needs to be done. If we reject such public
key, what behavior should happen in IKE level? Error message, drop
silently? Same as RFC6989?

In the security considerations section the 2nd last paragraph only
mentions that Curve25519 explictly, but does not mention curve448. Is
there difference between them, or should the text say "This is widely
seen as a security advantage for these curves, since...".

Typos: In section A.1.1 s/mutliplying/multiplying/

The test vectors should most likely also include Curve448 test
vectors, and perhaps even full IKEv2 KE payloads...
-- 
[email-masked]

---

From: Yoav Nir <[email-masked]>
Date: Wed, 26 Aug 2015 08:52:22 +0300

> On Aug 25, 2015, at 3:19 PM, Tero Kivinen <[email-masked]> wrote:
> 

Agree. Version -00 had only Curve25519 and the name remained. For the WG draft we can pick a different file name. 

The title is "New Safe Curves for IKEv2 Key Agreement”, which I think is slightly better than the CFRG draft ("Elliptic Curves for Security”), but perhaps too generic. Maybe we should follow the example of the TLS draft (“Curve25519 and Curve448 for Transport Layer Security (TLS)”) and title it “Curve25519 and Curve448 for IKEv2 Key Agreement”.

> "little-endian order" from this draft would be clearer.

I tried to follow the example of the TLS draft where the public keys are strings of octets while the private (or “secret”) keys are numbers. I see that I missed that in section 3.1. How about:

OLD:
   o  The Key Exchange Data is 32 octets encoded as an array of bytes in
      little-endian order as described in section 8 of [CFRG-Curves]
NEW:
   o  The Key Exchange Data is a 32-octet public key as described in 
      section 8 of [CFRG-Curves]
...or even...
NEW2:
   o  The Key Exchange Data is a 32-octet public key.

> silently? Same as RFC6989?

Tough one. The draft says something about implementation fingerprinting, but if some implementations drop this at the IKE level and some don’t that’s is a new avenue for fingerprinting. I don’t know which one is right. In any case, *if* you make the check *and* it fails *then* it’s appropriate to send an INVALID_SYNTAX (although I don’t understand why RFC 6989 recommends that over INVALID_KE_PAYLOAD) notification.

> seen as a security advantage for these curves, since…".

Nope. Just missed that when extending the document to cover Curve448.

> vectors, and perhaps even full IKEv2 KE payloads…

Yes. These were copied from the earlier TLS draft. They need some work.

Yoav

---

From: Yaron Sheffer <[email-masked]>
Date: Wed, 26 Aug 2015 09:33:51 +0300

> Tough one. The draft says something about implementation fingerprinting, but if some implementations drop this at the IKE level and some don’t that’s is a new avenue for fingerprinting. I don’t kno…

RFC 6989 recommends INVALID_SYNTAX because per RFC 7296, 
INVALID_KE_PAYLOAD is used for cases where the initiator is expected to 
retry the exchange. Incorrect DH values are at best a bug and at worst 
an attack, and should not result in an automatic retry.

Thanks,
	Yaron

---

From: Yoav Nir <[email-masked]>
Date: Wed, 26 Aug 2015 09:38:43 +0300

> On Aug 26, 2015, at 9:33 AM, Yaron Sheffer <[email-masked]> wrote:
> 
> RFC 6989 recommends INVALID_SYNTAX because per RFC 7296, INVALID_KE_PAYLOAD is used for cases where the initiator is expected to retry the exchange. Incorrect DH values are at best a bug and at worst an attack, and should not result in an automatic retry.

Ah. Makes sense. Section 2.5 has the string “invalid KE payload” several times, so it looked strange.

Yoav

---

From: Tero Kivinen <[email-masked]>
Date: Fri, 28 Aug 2015 14:18:34 +0300

Yoav Nir writes:
> The title is "New Safe Curves for IKEv2 Key Agreement”, which I
> think is slightly better than the CFRG draft ("Elliptic Curves for
> Security”), but perhaps too generic. Maybe we should follow the
> example of the TLS draft (“Curve25519 and Curve448 for Transport
> Layer Security (TLS)”) and title it “Curve25519 and Curve448 for
> IKEv2 Key Agreement”.

That title looks good.

> I tried to follow the example of the TLS draft where the public keys
> are strings of octets while the private (or “secret”) keys are
> numbers. I see that I missed that in section 3.1. How about: 
> 
> OLD:

I think this is better, i.e. keep the reference.

> Tough one. The draft says something about implementation
> fingerprinting, but if some implementations drop this at the IKE
> level and some don’t that’s is a new avenue for fingerprinting.

Yep.

> I don’t know which one is right. In any case, *if* you make the
> check *and* it fails *then* it’s appropriate to send an
> INVALID_SYNTAX (although I don’t understand why RFC 6989 recommends
> that over INVALID_KE_PAYLOAD) notification.

If we would use INVALID_KE_PAYLOAD then we would need to send
list of acceptable groups in it, and it would be stupid to say that
when other end is using group X, that INVALID_KE_PAYLOAD(use group
X)...

INVALID_SYNTAX tells that other end something that was invalid from
the protocol point of view, i.e. in this case it sent payload which
had bit set, where standard says it MUST not be set.

I think we either should say MUST, or MUST NOT (I do not really care
which one), but not SHOULD...
-- 
[email-masked]