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