On Mon, Dec 12, 2011 at 3:19 PM, Marshall Clow <[email protected]> wrote: > I've been testing out the LLVM static analysis tool > <http://clang-analyzer.llvm.org/> on various code bases, and it's lighting > up a particular construct used in OpenSSL. > > Let me state my position right up front: > I have no idea if this causes any problems in OpenSSL, but I don't think > this code is doing what the authors intended. > > > Example of what clang is whinging about (from rsa_eay.c): > > 547 BIGNUM local_d; > 548 BIGNUM *d = NULL; > 549 > 550 if (!(rsa->flags & RSA_FLAG_NO_CONSTTIME)) > 551 { > 552 d = &local_d; > 553 BN_with_flags(d, rsa->d, BN_FLG_CONSTTIME); > > What is says is: > Line 553: Left operand of '&' is a garbage value. > > Here's the relevant definition (from bn.h): > > 296 /* get a clone of a BIGNUM with changed flags, for *temporary* use only > 297 * (the two BIGNUMs cannot not be used in parallel!) */ > 298 #define BN_with_flags(dest,b,n) ((dest)->d=(b)->d, \ > 299 (dest)->top=(b)->top, \ > 300 (dest)->dmax=(b)->dmax, \ > 301 (dest)->neg=(b)->neg, \ > 302 (dest)->flags=(((dest)->flags & BN_FLG_MALLOCED) \ > 303 | ((b)->flags & ~BN_FLG_MALLOCED) \ > 304 | BN_FLG_STATIC_DATA \ > 305 | (n))) > > The problem is on line 302, which (I believe) intends to clear all the bits > in dest->flags except the BN_FLG_MALLOCED bit (0x01). But in the code in > rya_eay.c, that bit is uninitialized.
That's not quite what it does, is it? It preserves the original MALLOCED flag from dest and copies the remaining flags from b, ored with n, which makes sense. > > So, we end up with a temporary BIGNUM where the BN_FLG_MALLOCED flag is > garbage. > > Again - I do not know if this actually causes any problems. I guess it only causes problems if someone attempts to free the copy, which I presume they will not in this case. However, it is a bug. Yay for LLVM! > > The same construct occurs in: > bn_gcd.c line 571 > crypto/rsa/rsa_gen.c line 173 > crypto/rsa/rsa_lib.c line 417 > crypto/bn/bn_gcd.c line 546 > rsa_eay.c line 767 > > -- Marshall > > Marshall Clow Idio Software <mailto:[email protected]> > > A.D. 1517: Martin Luther nails his 95 Theses to the church door and is > promptly moderated down to (-1, Flamebait). > -- Yu Suzuki > ______________________________________________________________________ OpenSSL Project http://www.openssl.org Development Mailing List [email protected] Automated List Manager [email protected]
