Re: [IPsec] review of draft-ietf-ipsecme-aes-ctr-ikev2-02

[Alfred Hönes <[e-mail]>]

Folks,
I do not want to re-iterate too frequently,
but it has become clear to me that my original review did not make
it to the list -- most likely due to a clerical error I had made.

Sean had responded to that message in detail on the list,
but it might help to have the original at hands for quoting,
and so I provide it below.

Feel free to skip forward to the "end forwarded message" separator
line if you are only interested in the still open issue.


--------  begin forwarded message  --------

> From: <[e-mail]>
> To: [e-mail]
> Message-Id: <[e-mail]>
> Date: Tue, 13 Oct 2009 17:09:25 +0200 (MESZ)
> Subject: review of draft-ietf-ipsecme-aes-ctr-ikev2-02

Hello,
I've arrived at completing a review of your I-D,
  draft-ietf-ipsecme-aes-ctr-ikev2-02,
and have a couple of comments.

First of all, this draft closes a gap that in the past could
only be bridged by extrapolation, incurring a high risk of
interoperability problems.  As such, this draft is very useful.

Yet, I'm not convinced that it is necessary to re-state so much
plain knowledge about AES and CTR mode in this memo, and not rarely
doing that even up to three times.

Therefore, I generally would appreciate an attempt to 'boil down'
the volume of this draft, reducing redundancies in the text (that,
as experience has shown, generally always also run the risk of
inconsistencies as well), and making more use of normative text
from existing specifications that can be referenced.

However, I could live with the current structure of the draft
if in certain places the focus were be changed from general
to the specifics of the application of AES-CTR in IKEv2.

Based on this general assumption, a linear walk-through of the
draft follows with detailed change recommendations.


Short substitutions are shown in POSIX edit style:  s/old/new/ .
To give more context, sometimes I quote larger blocks of text
literally and show the replacement proposed using the shorthand
notation:

   <original draft text>
---
   <modified text>

I use change bars ('|' in column 1) and occasionally insert
up/down pointing marker lines ('^^^'/'vvv') to emphasize
the location of textual issues and/or proposed corrections.
Modified text has been re-adjusted to match RFC formatting
rules, where appropriate.  I also try to accomodate published
RFC-Ed policy on style and punctuation etc. in my proposals.


(1)  Section 1, and General -- editorials

(1a)
In a few instances, text is inadvertantly immediately joined
with adjacent parenthetical parts.  Please ensure the presence
of white space in these places.
For instance, in the 3rd para of Section 1, please
    s/initial exchange(IKE_SA_INIT)/initial exchange (IKE_SA_INIT)/
                                                    ^
(1b)
In many places, articles are missing and singular/plural mismatches
occur.  I've tried to catch these all and address them in the items
below.

(1c)
The draft inconsistently switches between "AES_CTR" and "AES-CTR".
Please use a unique spelling -- preferably with the hypen.
This issue is not prosecuted in all details below and left as an
exercise. :-)


(2)  Section 2, last para

According to the rationale above, I suggest to apply the following
'facelifting' for the 4th (last) paragraph of Section 1:

   This document explains how IKEv2 makes use of AES_CTR algorithm for
   encrypting IKE messages that follow initial exchange: The second pair
   of messages (IKE_AUTH) in initial exchange, messages in
   CREATE_CHILD_SA exchange, messages in INFORMATIONAL exchange.
---
|  This document explains how IKEv2 makes use of the AES-CTR algorithm
                                                ^^^^^   ^
|  for encrypting IKE messages that follow the initial exchange: The
                                          ^^^^^
|  second pair of messages (IKE_AUTH) in the initial exchange, messages
                                        ^^^^^
|  in CREATE_CHILD_SA exchanges, and messages in INFORMATIONAL
                              ^
|  exchanges.
           ^

(3)  Section 2 (and ff.)

It is confusing for protocol designers and implementers to refer
to "parameters" of a cryptographic algorithm or building block
that in fact have been internal design decisions not exposed to
the external interface of the algorithm / building block.

Therefore, for the purpose of the application of AES and AES-CTR,
neither the number of rounds nor the block size of the AES cipher
are subject to higher level parametrization that, for instance,
would need to be carried in algorithm negotiation exchanges.

