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

["Valery Smyslov" <[e-mail]>]

Hi,

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.

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
and both AES-128-GCM_16 and AES-256-GCM_16 are SHOULD?
Could it be made more clear, e.g. by including key length in the table?

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. Well, I guess
that the text means that only AES-128-CCM_8 is SHOULD,
while other key lengthes are MAY, but the current text is really
confusing (yes, it is explained below in one of subsequent paras,
but I still prefer to have more clear statement). 
I'd again suggest to add key length into the table so that it is indicated explicitely.


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.


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?


   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).


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


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?


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).


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.


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.


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"?


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.


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


   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)


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


Regards,
Valery.