Uploaded image for project: 'Hadoop Common'
  1. Hadoop Common
  2. HADOOP-11343

Overflow is not properly handled in caclulating final iv for AES CTR

    Details

    • Target Version/s:

      Description

      In the AesCtrCryptoCodec calculateIV, as the init IV is a random generated 16 bytes,

      final byte[] iv = new byte[cc.getCipherSuite().getAlgorithmBlockSize()];
      cc.generateSecureRandom(iv);

      Then the following calculation of iv and counter on 8 bytes (64bit) space would easily cause overflow and this overflow gets lost. The result would be the 128 bit data block was encrypted with a wrong counter and cannot be decrypted by standard aes-ctr.

      /**
         * The IV is produced by adding the initial IV to the counter. IV length 
         * should be the same as {@link #AES_BLOCK_SIZE}
         */
        @Override
        public void calculateIV(byte[] initIV, long counter, byte[] IV) {
          Preconditions.checkArgument(initIV.length == AES_BLOCK_SIZE);
          Preconditions.checkArgument(IV.length == AES_BLOCK_SIZE);
          
          System.arraycopy(initIV, 0, IV, 0, CTR_OFFSET);
          long l = 0;
          for (int i = 0; i < 8; i++) {
            l = ((l << 8) | (initIV[CTR_OFFSET + i] & 0xff));
          }
          l += counter;
          IV[CTR_OFFSET + 0] = (byte) (l >>> 56);
          IV[CTR_OFFSET + 1] = (byte) (l >>> 48);
          IV[CTR_OFFSET + 2] = (byte) (l >>> 40);
          IV[CTR_OFFSET + 3] = (byte) (l >>> 32);
          IV[CTR_OFFSET + 4] = (byte) (l >>> 24);
          IV[CTR_OFFSET + 5] = (byte) (l >>> 16);
          IV[CTR_OFFSET + 6] = (byte) (l >>> 8);
          IV[CTR_OFFSET + 7] = (byte) (l);
        }
      
      1. HADOOP-11343.patch
        6 kB
        Jerry Chen
      2. HADOOP-11343.003.patch
        5 kB
        Yi Liu
      3. HADOOP-11343.002.patch
        5 kB
        Yi Liu
      4. HADOOP-11343.001.patch
        5 kB
        Yi Liu

        Activity

        Hide
        andrew.wang Andrew Wang added a comment -

        Mike Yoder do you mind taking a quick look at this? Jerry's description of the problem makes sense, I just want to make sure the proposed solution sounds okay to you.

        Show
        andrew.wang Andrew Wang added a comment - Mike Yoder do you mind taking a quick look at this? Jerry's description of the problem makes sense, I just want to make sure the proposed solution sounds okay to you.
        Hide
        yoderme Mike Yoder added a comment -

        What was the proposed solution? I didn't see a difference in the code above vs. what's in git right now. (I could've missed it...)

        The overflow happens at the "l += counter" line, yes? The statement above is "this overflow gets lost". Well, it doesn't actually get "lost", since this is integer arithmetic the MAX LONG wraps around to a MIN LONG and keeps going up. So the effects of the counter are constrained to bytes 8-15 of IV, and bytes 0-7 are fixed at the randomly generated value. We are not concerned with re-using IVs by wrapping a long - a long is 2^64 bits, and the counter is for each 16-byte block (or 2^4) so in order to wrap we'd have to have a file that's 2^68 bytes long. Not going to happen.

        There actually is no "standard aes-ctr". From http://csrc.nist.gov/publications/nistpubs/800-38a/sp800-38a.pdf,

        The Counter (CTR) mode is a confidentiality mode that features the application of the forward
        cipher to a set of input blocks, called counters, to produce a sequence of output blocks that are
        exclusive-ORed with the plaintext to produce the ciphertext, and vice versa. The sequence of
        counters must have the property that each block in the sequence is different from every other
        block. This condition is not restricted to a single message: across all of the messages that are
        encrypted under the given key, all of the counters must be distinct.

        And from Appendix B of that document:

        B.2 Choosing Initial Counter Blocks

        The initial counter blocks, T1, for each message that is encrypted under the given key must be
        chosen in a manner than ensures the uniqueness of all the counter blocks across all the messages.
        Two examples of approaches to choosing the initial counter blocks are given in this section.

        In the first approach, for a given key, all plaintext messages are encrypted sequentially. Within
        the messages, the same fixed set of m bits of the counter block is incremented by the standard
        incrementing function. The initial counter block for the initial plaintext message may be any
        string of b bits. The initial counter block for any subsequent message can be obtained by
        applying the standard incrementing function to the fixed set of m bits of the final counter block
        of the previous message. In effect, all of the plaintext messages that are ever encrypted under the
        given key are concatenated into a single message; consequently, the total number of plaintext
        blocks must not exceed 2^m . Procedures should be established to ensure the maintenance of the
        state of the final counter block of the latest encrypted message, and to ensure the proper
        sequencing of the messages.

        A second approach to satisfying the uniqueness property across messages is to assign to each
        message a unique string of b/2 bits (rounding up, if b is odd), in other words, a message nonce,
        and to incorporate the message nonce into every counter block for the message. The leading b/2
        bits (rounding up, if b is odd) of each counter block would be the message nonce, and the
        standard incrementing function would be applied to the remaining m bits to provide an index to
        the counter blocks for the message. Thus, if N is the message nonce for a given message, then
        the jth counter block is given by Tj = N | [j], for j = 1…n. The number of blocks, n, in any
        message must satisfy n < 2^m . A procedure should be established to ensure the uniqueness of the
        message nonces.

        This recommendation allows other methods and approaches for achieving the uniqueness
        property. Validation that an implementation of the CTR mode conforms to this recommendation
        will typically include an examination of the procedures for assuring the uniqueness of counter
        blocks within messages and across all messages that are encrypted under a given key.

        There are two "recommendations", and what's implemented follows the second recommendation. I believe that most CTR implementations (including Java) do something like this. (As an aside, I've seen an openssl implementation that follows the first recommendation.)

        Another note is that if this function is altered in any way, it will break the decryption of data that has already been encrypted. I don't know if that's much at all at this point, but one would still have to tread with care.

        I recommend that we leave this function alone... unless I misunderstood something about the problem.

        Show
        yoderme Mike Yoder added a comment - What was the proposed solution? I didn't see a difference in the code above vs. what's in git right now. (I could've missed it...) The overflow happens at the "l += counter" line, yes? The statement above is "this overflow gets lost". Well, it doesn't actually get "lost", since this is integer arithmetic the MAX LONG wraps around to a MIN LONG and keeps going up. So the effects of the counter are constrained to bytes 8-15 of IV, and bytes 0-7 are fixed at the randomly generated value. We are not concerned with re-using IVs by wrapping a long - a long is 2^64 bits, and the counter is for each 16-byte block (or 2^4) so in order to wrap we'd have to have a file that's 2^68 bytes long. Not going to happen. There actually is no "standard aes-ctr". From http://csrc.nist.gov/publications/nistpubs/800-38a/sp800-38a.pdf , The Counter (CTR) mode is a confidentiality mode that features the application of the forward cipher to a set of input blocks, called counters, to produce a sequence of output blocks that are exclusive-ORed with the plaintext to produce the ciphertext, and vice versa. The sequence of counters must have the property that each block in the sequence is different from every other block. This condition is not restricted to a single message: across all of the messages that are encrypted under the given key, all of the counters must be distinct. And from Appendix B of that document: B.2 Choosing Initial Counter Blocks The initial counter blocks, T1, for each message that is encrypted under the given key must be chosen in a manner than ensures the uniqueness of all the counter blocks across all the messages. Two examples of approaches to choosing the initial counter blocks are given in this section. In the first approach, for a given key, all plaintext messages are encrypted sequentially. Within the messages, the same fixed set of m bits of the counter block is incremented by the standard incrementing function. The initial counter block for the initial plaintext message may be any string of b bits. The initial counter block for any subsequent message can be obtained by applying the standard incrementing function to the fixed set of m bits of the final counter block of the previous message. In effect, all of the plaintext messages that are ever encrypted under the given key are concatenated into a single message; consequently, the total number of plaintext blocks must not exceed 2^m . Procedures should be established to ensure the maintenance of the state of the final counter block of the latest encrypted message, and to ensure the proper sequencing of the messages. A second approach to satisfying the uniqueness property across messages is to assign to each message a unique string of b/2 bits (rounding up, if b is odd), in other words, a message nonce, and to incorporate the message nonce into every counter block for the message. The leading b/2 bits (rounding up, if b is odd) of each counter block would be the message nonce, and the standard incrementing function would be applied to the remaining m bits to provide an index to the counter blocks for the message. Thus, if N is the message nonce for a given message, then the jth counter block is given by Tj = N | [j], for j = 1…n. The number of blocks, n, in any message must satisfy n < 2^m . A procedure should be established to ensure the uniqueness of the message nonces. This recommendation allows other methods and approaches for achieving the uniqueness property. Validation that an implementation of the CTR mode conforms to this recommendation will typically include an examination of the procedures for assuring the uniqueness of counter blocks within messages and across all messages that are encrypted under a given key. There are two "recommendations", and what's implemented follows the second recommendation. I believe that most CTR implementations (including Java) do something like this. (As an aside, I've seen an openssl implementation that follows the first recommendation.) Another note is that if this function is altered in any way, it will break the decryption of data that has already been encrypted. I don't know if that's much at all at this point, but one would still have to tread with care. I recommend that we leave this function alone... unless I misunderstood something about the problem.
        Hide
        jerrychenhf Jerry Chen added a comment -

        Thanks Mike for commenting.
        The code above is not the solution. It was copied from the source code.
        The risk of the existing implementation is actually, when generating the initIV, it was filled with 16 bytes random number. And in the calculateIV algorithm, it adds a long counter to the 8 bytes of initIV. This means that even the counter is not very big, long += counter still has a good possibility to overflow. For example, as the initIV is random, it might happens to be some big value in the 8 bytes, and a smaller counter will cause the result to be overflow.

        When saying that "the overflow get lost", the carry doesn't add up to the above 8 bytes. Since we have 16 bytes, but we are doing 8 bytes arithmetic. While for "standard aes-ctr", I mean that the increment of the counter is happening on 128bits (16 bytes) space, other than on 8 bytes space. Checking the following code from openssl:

        http://www.mcs.anl.gov/~bester/gsi/coverage/4.0.6/source-aes_ctr.c.html

        You can see that the AES_ctr128_inc increment the counter with 128bytes add arithmetic.

        We have two approaches for solving this:
        1. When generating 16 bytes initIV, we fill the lower 8 bytes with random IV and leave the higher 8 bytes zero for counter This is actually suggested in http://www.mcs.anl.gov/~bester/gsi/coverage/4.0.6/source-aes_ctr.c.html as following:

        • This algorithm assumes that the counter is in the x lower bits
        • of the IV (ivec), and that the application has full control over
        • overflow and the rest of the IV. This implementation takes NO
        • responsability for checking that the counter doesn't overflow
        • into the rest of the IV when incremented
          We choose the x = 8

        2. for calculateIV, we can always do a identical IV + Counter algorithm on 16 bytes other than on high (big endian) 8 bytes. If solution #1 is not done, from the probability, it is still possible that the IV + counter will overflow the whole 16 bytes if the initIV happens randomized to very large number. But it still has two benefits, a. the possibility would be much much lower for a 16 bytes number overflow with a long counter. b. Even the 16 bytes overflow, the algorithm is identical with the AES_ctr128_inc which means the encrypted block can be still decrypted by a "standard AES-ctr" cipher such as Java Cipher. The current calculateIV implementation cannot achieve this if the 8 bytes overflowed because the final IV used for the block will be different between a Java Cipher and our implementation, even with the same input if initIV and counter.

        We have get some initial code done and can provided the patch for review. Please help a review if possible.

        Thanks again for sharing the opinion.

        Show
        jerrychenhf Jerry Chen added a comment - Thanks Mike for commenting. The code above is not the solution. It was copied from the source code. The risk of the existing implementation is actually, when generating the initIV, it was filled with 16 bytes random number. And in the calculateIV algorithm, it adds a long counter to the 8 bytes of initIV. This means that even the counter is not very big, long += counter still has a good possibility to overflow. For example, as the initIV is random, it might happens to be some big value in the 8 bytes, and a smaller counter will cause the result to be overflow. When saying that "the overflow get lost", the carry doesn't add up to the above 8 bytes. Since we have 16 bytes, but we are doing 8 bytes arithmetic. While for "standard aes-ctr", I mean that the increment of the counter is happening on 128bits (16 bytes) space, other than on 8 bytes space. Checking the following code from openssl: http://www.mcs.anl.gov/~bester/gsi/coverage/4.0.6/source-aes_ctr.c.html You can see that the AES_ctr128_inc increment the counter with 128bytes add arithmetic. We have two approaches for solving this: 1. When generating 16 bytes initIV, we fill the lower 8 bytes with random IV and leave the higher 8 bytes zero for counter This is actually suggested in http://www.mcs.anl.gov/~bester/gsi/coverage/4.0.6/source-aes_ctr.c.html as following: This algorithm assumes that the counter is in the x lower bits of the IV (ivec), and that the application has full control over overflow and the rest of the IV. This implementation takes NO responsability for checking that the counter doesn't overflow into the rest of the IV when incremented We choose the x = 8 2. for calculateIV, we can always do a identical IV + Counter algorithm on 16 bytes other than on high (big endian) 8 bytes. If solution #1 is not done, from the probability, it is still possible that the IV + counter will overflow the whole 16 bytes if the initIV happens randomized to very large number. But it still has two benefits, a. the possibility would be much much lower for a 16 bytes number overflow with a long counter. b. Even the 16 bytes overflow, the algorithm is identical with the AES_ctr128_inc which means the encrypted block can be still decrypted by a "standard AES-ctr" cipher such as Java Cipher. The current calculateIV implementation cannot achieve this if the 8 bytes overflowed because the final IV used for the block will be different between a Java Cipher and our implementation, even with the same input if initIV and counter. We have get some initial code done and can provided the patch for review. Please help a review if possible. Thanks again for sharing the opinion.
        Hide
        jerrychenhf Jerry Chen added a comment -

        In summary, the basic principles we should use in this is:
        1. According the magnitude of the counter, the application should take responsibility of generating right 16bytes iv to avoid overflow counter into the initial IV bits.

        2. A so called "standard AES-ctr" handles iv + counter in 16 bytes arithmetic manner and doesn't aware and doesn't care about how many bytes are random IV space, how many bytes are counter space. It just increment a number over a 16 bytes number.

        Show
        jerrychenhf Jerry Chen added a comment - In summary, the basic principles we should use in this is: 1. According the magnitude of the counter, the application should take responsibility of generating right 16bytes iv to avoid overflow counter into the initial IV bits. 2. A so called "standard AES-ctr" handles iv + counter in 16 bytes arithmetic manner and doesn't aware and doesn't care about how many bytes are random IV space, how many bytes are counter space. It just increment a number over a 16 bytes number.
        Hide
        yoderme Mike Yoder added a comment -

        Let's back up a bit. I don't understand what the problem is that you are trying to address. You keep mentioning "overflow". Yes, bytes 8-15, when treated as a long and added to the counter, can wrap around instead of carrying the results into bytes 0-7. But what's the problem with that? The values of the IV will still be different for each block that's encrypted. No information is lost, and we still have to have a file larger than 2^68 bytes before we wrap the counter.

        You mention the openssl source code for AES_ctr128_encrypt() - that was exactly the code I was referring to when I said "(As an aside, I've seen an openssl implementation that follows the first recommendation.)" That's just one way to handle the IV.

        So I just went digging around for Java code that does AES-CTR, and found
        http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/8-b132/com/sun/crypto/provider/CounterMode.java#CounterMode

        Look for increment(). It does the same thing as the openssl implementation (which is actually a surprise to me, I thought it worked like calculateIV). I might be mistaken in this, but I thought that calculateIV() was written the way it was written completely on purpose. We should seek comment from Alejandro Abdelnur if he's still paying attention to emails that come from apache jira.

        You do need to address the upgrade question: changing this code will render useless any data encrypted with the current scheme, unless that data is first copied out of an EZ to clear text, the upgrade performed, and then the data copied back into the EZ. This is a very heavy price to pay.

        I'd also like to know what the use case is. You mention the Java Cipher - is your use case that you want to get the raw encrypted data and somehow decrypt it by hand outside of the normal path? If so, how would you get the key to decrypt it with? Why would you want to do this?

        Show
        yoderme Mike Yoder added a comment - Let's back up a bit. I don't understand what the problem is that you are trying to address. You keep mentioning "overflow". Yes, bytes 8-15, when treated as a long and added to the counter, can wrap around instead of carrying the results into bytes 0-7. But what's the problem with that? The values of the IV will still be different for each block that's encrypted. No information is lost, and we still have to have a file larger than 2^68 bytes before we wrap the counter. You mention the openssl source code for AES_ctr128_encrypt() - that was exactly the code I was referring to when I said "(As an aside, I've seen an openssl implementation that follows the first recommendation.)" That's just one way to handle the IV. So I just went digging around for Java code that does AES-CTR, and found http://grepcode.com/file/repository.grepcode.com/java/root/jdk/openjdk/8-b132/com/sun/crypto/provider/CounterMode.java#CounterMode Look for increment(). It does the same thing as the openssl implementation (which is actually a surprise to me, I thought it worked like calculateIV). I might be mistaken in this, but I thought that calculateIV() was written the way it was written completely on purpose. We should seek comment from Alejandro Abdelnur if he's still paying attention to emails that come from apache jira. You do need to address the upgrade question: changing this code will render useless any data encrypted with the current scheme, unless that data is first copied out of an EZ to clear text, the upgrade performed, and then the data copied back into the EZ. This is a very heavy price to pay. I'd also like to know what the use case is. You mention the Java Cipher - is your use case that you want to get the raw encrypted data and somehow decrypt it by hand outside of the normal path? If so, how would you get the key to decrypt it with? Why would you want to do this?
        Hide
        jerrychenhf Jerry Chen added a comment -

        Thanks Mike for following up.
        I agree that wrap around should work good for the this implementation. But the data it encrypted might cannot be decrypted by Java Cipher with the same key and initial IV. Because the iv + counter calculation here doesn't not follow the "convention" : Both openssl and java Cipher increment counter on 16 bytes space, but the currently calculateIV implementation increment on 8 bytes only and wrap around.

        Using the same key and same initial IV, the cipher text steam of AES-CTR mode will be different with other "standard" Ciphers, just because it use a different way of increment the counter. The result encrypted data will be the implementation specific and locked in here for all the cases. The data key can be managed or exported in many possible ways but what really matters here is the AES-CTR conformance of resulted data. Application may stored the resulted data in different layout such as add headers, but the different output data from the same configuration with the same algorithm is a serious problem.

        Yes. Dealing with the existing data would be something difficult. One possible solution is to add versioning mechanism in EZ attributes or file attributes in EZ.

        Show
        jerrychenhf Jerry Chen added a comment - Thanks Mike for following up. I agree that wrap around should work good for the this implementation. But the data it encrypted might cannot be decrypted by Java Cipher with the same key and initial IV. Because the iv + counter calculation here doesn't not follow the "convention" : Both openssl and java Cipher increment counter on 16 bytes space, but the currently calculateIV implementation increment on 8 bytes only and wrap around. Using the same key and same initial IV, the cipher text steam of AES-CTR mode will be different with other "standard" Ciphers, just because it use a different way of increment the counter. The result encrypted data will be the implementation specific and locked in here for all the cases. The data key can be managed or exported in many possible ways but what really matters here is the AES-CTR conformance of resulted data. Application may stored the resulted data in different layout such as add headers, but the different output data from the same configuration with the same algorithm is a serious problem. Yes. Dealing with the existing data would be something difficult. One possible solution is to add versioning mechanism in EZ attributes or file attributes in EZ.
        Hide
        jerrychenhf Jerry Chen added a comment -

        @Mike Yoder , When I deep further into the problem, I found that even considering only this implementation itself, it has problem because of the fact that the Encryptor and Decryptor underlayer uses Java Cipher or open ssl to do the encryption and decryption and use caculateIV method to initialize the iv to a specific position such as a seek operation.

        Just described as above, combination of the current caculateIV and other Cipher counter increment will cause problem if these two are not consistent.

        A strait forward proof of this problem is "The result stream output from CryptoOutputStream has a possibility of not be able to decrypted by CryptoInputStream" depending on seek operations.

        My previous statement "wrap around should work good for the this implementation. " assuming that the implementation use caculateIV to calculate all the final IV for all the blocks. But the actual situation now is interleave of caculateIV and increment algorithm of underlayer Cipher.

        Wish this makes the problem clear.

        For example, when using CryptoInputStream decrypt data, we first seek to a position which will use calculateIV to get the final IV feeding to underlayer Cipher (either java Cipher or openssl). If long overflow happens at calculateIV, and the underlayer Cipher get the wrong final IV to use. While the encryption process may follow the different pattern of calling calculateIV.

        Show
        jerrychenhf Jerry Chen added a comment - @Mike Yoder , When I deep further into the problem, I found that even considering only this implementation itself, it has problem because of the fact that the Encryptor and Decryptor underlayer uses Java Cipher or open ssl to do the encryption and decryption and use caculateIV method to initialize the iv to a specific position such as a seek operation. Just described as above, combination of the current caculateIV and other Cipher counter increment will cause problem if these two are not consistent. A strait forward proof of this problem is "The result stream output from CryptoOutputStream has a possibility of not be able to decrypted by CryptoInputStream" depending on seek operations. My previous statement "wrap around should work good for the this implementation. " assuming that the implementation use caculateIV to calculate all the final IV for all the blocks. But the actual situation now is interleave of caculateIV and increment algorithm of underlayer Cipher. Wish this makes the problem clear. For example, when using CryptoInputStream decrypt data, we first seek to a position which will use calculateIV to get the final IV feeding to underlayer Cipher (either java Cipher or openssl). If long overflow happens at calculateIV, and the underlayer Cipher get the wrong final IV to use. While the encryption process may follow the different pattern of calling calculateIV.
        Hide
        jerrychenhf Jerry Chen added a comment -

        Based on the above understanding, the upgrade would not be something be able to be considered. This is because once the issue happens in the encryption process, the decryption will fail unless the descryption strictly follow the same calling patten of calculateIV at the same sequnce of positions. But we know this is hard to achieve.

        So this fix would not be a pure improvement, but a crucial fix for unrecoverable data corrutption caused by encryption.

        Show
        jerrychenhf Jerry Chen added a comment - Based on the above understanding, the upgrade would not be something be able to be considered. This is because once the issue happens in the encryption process, the decryption will fail unless the descryption strictly follow the same calling patten of calculateIV at the same sequnce of positions. But we know this is hard to achieve. So this fix would not be a pure improvement, but a crucial fix for unrecoverable data corrutption caused by encryption.
        Hide
        wheat9 Haohui Mai added a comment - - edited

        You do need to address the upgrade question: changing this code will render useless any data encrypted with the current scheme, unless that data is first copied out of an EZ to clear text, the upgrade performed, and then the data copied back into the EZ. This is a very heavy price to pay.

        While any codecs can produce and consume encrypted texts, there are some questions to be answered to bridge the gap from encryption and security. For example, does the codec faithfully implement the algorithm? How well does it defend against timing attack? (some architecture will generate exception during overflow) What are the possible covert channels in the generated assembly?

        Much effort in crypto libraries are to analyze and to address these problems. Though miscalculating the IV may or may not be a big deal, I don't know how it can affect the answers of the above questions, as well as the security assurance.

        So this fix would not be a pure improvement, but a crucial fix for unrecoverable data corrutption caused by encryption.

        Agreed. I think this should be a blocker.

        Show
        wheat9 Haohui Mai added a comment - - edited You do need to address the upgrade question: changing this code will render useless any data encrypted with the current scheme, unless that data is first copied out of an EZ to clear text, the upgrade performed, and then the data copied back into the EZ. This is a very heavy price to pay. While any codecs can produce and consume encrypted texts, there are some questions to be answered to bridge the gap from encryption and security. For example, does the codec faithfully implement the algorithm? How well does it defend against timing attack? (some architecture will generate exception during overflow) What are the possible covert channels in the generated assembly? Much effort in crypto libraries are to analyze and to address these problems. Though miscalculating the IV may or may not be a big deal, I don't know how it can affect the answers of the above questions, as well as the security assurance. So this fix would not be a pure improvement, but a crucial fix for unrecoverable data corrutption caused by encryption. Agreed. I think this should be a blocker.
        Hide
        tucu00 Alejandro Abdelnur added a comment -

        If I recall correctly, if we want to change how the IV is calculated, it can be done in a backwards compatible way by introducing a new crypto SUITE. We store enough info in the file INODE to determine the SUITE used to encrypt a file. New files would be encrypted with the new SUITE, existing ones would continue using the original SUITE.

        We would just have to document that for versions of HDFS prior to ### (the one having this fix) it is strongly recommended not to swap the JCE implementation for the OpenSSL or viceversa as things could break.

        Show
        tucu00 Alejandro Abdelnur added a comment - If I recall correctly, if we want to change how the IV is calculated, it can be done in a backwards compatible way by introducing a new crypto SUITE. We store enough info in the file INODE to determine the SUITE used to encrypt a file. New files would be encrypted with the new SUITE, existing ones would continue using the original SUITE. We would just have to document that for versions of HDFS prior to ### (the one having this fix) it is strongly recommended not to swap the JCE implementation for the OpenSSL or viceversa as things could break.
        Hide
        jerrychenhf Jerry Chen added a comment -

        Thanks Alejandro Abdelnur for suggestion.
        As far as I see, the current issue is not a pure compatibility issue. A compatibility issue can be solved by using a different crypto SUITE. While the data corruption identified here can happen even if using this crypto SUITE only. This crypto SUITE itself is broken (generate corrupted data cannot be decrypted by itself) when IV + counter overflows on 8 bytes happens.

        This issue can be work rounded externally by fill lower 8 bytes with random IV and the higher 8 bytes (for counter) with zero. (To avoid overflow situation).

        But the current default KMS iv generation function will random the whole 16 bytes which makes to trigger condition of this issue much easier to satisfy.

        Show
        jerrychenhf Jerry Chen added a comment - Thanks Alejandro Abdelnur for suggestion. As far as I see, the current issue is not a pure compatibility issue. A compatibility issue can be solved by using a different crypto SUITE. While the data corruption identified here can happen even if using this crypto SUITE only. This crypto SUITE itself is broken (generate corrupted data cannot be decrypted by itself) when IV + counter overflows on 8 bytes happens. This issue can be work rounded externally by fill lower 8 bytes with random IV and the higher 8 bytes (for counter) with zero. (To avoid overflow situation). But the current default KMS iv generation function will random the whole 16 bytes which makes to trigger condition of this issue much easier to satisfy.
        Hide
        jerrychenhf Jerry Chen added a comment -

        The patched is provided. Please help review.

        The patch implement calculateIV on 16bytes addition identical to the way underlayer Cipher uses. This algorithm is modified from BigInteger add algorithm over an array of ints.

        The test cases are provided to guarantee calculateIV algorithm is correct implemented for designed and random cases.

        Show
        jerrychenhf Jerry Chen added a comment - The patched is provided. Please help review. The patch implement calculateIV on 16bytes addition identical to the way underlayer Cipher uses. This algorithm is modified from BigInteger add algorithm over an array of ints. The test cases are provided to guarantee calculateIV algorithm is correct implemented for designed and random cases.
        Hide
        jerrychenhf Jerry Chen added a comment -

        I also run test cases locally to compare large number of the results from the new caculateIV implementation and the old one, unless overflow happens on 8 bytes, the results are the same.

        Show
        jerrychenhf Jerry Chen added a comment - I also run test cases locally to compare large number of the results from the new caculateIV implementation and the old one, unless overflow happens on 8 bytes, the results are the same.
        Hide
        hadoopqa Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12684830/HADOOP-11343.patch
        against trunk revision 7caa3bc.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 1 new or modified test files.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5147//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5147//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12684830/HADOOP-11343.patch against trunk revision 7caa3bc. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5147//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5147//console This message is automatically generated.
        Hide
        yoderme Mike Yoder added a comment -

        Just described as above, combination of the current caculateIV and other Cipher counter increment will cause problem if these two are not consistent.

        Yeah, you're right. This is a good catch. Let me see if I can state this problem differently.

        If the underlying java (or openssl) ctr calculation is different than calculateIV, there is a problem IF

        • assume an initial IV of 00 00 00 00 00 00 00 00 ff ff ff ff ff ff ff ff
        • the file is 32 bytes
        • File A is written, all 32 bytes at once (one call to calculateIV with counter of 0)
        • File B is written, the first 16 bytes and then the second 16 bytes (two calls to calculateIV with counter of 0 and 1)
        • Then the last 16 bytes of files A and B will be different

        This actually isn't a problem if the files are read back exactly as they are written. But if you try to read file A in two steps, or read file B in one step, the second block will look corrupted. It seems possible to construct a test case for this.

        The code in the patch looks reasonable, although I haven't sat down with paper and pencil to work through the math. The test cases are convincing. Have you tested with both the openssl and java crypto implementations?

        I do believe that you still need to provide an upgrade path. This means defining a new crypto SUITE and make it the default. Existing files will use the old SUITE; the upgrade path is to simply copy all the files in an EZ; when writing the new files the new SUITE will be used and everything will work out.

        Show
        yoderme Mike Yoder added a comment - Just described as above, combination of the current caculateIV and other Cipher counter increment will cause problem if these two are not consistent. Yeah, you're right. This is a good catch. Let me see if I can state this problem differently. If the underlying java (or openssl) ctr calculation is different than calculateIV, there is a problem IF assume an initial IV of 00 00 00 00 00 00 00 00 ff ff ff ff ff ff ff ff the file is 32 bytes File A is written, all 32 bytes at once (one call to calculateIV with counter of 0) File B is written, the first 16 bytes and then the second 16 bytes (two calls to calculateIV with counter of 0 and 1) Then the last 16 bytes of files A and B will be different This actually isn't a problem if the files are read back exactly as they are written. But if you try to read file A in two steps, or read file B in one step, the second block will look corrupted. It seems possible to construct a test case for this. The code in the patch looks reasonable, although I haven't sat down with paper and pencil to work through the math. The test cases are convincing. Have you tested with both the openssl and java crypto implementations? I do believe that you still need to provide an upgrade path. This means defining a new crypto SUITE and make it the default. Existing files will use the old SUITE; the upgrade path is to simply copy all the files in an EZ; when writing the new files the new SUITE will be used and everything will work out.
        Hide
        tucu00 Alejandro Abdelnur added a comment -

        Got the issue now. +1 to Mike Yoder suggestion for the upgrade path.

        Show
        tucu00 Alejandro Abdelnur added a comment - Got the issue now. +1 to Mike Yoder suggestion for the upgrade path.
        Hide
        andrew.wang Andrew Wang added a comment -

        This makes sense to me too.

        Code-wise, it's a little tricky for the compatibility. We can add a new CipherSuite#AES_CTR_NOPADDING_FIXED (just a suggestion), but it's going to have the same getName as the original AES_CTR_NOPADDING. This name ("AES/CTR/NoPadding") is currently treated as unique, and used when we're determining the CipherSuite when creating an EZ, and also when instantiating the CryptoCodec to read data. We'll need to update these two places in the code to account for this correctly, FSNamesystem#createEncryptionZone and CryptoCodec#getInstance respectively.

        Jerry Chen does this make sense to you? I'd like to get this in ASAP, so I'm happy to take your patch and do the above if it helps.

        Show
        andrew.wang Andrew Wang added a comment - This makes sense to me too. Code-wise, it's a little tricky for the compatibility. We can add a new CipherSuite#AES_CTR_NOPADDING_FIXED (just a suggestion), but it's going to have the same getName as the original AES_CTR_NOPADDING. This name ("AES/CTR/NoPadding") is currently treated as unique, and used when we're determining the CipherSuite when creating an EZ, and also when instantiating the CryptoCodec to read data. We'll need to update these two places in the code to account for this correctly, FSNamesystem#createEncryptionZone and CryptoCodec#getInstance respectively. Jerry Chen does this make sense to you? I'd like to get this in ASAP, so I'm happy to take your patch and do the above if it helps.
        Hide
        hitliuyi Yi Liu added a comment - - edited

        Good catch Jerry, and good suggestion Mike Yoder. Also Thanks Tucu, Andrew and Haohui's discussion.
        Meanwhile, let me take a look at the OpenSSL and JCE implementation about this.

         I'd like to get this in ASAP, so I'm happy to take your patch and do the above if it helps.
        

        Agree

        Show
        hitliuyi Yi Liu added a comment - - edited Good catch Jerry, and good suggestion Mike Yoder . Also Thanks Tucu, Andrew and Haohui's discussion. Meanwhile, let me take a look at the OpenSSL and JCE implementation about this. I'd like to get this in ASAP, so I'm happy to take your patch and do the above if it helps. Agree
        Hide
        jerrychenhf Jerry Chen added a comment -

        Let me see if I can state this problem differently.

        Exactly.

        This actually isn't a problem if the files are read back exactly as they are written. But if you try to read file A in two steps, or read file B in one step, the second block will look corrupted.

        Yes. Logically, if the read follow exact the same pattern of calling caculateIV at exactly the same positions as write, it still can be decrypted. But that is very hard assumption and rarely the case in real application.

        Have you tested with both the openssl and java crypto implementations?

        Currently the test case use JceAesCtrCryptoCodec instance to test calculateIV. The implementation of calculateIV is common in the base abstract class AesCtrCryptoCodec and shared by the two CryptoCodec implementations. When a CryptoCodec has the real need to override calculateIV and it might be question the override version should be tested with the assumed of test cases. I think we can make it simple here and keep it is.

        I do believe that you still need to provide an upgrade path. This means defining a new crypto SUITE and make it the default. Existing files will use the old SUITE; the upgrade path is to simply copy all the files in an EZ; when writing the new files the new SUITE will be used and everything will work out.

        I would think this is not a normal upgrade situation as we think. The existing encrypted data already has problem to be decrypted with the old SUITE if the overflow condition descried above happens during encryption.

        It is an illusion to expect the old SUITE to deal the problem more properly than the fixed one. The calling pattern of calculateIV when writing has no logical relation to the pattern on reading. Just described, unless following the same pattern, the decryption problem will happen for sure when reading even with the old SUITE. So I would tend to consider this is a fix of "existing problem", other than "a revised or improved version".

        One benefit to create a new SUITE is that we can distinguish the two types of data: the data has potential of this problem or the data without. But just as Andrew pointing, it is a little sticky on this as we are creating two SUITEs for the same configuration of AES-CTR.

        I would prefer another way if we want to distinguish the two type of data: Add a version attribute to Encrytion file info. This can be addressed with a separate JIRA.

        Happy that we are coming closer on the solution. Thanks you all for review and comments.

        Show
        jerrychenhf Jerry Chen added a comment - Let me see if I can state this problem differently. Exactly. This actually isn't a problem if the files are read back exactly as they are written. But if you try to read file A in two steps, or read file B in one step, the second block will look corrupted. Yes. Logically, if the read follow exact the same pattern of calling caculateIV at exactly the same positions as write, it still can be decrypted. But that is very hard assumption and rarely the case in real application. Have you tested with both the openssl and java crypto implementations? Currently the test case use JceAesCtrCryptoCodec instance to test calculateIV. The implementation of calculateIV is common in the base abstract class AesCtrCryptoCodec and shared by the two CryptoCodec implementations. When a CryptoCodec has the real need to override calculateIV and it might be question the override version should be tested with the assumed of test cases. I think we can make it simple here and keep it is. I do believe that you still need to provide an upgrade path. This means defining a new crypto SUITE and make it the default. Existing files will use the old SUITE; the upgrade path is to simply copy all the files in an EZ; when writing the new files the new SUITE will be used and everything will work out. I would think this is not a normal upgrade situation as we think. The existing encrypted data already has problem to be decrypted with the old SUITE if the overflow condition descried above happens during encryption. It is an illusion to expect the old SUITE to deal the problem more properly than the fixed one. The calling pattern of calculateIV when writing has no logical relation to the pattern on reading. Just described, unless following the same pattern, the decryption problem will happen for sure when reading even with the old SUITE. So I would tend to consider this is a fix of "existing problem", other than "a revised or improved version". One benefit to create a new SUITE is that we can distinguish the two types of data: the data has potential of this problem or the data without. But just as Andrew pointing, it is a little sticky on this as we are creating two SUITEs for the same configuration of AES-CTR. I would prefer another way if we want to distinguish the two type of data: Add a version attribute to Encrytion file info. This can be addressed with a separate JIRA. Happy that we are coming closer on the solution. Thanks you all for review and comments.
        Hide
        hitliuyi Yi Liu added a comment - - edited

        Hi guys, I take an hour to look at this issue afternoon. Firstly it's a good catch from Jerry.

        In our code, we say for IV, we have two parts: the first 8 bytes, and the second 8 bytes (counter part).
        1.

        /* NOTE: the IV/counter CTR mode is big-endian.*/
        /* increment counter (128-bit int) by 1 */
        static void ctr128_inc(unsigned char *counter) {
          u32 n=16;
          u8  c;
        
          do {
            --n;
            c = counter[n];
            ++c;
            counter[n] = c;
            if (c) return;
          } while (n);
        }
        

        Above is OpenSSL implementation about counter increment.
        Here the counter is actually the IV (8 bytes IV part and 8 bytes counter part as our code).
        It's big-endian and handle overflow for whole 16 bytes.

        Increment the counter value.
        188
        189    private static void More ...increment(byte[] b) {
        190        int n = b.length - 1;
        191        while ((n >= 0) && (++b[n] == 0)) {
        192            n--;
        193        }
        194    }
        

        This snippet code is JCE implementation about counter increment.
        Here byte[] b is the counter, is actually the IV (8 bytes IV part and 8 bytes counter part as our code).
        It's big-endian and handle overflow for whole 16 bytes.

        So we can see both of them are exactly same and handle the overflow for whole 16 bytes (As Mike said). We need increase 1 to the first 8 bytes if overflow happen when we add the second 8 bytes with counter (l+counter), currently we haven't.

        2.
        About the increase 1 to the first 8 bytes when there is overflow for the counter part calculation, I don't like the current patch. It can be implemented more clear.
        Some obvious error in current patch:

        +    // Convert the counter to bytes in big-endian byte order
        +    for (int i = 7; i > 0; i--) {
        +      counterBytes[i] = (byte) counter;
        +      counter >>>= 8;
             }
        

        only loop 7 times, I wonder how it successes for the test.

        3.
        For the compatibility, I think there is no way. For example we call calculateIV and overflow happens. But when read, we don't call calculateIV at the same position. The decryption result will be always wrong.
        But I agree to add new SUITE to reject old DFSClient writing encryption data to new version HDFS...
        So maybe we can only fix it...

        Show
        hitliuyi Yi Liu added a comment - - edited Hi guys, I take an hour to look at this issue afternoon. Firstly it's a good catch from Jerry. In our code, we say for IV, we have two parts: the first 8 bytes, and the second 8 bytes (counter part). 1. /* NOTE: the IV/counter CTR mode is big-endian.*/ /* increment counter (128-bit int ) by 1 */ static void ctr128_inc(unsigned char *counter) { u32 n=16; u8 c; do { --n; c = counter[n]; ++c; counter[n] = c; if (c) return ; } while (n); } Above is OpenSSL implementation about counter increment. Here the counter is actually the IV (8 bytes IV part and 8 bytes counter part as our code). It's big-endian and handle overflow for whole 16 bytes. Increment the counter value. 188 189 private static void More ...increment( byte [] b) { 190 int n = b.length - 1; 191 while ((n >= 0) && (++b[n] == 0)) { 192 n--; 193 } 194 } This snippet code is JCE implementation about counter increment. Here byte[] b is the counter, is actually the IV (8 bytes IV part and 8 bytes counter part as our code). It's big-endian and handle overflow for whole 16 bytes. So we can see both of them are exactly same and handle the overflow for whole 16 bytes (As Mike said ). We need increase 1 to the first 8 bytes if overflow happen when we add the second 8 bytes with counter (l+counter), currently we haven't. 2 . About the increase 1 to the first 8 bytes when there is overflow for the counter part calculation, I don't like the current patch. It can be implemented more clear. Some obvious error in current patch: + // Convert the counter to bytes in big-endian byte order + for ( int i = 7; i > 0; i--) { + counterBytes[i] = ( byte ) counter; + counter >>>= 8; } only loop 7 times, I wonder how it successes for the test. 3 . For the compatibility, I think there is no way. For example we call calculateIV and overflow happens. But when read, we don't call calculateIV at the same position. The decryption result will be always wrong. But I agree to add new SUITE to reject old DFSClient writing encryption data to new version HDFS... So maybe we can only fix it...
        Hide
        hitliuyi Yi Liu added a comment - - edited

        I agree with Andrew Wang that we should fix it ASAP.

        And as I stated above, there is no way for compatibility, since when we append file, we call calculateIV and if overflow happens, the encrypted data will wrong, since we may read the data in different cases and can't guarantee we seek to the exact same position and do read.
        So my suggestion is:

        1. fix the overflow in a more clear way.
        2. Add a new cipher suite as Andrew and Mike said, and reject all old DFSClient writing encryption data to new version HDFS ...

        Show
        hitliuyi Yi Liu added a comment - - edited I agree with Andrew Wang that we should fix it ASAP. And as I stated above, there is no way for compatibility, since when we append file, we call calculateIV and if overflow happens, the encrypted data will wrong, since we may read the data in different cases and can't guarantee we seek to the exact same position and do read. So my suggestion is: 1. fix the overflow in a more clear way. 2. Add a new cipher suite as Andrew and Mike said, and reject all old DFSClient writing encryption data to new version HDFS ...
        Hide
        andrew.wang Andrew Wang added a comment -

        Hi all,

        I'd like to try and quantify the likelihood of hitting this overflow situation (hat tip to Mike Yoder who I discussed this with first).

        We're only in the danger zone when the random+counter overflows. The maximum HDFS file size (by default) is 64TB, or 2^46. Divided by 16B (2^4), which is the AES block size, we get 2^42. Now we divide by our random number 2^64, and get a 1 in 2^22 chance of hitting this, or 1 in 4 million.

        64TB is a quite pessimistic file size though, something more typical might be 1GB, or 2^30. Doing the same calculation (30-4-64), we get 1 in 2^38, or about 1 in 274 billion, which feels pretty remote.

        Given that even with the old calculateIV, the odds of hitting this are quite small, I think it might be okay to just fix it in place, without even a new CipherSuite.

        Yi's right that it'd be good to reject old DFSClients. We could even only reject if it's in a potential overflow situation, since we know the IV, file length, and a likely max file size. We could also detect old clients via an optional version PB field at read and create than hacking on a new CipherSuite. However, even this seems kind of optional if we're okay with the above odds.

        Thoughts?

        Show
        andrew.wang Andrew Wang added a comment - Hi all, I'd like to try and quantify the likelihood of hitting this overflow situation (hat tip to Mike Yoder who I discussed this with first). We're only in the danger zone when the random+counter overflows. The maximum HDFS file size (by default) is 64TB, or 2^46. Divided by 16B (2^4), which is the AES block size, we get 2^42. Now we divide by our random number 2^64, and get a 1 in 2^22 chance of hitting this, or 1 in 4 million. 64TB is a quite pessimistic file size though, something more typical might be 1GB, or 2^30. Doing the same calculation (30-4-64), we get 1 in 2^38, or about 1 in 274 billion, which feels pretty remote. Given that even with the old calculateIV, the odds of hitting this are quite small, I think it might be okay to just fix it in place, without even a new CipherSuite. Yi's right that it'd be good to reject old DFSClients. We could even only reject if it's in a potential overflow situation, since we know the IV, file length, and a likely max file size. We could also detect old clients via an optional version PB field at read and create than hacking on a new CipherSuite. However, even this seems kind of optional if we're okay with the above odds. Thoughts?
        Hide
        hitliuyi Yi Liu added a comment -

        Andrew, I agree with you and Mike Yoder about the likelihood analysis.
        I think we just need to fix the overflow without other changes.

        Show
        hitliuyi Yi Liu added a comment - Andrew, I agree with you and Mike Yoder about the likelihood analysis. I think we just need to fix the overflow without other changes.
        Hide
        hitliuyi Yi Liu added a comment -

        About converting counter to bytes in the patch, it's correct. My previous comment about this is invalid, I see there is counterBytes[0] = (byte) counter; after the loop.

        Show
        hitliuyi Yi Liu added a comment - About converting counter to bytes in the patch, it's correct. My previous comment about this is invalid, I see there is counterBytes [0] = (byte) counter; after the loop.
        Hide
        hitliuyi Yi Liu added a comment - - edited

        Thanks Jerry Chen for good catch and patch. My comments is as following:
        1.

        public static boolean add(byte[] x, byte[] y, byte[] result) 
        

        This function is unnecssary, we just need to add IV (16 bytes) with counter (8 bytes). And we can do it in a loop which is more clear.

        2.
        INT_MASK = 0xff, we only have few places using it, I think we don't need to define it. The name is incorrect, it's MASK for byte.

        *3. *

        +    for (int i = 7; i > 0; i--) {
        +      counterBytes[i] = (byte) counter;
        +      counter >>>= 8;
             }
        +    counterBytes[0] = (byte) counter;
        

        Please do it in a loop, then it's more clear. To further simply I think we can merge it with the loop of my comment #1, then the code is super clear.

        *4. *
        a) In genrally, code line should be < 80 chars.
        b) For loop or if, we should enclose using braces.
        c) code line indentation is 2 spaces, not 4 or a tab.

        Show
        hitliuyi Yi Liu added a comment - - edited Thanks Jerry Chen for good catch and patch. My comments is as following: 1. public static boolean add( byte [] x, byte [] y, byte [] result) This function is unnecssary, we just need to add IV (16 bytes) with counter (8 bytes). And we can do it in a loop which is more clear. 2. INT_MASK = 0xff , we only have few places using it, I think we don't need to define it. The name is incorrect, it's MASK for byte. *3. * + for ( int i = 7; i > 0; i--) { + counterBytes[i] = ( byte ) counter; + counter >>>= 8; } + counterBytes[0] = ( byte ) counter; Please do it in a loop, then it's more clear. To further simply I think we can merge it with the loop of my comment #1, then the code is super clear. *4. * a) In genrally, code line should be < 80 chars. b) For loop or if, we should enclose using braces. c) code line indentation is 2 spaces, not 4 or a tab.
        Hide
        jerrychenhf Jerry Chen added a comment -

        Thanks Yi for comments. Let's make some update and get it committed ASAP.
        Thanks you all for analyzing and reviewing.

        Show
        jerrychenhf Jerry Chen added a comment - Thanks Yi for comments. Let's make some update and get it committed ASAP. Thanks you all for analyzing and reviewing.
        Hide
        jerrychenhf Jerry Chen added a comment -

        As to Yi's comment:
        #1: The add function is a general implementation of add algorithm of any bytes borrowed from BigInteger. This is more general and clear. If you do want a 16bytes add arithmetic for long type specifically for performance reasons, I am OK with it.
        #2: INT_MASK is not named wrong. INT_MASK is also borrowed from BigInteger from LONG_MASK. In BigInteger, LONG_MASK is used to mask a int to long in unsigned way.
        #3: This algorithm of convert long to 8 bytes is borrowed from HBase util Bytes.java:
        /**

        • Convert a long value to a byte array using big-endian.
          *
        • @param val value to convert
        • @return the byte array
          */
          public static byte[] toBytes(long val)
          Unknown macro: { byte [] b = new byte[8]; for (int i = 7; i > 0; i--) { b[i] = (byte) val; val >>>= 8; } b[0] = (byte) val; return b; }

        #4: Yes. We need to fix for these code style stuff.

        Show
        jerrychenhf Jerry Chen added a comment - As to Yi's comment: #1: The add function is a general implementation of add algorithm of any bytes borrowed from BigInteger. This is more general and clear. If you do want a 16bytes add arithmetic for long type specifically for performance reasons, I am OK with it. #2: INT_MASK is not named wrong. INT_MASK is also borrowed from BigInteger from LONG_MASK. In BigInteger, LONG_MASK is used to mask a int to long in unsigned way. #3: This algorithm of convert long to 8 bytes is borrowed from HBase util Bytes.java: /** Convert a long value to a byte array using big-endian. * @param val value to convert @return the byte array */ public static byte[] toBytes(long val) Unknown macro: { byte [] b = new byte[8]; for (int i = 7; i > 0; i--) { b[i] = (byte) val; val >>>= 8; } b[0] = (byte) val; return b; } #4: Yes. We need to fix for these code style stuff.
        Hide
        hitliuyi Yi Liu added a comment -

        Thanks Jerry Chen.
        As discussed offline, I update the patch to address my comments and of course the credits are all of yours (you did the good find and posted the initial patch) . I just refine some parts to make code a bit more simple.

        Also thanks for your reply. Actually for 1#, 3#, their are just improvement, since we only need do additions for two bytes, it can be written in very few codes (see patch). For 2#, well, I see BigInteger uses similar name, so we can keep as it is, and in the new patch I just remove it, since we have only few places using it and we can use it directly as other hadoop code.

        For test, I keep them and just do some refine.

        Show
        hitliuyi Yi Liu added a comment - Thanks Jerry Chen . As discussed offline, I update the patch to address my comments and of course the credits are all of yours (you did the good find and posted the initial patch) . I just refine some parts to make code a bit more simple. Also thanks for your reply. Actually for 1#, 3#, their are just improvement, since we only need do additions for two bytes, it can be written in very few codes (see patch). For 2#, well, I see BigInteger uses similar name, so we can keep as it is, and in the new patch I just remove it, since we have only few places using it and we can use it directly as other hadoop code. For test, I keep them and just do some refine.
        Hide
        hitliuyi Yi Liu added a comment -

        Look again for the patch, we can remove the bytes copy too. Update it.

        Show
        hitliuyi Yi Liu added a comment - Look again for the patch, we can remove the bytes copy too. Update it.
        Hide
        hadoopqa Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12685260/HADOOP-11343.001.patch
        against trunk revision 0653918.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 1 new or modified test files.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5156//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5156//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12685260/HADOOP-11343.001.patch against trunk revision 0653918. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5156//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5156//console This message is automatically generated.
        Hide
        hadoopqa Hadoop QA added a comment -

        -1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12685274/HADOOP-11343.002.patch
        against trunk revision 0653918.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 1 new or modified test files.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. There were no new javadoc warning messages.

        -1 eclipse:eclipse. The patch failed to build with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in .

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5159//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5159//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12685274/HADOOP-11343.002.patch against trunk revision 0653918. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. -1 eclipse:eclipse . The patch failed to build with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in . +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5159//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5159//console This message is automatically generated.
        Hide
        andrew.wang Andrew Wang added a comment -

        Nice patch, thanks Yi for running with Jerry's patch. I spent some time staring at the bitshifts and it looks right, this is some nice tight code.

        Couple comments on the unit test, +1 pending though:

        • typo: Calcualted -> Calculated
        • We could use Longs.toByteArray from Guava rather than a new method
        • Could we add a randomized test that's in the danger zone for overflow? This could be as simple as testing with a really big counter and random IV, then with a really big bottom 8 of the IV and a random counter.
        Show
        andrew.wang Andrew Wang added a comment - Nice patch, thanks Yi for running with Jerry's patch. I spent some time staring at the bitshifts and it looks right, this is some nice tight code. Couple comments on the unit test, +1 pending though: typo: Calcualted -> Calculated We could use Longs.toByteArray from Guava rather than a new method Could we add a randomized test that's in the danger zone for overflow? This could be as simple as testing with a really big counter and random IV, then with a really big bottom 8 of the IV and a random counter.
        Hide
        hitliuyi Yi Liu added a comment -

        Thanks Andrew for nice review.
        I will update the unit test, how about I address comment 1, 2 and get the patch in first?
        I also thought we could add a randomized test that's in the danger zone for overflow, I will create a follow-on JIRA (cover really encryption/decryption with IV calculation overflow) and do it next Monday.

        Show
        hitliuyi Yi Liu added a comment - Thanks Andrew for nice review. I will update the unit test, how about I address comment 1, 2 and get the patch in first? I also thought we could add a randomized test that's in the danger zone for overflow, I will create a follow-on JIRA (cover really encryption/decryption with IV calculation overflow) and do it next Monday.
        Hide
        hitliuyi Yi Liu added a comment -

        Update the patch. Thanks Jerry for the good catch and initial patch. Thanks Andrew, Mike, Tucu, Haohui for the review and comments.

        Show
        hitliuyi Yi Liu added a comment - Update the patch. Thanks Jerry for the good catch and initial patch. Thanks Andrew, Mike, Tucu, Haohui for the review and comments.
        Hide
        andrew.wang Andrew Wang added a comment -

        SGTM, I'll commit this after Jenkins comes back

        Show
        andrew.wang Andrew Wang added a comment - SGTM, I'll commit this after Jenkins comes back
        Hide
        hadoopqa Hadoop QA added a comment -

        +1 overall. Here are the results of testing the latest attachment
        http://issues.apache.org/jira/secure/attachment/12685470/HADOOP-11343.003.patch
        against trunk revision 475c6b4.

        +1 @author. The patch does not contain any @author tags.

        +1 tests included. The patch appears to include 1 new or modified test files.

        +1 javac. The applied patch does not increase the total number of javac compiler warnings.

        +1 javadoc. There were no new javadoc warning messages.

        +1 eclipse:eclipse. The patch built with eclipse:eclipse.

        +1 findbugs. The patch does not introduce any new Findbugs (version 2.0.3) warnings.

        +1 release audit. The applied patch does not increase the total number of release audit warnings.

        +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common.

        +1 contrib tests. The patch passed contrib unit tests.

        Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5170//testReport/
        Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5170//console

        This message is automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - +1 overall . Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12685470/HADOOP-11343.003.patch against trunk revision 475c6b4. +1 @author . The patch does not contain any @author tags. +1 tests included . The patch appears to include 1 new or modified test files. +1 javac . The applied patch does not increase the total number of javac compiler warnings. +1 javadoc . There were no new javadoc warning messages. +1 eclipse:eclipse . The patch built with eclipse:eclipse. +1 findbugs . The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 release audit . The applied patch does not increase the total number of release audit warnings. +1 core tests . The patch passed unit tests in hadoop-common-project/hadoop-common. +1 contrib tests . The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HADOOP-Build/5170//testReport/ Console output: https://builds.apache.org/job/PreCommit-HADOOP-Build/5170//console This message is automatically generated.
        Hide
        andrew.wang Andrew Wang added a comment -

        Committed to trunk and branch-2. Thanks very much to Jerry for finding this bug, thorough explanations, and code contribution. Thanks as well to everyone who reviewed and commented, particularly Yi.

        Show
        andrew.wang Andrew Wang added a comment - Committed to trunk and branch-2. Thanks very much to Jerry for finding this bug, thorough explanations, and code contribution. Thanks as well to everyone who reviewed and commented, particularly Yi.
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-trunk-Commit #6659 (See https://builds.apache.org/job/Hadoop-trunk-Commit/6659/)
        HADOOP-11343. Overflow is not properly handled in caclulating final iv for AES CTR. Contributed by Jerry Chen. (wang: rev 0707e4eca906552c960e3b8c4e20d9913145eca6)

        • hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/TestCryptoCodec.java
        • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/AesCtrCryptoCodec.java
        • hadoop-common-project/hadoop-common/CHANGES.txt
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #6659 (See https://builds.apache.org/job/Hadoop-trunk-Commit/6659/ ) HADOOP-11343 . Overflow is not properly handled in caclulating final iv for AES CTR. Contributed by Jerry Chen. (wang: rev 0707e4eca906552c960e3b8c4e20d9913145eca6) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/TestCryptoCodec.java hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/AesCtrCryptoCodec.java hadoop-common-project/hadoop-common/CHANGES.txt
        Hide
        jerrychenhf Jerry Chen added a comment -

        Awesome! Thanks to Andrew for reviewing and get this committed. Thanks to Yi for nice refinement of the patch. Thanks to Mike, Tucu and Haohui for all the comments so that we have the chance to dive deep into the issue and reach a consensus on the solution.

        Show
        jerrychenhf Jerry Chen added a comment - Awesome! Thanks to Andrew for reviewing and get this committed. Thanks to Yi for nice refinement of the patch. Thanks to Mike, Tucu and Haohui for all the comments so that we have the chance to dive deep into the issue and reach a consensus on the solution.
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #27 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/27/)
        HADOOP-11343. Overflow is not properly handled in caclulating final iv for AES CTR. Contributed by Jerry Chen. (wang: rev 0707e4eca906552c960e3b8c4e20d9913145eca6)

        • hadoop-common-project/hadoop-common/CHANGES.txt
        • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/AesCtrCryptoCodec.java
        • hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/TestCryptoCodec.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #27 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/27/ ) HADOOP-11343 . Overflow is not properly handled in caclulating final iv for AES CTR. Contributed by Jerry Chen. (wang: rev 0707e4eca906552c960e3b8c4e20d9913145eca6) hadoop-common-project/hadoop-common/CHANGES.txt hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/AesCtrCryptoCodec.java hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/TestCryptoCodec.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Yarn-trunk #766 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/766/)
        HADOOP-11343. Overflow is not properly handled in caclulating final iv for AES CTR. Contributed by Jerry Chen. (wang: rev 0707e4eca906552c960e3b8c4e20d9913145eca6)

        • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/AesCtrCryptoCodec.java
        • hadoop-common-project/hadoop-common/CHANGES.txt
        • hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/TestCryptoCodec.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk #766 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/766/ ) HADOOP-11343 . Overflow is not properly handled in caclulating final iv for AES CTR. Contributed by Jerry Chen. (wang: rev 0707e4eca906552c960e3b8c4e20d9913145eca6) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/AesCtrCryptoCodec.java hadoop-common-project/hadoop-common/CHANGES.txt hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/TestCryptoCodec.java
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Hadoop-Hdfs-trunk-Java8 #27 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/27/)
        HADOOP-11343. Overflow is not properly handled in caclulating final iv for AES CTR. Contributed by Jerry Chen. (wang: rev 0707e4eca906552c960e3b8c4e20d9913145eca6)

        • hadoop-common-project/hadoop-common/CHANGES.txt
        • hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/TestCryptoCodec.java
        • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/AesCtrCryptoCodec.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-Hdfs-trunk-Java8 #27 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/27/ ) HADOOP-11343 . Overflow is not properly handled in caclulating final iv for AES CTR. Contributed by Jerry Chen. (wang: rev 0707e4eca906552c960e3b8c4e20d9913145eca6) hadoop-common-project/hadoop-common/CHANGES.txt hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/TestCryptoCodec.java hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/AesCtrCryptoCodec.java
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Hadoop-Hdfs-trunk #1959 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1959/)
        HADOOP-11343. Overflow is not properly handled in caclulating final iv for AES CTR. Contributed by Jerry Chen. (wang: rev 0707e4eca906552c960e3b8c4e20d9913145eca6)

        • hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/TestCryptoCodec.java
        • hadoop-common-project/hadoop-common/CHANGES.txt
        • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/AesCtrCryptoCodec.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-Hdfs-trunk #1959 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1959/ ) HADOOP-11343 . Overflow is not properly handled in caclulating final iv for AES CTR. Contributed by Jerry Chen. (wang: rev 0707e4eca906552c960e3b8c4e20d9913145eca6) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/TestCryptoCodec.java hadoop-common-project/hadoop-common/CHANGES.txt hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/AesCtrCryptoCodec.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Mapreduce-trunk #1981 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1981/)
        HADOOP-11343. Overflow is not properly handled in caclulating final iv for AES CTR. Contributed by Jerry Chen. (wang: rev 0707e4eca906552c960e3b8c4e20d9913145eca6)

        • hadoop-common-project/hadoop-common/CHANGES.txt
        • hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/TestCryptoCodec.java
        • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/AesCtrCryptoCodec.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk #1981 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1981/ ) HADOOP-11343 . Overflow is not properly handled in caclulating final iv for AES CTR. Contributed by Jerry Chen. (wang: rev 0707e4eca906552c960e3b8c4e20d9913145eca6) hadoop-common-project/hadoop-common/CHANGES.txt hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/TestCryptoCodec.java hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/AesCtrCryptoCodec.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #27 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/27/)
        HADOOP-11343. Overflow is not properly handled in caclulating final iv for AES CTR. Contributed by Jerry Chen. (wang: rev 0707e4eca906552c960e3b8c4e20d9913145eca6)

        • hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/TestCryptoCodec.java
        • hadoop-common-project/hadoop-common/CHANGES.txt
        • hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/AesCtrCryptoCodec.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #27 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/27/ ) HADOOP-11343 . Overflow is not properly handled in caclulating final iv for AES CTR. Contributed by Jerry Chen. (wang: rev 0707e4eca906552c960e3b8c4e20d9913145eca6) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/TestCryptoCodec.java hadoop-common-project/hadoop-common/CHANGES.txt hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/AesCtrCryptoCodec.java
        Hide
        vinodkv Vinod Kumar Vavilapalli added a comment -

        Pulled this into 2.6.1 after Akira Ajisaka verified that the patch applies cleanly. Ran compilation and TestCryptoCodec before the push.

        Show
        vinodkv Vinod Kumar Vavilapalli added a comment - Pulled this into 2.6.1 after Akira Ajisaka verified that the patch applies cleanly. Ran compilation and TestCryptoCodec before the push.

          People

          • Assignee:
            jerrychenhf Jerry Chen
            Reporter:
            jerrychenhf Jerry Chen
          • Votes:
            0 Vote for this issue
            Watchers:
            12 Start watching this issue

            Dates

            • Due:
              Created:
              Updated:
              Resolved:

              Development