gpg: Avoid writing a zero length last chunk in AEAD mode.
authorWerner Koch <wk@gnupg.org>
Wed, 28 Feb 2018 08:31:39 +0000 (09:31 +0100)
committerWerner Koch <wk@gnupg.org>
Wed, 28 Feb 2018 08:39:57 +0000 (09:39 +0100)
* g10/cipher-aead.c (write_header): Do not call set_nonce_and_ad.
(write_final_chunk): Do not increase chunkindex.
(do_flush): Call set_nonce_and_ad immediately before the first
encryption of a chunk.  Bump up the chunkindex after writing the tag.
(do_free): Do not insert a zero length last chunk.
* g10/decrypt-data.c (aead_underflow): Fix the corresponding bug.
--

This fixes a bug in writing a zero length last chunk right before the
final chunk (which has by design a zero length).  We also need to
adjust the decryption part because that assumed this zero length last
chunk.

Note that we use the term "last chunk" for the chunk which directly
precedes the "final chunk" which ends the entire encryption.

GnuPG-bug-id: 3774
Signed-off-by: Werner Koch <wk@gnupg.org>
g10/cipher-aead.c
g10/decrypt-data.c

index 1457baa..f9a996c 100644 (file)
@@ -200,9 +200,6 @@ write_header (cipher_filter_context_t *cfx, iobuf_t a)
   if (err)
     return err;
 
   if (err)
     return err;
 
-  err = set_nonce_and_ad (cfx, 0);
-  if (err)
-    return err;
   cfx->wrote_header = 1;
 
  leave:
   cfx->wrote_header = 1;
 
  leave:
@@ -238,9 +235,6 @@ write_final_chunk (cipher_filter_context_t *cfx, iobuf_t a)
   gpg_error_t err;
   char dummy[1];
 
   gpg_error_t err;
   char dummy[1];
 
-  if (cfx->chunklen)
-    cfx->chunkindex++;
-
   err = set_nonce_and_ad (cfx, 1);
   if (err)
     goto leave;
   err = set_nonce_and_ad (cfx, 1);
   if (err)
     goto leave;
@@ -297,8 +291,18 @@ do_flush (cipher_filter_context_t *cfx, iobuf_t a, byte *buf, size_t size)
       if (cfx->buflen == cfx->bufsize || finalize)
         {
           if (DBG_FILTER)
       if (cfx->buflen == cfx->bufsize || finalize)
         {
           if (DBG_FILTER)
-            log_debug ("encrypting: buflen=%zu %s n=%zu\n",
-                       cfx->buflen, finalize?"(finalize)":"", n);
+            log_debug ("encrypting: size=%zu buflen=%zu %s n=%zu\n",
+                       size, cfx->buflen, finalize?"(finalize)":"", n);
+
+          if (!cfx->chunklen)
+            {
+              if (DBG_FILTER)
+                log_debug ("start encrypting a new chunk\n");
+              err = set_nonce_and_ad (cfx, 0);
+              if (err)
+                goto leave;
+            }
+
           if (finalize)
             gcry_cipher_final (cfx->cipher_hd);
           if (DBG_FILTER)
           if (finalize)
             gcry_cipher_final (cfx->cipher_hd);
           if (DBG_FILTER)
@@ -314,8 +318,10 @@ do_flush (cipher_filter_context_t *cfx, iobuf_t a, byte *buf, size_t size)
            * be called after gcry_cipher_final and before
            * gcry_cipher_gettag - at least with libgcrypt 1.8 and OCB
            * mode.  */
            * be called after gcry_cipher_final and before
            * gcry_cipher_gettag - at least with libgcrypt 1.8 and OCB
            * mode.  */
-          gcry_cipher_encrypt (cfx->cipher_hd, cfx->buffer, cfx->buflen,
-                               NULL, 0);
+          err = gcry_cipher_encrypt (cfx->cipher_hd, cfx->buffer, cfx->buflen,
+                                     NULL, 0);
+          if (err)
+            goto leave;
           if (finalize && DBG_FILTER)
             log_printhex (cfx->buffer, cfx->buflen, "ciphr(1):");
           err = my_iobuf_write (a, cfx->buffer, cfx->buflen);
           if (finalize && DBG_FILTER)
             log_printhex (cfx->buffer, cfx->buflen, "ciphr(1):");
           err = my_iobuf_write (a, cfx->buffer, cfx->buflen);
@@ -328,19 +334,14 @@ do_flush (cipher_filter_context_t *cfx, iobuf_t a, byte *buf, size_t size)
           if (finalize)
             {
               if (DBG_FILTER)
           if (finalize)
             {
               if (DBG_FILTER)
-                log_debug ("chunklen=%ju  total=%ju\n",
+                log_debug ("writing tag: chunklen=%ju total=%ju\n",
                            (uintmax_t)cfx->chunklen, (uintmax_t)cfx->total);
               err = write_auth_tag (cfx, a);
               if (err)
                 goto leave;
 
                            (uintmax_t)cfx->chunklen, (uintmax_t)cfx->total);
               err = write_auth_tag (cfx, a);
               if (err)
                 goto leave;
 
-              if (DBG_FILTER)
-                log_debug ("starting a new chunk (cur size=%zu)\n", size);
               cfx->chunkindex++;
               cfx->chunklen = 0;
               cfx->chunkindex++;
               cfx->chunklen = 0;
-              err = set_nonce_and_ad (cfx, 0);
-              if (err)
-                goto leave;
               finalize = 0;
             }
         }
               finalize = 0;
             }
         }