It is particularly confusing that the draft pretends Rounds and
Block Size were "choices" needing specification in this context.

The number of (internal) rounds is totally irrelevant for the
application of the AES.  Any attempt to mangle with this 'parameter'
would raise significant security concerns and conformance issues.
I strongly request to elide all mention of "rounds" from the draft.

The block size (128 bits) is an invariant of AES and should only
be mentioned in contexts where it really matters.

The consequences of these recommendations are included in the change
recommendations below.

The first paragraph of Section 2 essentially says all that's needed
to be said, but it might be gradually better to replace "can process"
by "processes" -- due to the lack of an alternative:

|  AES [AES] is a symmetric block cipher that can process data blocks of
   128 bits, using cipher keys with lengths of 128, 192, and 256 bits.
---                                           vvvvvvvvv
|  AES [AES] is a symmetric block cipher that processes data blocks of
   128 bits, using cipher keys with lengths of 128, 192, and 256 bits.

The second paragraph sketches the embedding of the building blocks,
and it should refrain from further discussing "choices".
Instead, I suggest to outline the role of the following section,
as a guide to the reader, and try to better delineate it from the
subsequent sections that specify the details for AES-CTR with IKEv2:

   The use of AES algorithm operations in IKEv2 is the same as what
   defined in [AES].  The use of Counter Mode is defined the same as how
|  AES_CTR is used to encrypt ESP payload [RFC3686].  The choices of Key
|  Size, Rounds and Block Size are defined as following which are
|  compatible with [RFC3686].
---
   The use of AES algorithm operations in IKEv2 is the same as what
   defined in [AES].  The use of Counter Mode is defined the same as how
|  AES-CTR is used to encrypt ESP payloads [RFC3686].
|
|  Section 2.1 sketches the operation and properties of counter mode
|  at a high level and indicates how it is to be used inside IKEv2;
|  Sections 3 through 6 will give the normative descriptions for the
|  details of this use case of AES-CTR.


(4)  Section 2.1

(4a)  1st para -- editorial fixes

Please adjust:

   This section gives description for AES Counter Mode algorithm and
   cites algorithm description part in section 2.1 of [RFC3686]
---                  vvv               vvvvv
|  This section gives a description for the AES Counter Mode algorithm
|  and cites the algorithm description part in Section 2.1 of [RFC3686].
            ^^^^^                              ^                       ^

(4b)  3rd para

Please delete te comma in the first sentence; two-handed enumerations
with "and"/"or" do not need that:

   Only AES Counter mode (AES-CTR) is discussed in this specification.
|  AES-CTR requires the encryptor to generate a unique per-packet value,
   and communicate this value to the decryptor.  [...]
---
   Only AES Counter mode (AES-CTR) is discussed in this specification.
|  AES-CTR requires the encryptor to generate a unique per-packet value
   and communicate this value to the decryptor.  [...]

(4c)  4th para

I fear the text copying from RFC 3686 went too far here, without the
necessary adoption.  This draft addresses IEKv2-inernal use of AES-CTR,
and that happens within the *IKE* SA, not in the *IPsec* SAs.
So please correct, for the purpose of this memo:

   This specification calls for the use of a nonce for additional
   protection against precomputation attacks.  The nonce value need not
   be secret.  However, the nonce MUST be unpredictable prior to the
|  establishment of the IPsec security association that is making use of
   AES-CTR.
---                     ^^^^^
   This specification calls for the use of a nonce for additional
   protection against precomputation attacks.  The nonce value need not
   be secret.  However, the nonce MUST be unpredictable prior to the
|  establishment of the IKE security association that is making use of
                        ^^^
   AES-CTR.

(4d)  5th para -- clarifications / textual enhancements

I suggest to change:

   AES-CTR has many properties that make it an attractive encryption
|  algorithm for in high-speed networking.  AES-CTR uses the AES block
   cipher to create a stream cipher.  Data is encrypted and decrypted by
   XORing with the key stream produced by AES encrypting sequential
   counter block values.  AES-CTR is easy to implement, and AES-CTR can
   be pipelined and parallelized.  AES-CTR also supports key stream
   precomputation.
