From 36a4a67b2b016b5918c06b8c001cdb94c387c0e2 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Bodo=20M=C3=B6ller?= Date: Sun, 14 Sep 2008 13:42:40 +0000 Subject: [PATCH] Some precautions to avoid potential security-relevant problems. --- CHANGES | 32 ++++++++ crypto/bn/bn_div.c | 15 +++- crypto/bn/bn_nist.c | 183 +++++++++++++++++++++++++++++++------------ crypto/md32_common.h | 2 +- ssl/ssl_asn1.c | 2 +- 5 files changed, 181 insertions(+), 53 deletions(-) diff --git a/CHANGES b/CHANGES index 8f8b369158..443c3d84c0 100644 --- a/CHANGES +++ b/CHANGES @@ -4,6 +4,22 @@ Changes between 0.9.8h and 0.9.8i [xx XXX xxxx] + *) Various precautionary measures: + + - Avoid size_t integer overflow in HASH_UPDATE (md32_common.h). + + - Avoid a buffer overflow in d2i_SSL_SESSION() (ssl_asn1.c). + (NB: This would require knowledge of the secret session ticket key + to exploit, in which case you'd be SOL either way.) + + - Change bn_nist.c so that it will properly handle input BIGNUMs + outside the expected range. + + - Enforce the 'num' check in BN_div() (bn_div.c) for non-BN_DEBUG + builds. + + [Neel Mehta, Bodo Moeller] + *) Add support for Local Machine Keyset attribute in PKCS#12 files. [Steve Henson] @@ -28,6 +44,22 @@ Changes between 0.9.8g and 0.9.8h [28 May 2008] + *) Various precautionary measures: + + - Avoid size_t integer overflow in HASH_UPDATE (md32_common.h). + + - Avoid a buffer overflow in d2i_SSL_SESSION() (ssl_asn1.c). + (NB: This would require knowledge of the secret session ticket key + to exploit, in which case you'd be SOL either way.) + + - Change bn_nist.c so that it will properly handle input BIGNUMs + outside the expected range. + + - Enforce the 'num' check in BN_div() (bn_div.c) for non-BN_DEBUG + builds. + + [Neel Mehta, Bodo Moeller] + *) Fix flaw if 'Server Key exchange message' is omitted from a TLS handshake which could lead to a cilent crash as found using the Codenomicon TLS test suite (CVE-2008-1672) diff --git a/crypto/bn/bn_div.c b/crypto/bn/bn_div.c index 8655eb118e..1e8e57626b 100644 --- a/crypto/bn/bn_div.c +++ b/crypto/bn/bn_div.c @@ -187,6 +187,17 @@ int BN_div(BIGNUM *dv, BIGNUM *rm, const BIGNUM *num, const BIGNUM *divisor, BN_ULONG d0,d1; int num_n,div_n; + /* Invalid zero-padding would have particularly bad consequences + * in the case of 'num', so don't just rely on bn_check_top() for this one + * (bn_check_top() works only for BN_DEBUG builds) */ + if (num->top > 0 && num->d[num->top - 1] == 0) + { + BNerr(BN_F_BN_DIV,BN_R_NOT_INITIALIZED); + return 0; + } + + bn_check_top(num); + if ((BN_get_flags(num, BN_FLG_CONSTTIME) != 0) || (BN_get_flags(divisor, BN_FLG_CONSTTIME) != 0)) { return BN_div_no_branch(dv, rm, num, divisor, ctx); @@ -194,7 +205,7 @@ int BN_div(BIGNUM *dv, BIGNUM *rm, const BIGNUM *num, const BIGNUM *divisor, bn_check_top(dv); bn_check_top(rm); - bn_check_top(num); + /* bn_check_top(num); */ /* 'num' has been checked already */ bn_check_top(divisor); if (BN_is_zero(divisor)) @@ -419,7 +430,7 @@ static int BN_div_no_branch(BIGNUM *dv, BIGNUM *rm, const BIGNUM *num, bn_check_top(dv); bn_check_top(rm); - bn_check_top(num); + /* bn_check_top(num); */ /* 'num' has been checked in BN_div() */ bn_check_top(divisor); if (BN_is_zero(divisor)) diff --git a/crypto/bn/bn_nist.c b/crypto/bn/bn_nist.c index e14232fdbb..e7c3d6bac2 100644 --- a/crypto/bn/bn_nist.c +++ b/crypto/bn/bn_nist.c @@ -59,6 +59,7 @@ #include "bn_lcl.h" #include "cryptlib.h" + #define BN_NIST_192_TOP (192+BN_BITS2-1)/BN_BITS2 #define BN_NIST_224_TOP (224+BN_BITS2-1)/BN_BITS2 #define BN_NIST_256_TOP (256+BN_BITS2-1)/BN_BITS2 @@ -101,60 +102,98 @@ static const BN_ULONG _nist_p_521[] = {0xFFFFFFFF,0xFFFFFFFF,0xFFFFFFFF, 0xFFFFFFFF,0x000001FF}; #endif + +static const BIGNUM _bignum_nist_p_192 = + { + (BN_ULONG *)_nist_p_192, + BN_NIST_192_TOP, + BN_NIST_192_TOP, + 0, + BN_FLG_STATIC_DATA + }; + +static const BIGNUM _bignum_nist_p_224 = + { + (BN_ULONG *)_nist_p_224, + BN_NIST_224_TOP, + BN_NIST_224_TOP, + 0, + BN_FLG_STATIC_DATA + }; + +static const BIGNUM _bignum_nist_p_256 = + { + (BN_ULONG *)_nist_p_256, + BN_NIST_256_TOP, + BN_NIST_256_TOP, + 0, + BN_FLG_STATIC_DATA + }; + +static const BIGNUM _bignum_nist_p_384 = + { + (BN_ULONG *)_nist_p_384, + BN_NIST_384_TOP, + BN_NIST_384_TOP, + 0, + BN_FLG_STATIC_DATA + }; + +static const BIGNUM _bignum_nist_p_521 = + { + (BN_ULONG *)_nist_p_521, + BN_NIST_521_TOP, + BN_NIST_521_TOP, + 0, + BN_FLG_STATIC_DATA + }; + + const BIGNUM *BN_get0_nist_prime_192(void) { - static BIGNUM const_nist_192 = { (BN_ULONG *)_nist_p_192, - BN_NIST_192_TOP, BN_NIST_192_TOP, 0, BN_FLG_STATIC_DATA }; - return &const_nist_192; + return &_bignum_nist_p_192; } const BIGNUM *BN_get0_nist_prime_224(void) { - static BIGNUM const_nist_224 = { (BN_ULONG *)_nist_p_224, - BN_NIST_224_TOP, BN_NIST_224_TOP, 0, BN_FLG_STATIC_DATA }; - return &const_nist_224; + return &_bignum_nist_p_224; } const BIGNUM *BN_get0_nist_prime_256(void) { - static BIGNUM const_nist_256 = { (BN_ULONG *)_nist_p_256, - BN_NIST_256_TOP, BN_NIST_256_TOP, 0, BN_FLG_STATIC_DATA }; - return &const_nist_256; + return &_bignum_nist_p_256; } const BIGNUM *BN_get0_nist_prime_384(void) { - static BIGNUM const_nist_384 = { (BN_ULONG *)_nist_p_384, - BN_NIST_384_TOP, BN_NIST_384_TOP, 0, BN_FLG_STATIC_DATA }; - return &const_nist_384; + return &_bignum_nist_p_384; } const BIGNUM *BN_get0_nist_prime_521(void) { - static BIGNUM const_nist_521 = { (BN_ULONG *)_nist_p_521, - BN_NIST_521_TOP, BN_NIST_521_TOP, 0, BN_FLG_STATIC_DATA }; - return &const_nist_521; + return &_bignum_nist_p_521; } -#define BN_NIST_ADD_ONE(a) while (!(*(a)=(*(a)+1)&BN_MASK2)) ++(a); static void nist_cp_bn_0(BN_ULONG *buf, BN_ULONG *a, int top, int max) - { + { int i; - BN_ULONG *_tmp1 = (buf), *_tmp2 = (a); - for (i = (top); i != 0; i--) - *_tmp1++ = *_tmp2++; - for (i = (max) - (top); i != 0; i--) - *_tmp1++ = (BN_ULONG) 0; - } + BN_ULONG *_tmp1 = (buf), *_tmp2 = (a); + + OPENSSL_assert(top <= max); + for (i = (top); i != 0; i--) + *_tmp1++ = *_tmp2++; + for (i = (max) - (top); i != 0; i--) + *_tmp1++ = (BN_ULONG) 0; + } static void nist_cp_bn(BN_ULONG *buf, BN_ULONG *a, int top) - { + { int i; - BN_ULONG *_tmp1 = (buf), *_tmp2 = (a); - for (i = (top); i != 0; i--) - *_tmp1++ = *_tmp2++; - } + BN_ULONG *_tmp1 = (buf), *_tmp2 = (a); + for (i = (top); i != 0; i--) + *_tmp1++ = *_tmp2++; + } #if BN_BITS2 == 64 #define bn_cp_64(to, n, from, m) (to)[n] = (m>=0)?((from)[m]):0; @@ -199,6 +238,11 @@ int BN_nist_mod_192(BIGNUM *r, const BIGNUM *a, const BIGNUM *field, *res; size_t mask; + field = &_bignum_nist_p_192; /* just to make sure */ + + if (BN_is_negative(a) || a->top > 2*BN_NIST_192_TOP) + return BN_nnmod(r, field, a, ctx); + i = BN_ucmp(field, a); if (i == 0) { @@ -208,9 +252,6 @@ int BN_nist_mod_192(BIGNUM *r, const BIGNUM *a, const BIGNUM *field, else if (i > 0) return (r == a) ? 1 : (BN_copy(r ,a) != NULL); - if (top == BN_NIST_192_TOP) - return BN_usub(r, a, field); - if (r != a) { if (!bn_wexpand(r, BN_NIST_192_TOP)) @@ -245,6 +286,11 @@ int BN_nist_mod_192(BIGNUM *r, const BIGNUM *a, const BIGNUM *field, r->top = BN_NIST_192_TOP; bn_correct_top(r); + if (BN_ucmp(field, r) <= 0) + { + if (!BN_usub(r, r, field)) return 0; + } + return 1; } @@ -272,6 +318,11 @@ int BN_nist_mod_224(BIGNUM *r, const BIGNUM *a, const BIGNUM *field, *res; size_t mask; + field = &_bignum_nist_p_224; /* just to make sure */ + + if (BN_is_negative(a) || a->top > 2*BN_NIST_224_TOP) + return BN_nnmod(r, field, a, ctx); + i = BN_ucmp(field, a); if (i == 0) { @@ -281,9 +332,6 @@ int BN_nist_mod_224(BIGNUM *r, const BIGNUM *a, const BIGNUM *field, else if (i > 0) return (r == a)? 1 : (BN_copy(r ,a) != NULL); - if (top == BN_NIST_224_TOP) - return BN_usub(r, a, field); - if (r != a) { if (!bn_wexpand(r, BN_NIST_224_TOP)) @@ -333,6 +381,11 @@ int BN_nist_mod_224(BIGNUM *r, const BIGNUM *a, const BIGNUM *field, r->top = BN_NIST_224_TOP; bn_correct_top(r); + if (BN_ucmp(field, r) <= 0) + { + if (!BN_usub(r, r, field)) return 0; + } + return 1; #else /* BN_BITS!=32 */ return 0; @@ -364,6 +417,11 @@ int BN_nist_mod_256(BIGNUM *r, const BIGNUM *a, const BIGNUM *field, *res; size_t mask; + field = &_bignum_nist_p_256; /* just to make sure */ + + if (BN_is_negative(a) || a->top > 2*BN_NIST_256_TOP) + return BN_nnmod(r, field, a, ctx); + i = BN_ucmp(field, a); if (i == 0) { @@ -373,9 +431,6 @@ int BN_nist_mod_256(BIGNUM *r, const BIGNUM *a, const BIGNUM *field, else if (i > 0) return (r == a)? 1 : (BN_copy(r ,a) != NULL); - if (top == BN_NIST_256_TOP) - return BN_usub(r, a, field); - if (r != a) { if (!bn_wexpand(r, BN_NIST_256_TOP)) @@ -470,6 +525,11 @@ int BN_nist_mod_256(BIGNUM *r, const BIGNUM *a, const BIGNUM *field, r->top = BN_NIST_256_TOP; bn_correct_top(r); + if (BN_ucmp(field, r) <= 0) + { + if (!BN_usub(r, r, field)) return 0; + } + return 1; #else /* BN_BITS!=32 */ return 0; @@ -505,6 +565,11 @@ int BN_nist_mod_384(BIGNUM *r, const BIGNUM *a, const BIGNUM *field, *res; size_t mask; + field = &_bignum_nist_p_384; /* just to make sure */ + + if (BN_is_negative(a) || a->top > 2*BN_NIST_384_TOP) + return BN_nnmod(r, field, a, ctx); + i = BN_ucmp(field, a); if (i == 0) { @@ -514,9 +579,6 @@ int BN_nist_mod_384(BIGNUM *r, const BIGNUM *a, const BIGNUM *field, else if (i > 0) return (r == a)? 1 : (BN_copy(r ,a) != NULL); - if (top == BN_NIST_384_TOP) - return BN_usub(r, a, field); - if (r != a) { if (!bn_wexpand(r, BN_NIST_384_TOP)) @@ -631,6 +693,11 @@ int BN_nist_mod_384(BIGNUM *r, const BIGNUM *a, const BIGNUM *field, r->top = BN_NIST_384_TOP; bn_correct_top(r); + if (BN_ucmp(field, r) <= 0) + { + if (!BN_usub(r, r, field)) return 0; + } + return 1; #else /* BN_BITS!=32 */ return 0; @@ -649,11 +716,33 @@ int BN_nist_mod_521(BIGNUM *r, const BIGNUM *a, const BIGNUM *field, BN_ULONG *r_d; BIGNUM *tmp; + field = &_bignum_nist_p_521; /* just to make sure */ + + if (BN_is_negative(a)) + return BN_nnmod(r, field, a, ctx); + /* check whether a reduction is necessary */ top = a->top; if (top < BN_NIST_521_TOP || ( top == BN_NIST_521_TOP && - (!(a->d[BN_NIST_521_TOP-1] & ~(BN_NIST_521_TOP_MASK))))) - return (r == a)? 1 : (BN_copy(r ,a) != NULL); + (!(a->d[BN_NIST_521_TOP-1] & ~(BN_NIST_521_TOP_MASK))))) + { + int i = BN_ucmp(field, a); + if (i == 0) + { + BN_zero(r); + return 1; + } + else + { +#ifdef BN_DEBUG + OPENSSL_assert(i > 0); /* because 'field' is 1111...1111 */ +#endif + return (r == a)? 1 : (BN_copy(r ,a) != NULL); + } + } + + if (BN_num_bits(a) > 2*521) + return BN_nnmod(r, field, a, ctx); BN_CTX_start(ctx); tmp = BN_CTX_get(ctx); @@ -673,15 +762,11 @@ int BN_nist_mod_521(BIGNUM *r, const BIGNUM *a, const BIGNUM *field, if (!BN_uadd(r, tmp, r)) goto err; - top = r->top; - r_d = r->d; - if (top == BN_NIST_521_TOP && - (r_d[BN_NIST_521_TOP-1] & ~(BN_NIST_521_TOP_MASK))) + + if (BN_ucmp(field, r) <= 0) { - BN_NIST_ADD_ONE(r_d) - r->d[BN_NIST_521_TOP-1] &= BN_NIST_521_TOP_MASK; + if (!BN_usub(r, r, field)) goto err; } - bn_correct_top(r); ret = 1; err: diff --git a/crypto/md32_common.h b/crypto/md32_common.h index 089c450290..61bcd9786f 100644 --- a/crypto/md32_common.h +++ b/crypto/md32_common.h @@ -301,7 +301,7 @@ int HASH_UPDATE (HASH_CTX *c, const void *data_, size_t len) { p=(unsigned char *)c->data; - if ((n+len) >= HASH_CBLOCK) + if (len >= HASH_CBLOCK || len+n >= HASH_CBLOCK) { memcpy (p+n,data,HASH_CBLOCK-n); HASH_BLOCK_DATA_ORDER (c,p,1); diff --git a/ssl/ssl_asn1.c b/ssl/ssl_asn1.c index 6e14f4d834..0f9a3489dd 100644 --- a/ssl/ssl_asn1.c +++ b/ssl/ssl_asn1.c @@ -353,7 +353,7 @@ SSL_SESSION *d2i_SSL_SESSION(SSL_SESSION **a, const unsigned char **pp, memcpy(ret->session_id,os.data,os.length); M_ASN1_D2I_get_x(ASN1_OCTET_STRING,osp,d2i_ASN1_OCTET_STRING); - if (ret->master_key_length > SSL_MAX_MASTER_KEY_LENGTH) + if (os.length > SSL_MAX_MASTER_KEY_LENGTH) ret->master_key_length=SSL_MAX_MASTER_KEY_LENGTH; else ret->master_key_length=os.length; -- 2.25.1