Fix bug 1192028, r=ryan.sleevi
authorDavid Keeler <dkeeler@mozilla.com>
Thu, 15 Oct 2015 19:51:07 +0200
changeset 11654 8ac7f47eecbbde13cf873e91123e4a82b228c343
parent 11653 04625985b8ba79f0d182bbccebfe2d4f9d66c825
child 11655 4dc247276e581ccdafc8d4c6236cb6b055348f65
push id808
push userkaie@kuix.de
push dateThu, 15 Oct 2015 17:52:28 +0000
reviewersryan.sleevi
bugs1192028
Fix bug 1192028, r=ryan.sleevi
lib/util/secasn1d.c
--- a/lib/util/secasn1d.c
+++ b/lib/util/secasn1d.c
@@ -946,31 +946,115 @@ sec_asn1d_parse_more_length (sec_asn1d_s
     }
 
     if (state->pending == 0)
 	state->place = afterLength;
 
     return count;
 }
 
+/*
+ * Helper function for sec_asn1d_prepare_for_contents.
+ * Checks that a value representing a number of bytes consumed can be
+ * subtracted from a remaining length. If so, returns PR_TRUE.
+ * Otherwise, sets the error SEC_ERROR_BAD_DER, indicates that there was a
+ * decoding error in the given SEC_ASN1DecoderContext, and returns PR_FALSE.
+ */
+static PRBool
+sec_asn1d_check_and_subtract_length (unsigned long *remaining,
+                                     unsigned long consumed,
+                                     SEC_ASN1DecoderContext *cx)
+{
+    PORT_Assert(remaining);
+    PORT_Assert(cx);
+    if (!remaining || !cx) {
+        PORT_SetError (SEC_ERROR_INVALID_ARGS);
+        cx->status = decodeError;
+        return PR_FALSE;
+    }
+    if (*remaining < consumed) {
+        PORT_SetError (SEC_ERROR_BAD_DER);
+        cx->status = decodeError;
+        return PR_FALSE;
+    }
+    *remaining -= consumed;
+    return PR_TRUE;
+}
 
 static void
 sec_asn1d_prepare_for_contents (sec_asn1d_state *state)
 {
     SECItem *item;
     PLArenaPool *poolp;
     unsigned long alloc_len;
 
 #ifdef DEBUG_ASN1D_STATES
     {
         printf("Found Length %d %s\n", state->contents_length,
                state->indefinite ? "indefinite" : "");
     }
 #endif
 
+    /**
+     * The maximum length for a child element should be constrained to the
+     * length remaining in the first definite length element in the ancestor
+     * stack. If there is no definite length element in the ancestor stack,
+     * there's nothing to constrain the length of the child, so there's no
+     * further processing necessary.
+     *
+     * It's necessary to walk the ancestor stack, because it's possible to have
+     * definite length children that are part of an indefinite length element,
+     * which is itself part of an indefinite length element, and which is
+     * ultimately part of a definite length element. A simple example of this
+     * would be the handling of constructed OCTET STRINGs in BER encoding.
+     *
+     * This algorithm finds the first definite length element in the ancestor
+     * stack, if any, and if so, ensures that the length of the child element
+     * is consistent with the number of bytes remaining in the constraining
+     * ancestor element (that is, after accounting for any other sibling
+     * elements that may have been read).
+     *
+     * It's slightly complicated by the need to account both for integer
+     * underflow and overflow, as well as ensure that for indefinite length
+     * encodings, there's also enough space for the End-of-Contents (EOC)
+     * octets (Tag = 0x00, Length = 0x00, or two bytes).
+     */
+
+    /* Determine the maximum length available for this element by finding the
+     * first definite length ancestor, if any. */
+    sec_asn1d_state *parent = sec_asn1d_get_enclosing_construct(state);
+    while (parent && parent->indefinite) {
+        parent = sec_asn1d_get_enclosing_construct(parent);
+    }
+    /* If parent is null, state is either the outermost state / at the top of
+     * the stack, or the outermost state uses indefinite length encoding. In
+     * these cases, there's nothing external to constrain this element, so
+     * there's nothing to check. */
+    if (parent) {
+        unsigned long remaining = parent->pending;
+        parent = state;
+        do {
+            if (!sec_asn1d_check_and_subtract_length(
+                     &remaining, parent->consumed, state->top) ||
+                /* If parent->indefinite is true, parent->contents_length is
+                 * zero and this is a no-op. */
+                !sec_asn1d_check_and_subtract_length(
+                     &remaining, parent->contents_length, state->top) ||
+                /* If parent->indefinite is true, then ensure there is enough
+                 * space for an EOC tag of 2 bytes. */
+                (parent->indefinite && !sec_asn1d_check_and_subtract_length(
+                                            &remaining, 2, state->top))) {
+                /* This element is larger than its enclosing element, which is
+                 * invalid. */
+                return;
+            }
+        } while ((parent = sec_asn1d_get_enclosing_construct(parent)) &&
+                 parent->indefinite);
+    }
+
     /*
      * XXX I cannot decide if this allocation should exclude the case
      *     where state->endofcontents is true -- figure it out!
      */
     if (state->allocate) {
 	void *dest;
 
 	PORT_Assert (state->dest == NULL);
@@ -1002,31 +1086,16 @@ sec_asn1d_prepare_for_contents (sec_asn1
     }
 
     /*
      * Remember, length may be indefinite here!  In that case,
      * both contents_length and pending will be zero.
      */
     state->pending = state->contents_length;
 
-    /* If this item has definite length encoding, and 
-    ** is enclosed by a definite length constructed type,
-    ** make sure it isn't longer than the remaining space in that 
-    ** constructed type.  
-    */
-    if (state->contents_length > 0) {
-	sec_asn1d_state *parent = sec_asn1d_get_enclosing_construct(state);
-	if (parent && !parent->indefinite && 
-	    state->consumed + state->contents_length > parent->pending) {
-	    PORT_SetError (SEC_ERROR_BAD_DER);
-	    state->top->status = decodeError;
-	    return;
-	}
-    }
-
     /*
      * An EXPLICIT is nothing but an outer header, which we have
      * already parsed and accepted.  Now we need to do the inner
      * header and its contents.
      */
     if (state->explicit) {
 	state->place = afterExplicit;
 	state = sec_asn1d_push_state (state->top,