---
   AES-CTR has many properties that make it an attractive encryption
|  algorithm for use in high-speed networking.  AES-CTR uses the AES
                ^^^^^
   block cipher to create a stream cipher.  Data is encrypted and
|  decrypted by XORing with the key stream produced by encrypting
                                                      ^
|  sequential counter block values with AES in ECB mode.  AES-CTR is
                                  ^^^^^^^^^^^^^^^^^^^^^
   easy to implement, and AES-CTR can be pipelined and parallelized.
   AES-CTR also supports key stream precomputation.

(4e)  6th para -- typo

Please correct:  s/AES- CTR/AES-CTR/  .
                      ^^       ^
(4f)  7th para -- punctuation

There should not be a comma in  "... one could use ..., to process ..."
                                                      ^

(4g)  9th para -- language improvement

Please swap words in the first line to restore correct semantics:

                vvvvvvvv
|  AES-CTR uses the only AES encrypt operation (for both encryption and
   decryption), making AES-CTR implementations smaller than
   implementations of many other AES modes.
---             vvvvvvvv
|  AES-CTR uses only the AES encrypt operation (for both encryption and
   decryption), making AES-CTR implementations smaller than
   implementations of many other AES modes.

(4h)  10th para

As in item (4c) above, two last sentences in this paragraph have not
been properly transformed into the intended context, inside IKEv2.
Please change:

              [...].  Extraordinary measures would be needed to prevent
   reuse of an IV value with the static key across power cycles.  To be
|  safe, implementations MUST use fresh keys with AES-CTR.  The Internet
|  Key Exchange [RFC4306] protocol can be used to establish fresh keys.
   IKE can also provide the nonce value.
---
              [...].  Extraordinary measures would be needed to prevent
   reuse of an IV value with the static key across power cycles.  To be
|  safe, implementations MUST use fresh keys with AES-CTR.  The initial
                                                                ^^^^^^^^
|  exchange of the IKEv2 protocol [RFC4306] can be used to establish
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|  fresh keys for an IKEv2 SA, and it also provides the nonce value.
              ^^^^^^^^^^^^^^^^^^^^^^^             ^

(5)  Section 2.2

I recommend to drop the entire section.
Detailed reasoning:

|2.2.  Key Sizes and Rounds

See the rationale in item (3) above.

|  AES supports three key sizes: 128 bits, 192 bits, and 256 bits.  [...]

That already has been stated earlier in the draft and hence is redundant.

|                                                           [...].  All
|  IKEv2 implementations that implement AES-CTR MUST support the 128 key
|  size.  An IKEv2 implementation MAY support key sizes of 192 and 256
|  bits.

That belongs into the normative part of the draft.
I suggest putting these two sentences as a new (2nd) paragraph into
Section 5.

|  AES MUST use different rounds for each of the key sizes:
|
|     When a 128-bit key is used, implementations MUST use 10 rounds.
|
|     When a 192-bit key is used, implementations MUST use 12 rounds.
|
|     When a 256-bit key is used, implementations MUST use 14 rounds.

As pointed out in item (3) above, that is an internal detail of the
AES design and not subject to importance or change by the user of AES.
This part definitely does not belong into this document.


(6)  Section 2.3

I recommend to drop the entire section.
Detailed reasoning:

|2.3.  Block Size
|
|  The AES algorithm has a block size of 128 bits (16 octets), i.e., AES
|  generate 128 bits of key stream.  [...]

This statement is misleading; the correct description can be found
earlier, in Section 2.1.

|                            [...].  For encryption or decryption, a
|  user XOR the key stream with 128 bits of plaintext or ciphertext
|  blocks.  If the generated key stream is longer than the plaintext or
|  ciphertext, the extra key stream bits are simply discarded.  For this
|  reason, AES-CTR does not require the plaintext to be padded to a
|  multiple of the block size.

This all has been explained at length and in detail in Section 2.1.
Why would we want to repeat it again here?


(7)  Section 3.1

In the first part of the text here, an article is missing.
The second part again rephrases parts of Section 2.1, and it does so
in a confusingly general way that does not help the implementor of
AES-CTR for use within IKEv2.  I request to drop that text.
Instead, I suggest to add a pointer to where the IV is used.

