Mailing-List: contact common-issues-help@hadoop.apache.org; run by ezmlm
Precedence: bulk
Reply-To: common-issues@hadoop.apache.org
Date: Fri, 5 Dec 2014 01:56:12 +0000 (UTC)
From: "Yi Liu (JIRA)" <jira@apache.org>
To: common-issues@hadoop.apache.org
Message-ID: <JIRA.12758827.1417496230000.80164.1417744572808@Atlassian.JIRA>
In-Reply-To: <JIRA.12758827.1417496230000@Atlassian.JIRA>
References: <JIRA.12758827.1417496230000@Atlassian.JIRA>
 <JIRA.12758827.1417496230253@arcas>
Subject: [jira] [Comment Edited] (HADOOP-11343) Overflow is not properly
 handled in caclulating final iv for AES CTR
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


    [ https://issues.apache.org/jira/browse/HADOOP-11343?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14234916#comment-14234916 ] 

Yi Liu edited comment on HADOOP-11343 at 12/5/14 1:55 AM:
----------------------------------------------------------

Thanks [~jerrychenhf] for good catch and patch.  My comments is as following:
*1.*
{code}
public static boolean add(byte[] x, byte[] y, byte[] result) 
{code}
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. *
{code}
+    for (int i = 7; i > 0; i--) {
+      counterBytes[i] = (byte) counter;
+      counter >>>= 8;
     }
+    counterBytes[0] = (byte) counter;
{code}
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.


was (Author: hitliuyi):
Thanks [~jerrychenhf] for good catch and patch.  My comments is as following:
*1.*
{code}
public static boolean add(byte[] x, byte[] y, byte[] result) 
{code}
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 simple.

*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. *
{code}
+    for (int i = 7; i > 0; i--) {
+      counterBytes[i] = (byte) counter;
+      counter >>>= 8;
     }
+    counterBytes[0] = (byte) counter;
{code}
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 simple.

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

> Overflow is not properly handled in caclulating final iv for AES CTR
> --------------------------------------------------------------------
>
>                 Key: HADOOP-11343
>                 URL: https://issues.apache.org/jira/browse/HADOOP-11343
>             Project: Hadoop Common
>          Issue Type: Bug
>          Components: security
>    Affects Versions: 2.6.0
>            Reporter: Jerry Chen
>            Assignee: Jerry Chen
>            Priority: Blocker
>         Attachments: HADOOP-11343.patch
>
>
> 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.
> {code}
> /**
>    * 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);
>   }
> {code}


--
This message was sent by Atlassian JIRA
(v6.3.4#6332)