Re: [openssl-dev] [openssl.org #4628] EVP_f_cipher regression due to overlapping regions check

[David Benjamin via RT]

I may not have time to fully digest the change before the release date, but
I'm not sure this snippet quite works:

   if (ctx->read_start == ctx->read_end) { /* time to read more data */
   ctx->read_end = ctx->read_start = &(ctx->buf[BUF_OFFSET]);
   ctx->read_end += BIO_read(next, ctx->read_start, ENC_BLOCK_SIZE);
   }
   i = ctx->read_end - ctx->read_start;

   if (i <= 0) {

It's kind of an odd error-checking pattern and is only saved from undefined
behavior by BUF_OFFSET. (Is a custom BIO allowed to return -1,000,000 on
error or must it be -1? There are definitely some OpenSSL APIs which return
-2 expecting that the usual error-check patterns don't care.) Anyway, I
believe it gets stuck if non-blocking BIO causes BIO_read to fail on a
retryable error like EWOULDBLOCK and we try again. I see calls to
BIO_should_retry, so I gather this BIO is intended to work in front of a
non-blocking BIO.

Since the error path should only be reachable when BIO_read fails, maybe
move that inside the "read more data" codepath? Then you don't need pointer
tricks to avoid duplicating the code.

David

On Sun, Aug 21, 2016 at 5:57 PM Andy Polyakov via RT  wrote:

> There are two commits, one that addresses bio_enc problems and one
> adding test. Please double-check.
>
>
> --
> 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

Re: [openssl-dev] [openssl.org #4628] EVP_f_cipher regression due to overlapping regions check

[Greg Hudson via RT]

The krb5 PKINIT tests still pass.


-- 
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

Re: [openssl-dev] [openssl.org #4628] EVP_f_cipher regression due to overlapping regions check

[Andy Polyakov via RT]

There are two commits, one that addresses bio_enc problems and one
adding test. Please double-check.


-- 
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

Re: [openssl-dev] [openssl.org #4628] EVP_f_cipher regression due to overlapping regions check

[Michel via RT]

Hi,

As I obviously needed to improve my test program,
I am now encrypting and decrypting files trying all ciphers in all their
available modes.
( ChaCha20, AES-128, AES-192, AES-256, Blowfish, Cast5, Camellia-128,
Camellia-192, Camellia-256, IDEA, Seed, 3 Keys Triple-DES, 2 Keys Triple-DES
)
( Poly1305, OCB, GCM, OFB, CFB, CFB1, CFB8, CTR, CBC )

You are certainly already informed (as I believe it may be caused by the
same problem that David diagnosed) 
but I felt preferable to report that even with an output buffer larger than
the expected data size, 
I always got a heap corrupted, but *ONLY* when I use the CBC mode.

FWIW, here is the call stack :
[External Code] 
Test.exe!CRYPTO_free(void * str, const char * file, int line) Line 179  C
Test.exe!buffer_free(bio_st * a) Line 76C
Test.exe!BIO_free(bio_st * a) Line 72   C
Test.exe!OCrypto::IO::Free() Line 1204  C++
Test.exe!WinFile::Close() Line 388  C++
Test.exe!WinFile::~WinFile() Line 456   C++
Test.exe!LoadData(const char * sFullFileName) Line 186  C++
Test.exe!main(int argc, char * * argv) Line 154 C++
[External Code] 

HEAP CORRUPTION DETECTED

All ciphers look to work good in all other modes than CBC
(still using a buffer greater than needed).

Regards,

Michel.



-- 
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

Re: [openssl-dev] [openssl.org #4628] EVP_f_cipher regression due to overlapping regions check

[Michel via RT]

Hi David,
After checking you are obviously right.
Contrary to my belief, my internal buffer was always larger than the longest
line I read.
:-(
Sorry for the noise, but thanks David for the explanations.
It helps me to fix my software (even if I will keep some spare bytes for
some time)
;-(
 


-- 
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

Re: [openssl-dev] [openssl.org #4628] EVP_f_cipher regression due to overlapping regions check

[Michel]

Hi David,
After checking you are obviously right.
Contrary to my belief, my internal buffer was always larger than the longest
line I read.
:-(
Sorry for the noise, but thanks David for the explanations.
It helps me to fix my software (even if I will keep some spare bytes for
some time)
;-(
 

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

Re: [openssl-dev] [openssl.org #4628] EVP_f_cipher regression due to overlapping regions check

[David Benjamin via RT]

On Sun, Jul 31, 2016 at 6:18 PM Michel via RT  wrote:

> > 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.
>
> I am surprised. I should have been [un-?]lucky for once.
> FWIW, here is what I did :
> I have some files containing about 1 of variable length lines (range is
> from about 60 to 260 bytes).
> File size is about 900 Kb to 1.5 Mb.
> Files can be cleartext or encrypted (in this case they can be optionaly
> base64 encoded).
> So I have a software that chain as follow :
> File bio ->
> Base64 bio (opt) ->
> Cipher bio (opt) ->
> Memory bio.
>
> For my test I read and wrote each case using 2 different ciphers : aes-128
> and camelia-192.
> Everything looks good : no crash, no data lost or damaged, no memory leak
> and no stack overwritten.
>
> I certainly misunderstand something, but I will be happy to test again my
> use case if it can be of any help.
>

(I'm not entirely sure which direction the arrows are meant to be going.)

You need the read to not be a multiple of 16 to trigger the issue. (Well,
that's the simplest trigger. I also got decrypt BIOs to trigger with
outl=128 because EVP_DecryptUpdate always holds back the final block.) Also
if your code will secretly tolerate the cipher BIO returning more data than
outl, you won't notice. But the API contract of BIO_read is that it will
not return more than outl bytes of data.

Reads of size 100 into a buffer of size 100 will do the trick:

outl = 100 =>
buf_len = 128 =>
i = 128 (assuming we got a full read) =>
we ask EVP_CipherUpdate to decrypt 128 bytes =>
128 bytes of output into out =>
buffer overflow

David

-- 
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

Re: [openssl-dev] [openssl.org #4628] EVP_f_cipher regression due to overlapping regions check

[Michel via RT]

> 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.

I am surprised. I should have been [un-?]lucky for once.
FWIW, here is what I did :
I have some files containing about 1 of variable length lines (range is
from about 60 to 260 bytes).
File size is about 900 Kb to 1.5 Mb.
Files can be cleartext or encrypted (in this case they can be optionaly
base64 encoded).
So I have a software that chain as follow : 
File bio -> 
Base64 bio (opt) ->
Cipher bio (opt) ->
Memory bio.

For my test I read and wrote each case using 2 different ciphers : aes-128
and camelia-192.
Everything looks good : no crash, no data lost or damaged, no memory leak
and no stack overwritten.

I certainly misunderstand something, but I will be happy to test again my
use case if it can be of any help.

Regards,

Michel.


-- 
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

Re: [openssl-dev] [openssl.org #4628] EVP_f_cipher regression due to overlapping regions check

[Michel]

> 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.

I am surprised. I should have been [un-?]lucky for once.
FWIW, here is what I did :
I have some files containing about 1 of variable length lines (range is
from about 60 to 260 bytes).
File size is about 900 Kb to 1.5 Mb.
Files can be cleartext or encrypted (in this case they can be optionaly
base64 encoded).
So I have a software that chain as follow : 
File bio -> 
Base64 bio (opt) ->
Cipher bio (opt) ->
Memory bio.

For my test I read and wrote each case using 2 different ciphers : aes-128
and camelia-192.
Everything looks good : no crash, no data lost or damaged, no memory leak
and no stack overwritten.

I certainly misunderstand something, but I will be happy to test again my
use case if it can be of any help.

Regards,

Michel.

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

Re: [openssl-dev] [openssl.org #4628] EVP_f_cipher regression due to overlapping regions check

[David Benjamin via RT]

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  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

Re: [openssl-dev] [openssl.org #4628] EVP_f_cipher regression due to overlapping regions check

[Michel]

Not speaking for Greg, but for me, it is now working fine again.
Thanks Andy !

-Message d'origine-
De : openssl-dev [mailto:openssl-dev-boun...@openssl.org] De la part de Rich
Salz via RT
Envoyé : dimanche 31 juillet 2016 15:58
À : ghud...@mit.edu
Cc : openssl-dev@openssl.org
Objet : [openssl-dev] [openssl.org #4628] EVP_f_cipher regression due to
overlapping regions check

Does current master work? I think Andy checked in a fix.

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

Re: [openssl-dev] [openssl.org #4628] EVP_f_cipher regression due to overlapping regions check

[Michel via RT]

Not speaking for Greg, but for me, it is now working fine again.
Thanks Andy !

-Message d'origine-
De : openssl-dev [mailto:openssl-dev-boun...@openssl.org] De la part de Rich
Salz via RT
Envoyé : dimanche 31 juillet 2016 15:58
À : ghud...@mit.edu
Cc : openssl-dev@openssl.org
Objet : [openssl-dev] [openssl.org #4628] EVP_f_cipher regression due to
overlapping regions check

Does current master work? I think Andy checked in a fix.


-- 
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

Re: [openssl-dev] [openssl.org #4628] EVP_f_cipher regression due to overlapping regions check

[Andy Polyakov via RT]

> Does current master work? I think Andy checked in a fix.

Rich was few minutes ahead. Now it's committed. Provided test case was
verified to work. Thanks for report.




-- 
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