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

[Shen Sean <[e-mail]>]

Hi, Alfred,
Thanks for your detailed review, please see in lines:

2009/10/13 Alfred HÎnes <[e-mail]>


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.

[Sean] We did have a concise version ealier, eventually we decided to
include the necessary AES and CTR mode description, so that the draft is
more complete without dirrecting readers to many other document. We were
carefully to only include wide accepted and standard discriptions,
therefore, unless there are changes to AES or CTR mode themselfies (in that
case, maybe AES/CTR themselves are bigger problems we should worry about ,
:) ), there won't be a risk of inconsistency.


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.

[Sean] There are no special specifications when AES-CTR is used in IKEv2,
instead, IKEv2 just provide what AES-CTR need, that's what the draft gives
in section 3,4,5.


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

[Sean] Right, I will check all the parenthesis.

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

[Sean] Thanks, I will correct them.


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

[Sean] I will unify them as AES-CTR.



(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.
          ^
[Sean] Done.

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

[Sean] The intention of such a document is to give what a IKEv2 product
MUST/SHOULD/MAY provide. A user may not have "choices" of rounds or size,
but a vendor need to know what to provide.

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.
[Sean] Done.

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 payload [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.  [...]
[Sean] Done.

(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.
[Sean] Done.

(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.
[Sean]
Modified the "... for use in ...".
Wrote the second one as "... values with AES."

(4e)  6th para -- typo

Please correct:  s/AES- CTR/AES-CTR/  .
[Sean] Done

                     ^^       ^
(4f)  7th para -- punctuation

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

(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.
[Sean] Done

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

[Sean] Done             ^^^^^^^^^^^^^^^^^^^^^^^             ^


(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.
[Sean] As answered in item (3).

(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?
[Sean] The point here is plaintext padding.

(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.
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Sean] Done.

(8)  Section 3.2

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

   s/for Encrypted Payload/for the Encrypted Payload/
        ^                     ^^^^^
[Sean] Done

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

[Sean] Done.

(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.  [...]
                                                             ^^^^^
[Sean] Done.

(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.
[Sean] Done.

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!
[Sean] The words here are not giving description for AES-CTR usage as in
RFC3686, it mentions Child-SA because IKEv2 negotiates transformes used for
Child-SA.

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.
[Sean] Done.

(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.
  [...]
[Sean] Done.


(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.
[Sean] Added the "value".

(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.
                                        ^^^^^^^^^^^^
[Sean] Done

(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.  [...]
[Sean] Done.


(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).  [...]
        ^
[Sean] Corrected the singular/plural.

(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.
[Sean] Done.

(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.
[Sean] It's a good point, but for IANA's reference to this document, I
assume iana will updated their reference (following some rocedure?) when
this document is processed. Let me know if we have to request it in the
draft.
I added the a reference of IANA-Para to the IANA-IKEv2-parameters link.

Best,

Sean