[openssl.org #2696] [PATCH] RFC 3779 ASID code also sometimes allows inverted ranges

[Rob Austein via RT]

This is a follow-on to ticket #2482, which fixed an inverted range
problem (range end before range start) in crypto/xv509v3/v3_addr.c.

Turns out the other RFC 3779 module, crypto/xv509v3/v3_asid.c, has
essentially the same error, both allowing generation of an inverted
range and failing to detect an inverted range under certain
conditions, which violates RFC 3779 canonical form.

Credit for hand crafting the test case which triggered this goes to
Andrew Chi.

The attached patch was written against OpenSSL 1.0.0f but should apply
to all supported versions.



--- crypto/x509v3/v3_asid.c.~1~ 2011-01-03 07:53:33.000000000 -0500
+++ crypto/x509v3/v3_asid.c     2012-01-26 11:20:52.000000000 -0500
@@ -358,6 +358,20 @@
       goto done;
   }
 
+  /*
+   * Check for inverted range.
+   */
+  i = sk_ASIdOrRange_num(choice->u.asIdsOrRanges) - 1;
+  {
+    ASIdOrRange *a = sk_ASIdOrRange_value(choice->u.asIdsOrRanges, i);
+    ASN1_INTEGER *a_min, *a_max;
+    if (a != NULL && a->type == ASIdOrRange_range) {
+      extract_min_max(a, &a_min, &a_max);
+      if (ASN1_INTEGER_cmp(a_min, a_max) > 0)
+       goto done;
+    }
+  }
+
   ret = 1;
 
  done:
@@ -392,9 +406,18 @@
     return 1;
 
   /*
-   * We have a list.  Sort it.
+   * If not a list, or if empty list, it's broken.
+   */
+  if (choice->type != ASIdentifierChoice_asIdsOrRanges ||
+      sk_ASIdOrRange_num(choice->u.asIdsOrRanges) == 0) {
+    X509V3err(X509V3_F_ASIDENTIFIERCHOICE_CANONIZE,
+             X509V3_R_EXTENSION_VALUE_ERROR);
+    return 0;
+  }
+
+  /*
+   * We have a non-empty list.  Sort it.
    */
-  OPENSSL_assert(choice->type == ASIdentifierChoice_asIdsOrRanges);
   sk_ASIdOrRange_sort(choice->u.asIdsOrRanges);
 
   /*
@@ -415,6 +438,13 @@
     OPENSSL_assert(ASN1_INTEGER_cmp(a_min, b_min) <= 0);
 
     /*
+     * Punt inverted ranges.
+     */
+    if (ASN1_INTEGER_cmp(a_min, a_max) > 0 ||
+       ASN1_INTEGER_cmp(b_min, b_max) > 0)
+      goto done;
+
+    /*
      * Check for overlaps.
      */
     if (ASN1_INTEGER_cmp(a_max, b_min) >= 0) {
@@ -465,12 +495,26 @@
        break;
       }
       ASIdOrRange_free(b);
-      sk_ASIdOrRange_delete(choice->u.asIdsOrRanges, i + 1);
+      (void) sk_ASIdOrRange_delete(choice->u.asIdsOrRanges, i + 1);
       i--;
       continue;
     }
   }
 
+  /*
+   * Check for final inverted range.
+   */
+  i = sk_ASIdOrRange_num(choice->u.asIdsOrRanges) - 1;
+  {
+    ASIdOrRange *a = sk_ASIdOrRange_value(choice->u.asIdsOrRanges, i);
+    ASN1_INTEGER *a_min, *a_max;
+    if (a != NULL && a->type == ASIdOrRange_range) {
+      extract_min_max(a, &a_min, &a_max);
+      if (ASN1_INTEGER_cmp(a_min, a_max) > 0)
+       goto done;
+    }
+  }
+
   OPENSSL_assert(ASIdentifierChoice_is_canonical(choice)); /* Paranoia */
 
   ret = 1;
@@ -498,6 +542,7 @@
                               struct v3_ext_ctx *ctx,
                               STACK_OF(CONF_VALUE) *values)
 {
+  ASN1_INTEGER *min = NULL, *max = NULL;
   ASIdentifiers *asid = NULL;
   int i;
 
@@ -508,7 +553,6 @@
 
   for (i = 0; i < sk_CONF_VALUE_num(values); i++) {
     CONF_VALUE *val = sk_CONF_VALUE_value(values, i);
-    ASN1_INTEGER *min = NULL, *max = NULL;
     int i1, i2, i3, is_range, which;
 
     /*
@@ -578,18 +622,19 @@
       max = s2i_ASN1_INTEGER(NULL, s + i2);
       OPENSSL_free(s);
       if (min == NULL || max == NULL) {
-       ASN1_INTEGER_free(min);
-       ASN1_INTEGER_free(max);
        X509V3err(X509V3_F_V2I_ASIDENTIFIERS, ERR_R_MALLOC_FAILURE);
        goto err;
       }
+      if (ASN1_INTEGER_cmp(min, max) > 0) {
+       X509V3err(X509V3_F_V2I_ASIDENTIFIERS, X509V3_R_EXTENSION_VALUE_ERROR);
+       goto err;
+      }
     }
     if (!v3_asid_add_id_or_range(asid, which, min, max)) {
-      ASN1_INTEGER_free(min);
-      ASN1_INTEGER_free(max);
       X509V3err(X509V3_F_V2I_ASIDENTIFIERS, ERR_R_MALLOC_FAILURE);
       goto err;
     }
+    min = max = NULL;
   }
 
   /*
@@ -601,6 +646,8 @@
 
  err:
   ASIdentifiers_free(asid);
+  ASN1_INTEGER_free(min);
+  ASN1_INTEGER_free(max);
   return NULL;
 }