Hi, Alfred, Thanks for your detailed review, please see in lines: 2009/10/13 Alfred HÎnes <a...@tr-sys.de>
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
_______________________________________________ IPsec mailing list IPsec@ietf.org https://www.ietf.org/mailman/listinfo/ipsec