security-and-privacy/gnupg
security-and-privacy/gnupg
1
0
Fork 0
mirror of git://git.gnupg.org/gnupg.git synced 2025-03-12 23:01:14 +01:00
Code Activity

gpg: Fix corner cases in AEAD encryption.

Browse Source 
* g10/cipher-aead.c (write_final_chunk): Do not bump up the chunk
index if the previous chunk was empty.
* g10/decrypt-data.c (aead_underflow): Likewise.  Also handle a other
corner cases.  Add more debug output.
--

GnuPG-bug-id: 3774

This fixes the reported case when the encrypted data is a multiple of
the chunk size.  Then the chunk index for the final chunk was wrongly
incremented by 2.  The actual fix makes use of the fact that the
current dfx->CHUNKLEN is 0 in this case.  There is also some other
reorganizing to help with debugging.  The thing seems to work now but
the code is not very clean - should be reworked.  Creating test files
can be done with this script:

--8<---------------cut here---------------start------------->8---
csize=6
for len in 0 55 56 57; do
   awk </dev/null -v i=$len 'BEGIN{while(i){i--;printf"~"}}' \
     | gpg --no-options -v --rfc4880bis --batch --passphrase "abc" \
           --s2k-count 1025 --s2k-digest-algo sha256 -z0 \
           --force-aead --aead-algo eax --cipher aes -a \
           --chunk-size $csize -c >symenc-aead-eax-c$csize-$len.asc
done
--8<---------------cut here---------------end--------------->8---

A LEN of 56 triggered the bug which can be seen by looking at the
"authdata:" line in the --debug=crypt,filter output.

Signed-off-by: Werner Koch <wk@gnupg.org>
This commit is contained in:
Werner Koch 2018-02-27 13:53:52 +01:00
parent cbc7bacf2f
commit ebb0fcf6e0
No known key found for this signature in database
GPG Key ID: E3FDFF218E45B72B
2 changed files with 112 additions and 71 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
g10/cipher-aead.c
View File

@ -244,7 +244,8 @@ write_final_chunk (cipher_filter_context_t *cfx, iobuf_t a)
gpg_error_t err; gpg_error_t err;
char dummy[1]; char dummy[1];
cfx->chunkindex++; if (cfx->chunklen)
cfx->chunkindex++;
err = set_nonce (cfx); err = set_nonce (cfx);
if (err) if (err)

180
g10/decrypt-data.c
View File

