As originally posted to users@ by Victor Duchovni @ 2009/02/11 Attached is this as a patch file against todays 0.9.9 CVS HEAD; patch includes assert() --> OPENSSL_assert() fixes as well.
On Wed, Feb 11, 2009 at 10:41 AM, Ger Hobbelt <g...@hobbelt.com> wrote: > Good find! This is indeed wrong (sk_*_find returns -1 when the item > couldn't be found). > > A grep + code inspection of HEAD 20090202 (sorry, haven't synced to > the VERY latest yet) reveals almost all code checks for '>= 0' or '< > 0' as it should. > Code inspection also pops up a few spots in v3_addr.c which lack 'no > hit' checks entirely --> fix: > > > v3_addr_subset(): > > --- \\Debbie\ger\prj\1original\openssl\openssl\crypto\x509v3\v3_addr.c > 2008-12-16 > 19:39:49.000000000 +-0100 > +++ \\Debbie\ger\prj\3actual\openssl\crypto\x509v3\v3_addr.c 2009-02-11 > 10:40:35.000000000 +-0100 > @@ -1123,14 +1136,21 @@ > if (b == NULL || v3_addr_inherits(a) || v3_addr_inherits(b)) > return 0; > sk_IPAddressFamily_set_cmp_func(b, IPAddressFamily_cmp); > for (i = 0; i < sk_IPAddressFamily_num(a); i++) { > IPAddressFamily *fa = sk_IPAddressFamily_value(a, i); > int j = sk_IPAddressFamily_find(b, fa); > - IPAddressFamily *fb = sk_IPAddressFamily_value(b, j); > + IPAddressFamily *fb; > +#if 0 /* not needed: sk_IPAddressFamily_value() can handle > out-of-range indexes --> returns NULL */ > + if (j < 0) /* [i_a] missing check */ > + return 0; > +#endif > + fb = sk_IPAddressFamily_value(b, j); > + if (fb == NULL) /* [i_a] missing check */ > + return 0; > if (!addr_contains(fb->ipAddressChoice->u.addressesOrRanges, > fa->ipAddressChoice->u.addressesOrRanges, > length_from_afi(v3_addr_get_afi(fb)))) > return 0; > } > return 1; > } > > > v3_addr_validate_path_internal() is fine WITHOUT that check (and is > the only spot in my codebase where sk_*_find wasn't followed by a > check) as 'fp' is checked for NULL a little later: > > --- \\Debbie\ger\prj\1original\openssl\openssl\crypto\x509v3\v3_addr.c > 2008-12-16 > 19:39:49.000000000 +-0100 > +++ \\Debbie\ger\prj\3actual\openssl\crypto\x509v3\v3_addr.c 2009-02-11 > 10:40:35.000000000 +-0100 > @@ -1213,10 +1233,11 @@ > sk_IPAddressFamily_set_cmp_func(x->rfc3779_addr, IPAddressFamily_cmp); > for (j = 0; j < sk_IPAddressFamily_num(child); j++) { > IPAddressFamily *fc = sk_IPAddressFamily_value(child, j); > int k = sk_IPAddressFamily_find(x->rfc3779_addr, fc); > + /* [i_a] no need to check k: sk_IPAddressFamily_value() can handle > out-of-range indexes --> returns NULL */ > IPAddressFamily *fp = sk_IPAddressFamily_value(x->rfc3779_addr, k); > if (fp == NULL) { > if (fc->ipAddressChoice->type == IPAddressChoice_addressesOrRanges) { > validation_err(X509_V_ERR_UNNESTED_RESOURCE); > break; > } > > > > > plus fix for the spot which you identified: > > --- \\Debbie\ger\prj\1original\openssl\openssl\ssl\ssl_ciph.c 2009-01-05 > 18:26:52.000000000 +-0100 > +++ \\Debbie\ger\prj\3actual\openssl\ssl\ssl_ciph.c 2009-02-11 > 10:35:48.000000000 +-0100 > @@ -1712,13 +1713,13 @@ > MemCheck_off(); > comp=(SSL_COMP *)OPENSSL_malloc(sizeof(SSL_COMP)); > comp->id=id; > comp->method=cm; > load_builtin_compressions(); > if (ssl_comp_methods > - && !sk_SSL_COMP_find(ssl_comp_methods,comp)) > + && sk_SSL_COMP_find(ssl_comp_methods,comp) >= 0) /* [i_a] > following > email from Victor Duchovny 2009/02/11 */ > { > OPENSSL_free(comp); > MemCheck_on(); > > SSLerr(SSL_F_SSL_COMP_ADD_COMPRESSION_METHOD,SSL_R_DUPLICATE_COMPRESSION_ID); > return(1); > } > > > > All sk_*_find / sk_*_find_ex spots in the OpenSSL HEAD 20090202 have > been inspected + surroundings. > > > Hope that helps. It's at least worth an RT ticket, this one. > - Show quoted text - > > > > On Wed, Feb 11, 2009 at 4:44 AM, Victor Duchovni > <victor.ducho...@morganstanley.com> wrote: >> >> ssl/ssl_ciph.c: >> >> int SSL_COMP_add_compression_method(int id, COMP_METHOD *cm) >> { >> ... >> if (ssl_comp_methods >> && !sk_SSL_COMP_find(ssl_comp_methods,comp)) >> { >> OPENSSL_free(comp); >> MemCheck_on(); >> >> SSLerr(SSL_F_SSL_COMP_ADD_COMPRESSION_METHOD,SSL_R_DUPLICATE_COMPRESSION_ID); >> return(1); >> } >> ... >> } >> >> The "!sk_SSL_COMP_find()" looks wrong to me, I expect sk_SSL_COMP_find() >> to return the index of the matching entry, not true/false. So the test >> is only correct when the index of the matching entry is zero (just one >> entry on the stack). >> >> This looks somewhat similar to the 0.9.8j fix for true/false tests >> mis-applied to a non-boolean result from EVP_VerifyFinal(). Can a more >> systematic audit be performed to identify any other similar instances? >> >> First one needs a list of integer-valued functions where 0 vs. non-zero >> return values are semantically dubious, and then a search for boolean >> tests of the return value from such functions. Don't know whether any >> static code analyzers can help to identify this type of problem. >> >> -- >> Viktor. >> ______________________________________________________________________ >> OpenSSL Project http://www.openssl.org >> User Support Mailing List openssl-us...@openssl.org >> Automated List Manager majord...@openssl.org >> > > > > -- > Met vriendelijke groeten / Best regards, > > Ger Hobbelt > > -------------------------------------------------- > web: http://www.hobbelt.com/ > http://www.hebbut.net/ > mail: g...@hobbelt.com > mobile: +31-6-11 120 978 > -------------------------------------------------- > -- Met vriendelijke groeten / Best regards, Ger Hobbelt -------------------------------------------------- web: http://www.hobbelt.com/ http://www.hebbut.net/ mail: g...@hobbelt.com mobile: +31-6-11 120 978 --------------------------------------------------
--- /home/ger/prj/1original/openssl/openssl/./crypto/x509v3/v3_addr.c 2009-03-09 06:40:41.000000000 +0100 +++ ./crypto/x509v3/v3_addr.c 2009-03-10 23:05:48.000000000 +0100 @@ -61,7 +61,11 @@ #include <stdio.h> #include <stdlib.h> + +#if 0 /* [i_a] -- changed every assert() to OPENSSL_assert() */ #include <assert.h> +#endif + #include "cryptlib.h" #include <openssl/conf.h> #include <openssl/asn1.h> @@ -128,7 +132,7 @@ /* * Extract the AFI from an IPAddressFamily. */ -unsigned v3_addr_get_afi(const IPAddressFamily *f) +unsigned int v3_addr_get_afi(const IPAddressFamily *f) { return ((f != NULL && f->addressFamily != NULL && @@ -144,10 +148,10 @@ */ static void addr_expand(unsigned char *addr, const ASN1_BIT_STRING *bs, const int length, const unsigned char fill) { - assert(bs->length >= 0 && bs->length <= length); + OPENSSL_assert(bs->length >= 0 && bs->length <= length); if (bs->length > 0) { memcpy(addr, bs->data, bs->length); if ((bs->flags & 7) != 0) { @@ -245,7 +249,7 @@ int i; for (i = 0; i < sk_IPAddressFamily_num(addr); i++) { IPAddressFamily *f = sk_IPAddressFamily_value(addr, i); - const unsigned afi = v3_addr_get_afi(f); + const unsigned int afi = v3_addr_get_afi(f); switch (afi) { case IANA_AFI_IPV4: BIO_printf(out, "%*sIPv4", indent, ""); @@ -454,7 +461,7 @@ if ((aor = IPAddressOrRange_new()) == NULL) return 0; aor->type = IPAddressOrRange_addressRange; - assert(aor->u.addressRange == NULL); + OPENSSL_assert(aor->u.addressRange == NULL); if ((aor->u.addressRange = IPAddressRange_new()) == NULL) goto err; if (aor->u.addressRange->min == NULL && @@ -523,7 +530,7 @@ for (i = 0; i < sk_IPAddressFamily_num(addr); i++) { f = sk_IPAddressFamily_value(addr, i); - assert(f->addressFamily->data != NULL); + OPENSSL_assert(f->addressFamily->data != NULL); if (f->addressFamily->length == keylen && !memcmp(f->addressFamily->data, key, keylen)) return f; @@ -653,9 +660,9 @@ static void extract_min_max(IPAddressOrRange *aor, unsigned char *min, unsigned char *max, int length) { - assert(aor != NULL && min != NULL && max != NULL); + OPENSSL_assert(aor != NULL && min != NULL && max != NULL); switch (aor->type) { case IPAddressOrRange_addressPrefix: addr_expand(min, aor->u.addressPrefix, length, 0x00); @@ -881,7 +890,7 @@ } sk_IPAddressFamily_set_cmp_func(addr, IPAddressFamily_cmp); sk_IPAddressFamily_sort(addr); - assert(v3_addr_is_canonical(addr)); + OPENSSL_assert(v3_addr_is_canonical(addr)); return 1; } @@ -1128,7 +1140,14 @@ for (i = 0; i < sk_IPAddressFamily_num(a); i++) { IPAddressFamily *fa = sk_IPAddressFamily_value(a, i); int j = sk_IPAddressFamily_find(b, fa); - IPAddressFamily *fb = sk_IPAddressFamily_value(b, j); + IPAddressFamily *fb; +#if 0 /* not needed: sk_IPAddressFamily_value() can handle out-of-range indexes --> returns NULL */ + if (j < 0) /* [i_a] missing check */ + return 0; +#endif + fb = sk_IPAddressFamily_value(b, j); + if (fb == NULL) /* [i_a] missing check */ + return 0; if (!addr_contains(fb->ipAddressChoice->u.addressesOrRanges, fa->ipAddressChoice->u.addressesOrRanges, length_from_afi(v3_addr_get_afi(fb)))) @@ -1165,9 +1184,9 @@ int i, j, ret = 1; X509 *x; - assert(chain != NULL && sk_X509_num(chain) > 0); - assert(ctx != NULL || ext != NULL); - assert(ctx == NULL || ctx->verify_cb != NULL); + OPENSSL_assert(chain != NULL && sk_X509_num(chain) > 0); + OPENSSL_assert(ctx != NULL || ext != NULL); + OPENSSL_assert(ctx == NULL || ctx->verify_cb != NULL); /* * Figure out where to start. If we don't have an extension to @@ -1180,7 +1199,7 @@ } else { i = 0; x = sk_X509_value(chain, i); - assert(x != NULL); + OPENSSL_assert(x != NULL); if ((ext = x->rfc3779_addr) == NULL) goto done; } @@ -1199,7 +1218,7 @@ */ for (i++; i < sk_X509_num(chain); i++) { x = sk_X509_value(chain, i); - assert(x != NULL); + OPENSSL_assert(x != NULL); if (!v3_addr_is_canonical(x->rfc3779_addr)) validation_err(X509_V_ERR_INVALID_EXTENSION); if (x->rfc3779_addr == NULL) { @@ -1216,6 +1235,7 @@ for (j = 0; j < sk_IPAddressFamily_num(child); j++) { IPAddressFamily *fc = sk_IPAddressFamily_value(child, j); int k = sk_IPAddressFamily_find(x->rfc3779_addr, fc); + /* [i_a] no need to check k: sk_IPAddressFamily_value() can handle out-of-range indexes --> returns NULL */ IPAddressFamily *fp = sk_IPAddressFamily_value(x->rfc3779_addr, k); if (fp == NULL) { if (fc->ipAddressChoice->type == IPAddressChoice_addressesOrRanges) { @@ -1239,7 +1259,7 @@ /* * Trust anchor can't inherit. */ - assert(x != NULL); + OPENSSL_assert(x != NULL); if (x->rfc3779_addr != NULL) { for (j = 0; j < sk_IPAddressFamily_num(x->rfc3779_addr); j++) { IPAddressFamily *fp = sk_IPAddressFamily_value(x->rfc3779_addr, j);