[openssl.org #3415] Bug report: Uninitialized memory reads reported by valgrind for ECDSA signatures
Fix applied: https://git.openssl.org/gitweb/?p=openssl.git;a=commit;h=f8571ce82292ed340ed6302635f9bd6dfbc1043a Approach based on Rich Salz's suggestion. This seems to be the most pragmatic way forward, although it does have the disadvantage that this will mask any other future problems in the bn library due to incorrect unlitialised reads of this data. Matt __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: [openssl.org #3415] Bug report: Uninitialized memory reads reported by valgrind for ECDSA signatures
On 3 July 2014 23:04, Salz, Rich wrote: > Why not just have bn_expand_internal call memset? Exactly, this makes more sense. > > ; git diff bn_lib.c > diff --git a/crypto/bn/bn_lib.c b/crypto/bn/bn_lib.c > index b1e224b..86d1d37 100644 > --- a/crypto/bn/bn_lib.c > +++ b/crypto/bn/bn_lib.c > @@ -324,6 +324,9 @@ static BN_ULONG *bn_expand_internal(const BIGNUM *b, int > words) > BNerr(BN_F_BN_EXPAND_INTERNAL,ERR_R_MALLOC_FAILURE); > return(NULL); > } > +#ifdef PURIFY > + memset(a, 0, sizeof(BN_ULONG)*words); > +#endif > #if 1 > B=b->d; > /* Check if the previous number needs to be copied */ > ; > > -- > Principal Security Engineer > Akamai Technologies, Cambridge, MA > IM: rs...@jabber.me; Twitter: RichSalz > __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
Re: [openssl.org #3415] Bug report: Uninitialized memory reads reported by valgrind for ECDSA signatures
On Čt, 2014-07-03 at 23:47 +0200, Matt Caswell via RT wrote: > I've put together a fix (see below), but not pushed it because I was working > on > the assumption that if you had PURIFY defined then you wouldn't care about > constant time operation. I've since been told that possibly some distros > define > PURIFY in their production builds. Anyone know of an example where this is > used > in a production build? We use -DPURIFY in regular openssl packages in Red Hat Enterprise Linux and Fedora. -- 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 Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
RE: [openssl.org #3415] Bug report: Uninitialized memory reads reported by valgrind for ECDSA signatures
Why not just have bn_expand_internal call memset? ; git diff bn_lib.c diff --git a/crypto/bn/bn_lib.c b/crypto/bn/bn_lib.c index b1e224b..86d1d37 100644 --- a/crypto/bn/bn_lib.c +++ b/crypto/bn/bn_lib.c @@ -324,6 +324,9 @@ static BN_ULONG *bn_expand_internal(const BIGNUM *b, int words) BNerr(BN_F_BN_EXPAND_INTERNAL,ERR_R_MALLOC_FAILURE); return(NULL); } +#ifdef PURIFY + memset(a, 0, sizeof(BN_ULONG)*words); +#endif #if 1 B=b->d; /* Check if the previous number needs to be copied */ ; -- Principal Security Engineer Akamai Technologies, Cambridge, MA IM: rs...@jabber.me; Twitter: RichSalz
[openssl.org #3415] Bug report: Uninitialized memory reads reported by valgrind for ECDSA signatures
I've done some digging on this and its kind of interesting. What is happening is that the code is calling the BN_consttime_swap function. This takes a condition variable and two BIGNUMs a and b, and swaps the value of a and b over if the condition is set. Inside a BIGNUM structure there is a pointer to an array of ints "d", along with a value dmax which says how many ints have been allocated in the array and a value top which says how many of those are actually being used. The values in the array from top up to dmax-1 may not have been initialised. BN_consttime_swap ignores that and goes through the array values and operates on them anyway even if they haven't been initialised yet to ensure constant time operation. This doesn't actually matter because we're never actually going to *use* that data. I've put together a fix (see below), but not pushed it because I was working on the assumption that if you had PURIFY defined then you wouldn't care about constant time operation. I've since been told that possibly some distros define PURIFY in their production builds. Anyone know of an example where this is used in a production build? A work around might be to use a suppression file for valgrind...but obviously if you happen to use some other tool then that doesn't help, so I'm not sure about that. Alternatively I could try and initialise everything in constant timebut looking for good ideas on how to do that! :-) Hmmmneeds some more thought. Matt commit 51518506c10cde225d4eb7590b2bc4f0ea67c959 Author: Matt Caswell Date: Thu Jul 3 22:09:08 2014 +0100 Added PURIFY section to initialise variables in BN_consttime_swap to stop valgrind complaining. Assumes constant time is not important when PURIFY is defined. PR#3415 diff --git a/crypto/bn/bn_lib.c b/crypto/bn/bn_lib.c index b1e224b..3db3885 100644 --- a/crypto/bn/bn_lib.c +++ b/crypto/bn/bn_lib.c @@ -844,6 +844,19 @@ void BN_consttime_swap(BN_ULONG condition, BIGNUM *a, BIGNUM *b, int nwords) bn_wcheck_size(a, nwords); bn_wcheck_size(b, nwords); +#ifdef PURIFY + /* Valgrind complains because we process the whole array even + * if it's not initialised yet. This doesn't matter in this function - + * what's important is constant time operation (we're not actually + * going to use the data) + * It is assumed that when running with PURIFY defined we don't have to + * worry about constant time - hence just go through and initialise it + * all + */ + for (i=a->top; idmax; i++) a->d[i]=0; + for (i=b->top; idmax; i++) b->d[i]=0; +#endif + assert(a != b); assert((condition & (condition - 1)) == 0); assert(sizeof(BN_ULONG) >= sizeof(int)); __ OpenSSL Project http://www.openssl.org Development Mailing List openssl-dev@openssl.org Automated List Manager majord...@openssl.org
[openssl.org #3415] Bug report: Uninitialized memory reads reported by valgrind for ECDSA signatures
The OpenSSL FAQ says that with a -DPURIFY build no messages about uninitialized data should appear: https://www.openssl.org/support/faq.html#PROG14 "14. Why does Valgrind complain about the use of uninitialized data? When OpenSSL's PRNG routines are called to generate random numbers the supplied buffer contents are mixed into the entropy pool: so it technically does not matter whether the buffer is initialized at this point or not. Valgrind (and other test tools) will complain about this. When using Valgrind, make sure the OpenSSL library has been compiled with the PURIFY macro defined (-DPURIFY) to get rid of these warnings." The following test was done with Git commit id 802fdcda1ebc4241a8e02af0046ba2f5264f71f6 from the OpenSSL_1_0_2-stable branch on Linux Intel 64-bit. I added the following line to the "Configure" script (I want to compile with -DPURIFY, but without actually running the compiler under the "purify" command): "mypurify", "gcc:-g -DPURIFY -Wall::(unknown)::", Executed the following commands: $ ./Configure mypurify no-dso no-shared no-asm $ make depend $ make clean $ make $ make test Go to the "test" directory and execute the "ecdsatest" executable under valgrind: $ cd test $ valgrind ./ecdsatest ==31925== Memcheck, a memory error detector ==31925== Copyright (C) 2002-2009, and GNU GPL'd, by Julian Seward et al. ==31925== Using Valgrind-3.5.0 and LibVEX; rerun with -h for copyright info ==31925== Command: ./ecdsatest ==31925== some tests from X9.62: testing prime192v1: ok testing prime239v1: ==31925== Use of uninitialised value of size 8 ==31925==at 0x459E40: bn_GF2m_mul_1x1 (bn_gf2m.c:145) ==31925==by 0x45A030: bn_GF2m_mul_2x2 (bn_gf2m.c:215) ==31925==by 0x45A99A: BN_GF2m_mod_mul_arr (bn_gf2m.c:417) ==31925==by 0x438959: ec_GF2m_simple_field_mul (ec2_smpl.c:702) ==31925==by 0x438BC3: gf2m_Madd (ec2_mult.c:128) ==31925==by 0x439391: ec_GF2m_montgomery_point_multiply (ec2_mult.c:284) ==31925==by 0x439616: ec_GF2m_simple_mul (ec2_mult.c:355) ==31925==by 0x419B9D: EC_POINTs_mul (ec_lib.c:1057) ==31925==by 0x419C2C: EC_POINT_mul (ec_lib.c:1071) ==31925==by 0x41C9A9: EC_KEY_generate_key (ec_key.c:284) ==31925==by 0x401B32: x9_62_test_internal (ecdsatest.c:202) ==31925==by 0x401DF5: x9_62_tests (ecdsatest.c:266) ==31925== ==31925== Use of uninitialised value of size 8 ==31925==at 0x459E58: bn_GF2m_mul_1x1 (bn_gf2m.c:146) ==31925==by 0x45A030: bn_GF2m_mul_2x2 (bn_gf2m.c:215) ==31925==by 0x45A99A: BN_GF2m_mod_mul_arr (bn_gf2m.c:417) ==31925==by 0x438959: ec_GF2m_simple_field_mul (ec2_smpl.c:702) ==31925==by 0x438BC3: gf2m_Madd (ec2_mult.c:128) ==31925==by 0x439391: ec_GF2m_montgomery_point_multiply (ec2_mult.c:284) ==31925==by 0x439616: ec_GF2m_simple_mul (ec2_mult.c:355) ==31925==by 0x419B9D: EC_POINTs_mul (ec_lib.c:1057) ==31925==by 0x419C2C: EC_POINT_mul (ec_lib.c:1071) ==31925==by 0x41C9A9: EC_KEY_generate_key (ec_key.c:284) ==31925==by 0x401B32: x9_62_test_internal (ecdsatest.c:202) ==31925==by 0x401DF5: x9_62_tests (ecdsatest.c:266) ... and so on... The full report is to big to include it inline, I add it as a compressed attachment. -- Stephan ecdsatest.valgrind.txt.gz Description: GNU Zip compressed data
