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

From: "Waltermire, David A. (Fed)" <[email-masked]>
Date: Sat, 3 Sep 2016 02:41:17 +0000

All,

This message starts a Working Group Last Call (WGLC) for draft-ietf-ipsecme-rfc4307bis-11.

The version to be reviewed is https://www.ietf.org/id/draft-ietf-ipsecme-rfc4307bis-11.txt.

Please send your comments, questions, and edit proposals to the WG mail list until September 19th, 2016.  If you believe that the document is ready to be submitted to the IESG for consideration as a Standards Track RFC please send a short message stating this.

Best Regards,
Dave and Tero

---

From: Paul Wouters <[email-masked]>
Date: Mon, 5 Sep 2016 13:50:21 -0400 (EDT)

> Please send your comments, questions, and edit proposals to the WG mail list until September 19th, 2016.  If you believe that the document is ready to be submitted to the IESG for consideration as…

And since a lot of the regular active people on this list are
co-authors, it is extra important to hear from others!

Paul

---

From: "Valery Smyslov" <[email-masked]>
Date: Tue, 13 Sep 2016 15:34:35 +0300

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.

---

From: Paul Wouters <[email-masked]>
Date: Tue, 13 Sep 2016 11:51:24 -0400 (EDT)

> so I'm acting here more or less like "fresh" reader.

Thanks for the review!

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

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

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

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.

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

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

> See my comment above to the similar sentence in Section 3.2

Same.

> As far as I understand the RSASSA-PSS parameters default to using a SHA1 for 
> both hashAlgorithm and maskGenAlgorithm.

I'll leave this one to Tero.

> I think AH must also be mentioned here (as RFC 7321 defines algorithms for 

Agreed.

>   integrity algorithms described in Section 3.3.

Agreed.

> Either list these "others" (at least some of them) or remove reference to 

Agreed. I will propose removing "and others".

> This sentence makes little sence for me. Shouldn't second "mentioned" be 

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.

> transforms, then the sentence should be more explicit abot that.

Agreed, it should just be omitted.

> See my comment above about similar sentence in Section 3.2

Same as above.

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

> See my comment above about similar sentence in Section 3.2

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

Paul

---

From: "Valery Smyslov" <[email-masked]>
Date: Tue, 13 Sep 2016 21:56:12 +0300

Hi Paul, 

> lengths. Would "This requirement" instead of "This requirement level"

If you don't want to add key length column to the table,
(what I think is a preferred way) then I suggest to add some
words to corresponding paras that will reiterate:
"these requirements are for those key sizes".

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

OK. 

And again I think it should be reiterated in the text below the table (besides the comment).

> RFC Editor to ensure that.

I was thinking of complaining about their current appearance
(idented would be definitely better), but then I decided
that it is a feature of xml2rfc. But you are right that RFC Editor
can make them more better-looking.

> are not separate for keysize.

We are talking about updating RFC4307 and it does include key size for AES-CBC (Section 3.1.1):

   For confidentiality, implementations
   MUST- implement 3DES-CBC and SHOULD+ implement AES-128-CBC.  For
   integrity, HMAC-SHA1 MUST be implemented.

Note, that it explicitely references AES-CBC with 128 bit key length, not any other.

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

Sure.

First, not any good MAC is a good PRF (but vice versa is true).

Secondly, besides cryptographic properties there are also other
considerations, like performance. For example, in Russia
there is a standard MAC that is based on GOST cipher
with reduced number of rounds. It is very fast, but it cannot
be used as PRF.

Coming back from Russian quirks, I can imagine the situation when 
I want to use SHA2-512 for PRF (because I don't want my conversation
to be hacked offline in the future), but SHA1-HMAC-96 for MAC 
(because ICV tags are smaller comparing to SHA2 and I'm pretty 
confident in HMAC SHA1 security against _real_time_ attacks). 

And finally, I strongly oppose including such a restriction in 
this document, because from my point of view it is out of scope
of the document. It has nothing with defining requirement
levels for algorithms. Moreover SHOULD is a pretty strong requirement...

>  industry-wide trend to deprecate its usage"

OK.

>> See my comment above to the similar sentence in Section 3.2

OK.

> Agreed. I will propose removing "and others".

OK.

>    was not mentioned in RFC4307.

Works for me.

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

Actually, it is already called AUTH_AES_XCBC_96 in the IANA registry.
I wanted to point that it is erroneously called AUTH_AES-XCBC in this paragraph.

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

OK.

Regards,
Valery.

---

From: Tero Kivinen <[email-masked]>
Date: Thu, 15 Sep 2016 14:01:13 +0300

Paul Wouters writes:
> > 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
> >

This is aligned with RFC7427, which has 3 examples for RSASSA-PSS. The
first one is RSASSA-PSS with empty parameters. This means signature
uses SHA-1 and it is MUST NOT. This is the one implementations
generate if they use default parameters. The second one is an example
with RSASSA-PSS with Default parameters explictly encoded. This is not
according to the DER, but RFC4055 still requires that implementations
need to understand it, even when generating they should be using the
Empty Parameters version, and it still uses SHA1 so it is also MUST
NOT. 

The 3rd example in RFC7427 is RSASSA-PSS with SHA-256, and that is
specified as MUST.

In the beginning of the 4.2 we already explictly specify levels for
hash functions, i.e. SHA1 is MUST NOT. The next table only lists the
version we have in the RFC7427 and which are not MAY. 

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

It is already called that in the IANA registry. I fixed the "-" to "_"
in the Berlin IETF, while talking to the IANA people for other things,
and when noticed that typo in registry.

So the IANA registry already says "AUTH_AES_XCBC_96" and that is what
we should use also in this document, or we can also talk about
AES-XCBC (without AUTH_ and _96), when we are talking about the
algorithm in general (like AES-GCM vs AES-CBC). In this case, I think
the AUTH_AES_XCBC_96 is correct one.
-- 
[email-masked]

---

From: "Valery Smyslov" <[email-masked]>
Date: Thu, 15 Sep 2016 14:50:23 +0300

Hi Tero,

> This is aligned with RFC7427, which has 3 examples for RSASSA-PSS. 

Ah, I see your reason. However, I think that few extra words (probably in 
a comment) would make things more clear. Just clarify that both 
Empty and Default parameters mean using SHA1 which is MUST NOT.
Note, that RFC7427 lists them only in Appendix, which is optional
for reading and implementers might have been confused.

Regards,
Valery.