Add GCM, CTR, and CTS modes to AES.

nss/mozilla/security/nss/lib/freebl/ctr.c

+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+/* $Id: gsm.c,v 1.27 2012/04/25 14:49:43 gerv%gerv.net Exp $ */
+
+#ifdef FREEBL_NO_DEPEND
+#include "stubs.h"
+#endif
+#include "prtypes.h"
+#include "blapit.h"
+#include "blapii.h"
+#include "ctr.h"
+#include "pkcs11t.h"
+#include "secerr.h"
+
+SECStatus
+CTR_InitContext(CTRContext *ctr, void *context, freeblCipherFunc cipher, 
+                const unsigned char *param, unsigned int blocksize)
+{
+    const CK_AES_CTR_PARAMS *ctrParams = (const CK_AES_CTR_PARAMS *)param;
+
+    ctr->bufPtr = blocksize; /* no unused data in the buffer */
+    ctr->cipher = cipher;
+    ctr->context = context;
+    ctr->counterBits =  ctrParams->ulCounterBits;

[Ryan Sleevi, 2012/09/11 19:34:30]

s/=  ctrParams/= ctrParams/ (extra space)

[Ryan Sleevi, 2012/09/11 19:34:30]

It seems you should be checking here, instead of / in addition to line 72, that
counterBits <= blocksize * BITS_PER_BYTE, and if it is not, then return
SECFailure.

+    PORT_Memcpy(ctr->counter, ctrParams->cb, blocksize);
+    return SECSuccess;
+}
+
+CTRContext *
+CTR_CreateContext(void *context, freeblCipherFunc cipher, 
+                  const unsigned char *param, unsigned int blocksize)
+{
+    CTRContext *ctr;
+    SECStatus rv;
+
+    /* first fill in the Counter context */
+    ctr = PORT_ZNew(CTRContext);
+    if (ctr == NULL) {
+        return NULL;
+    }
+    rv = CTR_InitContext(ctr, context, cipher, param, blocksize);
+    if (rv != SECSuccess) {
+        CTR_DestroyContext(ctr, PR_TRUE);
+        ctr = NULL;
+    }
+    return ctr;
+}
+
+void
+CTR_DestroyContext(CTRContext *ctr, PRBool freeit)
+{
+    PORT_Memset(ctr, 0, sizeof (CTRContext));

[Ryan Sleevi, 2012/09/11 19:34:30]

Why the additional memset here?

In looking at the other ciphers, such as RC4, they tend to be implemented in
terms of:

if (freeit)
  PORT_ZFree(ctr, sizeof(*ctr))

Even AES_DestroyContext doesn't zeroize out its *cx.

If this is for security purposes, shouldn't there be a "probably won't be
optimized out by a compiler" form of cleansing?

[wtc, 2012/09/14 01:16:42]

The form used here is also common. See, for example,
SHA1_DestroyContext.

Nelson commented out the memset call in AES_DestroyContext
in his patch for bug 303334 to improve performance.
Yes, this is to erase security-sensitive data from memory.
We should add a NSS_SecureMemset function to lib/util/secport.h.

[rjrejyea, 2012/09/19 21:43:40]

That's a very good suggestion.

bob

+    if (freeit) {
+        PORT_Free(ctr);
+    }
+}
+
+/*
+ * Used by counter mode. Increment the counter block. Not all bits in the
+ * counter block are part of the counter, counter_bits tells how many bits
+ * are part of the counter. The counter_block is block_size long. It's a
+ * big endian value.
+ */
+static void
+ctr_GetNextCtr(unsigned char *counter, unsigned int counterBits, 
+                unsigned int blocksize)
+{
+    unsigned char *counterPtr = counter + blocksize - 1;
+    unsigned char mask, count;
+
+    PORT_Assert(counterBits <= blocksize*BITS_PER_BYTE);
+    while (counterBits >= BITS_PER_BYTE) {
+        if (++(*(counterPtr--))) {
+            return;

[wtc, 2012/09/19 22:19:33]

Since counterBits is usually a multiple of 8, the counter
rollover check needs to be performed earlier, here.  I
agree it is easier to just track the number of blocks
processed than detecting counter bits rollover.

Also, PKCS #11 wants us to stop after the counter bits become
0 ("overflow"). I think that's too strict, and problematic
for GCM, whose counter bits don't always start with
0^31 || 1.  This is why rsleevi and I describe the
condition as "rollover". The counter bits should be allowed
to wrap around as long as we don't reuse the same value.

+        }
+        counterBits -= BITS_PER_BYTE;
+    }
+    if (counterBits == 0) {
+        return;
+    }
+    /* increment the final partial byte */
+    mask = (1 << counterBits)-1;
+    count = (*counterPtr)++ & mask;
+    *counterPtr = ((*counterPtr) & ~mask) | count;

[Ryan Sleevi, 2012/09/11 19:34:30]

This does not appear to handle counter rollover correctly, as specified by
PKCS#11.

From 6.9.3 of the Draft 7 of 2.30
 
If an attempt to encrypt/decrypt is made which will cause an overflow of the
counter block’s counter bits, then the mechanism shall return
CKR_DATA_LEN_RANGE.
Note that the mechanism should allow the final post increment of the counter to
overflow (if it implements it this way) but not allow any further processing
after this point. E.g. if ulCounterBits = 2 and the counter bits start as 1 then
only 3 blocks of data can be processed.

[rjrejyea, 2012/09/19 21:43:40]

I was under the impression that the counter was supposed to roll over so that
you could handle 1,2,3, then 0. I can accept that I misread the spec, however.
It's certainly easier to prevent the rollover condition, which is clearly more
secure, so I'm OK with a change that implements Ryan's suggection.

[Ryan Sleevi, 2012/09/19 21:53:15]

Sorry, this may have been read opposite of how I intended.

"Rolloverflow" is supposed to be supported (eg: [2, 3, 0, 1], [3, 0, 1, 2]), but
it is expected that if you end up rolling back to the initial counter value, it
should return CKR_DATA_LEN_RANGE.

Meaning the four-block sequence [2, 3, 0, 1], should, if supplied a fifth block,
return CKR_DATA_LEN_RANGE, rather than incrementing the counter to the sequence
[2, 3, 0, 1, 2].

The reason why I made the comment is because of line 74:

if (++(*counterPtr--)) { } seems to be checking against the counter looping back
to zero (unless I've missed something), which is why I thought/think that the
sequence [2, 3, 0, 1] is being blocked.

Bitmath is hard, I may have misread here.

[rjrejyea, 2012/09/19 21:59:36]

OK, so the bug here is that we need to store *counterPtr & mask in the data
structure and blow up in count == that value. So you did catch a bug here.

bob

[Ryan Sleevi, 2012/09/19 22:06:21]

Also, I didn't see the check here (or elsewhere) about ensuring that (total
length of to-be-en/de-crypted) didn't exceed the number of blocks left in the
counter.

Since Freebl only supports a fire-and-forget en/decrypt (not streaming
en/decrypt), this would mean that, somewhere, a check to make sure that the 2 ^
(counterBits) was greater-than-or-equal to the total number of blocks to be
encrypted. Since rollover of the [2, 3, 0, 1, 2] form should be prevented.

[Ryan Sleevi, 2012/09/19 22:09:42]

I don't think that will be sufficient. *counterPtr is only comparing a byte at a
time. Rollover on the byte level can happen, it's only when the total counter
itself repeats that it's problematic.

I think it's probably easier to make the check on the _Update function, by
checking "How many blocks can I process for counterBits vs how many blocks will
I process in this iteration" (even if only 1 iteration is ever performed)

+    return;
+}
+
+static void
+ctr_xor(unsigned char *target, const unsigned char *x, 
+         const unsigned char *y, int count)
+{
+    int i;
+    for (i=0; i < count; i++) {
+        *target++ = *x++ ^ *y++;
+    }
+}
+
+SECStatus
+CTR_Update(CTRContext *ctr, unsigned char *outbuf, 
+                unsigned int *outlen, unsigned int maxout, 
+                const unsigned char *inbuf, unsigned int inlen,
+                unsigned int blocksize)
+{
+    unsigned char cipherblock[MAX_BLOCK_SIZE];
+    unsigned int tmp;
+    SECStatus rv;
+
+    if (maxout < inlen) {
+        *outlen = inlen;
+        PORT_SetError(SEC_ERROR_OUTPUT_LEN);
+        return SECFailure;
+    }
+    *outlen = 0;
+    if (ctr->bufPtr != blocksize) {
+        int needed = PR_MIN(blocksize-ctr->bufPtr, inlen);
+        ctr_xor(outbuf, inbuf, ctr->buffer+ctr->bufPtr, needed);
+        ctr->bufPtr += needed;
+        outbuf += needed;
+        inbuf += needed;
+        *outlen += needed;
+        inlen -= needed;
+        if (inlen == 0) {
+            return SECSuccess;
+        }
+    }
+        
+    while (inlen >= blocksize) {
+        rv = (*ctr->cipher)(ctr->context, cipherblock, &tmp, blocksize,
+                        ctr->counter, blocksize, blocksize);
+        ctr_GetNextCtr(ctr->counter, ctr->counterBits, blocksize);
+        if (rv != SECSuccess) {
+            return SECFailure;
+        }
+        ctr_xor(outbuf, inbuf, cipherblock, blocksize);
+        outbuf += blocksize;
+        inbuf += blocksize;
+        *outlen += blocksize;
+        inlen -= blocksize;
+    }
+    if (inlen == 0) {
+        return SECSuccess;
+    }
+    rv = (*ctr->cipher)(ctr->context, ctr->buffer, &tmp, blocksize,
+                        ctr->counter, blocksize, blocksize);
+    ctr_GetNextCtr(ctr->counter, ctr->counterBits, blocksize);
+    if (rv != SECSuccess) {
+        return SECFailure;
+    }
+    ctr_xor(outbuf, inbuf, ctr->buffer, inlen);
+    ctr->bufPtr = inlen;
+    *outlen += inlen;
+    return SECSuccess;
+}