Hey folks,

I do not believe this fix works and introduces buffer overflows.

Looking at this change:
https://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=abdb460d8abe68fedcf00b52d2ba4bf4b7c1725c

It makes EVP_CipherUpdate write to out directly. While not unreasonable
(this BIO probably should never buffer more than a block and otherwise use
the buffer passed in), one must take care to never output more than outl
bytes. There are a number of problems here:

1. The logic to compute buf_len above rounds outl *up* to a multiple of
EVP_MAX_BLOCK_LENGTH. Thus, if outl > buf_len and BIO_read returns the full
buf_len bytes, EVP_CipherUpdate, may write up to buf_len bytes and exceed
outl.

2. If the EVP_CIPHER_CTX contains a partial block buffered, even if outl ==
buf_len, we may *still* output more than outl bytes. For instance, the BIO
may be connected to a pipe and BIO_read return less than buf_len. That will
feed a partial block into the EVP_CIPHER_CTX and, the next time around, we
output more data than expected.

3. Actually, #2 even means the EVP_CIPHER overlapping buffers check is
wrong. The true requirement is not "if the buffers alias, then in == out",
but "if the buffers alias, then out + ctx->num == in". EVP_CIPHER's block
cipher handling is a very leaky abstraction.

I was able to trigger a crash simply by chaining an encrypt BIO with a
memory BIO containing a large plaintext and then stream 100 bytes out of it
at a time. BIO_read would consistently return 128 and, by the time the
function returned, the stack was thoroughly clobbered.

#3 suggests a very minimal fix. Revert the direct output codepath (not
wrong, but I think the BIO needs a complete rewrite with a block-size
buffer for that sort of thing anyway). Then, instead of reading BUF_OFFSET
bytes in at the BIO_read call, read ctx->num bytes in. Then go and fix the
EVP_CIPHER overlap check so it handles the ctx->num != 0 case correctly.

NB: I'm unclear on what the BUF_OFFSET offset was originally for. The
comment just says to read the EVP_Cipher documentation, but there is no pod
file for EVP_Cipher. I am assuming it was to get around this partial block
problem, but perhaps I'm missing something and my suggestion is also wrong.

Hope this helps,

David

PS: Should the new codepath have been setting ctx->cont? The others do.
Though it looks like it might be a no-op when set to 1? I'm not sure.

PPS: This sort of streaming stuff is quite hairy. Like the poly1305
streaming bits, it would make sense to write some tests here. Get a long
plaintext/ciphertext vector or two (I'd test both a block cipher and a
stream cipher) and make sure everything behaves correctly against various
interesting read patterns. Both when the outl pattern varies and when the
inner BIO_read return pattern varies.

On Sun, Jul 31, 2016 at 1:53 PM Rich Salz via RT <r...@openssl.org> wrote:

> Resolved by Andy's fix. Closing.
>
> --
> Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4628
> Please log in as guest with password guest if prompted
>
> --
> openssl-dev mailing list
> To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
>

-- 
Ticket here: http://rt.openssl.org/Ticket/Display.html?id=4628
Please log in as guest with password guest if prompted

-- 
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

Reply via email to