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

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Rich Salz via RT

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)
==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 0x63B619F: _IO_file_xsputn@@GLIBC_2.2.5 (fileops.c:1302)
==11437==    by 0x63878B4: vfprintf (vfprintf.c:1661)
==11437==    by 0x6390388: printf (printf.c:33)
==11437==    by 0x40DC22: was_main (AesDemo.c:114)
==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== Syscall param write(buf) points to uninitialised byte(s)
==11437==    at 0x64282F0: __write_nocancel (syscall-template.S:81)
==11437==    by 0x63B5A82: _IO_file_write@@GLIBC_2.2.5 (fileops.c:1261)
==11437==    by 0x63B6F5B: _IO_do_write@@GLIBC_2.2.5 (fileops.c:538)
==11437==    by 0x63B6120: _IO_file_xsputn@@GLIBC_2.2.5 (fileops.c:1332)
==11437==    by 0x63863D9: vfprintf (vfprintf.c:1692)
==11437==    by 0x6390388: printf (printf.c:33)
==11437==    by 0x40DC22: was_main (AesDemo.c:114)
==11437==    by 0x40DCCA: AesDemo (AesDemo.c:125)
==11437==    by 0x40EEDD: main (OpenSslAesTest.cpp:225)
==11437==  Address 0x4025014 is not stack'd, malloc'd or (recently) free'd
==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==
==11437== HEAP SUMMARY:
==11437==     in use at exit: 0 bytes in 0 blocks
==11437==   total heap usage: 17 allocs, 17 frees, 1,681 bytes allocated
==11437==
==11437== All heap blocks were freed -- no leaks are possible
==11437==
==11437== For counts of detected and suppressed errors, rerun with: -v
==11437== ERROR SUMMARY: 161 errors from 9 contexts (suppressed: 0 from 0)


Originally I started with this installed (latest auto-updated Ubuntu desktop)
$ openssl version -a
OpenSSL 1.0.1f 6 Jan 2014
built on: Wed Oct 15 17:43:26 UTC 2014
platform: debian-amd64
options:  bn(64,64) rc4(16x,int) des(idx,cisc,16,int) blowfish(idx)
compiler: cc -fPIC -DOPENSSL_PIC -DOPENSSL_THREADS -D_REENTRANT -DDSO_DLFCN -DHAVE_DLFCN_H -m64 -DL_ENDIAN -DTERMIO -g -O2 -fstack-protector --param=ssp-buffer-size=4 -Wformat -Werror=format-security -D_FORTIFY_SOURCE=2 -Wl,-Bsymbolic-functions -Wl,-z,relro -Wa,--noexecstack -Wall -DMD32_REG_T=int -DOPENSSL_IA32_SSE2 -DOPENSSL_BN_ASM_MONT -DOPENSSL_BN_ASM_MONT5 -DOPENSSL_BN_ASM_GF2m -DSHA1_ASM -DSHA256_ASM -DSHA512_ASM -DMD5_ASM -DAES_ASM -DVPAES_ASM -DBSAES_ASM -DWHIRLPOOL_ASM -DGHASH_ASM
OPENSSLDIR: "/usr/lib/ssl"

I have downloaded 1.0.1j but I am struggling to get Ubuntu/netbeans to build with the new version as OpenSSL is quite closely tied in to the ubuntu core. So while the command line tools are updated:

$ openssl version -a
OpenSSL 1.0.1j 15 Oct 2014
built on: Thu Dec  4 14:57:03 AEDT 2014
platform: linux-x86_64
options:  bn(64,64) rc4(16x,int) des(idx,cisc,16,int) idea(int) blowfish(idx)
compiler: gcc -DOPENSSL_THREADS -D_REENTRANT -DDSO_DLFCN -DHAVE_DLFCN_H -Wa,--noexecstack -m64 -DL_ENDIAN -DTERMIO -O3 -Wall -DOPENSSL_IA32_SSE2 -DOPENSSL_BN_ASM_MONT -DOPENSSL_BN_ASM_MONT5 -DOPENSSL_BN_ASM_GF2m -DSHA1_ASM -DSHA256_ASM -DSHA512_ASM -DMD5_ASM -DAES_ASM -DVPAES_ASM -DBSAES_ASM -DWHIRLPOOL_ASM -DGHASH_ASM
OPENSSLDIR: "/usr/local/ssl"


I can't get my program to compile with the new library. Tweaking the library and linker paths gives me compile errors. but I'm working on that. In the meantime, if you could resist the temptation to simply reply "use the latest version" that would be helpful. Perhaps suggestions on how to force gcc to use the right paths.

thanks
Chris



______________________________________________________________________
OpenSSL Project                                 http://www.openssl.org
Development Mailing List                       [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

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

Rich Salz via RT
> 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 Project                                 http://www.openssl.org
Development Mailing List                       [hidden email]
Automated List Manager                           [hidden email]
Reply | Threaded
Open this post in threaded view
|

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

Rich Salz via RT
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 <[hidden email]>
 To: [hidden email]
Cc: [hidden email]
 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
[hidden email]
https://mta.opensslfoundation.net/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

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

Rich Salz via RT
In reply to this post by Rich Salz 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 <[hidden email]>
 To: "[hidden email]" <[hidden email]>
Cc: "[hidden email]" <[hidden email]>
 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 <[hidden email]>
 To: [hidden email]
Cc: [hidden email]
 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
[hidden email]
https://mta.opensslfoundation.net/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

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

Rich Salz via RT
In reply to this post by Rich Salz via RT
> 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
[hidden email]
https://mta.opensslfoundation.net/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

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

Rich Salz 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 <[hidden email]>
 To: [hidden email]
Cc: [hidden email]
 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
[hidden email]
https://mta.opensslfoundation.net/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

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

Rich Salz 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")

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-dev mailing list
[hidden email]
https://mta.opensslfoundation.net/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

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

Tomas Mraz-2
On St, 2014-12-10 at 18:35 +0100, Andy Polyakov via RT wrote:

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

As the Fedora OpenSSL maintainer I would say it is not worth fixing in
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.)


_______________________________________________
openssl-dev mailing list
[hidden email]
https://mta.opensslfoundation.net/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

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

Rich Salz via RT
On St, 2014-12-10 at 18:35 +0100, Andy Polyakov via RT wrote:

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

As the Fedora OpenSSL maintainer I would say it is not worth fixing in
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.)



_______________________________________________
openssl-dev mailing list
[hidden email]
https://mta.opensslfoundation.net/mailman/listinfo/openssl-dev