AES CTR mode broken because no carry is done on the counter

[X-Ryl669]

The ctr code is currently like this:

      c = iv.slice(0);
      d = data.slice(0);
      bl = sjcl.bitArray.bitLength(d);
      for (i=0; i<l; i+=4) {
        e = prf.encrypt(c);
        d[i] ^= e[0];
        d[i+1] ^= e[1];
        d[i+2] ^= e[2];
        d[i+3] ^= e[3];
        c[3]++;
      }

Here, only the last word of the iv (counter in CTR mode) is incremented. However, since the IV can be random (or user specified), the last word can contain 0xFFFFFFFF, thus when incremented, it'll not transmit the carry to the before-last word and the encryption/decryption will be broken.
Please notice that the number of words to en/decrypt will unlikely be low, so this carry issue might have been missed by the tests if the last word is not overflowing.

Solution: Test before incrementing that the last word is not overflowing and if it is, increment the before last word (and so on).

[X-Ryl669]

The following patch should fix it (tests ok):

diff --git a/core/ctr.js b/core/ctr.js
index 3ba3db1..b1afb71 100644
--- a/core/ctr.js
+++ b/core/ctr.js
@@ -64,7 +64,13 @@ sjcl.beware["CTR mode is dangerous because it doesn't protect message integrity.
         d[i+1] ^= e[1];
         d[i+2] ^= e[2];
         d[i+3] ^= e[3];
-        c[3]++;
+        for(var carry = 3; carry >= 0; carry--) {
+          if (c[carry] < 0xFFFFFFFF) {
+             c[carry]++;
+             break;
+          }
+          c[carry] = 0;
+        }
       }
       return sjcl.bitArray.clamp(d, bl);
     }

[X-Ryl669]

Added test generated with openssl (like written in the test file) that shows the issue.
It fails with the current code and with the PR, it passes.

[poursal]

I was looking into this fix and how CCM is implemented. From the nonce and how it is used I can see that an overflow will happen (much like this one) if the IV is: 00 00 00 03 02 01 00 A0 A1 A2 A3 FF FF and we operate on the plain text FF FF times.

Do you think my understanding is valid (@Nilos)?

[X-Ryl669]

Yes, it's the same issue, see ccm.js:211.
A similar patch as above should fix it. I'm wondering if it wouldn't be better to call the AES CTR code directly (this would make the library smaller).