@@ -361,38 +362,42 @@ do_free (cipher_filter_context_t *cfx, iobuf_t a)
   if (DBG_FILTER)
     log_debug ("do_free: buflen=%zu\n", cfx->buflen);
 
   if (DBG_FILTER)
     log_debug ("do_free: buflen=%zu\n", cfx->buflen);
 
-  /* FIXME: Check what happens if we just wrote the last chunk and no
-   * more bytes were to encrypt.  We should then not call finalize and
-   * write the auth tag again, right?  May this at all happen?  */
-
-  /* Call finalize which will also allow us to flush out and encrypt
-   * the last arbitrary length buffer.  */
-  gcry_cipher_final (cfx->cipher_hd);
-
-  /* Encrypt any remaining bytes.  */
   if (cfx->buflen)
     {
       if (DBG_FILTER)
   if (cfx->buflen)
     {
       if (DBG_FILTER)
-        log_debug ("processing last %zu bytes of the last chunk\n",
-                   cfx->buflen);
-      gcry_cipher_encrypt (cfx->cipher_hd, cfx->buffer, cfx->buflen, NULL, 0);
+        log_debug ("encrypting last %zu bytes of the last chunk\n",cfx->buflen);
+
+      if (!cfx->chunklen)
+        {
+          if (DBG_FILTER)
+            log_debug ("start encrypting a new chunk\n");
+          err = set_nonce_and_ad (cfx, 0);
+          if (err)
+            goto leave;
+        }
+
+      gcry_cipher_final (cfx->cipher_hd);
+      err = gcry_cipher_encrypt (cfx->cipher_hd, cfx->buffer, cfx->buflen,
+                                 NULL, 0);
+      if (err)
+        goto leave;
       err = my_iobuf_write (a, cfx->buffer, cfx->buflen);
       if (err)
         goto leave;
       /* log_printhex (cfx->buffer, cfx->buflen, "wrote:"); */
       cfx->chunklen += cfx->buflen;
       cfx->total += cfx->buflen;
       err = my_iobuf_write (a, cfx->buffer, cfx->buflen);
       if (err)
         goto leave;
       /* log_printhex (cfx->buffer, cfx->buflen, "wrote:"); */
       cfx->chunklen += cfx->buflen;
       cfx->total += cfx->buflen;
-    }
-  else /* Dummy encryption.  */
-    gcry_cipher_encrypt (cfx->cipher_hd, cfx->buffer, 0, NULL, 0);
 
 
-  /* Get and write the authentication tag.  */
-  if (DBG_FILTER)
-    log_debug ("chunklen=%ju  total=%ju\n",
-               (uintmax_t)cfx->chunklen, (uintmax_t)cfx->total);
-  err = write_auth_tag (cfx, a);
-  if (err)
-    goto leave;
+      /* Get and write the authentication tag.  */
+      if (DBG_FILTER)
+        log_debug ("writing tag: chunklen=%ju total=%ju\n",
+                   (uintmax_t)cfx->chunklen, (uintmax_t)cfx->total);
+      err = write_auth_tag (cfx, a);
+      if (err)
+        goto leave;
+      cfx->chunkindex++;
+      cfx->chunklen = 0;
+    }
 
   /* Write the final chunk.  */
   if (DBG_FILTER)
 
   /* Write the final chunk.  */
   if (DBG_FILTER)
index 5594d90..a3151b5 100644 (file)
@@ -770,17 +770,25 @@ aead_underflow (decode_filter_ctx_t dfx, iobuf_t a, byte *buf, size_t *ret_len)
 
   if (dfx->eof_seen)
     {
 
   if (dfx->eof_seen)
     {
-      if (DBG_FILTER)
-        log_debug ("eof seen: holdback buffer has the last and final tag\n");
 
 
-      log_assert (dfx->holdbacklen >= 32);
       if (dfx->chunklen)
         {
       if (dfx->chunklen)
         {
+          if (DBG_FILTER)
+            log_debug ("eof seen: holdback has the last and final tag\n");
+          log_assert (dfx->holdbacklen >= 32);
           err = aead_checktag (dfx, 0, dfx->holdback);
           if (err)
             goto leave;
           dfx->chunklen = 0;
           dfx->chunkindex++;
           err = aead_checktag (dfx, 0, dfx->holdback);
           if (err)
             goto leave;
           dfx->chunklen = 0;
           dfx->chunkindex++;
+          off = 16;
+        }
+      else
+        {
+          if (DBG_FILTER)
+            log_debug ("eof seen: holdback has the final tag\n");
+          log_assert (dfx->holdbacklen >= 16);
+          off = 0;
         }
 
       /* Check the final chunk.  */
         }
 
       /* Check the final chunk.  */
@@ -796,7 +804,7 @@ aead_underflow (decode_filter_ctx_t dfx, iobuf_t a, byte *buf, size_t *ret_len)
                      gpg_strerror (err));
           goto leave;
         }
                      gpg_strerror (err));
           goto leave;
         }
-      err = aead_checktag (dfx, 1, dfx->holdback+16);
+      err = aead_checktag (dfx, 1, dfx->holdback+off);
       if (err)
         goto leave;
       err = gpg_error (GPG_ERR_EOF);
       if (err)
         goto leave;
       err = gpg_error (GPG_ERR_EOF);