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 <[email protected]> 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
> <[email protected]> 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 [email protected]
>> Automated List Manager [email protected]
>>
>
>
>
> --
> Met vriendelijke groeten / Best regards,
>
> Ger Hobbelt
>
> --------------------------------------------------
> web: http://www.hobbelt.com/
> http://www.hebbut.net/
> mail: [email protected]
> mobile: +31-6-11 120 978
> --------------------------------------------------
>
--
Met vriendelijke groeten / Best regards,
Ger Hobbelt
--------------------------------------------------
web: http://www.hobbelt.com/
http://www.hebbut.net/
mail: [email protected]
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);
