Re: [openssl-dev] [openssl.org #3622] bug: crypto, valgrind reports improper memory access with AES128 cbc and longer plaintext
The Tester via RT
The demo program actually allocates a whole extra block for the output, so there should always be extra space available. Switching to calloc instead of malloc does not hide the issue, which suggests that it's actually a problem somewhere in the bowels of OpenSSL, copying unassigned values into the output. Likewise, the demo program uses null-terminated strings because they're easy to see in operation. My real program uses manually padded, known-size binary packets but adding extra code to show that did not seem worth while. It would likely lead to a bunch of other questions about design decisions and other irrelevancies when the problem is that valgrind is unhappy about the way OpenSSL (appears to) work.
I've just re-tested, pasting the code in to both C and C++ netbeans projects (since that's what my main project uses) and fixing the C++ convert-from-const errors as well as adding aes.h. Both give the same valgrind issues for me. I'm using "gcc version 4.8.2 (Ubuntu 4.8.2-19ubuntu1)" and "valgrind-3.10.0.SVN" if that might make a difference.
Experimentation shows that the magic length is 96 bytes - strlen()=94 works fine on my machine, strlen()=95 produces the valgrind complaints. That means input length of 96, since the code uses strlen()+1. What's magic about a 96 byte input size? (other than being 6 AES128 blocks)
Since I have a new Fedora 20 virtual machine handy I have also run on that with the same result:Using OpenSSL version "OpenSSL 1.0.1e-fips 11 Feb 2013"
==2793== Using Valgrind-3.9.0 and LibVEX; rerun with -h for copyright info
...
==2793== Conditional jump or move depends on uninitialised value(s)
==2793== at 0x4C2A79E: strncmp (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==2793== by 0x400FA1: main (in /home/digidev/test/a.out)
==2793== Uninitialised value was created by a stack allocation
==2793== at 0x4EC0DB7: aesni_cbc_encrypt (in /usr/lib64/libcrypto.so.1.0.1e)
...
I hoped that the padding functions would mean that manually padding the inputs was not necessary. Admittedly in my real code I am doing manual padding to get control over the padding - the hardware I'm communicating with does not pad or accept padding on a plaintext that is an exact multiple of the block size where OpenSSL/PKCS does. But the demo uses auto padding, and I'd hoped that it would work.
aes_encrypt function has this:
/* update ciphertext with the final remaining bytes */
EVP_EncryptFinal_ex(e, ciphertext + encryptedLength, &paddingLength);
*len = encryptedLength + paddingLength;
Surely this means that the output is padded and therefore the input does not need to be a multiple of the block size. The program claims to work without manual padding, anyway.
As far as querying the block size, that has ramifications beyond my program so changing it would break compatibility with hardware we've already shipped (for example). All I could do if I queried was check against the hard-coded value and exit abruptly since my program will not work.
ThanksChris
From: Andy Polyakov via RT <r...@openssl.org>
To: prw...@yahoo.com.au
Cc: opens...@openssl.org
Sent: Saturday, 6 December 2014, 3:27
Subject: Re: [openssl.org #3622] bug: crypto, valgrind reports improper memory access with AES128 cbc and longer plaintext
> I started with an AES256 demo I found at https://github.com/saju/misc and modified the initialisations to use AES128. The test strings that program uses are quite short - less than 100 characters. If I add a significantly longer string to those test values Valgrind reports a string of what I suspect are buffer overruns. Note that I discovered this in my real code and this is a simple test case that seems to demonstrate the same problem. I also print the library version that the program is using.
I don't get any valgrind errors, not a single one. But then I had to add
-DAES_BLOCK_SIZE=16 at compiler command line, as program in question
failed to include <openssl/aes.h>. Well, I don't really want to say
"failed to include", because it implies that I'd suggest to do so, when
I don't actually mean to. Correct solution in real life would be to
query cipher block size with EVP_CIPHER_block_size, as opposite to
relying on cipher-specific header. It's just that I see no point in
fixing that program.
As for alleged buffer overruns in your program. You have to recognize
and remember that AES is a block cipher, which means that CBC encrypt
output and decrypt input lengths has to be divisible by block size.
[Ideally even encrypt input and decrypt output lengths should be
divisible, but EVP gives you some help by padding encrypt input.] In
other words, is it possible that you allocate buffer based in input
string length, which would actually be prone to overrun. Though I again
say things I don't want to say, namely implicitly suggest that it's OK
to encrypt null-terminated strings with CBC without application
controlled padding. Though, for completeness sake one can mention that
there are CBC variants suitable for this purpose, namely Cipher Text
Stealing modes, but these are not supported by OpenSSL (yet?).
_______________________________________________
openssl-dev mailing list
opens...@openssl.org
https://mta.opensslfoundation.net/mailman/listinfo/openssl-dev
The Tester via RT
Is there a better demo program I can use as the basis for my code? I'd be happy to redo my stuff based on anything that I can download and run without having to dig through the OpenSSL/test sets to try to find hints (which I've done, but there didn't seem to be a simple AES128/cbc test that I could find. The only matches from the string were in test binaries for ssl and evp).
And one apparently irrelevant error: the first version on Fedora below I forgot to change the demo code from ASE256 ot AES128... but the Valgrind complaints are the same when I make the change.
...
==2810== Conditional jump or move depends on uninitialised value(s)==2810== at 0x4C2A79E: strncmp (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==2810== by 0x400FF1: main (in /home/digidev/test/a.out)
==2810== Uninitialised value was created by a stack allocation
==2810== at 0x4EC0DB7: aesni_cbc_encrypt (in /usr/lib64/libcrypto.so.1.0.1e)
...
The advantage of the Fedora VM is that it's literally brand new out of the box - I downloaded the latest Fedora Desktop ISO, installed using the defaults, then ran through the "yum install..." iterations I needs to be able to reproduce the problem. I suspect that you could do that in one line:
yum install gpp gcc-c++ valgrind openssl openssl-devel
(I appreciate that this is ~500MB of download and an hour or so to install, but it does mean that you should be able to reproduce the problem if you're really desperate).
Chris
Andy Polyakov via RT
program". I mean you said "I discovered this [suspected buffer overrun]
in my real code" and as you didn't present it, I found it appropriate to
remind that strlen-based allocations are prone to overruns, and if it
would be case, it would be something for you to address.
> My real program uses manually padded, known-size binary packets
have this digression :-)
> I've just re-tested, pasting the code in to both C and C++ netbeans projects (since that's what my main project uses) and fixing the C++ convert-from-const errors as well as adding aes.h. Both give the same valgrind issues for me. I'm using "gcc version 4.8.2 (Ubuntu 4.8.2-19ubuntu1)" and "valgrind-3.10.0.SVN" if that might make a difference.
> Experimentation shows that the magic length is 96 bytes - strlen()=94 works fine on my machine, strlen()=95 produces the valgrind complaints. That means input length of 96, since the code uses strlen()+1. What's magic about a 96 byte input size? (other than being 6 AES128 blocks)
>
>
> Since I have a new Fedora 20 virtual machine handy I have also run on that with the same result:Using OpenSSL version "OpenSSL 1.0.1e-fips 11 Feb 2013"
> ==2793== Using Valgrind-3.9.0 and LibVEX; rerun with -h for copyright info
> ...
> ==2793== Conditional jump or move depends on uninitialised value(s)
> ==2793== at 0x4C2A79E: strncmp (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
> ==2793== by 0x400FA1: main (in /home/digidev/test/a.out)
> ==2793== Uninitialised value was created by a stack allocation
> ==2793== at 0x4EC0DB7: aesni_cbc_encrypt (in /usr/lib64/libcrypto.so.1.0.1e)
> ...
branch, master and 1.0.2). 1.0.1 is actually known to upset valgrind
(see RT#2862), but it looks more like valgrind bug. Well, it's somewhat
in between: one can argue that valgrind has formal right to complain,
but at the same time aesni_cbc_encrypt doesn't actually violate ABI
constrains. Latter means that result is always correct and code in
question doesn't actually overrun any buffers nor uses uninitialized
values. The reason for why I failed to initially reproduce it in master
and 1.0.2 is that module in question was updated after 1.0.1 release to
not rely on red zone (the thing valgrind is complaining about). But this
was done for reason other than appeasing valgrind.
In case you wonder why problem pops up with longer lengths. This is
because with shorter lengths it's possible to keep everything in
processor registers. And with longer length it has to spill one value to
stack.
The Tester via RT
- valgrind complaints about 1.0.1 OpenSLL are extremely unlikely to affect my program in operation (you will probably say "will not affect")
- when OpenSLL 1.0.2 eventually percolates through to Ubuntu and Fedora valgrind will stop complaining.
That's good, because I'm seeing a significant speed increase over CryptoPP in my test code and my program is currently CPU bound in the crypto code. Hopefully this will push the performance bottleneck somewhere else for a while :)
Thanks for your helpChris
From: Andy Polyakov via RT <r...@openssl.org>
To: prw...@yahoo.com.au
Cc: opens...@openssl.org
Sent: Tuesday, 9 December 2014, 21:02
Subject: Re: [openssl-dev] [openssl.org #3622] bug: crypto, valgrind reports improper memory access with AES128 cbc and longer plaintext
Andy Polyakov via RT
> - valgrind complaints about 1.0.1 OpenSLL are extremely unlikely to affect my program in operation (you will probably say "will not affect")
it's false positive.
> - when OpenSLL 1.0.2 eventually percolates through to Ubuntu and Fedora valgrind will stop complaining.
valgrind or OpenSSL. Even though I argue that it's valgrind bug, I'm
ready to assist in addressing the issue on OpenSSL side. In other words
try to report it to your favorite distro vendor. Refer to this ticket.
But for now, I'm dismissing the case.
Tomas Mraz
> > Excellent. My summary is:
> > - valgrind complaints about 1.0.1 OpenSLL are extremely unlikely to affect my program in operation (you will probably say "will not affect")
>
> Well, as there is suggestion of what I would say, I would only say that
> it's false positive.
>
> > - when OpenSLL 1.0.2 eventually percolates through to Ubuntu and Fedora valgrind will stop complaining.
>
> Another alternative is that they recognize it as bug worthy fixing,
> valgrind or OpenSSL. Even though I argue that it's valgrind bug, I'm
> ready to assist in addressing the issue on OpenSSL side. In other words
> try to report it to your favorite distro vendor. Refer to this ticket.
> But for now, I'm dismissing the case.
OpenSSL. We will rebase to 1.0.2 final in Fedora Rawhide once it is
released.
--
Tomas Mraz
No matter how far down the wrong road you've gone, turn back.
Turkish proverb
(You'll never know whether the road is wrong though.)