Re: [openssl-dev] [openssl.org #3622] bug: crypto, valgrind reports improper memory access with AES128 cbc and longer plaintext
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) - 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: prwh...@yahoo.com.au Cc: openssl-dev@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 The demo program actually allocates a whole extra block for the output, so there should always be extra space available. Yes, which is why I said as for alleged buffer overruns in *your* 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 Excellent! If you would have clarified this from beginning, we wouldn't 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) ... OK. Keyword here is that it's 1.0.1 (I was testing against development 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. ___ openssl-dev mailing list openssl-dev@openssl.org https://mta.opensslfoundation.net/mailman/listinfo/openssl-dev
Re: [openssl-dev] [openssl.org #3622] bug: crypto, valgrind reports improper memory access with AES128 cbc and longer plaintext
Thanks for the response, Andy, it's good to know that the demo program does actually work for someone. Sorry for the delay, I'm kinda busy with other things right now. Also, I realised the link was truncated, but it looks as though you found the demo anyway. https://github.com/saju/misc/blob/master/misc/openssl_aes.c 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: prwh...@yahoo.com.au Cc: openssl-dev@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
[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.
From that demo:
const char *input[] = {
a,
abcd,
this is a test,
this is a bigger test,
// \nWho are you ?\nI am the 'Doctor'.\n'Doctor' who ?\nPrecisely!,
qwertyuiopasdfghjkl;zxcvbnm,/.';][=-0987654321`QWERTYUIOP[ASDFRGTHYJULO;PZXCVBNM,./qwertyuiopasdfghjkl;zxcvbnm,/.';][=-0987654321`QWERTYUIOP[ASDFRGTHYJULO;PZXCVBNM,./qwertyuiopasdfghjkl;zxcvbnm,/.';][=-0987654321`QWERTYUIOP[ASDFRGTHYJULO;PZXCVBNM,./qwertyuiopasdfghjkl;zxcvbnm,/.';][=-0987654321`QWERTYUIOP[ASDFRGTHYJULO;PZXCVBNM,./,
NULL};...
printf(Using OpenSSL version \%s\\n, SSLeay_version(SSLEAY_VERSION));
I added the qwerty.. string and the version output.
Using OpenSSL version OpenSSL 1.0.1f 6 Jan 2014
OK: enc/dec ok for a
OK: enc/dec ok for abcd
OK: enc/dec ok for this is a test
OK: enc/dec ok for this is a bigger test
OK: enc/dec ok for
qwertyuiopasdfghjkl;zxcvbnm,/.';][=-0987654321`QWERTYUIOP[ASDFRGTHYJULO;PZXCVBNM,./qwertyuiopasdfghjkl;zxcvbnm,/.';][=-0987654321`QWERTYUIOP[ASDFRGTHYJULO;PZXCVBNM,./qwertyuiopasdfghjkl;zxcvbnm,/.';][=-0987654321`QWERTYUIOP[ASDFRGTHYJULO;PZXCVBNM,./qwertyuiopasdfghjkl;zxcvbnm,/.';][=-0987654321`QWERTYUIOP[ASDFRGTHYJULO;PZXCVBNM,./
But if I use Valgrind I get this log file:
valgrind --track-origins=yes --leak-check=full --show-reachable=yes
--log-file=valgrind.log ./demo
==11437== Memcheck, a memory error detector
==11437== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==11437== Using Valgrind-3.10.0.SVN and LibVEX; rerun with -h for copyright info
==11437== Command: ./demo
==11437== Parent PID: 6494
==11437==
==11437== Conditional jump or move depends on uninitialised value(s)
==11437== at 0x4C2E8CE: strncmp (vg_replace_strmem.c:569)
==11437== by 0x40DBE1: was_main (AesDemo.c:111)
==11437== by 0x40DCCA: AesDemo (AesDemo.c:125)
==11437== by 0x40EEDD: main (OpenSslAesTest.cpp:225)
==11437== Uninitialised value was created by a stack allocation
==11437== at 0x4EBADF7: ??? (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==11437==
==11437== Conditional jump or move depends on uninitialised value(s)
==11437== at 0x4C2E8D9: strncmp (vg_replace_strmem.c:569)
==11437== by 0x40DBE1: was_main (AesDemo.c:111)
==11437== by 0x40DCCA: AesDemo (AesDemo.c:125)
==11437== by 0x40EEDD: main (OpenSslAesTest.cpp:225)
==11437== Uninitialised value was created by a stack allocation
==11437== at 0x4EBADF7: ??? (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==11437==
==11437== Conditional jump or move depends on uninitialised value(s)
==11437== at 0x4C2E8DB: strncmp (vg_replace_strmem.c:569)
==11437== by 0x40DBE1: was_main (AesDemo.c:111)
==11437== by 0x40DCCA: AesDemo (AesDemo.c:125)
==11437== by 0x40EEDD: main (OpenSslAesTest.cpp:225)
==11437== Uninitialised value was created by a stack allocation
==11437== at 0x4EBADF7: ??? (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==11437==
==11437== Conditional jump or move depends on uninitialised value(s)
==11437== at 0x4C2E907: strncmp (vg_replace_strmem.c:569)
==11437== by 0x40DBE1: was_main (AesDemo.c:111)
==11437== by 0x40DCCA: AesDemo (AesDemo.c:125)
==11437== by 0x40EEDD: main (OpenSslAesTest.cpp:225)
==11437== Uninitialised value was created by a stack allocation
==11437== at 0x4EBADF7: ??? (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==11437==
==11437== Conditional jump or move depends on uninitialised value(s)
==11437== at 0x4C2E8F1: strncmp (vg_replace_strmem.c:569)
==11437== by 0x40DBE1: was_main (AesDemo.c:111)
==11437== by 0x40DCCA: AesDemo (AesDemo.c:125)
==11437== by 0x40EEDD: main (OpenSslAesTest.cpp:225)
==11437== Uninitialised value was created by a stack allocation
==11437== at 0x4EBADF7: ??? (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==11437==
==11437== Conditional jump or move depends on uninitialised value(s)
==11437== at 0x4C2E8F3: strncmp (vg_replace_strmem.c:569)
==11437== by 0x40DBE1: was_main (AesDemo.c:111)
==11437== by 0x40DCCA: AesDemo (AesDemo.c:125)
==11437== by 0x40EEDD: main (OpenSslAesTest.cpp:225)
==11437== Uninitialised value was created by a stack allocation
==11437== at 0x4EBADF7: ??? (in /lib/x86_64-linux-gnu/libcrypto.so.1.0.0)
==11437==
==11437== Conditional jump or move depends on uninitialised value(s)
==11437== at 0x63878F3: vfprintf (vfprintf.c:1661)
==11437== by 0x6390388: printf (printf.c:33)
==11437== by 0x40DC22: was_main (AesDemo.c:114)