In total, please change:
                                         v
|  The IV field MUST be eight octets when AES_CTR algorithm is used for
   encryption.  The IV MUST be chosen by the encryptor in a manner that
   ensures that the same IV value is NOT used more than once with a
   given encryption key.  The encryptor can generate the IV in any
   manner that ensures uniqueness.  Common approaches to IV generation
   include incrementing a counter for each packet and linear feedback
   shift registers (LFSRs).
---                                      vvvvv   v
   The IV field MUST be eight octets when the AES-CTR algorithm is used
   for encryption.  The IV MUST be chosen by the encryptor in a manner
   that ensures that the same IV value is NOT used more than once with a
|  given encryption key.  See Section 4 for how the IV is used to
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|  construct counter blocks for AES-CTR use within IKEv2.
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


(8)  Section 3.2

Again, a definite article is missing here.  Please adjust:

    s/for Encrypted Payload/for the Encrypted Payload/
         ^                     ^^^^^


(9)  Section 3.3, 2nd para

In two more instances, the definitie article is missing.
Please adjust:

|  It should be noticed that ESP [RFC4303] Encrypted Payload requires
|  alignment on a 4-byte boundary while IKEv2 [RFC4306] Encrypted
   Payload does not have such a requirement.
---                         vvvvv
|  It should be noticed that the ESP [RFC4303] Encrypted Payload
|  requires alignment on a 4-byte boundary while the IKEv2 [RFC4306]
                                                ^^^^^
   Encrypted Payload does not have such a requirement.


(10)  Section 4

Again, please adjust ...

(10a)  2nd para

   The Encrypted Payload is the XOR of the plaintext and key stream.
|  The key stream is generated by inputing Counter Blocks into AES
   algorithm.  [...]
---
   The Encrypted Payload is the XOR of the plaintext and key stream.
|  The key stream is generated by inputing Counter Blocks into the AES
   algorithm.  [...]
                                                              ^^^^^
(10b)  last para

   Section 2 provides references to other documents for implementing
|  AES_CTR encryption/decryption process.
---
   Section 2 provides references to other documents for implementing
|  the AES-CTR encryption/decryption process.
   ^^^^   ^

(11)  Section 5

|  This section describes the conventions used by IKEv2 protocol to
|  generate encryption keys and nonces for use with AES-CTR algorithm in
|  IKE-SA negotiation.  The identifiers and attributes related to AES-
|  CTR required during IKE-SA and Child-SA negotiation are also defined.

Please fix missing articles and a singular/plural issue.

I'm not sure whether the mention of Child-SA is appropriate here.
IMHO, that is covere in RFC 3686, and should not be re-stated here.
I propose changed wording below.  Please check!

Further, as mentioned in item (5) above, I suggest to move text
from the obsolescent Section 2.2 into this section.

This results in the following revised text:
                                                 vvvvv
|  This section describes the conventions used by the IKEv2 protocol to
|  generate encryption keys and nonces for use with the AES-CTR
                                  v                ^^^^^
|  algorithm in IKE-SA negotiations.  The identifiers and attributes
|  related to AES-CTR as defined in [RFC3686] are used in the same
|  manner during IKE-SA negotiation.
|
|  All IKEv2 implementations that implement AES-CTR MUST support the 128
|  key size.  An IKEv2 implementation MAY support key sizes of 192 and
|  256 bits.


(12)  Section 5.1

(12a)  2nd para

Again, an article is missing, and item (1a) applies.

After an abbreviation has been introduced, it may be used to
streamline the text; therefore I suggest to delete the repeated
expansion of "PRF".

Hence, please change:

   IKEv2 negotiates four cryptographic algorithms with its peer using
|  IKE_SA_INIT exchange.  They include an encryption algorithm and a
|  pseudo-random function(PRF).  All the payloads of IKEv2 messages that
   follow the IKE_SA_INIT exchange are encrypted using the negotiated
|  encryption algorithm.  The pseudo-random function(PRF)is used to
   generate the keying material required for the encryption algorithm.
---
   IKEv2 negotiates four cryptographic algorithms with its peer using
|  the IKE_SA_INIT exchange.  They include an encryption algorithm and a
   ^^^^                  v
