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

[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

      From: The Tester <prwh1x4 at yahoo.com.au>
 To: "rt at openssl.org" <rt at openssl.org> 
Cc: "openssl-dev at openssl.org" <openssl-dev at openssl.org> 
 Sent: Tuesday, 9 December 2014, 15:35
 Subject: Re: [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 <rt at openssl.org>
 To: prwh1x4 at yahoo.com.au 
Cc: openssl-dev at 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?).