[openssl-dev] [openssl.org #3622] bug: crypto, valgrind reports improper memory access with AES128 cbc and longer plaintext

[The Tester via RT]

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 <rt at openssl.org>
 To: prwh1x4 at yahoo.com.au 
Cc: openssl-dev at 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.