|  pseudo-random function (PRF).  All the payloads of IKEv2 messages
   that follow the IKE_SA_INIT exchange are encrypted using the
|  negotiated encryption algorithm.  The PRF is used to generate the
                                        ^   ^
   keying material required for the encryption algorithm.

(12b)  3rd para

Item (1b) again:

|  AES_CTR encryption algorithm needs an encryption key and a nonce.
   [...]
---vvvv
|  The AES_CTR encryption algorithm needs an encryption key and a nonce.
   [...]


(13)  Section 5.2

I suggest to insert the word "value" before "of 13", and to avoid
the unnecessary repetition of the code point.
The above concerns w.r.t. "Child-SA" apply.
So please change:

|  IKEv2 uses the IANA allocated encryption identifier of 13 for
|  ENCR_AES_CTR with an explicit IV (ENCR_AES_CTR 13) as the transform
|  ID during IKE-SA and Child-SA negotiation.
---
|  IKEv2 uses the IANA allocated encryption identifier value of 13 for
                                   v                  ^^^^^^^        vvv
|  ENCR_AES_CTR with an explicit IV as the transform ID during IKE-SA
   negotiation.


(14)  Section 6

(14a)  2nd para -- language improvement

Please adjust:

   AES_CTR provides high confidentiality when used properly.  However,
|  as a stream mode cipher, the security of will lose when AES-CTR is
   misused.
---                                      ^^^^^^^^^^^^
   AES_CTR provides high confidentiality when used properly.  However,
|  as a stream mode cipher, the security will be lost when AES-CTR is
   misused.
                                         ^^^^^^^^^^^^

(14b)  3rd para

Following (1b), please correct:

   Generally, a stream cipher should not use static keys.  This is
   because key streams will be easily canceled when two ciphertext use
|  the same key stream (check detailed description of this attack in
   [RFC3686]).  Therefore, IKEv2 should avoid an identical key being
|  used for different IKE SA or a same key stream being used on
   different blocks of plaintext.  [...]
---
   Generally, a stream cipher should not use static keys.  This is
   because key streams will be easily canceled when two ciphertext use
|  the same key stream (check the detailed description of this attack in
                             ^^^^^
   [RFC3686]).  Therefore, IKEv2 should avoid an identical key being
|  used for different IKE SAs or a same key stream being used on
                            ^
   different blocks of plaintext.  [...]

(14c)  4th para -- language improvements

                           v                       vvvvv
|  A stream cipher like AES_CTR is also vulnerable under data forgery
|  attack (check [RFC3686] for a demonstration of this attack).  [...]
---                        v                       vvvvvvv
|  A stream cipher like AES-CTR is also vulnerable against data forgery
|  attacks (check [RFC3686] for a demonstration of this attack).  [...]
         ^

(14d)  5th para

Again, according to (1b) please correct:
                                                                  v
|              [...].  Since IKEv2 are not likely to carry traffics in
   such a high quantity, this won't be a big concern here.  However,
   when large amount of traffic appear in the future or under abnormal
   circumstances, implementations SHOULD generate a fresh key before
   2^64 blocks are encrypted with the same key.
---
               [...].  Since IKEv2 are not likely to carry traffic in
   such a high quantity, this won't be a big concern here.  However,
|  when a large amount of traffic appears in the future or under
       ^^^                              ^
   abnormal circumstances, implementations SHOULD generate a fresh key
   before 2^64 blocks are encrypted with the same key.


(15)  Section 7

I suggest to more clearly indicate what this draft is expecting IANA
to do: adding a reference to this memo for an existing registration.

|  IANA has assigned 13 as the transform ID for ENCR_AES_CTR encryption
|  with an explicit IV.  This ID is to be used during IKE_SA
|  negotiation.
---
|  Per [RFC3686], IANA has assigned 13 as the "IKEv2 Encryption
|  Transform ID" to the name "ENCR_AES_CTR" for AES-CTR encryption with
|  an explicit IV, in the IANA "Internet Key Exchange Version 2 (IKEv2)
|  Parameters" registry.  This document specifies how to use this
|  transform during IKE_SA negotiations.  Hence IANA should add to that
|  entry a reference to this RFC.


