I do not believe this fix works and introduces buffer overflows.
Looking at this change:
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
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,
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.