Re: [IPsec] WGLC on draft-ietf-ipsecme-rfc4307bis-11

[Paul Wouters <[e-mail]>]

On Tue, 13 Sep 2016, Valery Smyslov wrote:

> here is my review of draft-ietf-ipsecme-rfc4307bis-13.
> I didn't participate in the recent discussions,
> so I'm acting here more or less like "fresh" reader.

Thanks for the review!

> Overall, I think that the document is in a good shape, however some 
> additional polishing is required to improve its clarity and eliminate
> possible sources of confusion.
>
>
> Section 3.1
>
> It is not clear from the table and subsequent comments what key length
> for AES corresponds to what requirement levels. Well, it is clear, that 
> 192-bit AES is MAY, however I'm a bit confused abouth other key lengthes.

> Does the comment (1) mean that both AES-128-CBC and AES-256-CBC are MUST

Yes.

> and both AES-128-GCM_16 and AES-256-GCM_16 are SHOULD?

Yes.

> Could it be made more clear, e.g. by including key length in the table?

We have kept key lengths out of the tables on purpose. It matches the
tables at IANA that also do not list separate items for different key
lengths. Would "This requirement" instead of "This requirement level"
make that more clear?

> And the comment (IoT) brings even more confusion. It states that
> "only 128-bit keys are at MUST level", while the line it refers to 
> (ENCR_AES_CCM_8) indicates SHOULD level.

That is an error. It should read "only 128-bit keys are at the SHOULD level"

Note that I was hoping the (1) and (IoT) comments would appear similarly
indented so it becomes a little more clear and was planning to ask the
RFC Editor to ensure that.

> The next para after the table is inaccurate. It states:
>
>   ENCR_AES_CBC is raised from SHOULD+ in [RFC4307] to MUST.  It is the
>   only shared mandatory-to-implement algorithm with RFC4307 and as a
>   result it is necessary for interoperability with IKEv2 implementation
>   compatible with RFC4307.
>
> However, it is not exactly true. The comment (1) indicates that both
> AES-128-CBC and AEC-256-CBC are now MUST, while in RFC4307 AES-256-CBC wasn't 
> mentioned at all, so it definitely wasn't SHOULD+.
> So, this statement is true for AES-128-CBC and not true for AES-256-CBC.

It is only because you do not want to see ENCR_AES_CBC without a key
size. We are talking about updating IANA entries, and the IANA entries
are not separate for keysize.

> Section 3.2
>
>   If an algorithm is selected as the integrity algorithm, it SHOULD
>   also be used as the PRF. 
>
> That requirement isn't clear for me. Where it comes from?
> How it is justified? Is the document in the position to make such restrictive 
> requirement? Isn't it a local policy matter?

It came from me, after having talked to Tero about a TAHI test suite
test case. From what I understand from Tero, historically it was not
mandated in the IKEv2 RFC to use the same algorithm, although there is
really no argument in favour of using two different algorithms. If the
algo is good enough for INTEG, it is good enough for PRF. If one algo
is not good enough for either INTEG or PRF, then it is also not good
enough for the other.

Our implementation does not allow one to specify a PRF different from
INTEG (unless INTEG is AUTH_NONE with an AEAD ENCR)

I believe we ended up on SHOULD instead of MUST so this could be seen
as advise, and not a hard requirement. So it does not update the IKEv2
core RFC in that respect.

Do you have any use case where it makes sense that PRF != INTEG ?

>   PRF_HMAC_SHA1 has been downgraded from MUST in RFC4307 to MUST- as
>   their is an industry-wide trend to deprecate its usage.
>
> I think some more reasons should be given besides "an industry-wide trend".
> Industry usually follows standards, so it's a bit silly to refer to industry 
> trends
> as a reason here. I think some words about current concerns in the security
> of SHA1 should be added (like for PRF_HMAC_MD5 below).

How about:

 	"as cryptographic attacks against SHA1 are increasing, resulting in an
 	 industry-wide trend to deprecate its usage"

>
> Section 3.3
>
>   AUTH_HMAC_SHA1_96 has been downgraded from MUST in RFC4307 to MUST-
>   as there is an industry-wide trend to deprecate its usage.
>
> See my comment above to the similar sentence in Section 3.2

Same.

> Section 4.2
>
>       |  RSASSA-PSS with Empty Parameters   | MUST NOT |         |
>       |  RSASSA-PSS with Default Parameters | MUST NOT |         |
>
> Well, I'm a confused with these requirements.
> As far as I understand the RSASSA-PSS parameters default to using a SHA1 for 
> both hashAlgorithm and maskGenAlgorithm.
> Isn't more clear for readers to include
>
>       |  RSASSA-PSS with SHA1            | MUST NOT    |         |
>
> instead of these two lines, which in their current form don't
> explicitely refer to any cryptographic algorithm and force
> reader to dig into RSASSA-PSS specification to just get
> know that it was SHA1 meant? Or did I miss something?

I'll leave this one to Tero.

> Minor/editorial issues.
>
> Abstract:
>
>   This
>   document does not update the algorithms used for packet encryption
>   using IPsec Encapsulated Security Payload (ESP).
>
> I think AH must also be mentioned here (as RFC 7321 defines algorithms for 
> both).

Agreed.

> Section 3.1
>
>   Algorithms that are not AEAD MUST be used in conjunction with an
>   integrity algorithms in Section 3.3.
>
> should be:
>
>   Algorithms that are not AEAD MUST be used in conjunction with
>   integrity algorithms described in Section 3.3.

Agreed.

> In the sentence
>
>   It has been recommended by the CRFG and others as an
>   alternative to AES-CBC and AES-GCM.
>
> What "others" mean? Isn't it too vague wording for Standards Track RFC?
> Either list these "others" (at least some of them) or remove reference to 
> them.

Agreed. I will propose removing "and others".

> Section 3.2
>
>   PRF_HMAC_SHA2_256 was not mentioned in RFC4307, as no SHA2 based
>   transforms were mentioned.
>
> This sentence makes little sence for me. Shouldn't second "mentioned" be 
> "defined at that time"?

They might have been defined, but what matters is that the document did
not reference them. Perhaps:

    As no SHA2 based transforms were referenced in RFC4307, PRF_HMAC_SHA2_256
    was not mentioned in RFC4307.

> Later:
>
>    PRF_HMAC_SHA2_256 MUST be implemented in
>    order to replace SHA1 and PRF_HMAC_SHA1.
>
> We are talking about PRFs here, so why SHA1 is mentioned?
> If you are talking about general desire to get rid of SHA1-based
> transforms, then the sentence should be more explicit abot that.

Agreed, it should just be omitted.

> Section 3.3
>
>   AUTH_HMAC_SHA2_256_128 was not mentioned in RFC4307, as no SHA2 based
>   transforms were mentioned. 
>
> See my comment above about similar sentence in Section 3.2

Same as above.

>
>   AUTH_AES-XCBC is only recommended in the scope of IoT, as Internet of
>   Things deployments tend to prefer AES based pseudo-random functions
>   in order to avoid implementing SHA2. 
>
> s/AUTH_AES-XCBC/AUTH_AES-XCBC_96    (as in IANA Registry)

Actually, we should add this to section 9 for renaming and call it
AUTH_AES_XCBC_96.

> Section 3.4
>
>   Group 19 or 256-bit random ECP group was not specified in RFC4307, as
>   this group were not specified at that time. 
>
>
> See my comment above about similar sentence in Section 3.2

The second "specified" here could be changed to "define" ?

Paul