Best regards,
  Alfred Hönes.

--------  end forwarded message  --------


Most of these items are resolved by Sean's comments.

The current discussion circles around the issues detailed in
items (3), (5), and (6) above.


Most recently, Sean Shen wrote:

> 2009/10/23 Alfred Hönes <[e-mail]>
>
>> Sean Shen wrote:
>>
>>>> ...
>>>>
>>> [Sean] The IKEv2 requirement in the draft is only about key lengths.
>>> I never pretended that the AES standard allows arbitary conbinations
>>> of key lengths and rounds.
>>> I checked the document again and noticed that in the second paragraph
>>> of section 2:
>>> "... The choices of Key Size, Rounds and Block Size are defined as
>>>   following which are compatible with [RFC3686]."
>>
>> That was one of my initial complaints ...
>>
>>> If this sentense misleads readers to think that users can choose all
>>> conbinations, I will rewrite it as:
>>>  "... The choices of Key Size are defined as following which are
>>>   compatible with [RFC3686]."
>>
>> ... and that's essentially what I had proposed for that paragraph.
>>
>> And yes, since that's written in the overview of Section 2, which
>> lays out the skeleton of the remainder of the section, the immediate
>> consequence of this change should be to drop sections 2.2 and 2.3
>> as well, as explained in my original review.
>> (To recall: The argument presented there was that, after dropping
>> inappropriate text, the remaining text in 2.2 & 2.3 would be a
>> simple -- yet verbose -- restatement of the first paragraph of
>> Section 2, and hence redundant anyway.)
>
> [Sean] Section 2.2 introduces key length of AES, the associated rounds,
> IKEv2's MUST/MAY requirements of key length. Section 2.3  introduces block
> size. These are all basic intro of algorithm.

... exactly, that's why Section 2 says the basics
(after the editorial fix):

|  AES [AES] is a symmetric block cipher that processes data blocks of
|  128 bits, using cipher keys with lengths of 128, 192, and 256 bits.

Regarding the remainder of the 1st para of Section 2.2, I said above
in item (5):

! |                                                           [...].  All
! |  IKEv2 implementations that implement AES-CTR MUST support the 128 key
! |  size.  An IKEv2 implementation MAY support key sizes of 192 and 256
! |  bits.
!
! That belongs into the normative part of the draft.
! I suggest putting these two sentences as a new (2nd) paragraph into
! Section 5.

After striking the out-of-scope text, 2.2 then is empty.  That's it.


> The overview of section 2 can be:
> " The use of AES algorithm operations in IKEv2 is the same as what defined
> in [AES].  The use of Counter Mode is defined the same as how AES_CTR is
> used to encrypt ESP payload [RFC3686].  Basic descriptions of AES and CTR
> mode are given in this section. Also the requirement of Key Size of IKEv2
> are discussed as following which are compatible with [RFC3686]."

I restate my suggestion from for that text item (3) above, which seems
to be much more clear and helpful for the reader of the document:

!    The use of AES algorithm operations in IKEv2 is the same as what
!    defined in [AES].  The use of Counter Mode is defined the same as how
! |  AES-CTR is used to encrypt ESP payloads [RFC3686].
! |
! |  Section 2.1 sketches the operation and properties of counter mode
! |  at a high level and indicates how it is to be used inside IKEv2;
! |  Sections 3 through 6 will give the normative descriptions for the
! |  details of this use case of AES-CTR.

The stripped-down first part says all necessary here.

The normative requirements on key size support belong into the
section that details all other aspects of interest of IEKv2,
Section 5.

The second part should serve as a guide for readers.
With this statement, it should be clear that 2.1 is an informative
sketch of other, referenced specifications; thus avoiding any
allusion of possible normative conflict.

>
> Best,
>
> Sean


Kind regards,
  Alfred.

-- 

+------------------------+--------------------------------------------+
| TR-Sys Alfred Hoenes   |  Alfred Hoenes   Dipl.-Math., Dipl.-Phys.  |
| Gerlinger Strasse 12   |  Phone: [phone], Fax: -18         |
| D-71254  Ditzingen     |  E-Mail:  [e-mail]                     |
+------------------------+--------------------------------------------+