@ -541,6 +541,7 @@ aead_underflow (decode_filter_ctx_t dfx, iobuf_t a, byte *buf, size_t *ret_len)
size_t totallen = 0; /* The number of bytes to return on success or EOF. */ size_t totallen = 0; /* The number of bytes to return on success or EOF. */
size_t off = 0; /* The offset into the buffer. */ size_t off = 0; /* The offset into the buffer. */
size_t len; /* The current number of bytes in BUF+OFF. */ size_t len; /* The current number of bytes in BUF+OFF. */
int last_chunk_done = 0; /* Flag that we processed the last chunk. */
int c; int c;
log_assert (size > 48); /* Our code requires at least this size. */ log_assert (size > 48); /* Our code requires at least this size. */
@ -551,16 +552,16 @@ aead_underflow (decode_filter_ctx_t dfx, iobuf_t a, byte *buf, size_t *ret_len)
memcpy (buf, dfx->holdback, len); memcpy (buf, dfx->holdback, len);
if (DBG_FILTER) if (DBG_FILTER)
log_debug ("aead_underflow: size=%zu len=%zu%s\n", log_debug ("aead_underflow: size=%zu len=%zu%s%s\n", size, len,
size, len, dfx->eof_seen? " eof":""); dfx->partial? " partial":"", dfx->eof_seen? " eof":"");
/* Read and fill up BUF. We need to watchout for an EOF so that we /* Read and fill up BUF. We need to watch out for an EOF so that we
* can detect the last chunk which is commonly shorter than the * can detect the last chunk which is commonly shorter than the
* chunksize. After the last data byte from the last chunk 32 more * chunksize. After the last data byte from the last chunk 32 more
* bytes are expected for the last chunk's tag and the following * bytes are expected for the last chunk's tag and the following
* final chunk's tag. To detect the EOF we need to read at least * final chunk's tag. To detect the EOF we need to try reading at least
* one further byte; however we try to ready 16 extra bytes to avoid * one further byte; however we try to ready 16 extra bytes to avoid
* singel byte reads in some lower layers. The outcome is that we * single byte reads in some lower layers. The outcome is that we
* have up to 48 extra extra octets which we will later put into the * have up to 48 extra extra octets which we will later put into the
* holdback buffer for the next invocation (which handles the EOF * holdback buffer for the next invocation (which handles the EOF
* case). */ * case). */
@ -648,56 +649,72 @@ aead_underflow (decode_filter_ctx_t dfx, iobuf_t a, byte *buf, size_t *ret_len)
len -= n; len -= n;
if (DBG_FILTER) if (DBG_FILTER)
log_debug ("bytes left: %zu at off=%zu\n", len, off); log_debug ("ndecrypted: %zu (nchunk=%zu) bytes left: %zu at off=%zu\n",
totallen, dfx->chunklen, len, off);
/* Check the tag. */ /* Check the tag. */
if (len < 16) if (len < 16)
{ {
/* The tag is not entirely in the buffer. Read the rest of /* The tag is not entirely in the buffer. Read the rest of
* the tag from the holdback buffer. The shift the holdback * the tag from the holdback buffer. Then shift the holdback
* buffer and fill it up again. */ * buffer and fill it up again. */
memcpy (tagbuf, buf+off, len); memcpy (tagbuf, buf+off, len);
memcpy (tagbuf + len, dfx->holdback, 16 - len); memcpy (tagbuf + len, dfx->holdback, 16 - len);
dfx->holdbacklen -= 16-len; dfx->holdbacklen -= 16-len;
memmove (dfx->holdback, dfx->holdback + (16-len), dfx->holdbacklen); memmove (dfx->holdback, dfx->holdback + (16-len), dfx->holdbacklen);
len = dfx->holdbacklen; if (dfx->eof_seen)
if (dfx->partial)
{ {
for (; len < 48; len++ ) /* We should have the last chunk's tag in TAGBUF and the
* final tag in HOLDBACKBUF. */
if (len || dfx->holdbacklen != 16)
{ {
if ((c = iobuf_get (a)) == -1) /* Not enough data for the last two tags. */
{ err = gpg_error (GPG_ERR_TRUNCATED);
dfx->eof_seen = 1; /* Normal EOF. */ goto leave;
break;
}
dfx->holdback[len] = c;
} }
len = 0;
last_chunk_done = 1;
} }
else else
{ {
for (; len < 48 && dfx->length; len++, dfx->length--) len = dfx->holdbacklen;
if (dfx->partial)
{ {
c = iobuf_get (a); for (; len < 48; len++ )
if (c == -1)
{ {
dfx->eof_seen = 3; /* Premature EOF. */ if ((c = iobuf_get (a)) == -1)
break; {
dfx->eof_seen = 1; /* Normal EOF. */
break;
}
dfx->holdback[len] = c;
} }
dfx->holdback[len] = c;
} }
if (!dfx->length) else
dfx->eof_seen = 1; /* Normal EOF. */ {
for (; len < 48 && dfx->length; len++, dfx->length--)
{
c = iobuf_get (a);
if (c == -1)
{
dfx->eof_seen = 3; /* Premature EOF. */
break;
}
dfx->holdback[len] = c;
}
if (!dfx->length)
dfx->eof_seen = 1; /* Normal EOF. */
}
if (len < 32)
{
/* Not enough data for the last two tags. */
err = gpg_error (GPG_ERR_TRUNCATED);
goto leave;
}
dfx->holdbacklen = len;
len = 0;
} }
if (len < 32)
{
/* Not enough data for the last two tags. */
err = gpg_error (GPG_ERR_TRUNCATED);
goto leave;
}
dfx->holdbacklen = len;
/* log_printhex (dfx->holdback, dfx->holdbacklen, "holdback:"); */
len = 0;
} }
else /* We already have the full tag. */ else /* We already have the full tag. */
{ {
@ -716,54 +733,73 @@ aead_underflow (decode_filter_ctx_t dfx, iobuf_t a, byte *buf, size_t *ret_len)
gpg_strerror (err)); gpg_strerror (err));
goto leave; goto leave;
} }
if (DBG_FILTER)
log_debug ("tag is valid\n");
/* Prepare a new chunk. */ /* Prepare a new chunk. */
dfx->chunklen = 0; if (!last_chunk_done)
dfx->chunkindex++; {
err = aead_set_nonce (dfx); dfx->chunklen = 0;
if (err) dfx->chunkindex++;
goto leave; err = aead_set_nonce (dfx);
err = aead_set_ad (dfx, 0); if (err)
if (err) goto leave;
goto leave; err = aead_set_ad (dfx, 0);
if (err)
goto leave;
}
continue; continue;
} }
if (dfx->eof_seen) if (!last_chunk_done)
{ {
/* This is the last block of the last chunk. Its length may if (dfx->eof_seen)
* not be a multiple of the block length. */ {
gcry_cipher_final (dfx->cipher_hd); /* This is the last block of the last chunk. Its length may
} * not be a multiple of the block length. */
err = gcry_cipher_decrypt (dfx->cipher_hd, buf + off, len, NULL, 0); gcry_cipher_final (dfx->cipher_hd);
if (err) }
{ err = gcry_cipher_decrypt (dfx->cipher_hd, buf + off, len, NULL, 0);
log_error ("gcry_cipher_decrypt failed (2): %s\n", gpg_strerror (err));
goto leave;
}
totallen += len;
dfx->chunklen += len;
dfx->total += len;
if (dfx->eof_seen)
{
if (DBG_FILTER)
log_debug ("eof seen: holdback buffer has the tags.\n");
log_assert (dfx->holdbacklen >= 32);
if (DBG_FILTER)
log_printhex (dfx->holdback, 16, "tag:");
err = gcry_cipher_checktag (dfx->cipher_hd, dfx->holdback, 16);
if (err) if (err)
{ {
log_error ("gcry_cipher_checktag failed (2): %s\n", log_error ("gcry_cipher_decrypt failed (2): %s\n",
gpg_strerror (err)); gpg_strerror (err));
goto leave; goto leave;
} }
totallen += len;
dfx->chunklen += len;
dfx->total += len;
if (DBG_FILTER)
log_debug ("ndecrypted: %zu (nchunk=%zu)\n", totallen, dfx->chunklen);
}
if (dfx->eof_seen)
{
if (DBG_FILTER)
log_debug ("eof seen: holdback buffer has the %s.\n",
last_chunk_done? "final tag":"last and final tag");
if (!last_chunk_done)
{
log_assert (dfx->holdbacklen >= 32);
if (DBG_FILTER)
log_printhex (dfx->holdback, 16, "tag:");
err = gcry_cipher_checktag (dfx->cipher_hd, dfx->holdback, 16);
if (err)
{
log_error ("gcry_cipher_checktag failed (2): %s\n",
gpg_strerror (err));
goto leave;
}
if (DBG_FILTER)
log_debug ("tag is valid\n");
}
/* Check the final chunk. */ /* Check the final chunk. */
dfx->chunkindex++; if (dfx->chunklen)
dfx->chunkindex++;
err = aead_set_nonce (dfx); err = aead_set_nonce (dfx);
if (err) if (err)
goto leave; goto leave;
@ -771,7 +807,7 @@ aead_underflow (decode_filter_ctx_t dfx, iobuf_t a, byte *buf, size_t *ret_len)
if (err) if (err)
goto leave; goto leave;
gcry_cipher_final (dfx->cipher_hd); gcry_cipher_final (dfx->cipher_hd);
/* decrypt an empty string. */ /* Decrypt an empty string. */
err = gcry_cipher_decrypt (dfx->cipher_hd, dfx->holdback, 0, NULL, 0); err = gcry_cipher_decrypt (dfx->cipher_hd, dfx->holdback, 0, NULL, 0);
if (err) if (err)
{ {
@ -779,8 +815,10 @@ aead_underflow (decode_filter_ctx_t dfx, iobuf_t a, byte *buf, size_t *ret_len)
gpg_strerror (err)); gpg_strerror (err));
goto leave; goto leave;
} }
/* log_printhex (dfx->holdback+16, 16, "tag:"); */ if (DBG_CRYPTO)
err = gcry_cipher_checktag (dfx->cipher_hd, dfx->holdback+16, 16); log_printhex (dfx->holdback+(last_chunk_done?0:16), 16, "tag:");
err = gcry_cipher_checktag (dfx->cipher_hd,
dfx->holdback+(last_chunk_done?0:16), 16);
if (err) if (err)
{ {
if (DBG_FILTER) if (DBG_FILTER)
@ -788,6 +826,8 @@ aead_underflow (decode_filter_ctx_t dfx, iobuf_t a, byte *buf, size_t *ret_len)
gpg_strerror (err)); gpg_strerror (err));
goto leave; goto leave;
} }
if (DBG_FILTER)
log_debug ("final tag is valid\n");
err = gpg_error (GPG_ERR_EOF); err = gpg_error (GPG_ERR_EOF);
} }