Merge pull request #723 from cyanogilvie/fix/ocb-decrypt-aliasing

Fix s_ocb_done aliasing bug in decrypt path

Author: sjaeckel

src/encauth/ocb/ocb_test.c

@@ -158,7 +158,7 @@ int ocb_test(void)
 
    int err, x, idx, res;
    unsigned long len;
-   unsigned char outct[MAXBLOCKSIZE], outtag[MAXBLOCKSIZE];
+   unsigned char outct[MAXBLOCKSIZE], outtag[MAXBLOCKSIZE], outpt[MAXBLOCKSIZE];
 
     /* AES can be under rijndael or aes... try to find it */
     if ((idx = find_cipher("aes")) == -1) {
@@ -179,6 +179,25 @@ int ocb_test(void)
            return CRYPT_FAIL_TESTVECTOR;
         }
 
+        /* Decrypt with separate input and output buffers.  Historically
+         * s_ocb_done() had an aliasing bug in its decrypt path that only
+         * surfaced when ct and pt were distinct buffers (the earlier
+         * in-place call below masked it).  Run this case first so it is
+         * exercised on every test vector.
+         */
+        XMEMSET(outpt, 0, sizeof(outpt));
+        if ((err = ocb_decrypt_verify_memory(idx, tests[x].key, 16, tests[x].nonce, outct, tests[x].ptlen,
+             outpt, tests[x].tag, len, &res)) != CRYPT_OK) {
+           return err;
+        }
+        if ((res != 1) || ltc_compare_testvector(outpt, tests[x].ptlen, tests[x].pt, tests[x].ptlen, "OCB separate-buffer", x)) {
+#ifdef LTC_TEST_DBG
+           printf("\n\nOCB: Failure-decrypt (separate buffers) - res = %d\n", res);
+#endif
+           return CRYPT_FAIL_TESTVECTOR;
+        }
+
+        /* Also exercise the in-place form for backward compatibility. */
         if ((err = ocb_decrypt_verify_memory(idx, tests[x].key, 16, tests[x].nonce, outct, tests[x].ptlen,
              outct, tests[x].tag, len, &res)) != CRYPT_OK) {
            return err;

src/encauth/ocb/s_ocb_done.c

@@ -77,10 +77,18 @@ int s_ocb_done(ocb_state *ocb, const unsigned char *pt, unsigned long ptlen,
    }
 
    if (mode == 1) {
-      /* decrypt mode, so let's xor it first */
-      /* xor C[m] into checksum */
+      /* decrypt mode: xor C[m] into checksum.  The function's parameter
+       * names are misleading (see header comment) -- in decrypt mode the
+       * input ciphertext lives in `pt` (not `ct`), and `ct` is the output
+       * plaintext buffer that has not been written yet.  Reading from `ct`
+       * here only happens to work when the caller aliases the input and
+       * output buffers (in-place decryption); with separate buffers the
+       * checksum is computed against uninitialised memory and the tag
+       * verification fails.  Use `pt` (the input parameter) so the code
+       * works for both in-place and separate-buffer callers.
+       */
       for (x = 0; x < (int)ptlen; x++) {
-         ocb->checksum[x] ^= ct[x];
+         ocb->checksum[x] ^= pt[x];
       }
    }