Fix gss_str_to_oid and gss_oid_to_str edge cases

Neither function correctly handled OIDs whose second arc exceeds 47
(theoretically possible if the first arc is 2).  gss_str_to_oid had
additional problems: it used scanf, it didn't consistently protect
against read overrun if the input buffer wasn't null-terminated, and
it could get confused by + or - characters in the first two arcs.  Fix
gss_oid_to_str and rewrite gss_str_to_oid.

Also add a test program.

ticket: 7524 (new)

1 files changed, 121 insertions, 125 deletions

diff --git a/src/lib/gssapi/generic/oid_ops.c b/src/lib/gssapi/generic/oid_ops.c
index de38dd7241..a088734905 100644
--- a/src/lib/gssapi/generic/oid_ops.c
+++ b/src/lib/gssapi/generic/oid_ops.c
@@ -228,16 +228,14 @@ generic_gss_test_oid_set_member(OM_uint32 *minor_status,
     return(GSS_S_COMPLETE);
 }
 
-/*
- * OID<->string routines.  These are uuuuugly.
- */
 OM_uint32
 generic_gss_oid_to_str(OM_uint32 *minor_status,
                        const gss_OID_desc * const oid,
                        gss_buffer_t oid_str)
 {
-    OM_uint32           number;
-    OM_uint32 i;
+    unsigned long       number, n;
+    OM_uint32           i;
+    int                 first;
     unsigned char       *cp;
     struct k5buf        buf;
 
@@ -260,14 +258,20 @@ generic_gss_oid_to_str(OM_uint32 *minor_status,
     cp = (unsigned char *) oid->elements;
     number = (unsigned long) cp[0];
     krb5int_buf_init_dynamic(&buf);
-    krb5int_buf_add_fmt(&buf, "{ %lu %lu ", (unsigned long)number/40,
-                        (unsigned long)number%40);
+    krb5int_buf_add(&buf, "{ ");
     number = 0;
     cp = (unsigned char *) oid->elements;
-    for (i=1; i<oid->length; i++) {
+    first = 1;
+    for (i = 0; i < oid->length; i++) {
         number = (number << 7) | (cp[i] & 0x7f);
         if ((cp[i] & 0x80) == 0) {
-            krb5int_buf_add_fmt(&buf, "%lu ", (unsigned long)number);
+            if (first) {
+                n = (number < 40) ? 0 : (number < 80) ? 1 : 2;
+                krb5int_buf_add_fmt(&buf, "%lu %lu ", n, number - (n * 40));
+                first = 0;
+            } else {
+                krb5int_buf_add_fmt(&buf, "%lu ", number);
+            }
             number = 0;
         }
     }
@@ -279,140 +283,132 @@ generic_gss_oid_to_str(OM_uint32 *minor_status,
     return k5buf_to_gss(minor_status, &buf, oid_str);
 }
 
+/* Return the length of a DER OID subidentifier encoding. */
+static size_t
+arc_encoded_length(unsigned long arc)
+{
+    size_t len = 1;
+
+    for (arc >>= 7; arc; arc >>= 7)
+        len++;
+    return len;
+}
+
+/* Encode a subidentifier into *bufp and advance it to the encoding's end. */
+static void
+arc_encode(unsigned long arc, unsigned char **bufp)
+{
+    unsigned char *p;
+
+    /* Advance to the end and encode backwards. */
+    p = *bufp = *bufp + arc_encoded_length(arc);
+    *--p = arc & 0x7f;
+    for (arc >>= 7; arc; arc >>= 7)
+        *--p = (arc & 0x7f) | 0x80;
+}
+
+/* Fetch an arc value from *bufp and advance past it and any following spaces
+ * or periods.  Return 1 on success, 0 if *bufp is not at a valid arc value. */
+static int
+get_arc(const unsigned char **bufp, const unsigned char *end,
+        unsigned long *arc_out)
+{
+    const unsigned char *p = *bufp;
+    unsigned long arc = 0, newval;
+
+    if (p == end || !isdigit(*p))
+        return 0;
+    for (; p < end && isdigit(*p); p++) {
+        newval = arc * 10 + (*p - '0');
+        if (newval < arc)
+            return 0;
+        arc = newval;
+    }
+    while (p < end && (isspace(*p) || *p == '.'))
+        p++;
+    *bufp = p;
+    *arc_out = arc;
+    return 1;
+}
+
+/*
+ * Convert a sequence of two or more decimal arc values into a DER-encoded OID.
+ * The values may be separated by any combination of whitespace and period
+ * characters, and may be optionally surrounded with braces.  Leading
+ * whitespace and trailing garbage is allowed.  The first arc value must be 0,
+ * 1, or 2, and the second value must be less than 40 if the first value is not
+ * 2.
+ */
 OM_uint32
 generic_gss_str_to_oid(OM_uint32 *minor_status,
                        gss_buffer_t oid_str,
-                       gss_OID * oid)
+                       gss_OID *oid_out)
 {
-    unsigned char       *cp, *bp, *startp;
-    int         brace;
-    long        numbuf;
-    long        onumbuf;
-    OM_uint32   nbytes;
-    int         i;
-    unsigned char *op;
+    const unsigned char *p, *end, *arc3_start;
+    unsigned char *out;
+    unsigned long arc, arc1, arc2;
+    size_t nbytes;
+    int brace = 0;
+    gss_OID oid;
 
     if (minor_status != NULL)
         *minor_status = 0;
 
-    if (oid != NULL)
-        *oid = GSS_C_NO_OID;
+    if (oid_out != NULL)
+        *oid_out = GSS_C_NO_OID;
 
     if (GSS_EMPTY_BUFFER(oid_str))
         return (GSS_S_CALL_INACCESSIBLE_READ);
 
-    if (oid == NULL)
+    if (oid_out == NULL)
         return (GSS_S_CALL_INACCESSIBLE_WRITE);
 
+    /* Skip past initial spaces and, optionally, an open brace. */
     brace = 0;
-    bp = oid_str->value;
-    cp = bp;
-    /* Skip over leading space */
-    while ((bp < &cp[oid_str->length]) && isspace(*bp))
-        bp++;
-    if (*bp == '{') {
+    p = oid_str->value;
+    end = p + oid_str->length;
+    while (p < end && isspace(*p))
+        p++;
+    if (p < end && *p == '{') {
         brace = 1;
-        bp++;
+        p++;
     }
-    while ((bp < &cp[oid_str->length]) && isspace(*bp))
-        bp++;
-    startp = bp;
-    nbytes = 0;
-
-    /*
-     * The first two numbers are chewed up by the first octet.
-     */
-    if (sscanf((char *)bp, "%ld", &numbuf) != 1) {
-        *minor_status = EINVAL;
-        return(GSS_S_FAILURE);
-    }
-    while ((bp < &cp[oid_str->length]) && isdigit(*bp))
-        bp++;
-    while ((bp < &cp[oid_str->length]) &&
-           (isspace(*bp) || *bp == '.'))
-        bp++;
-    if (sscanf((char *)bp, "%ld", &numbuf) != 1) {
-        *minor_status = EINVAL;
-        return(GSS_S_FAILURE);
-    }
-    while ((bp < &cp[oid_str->length]) && isdigit(*bp))
-        bp++;
-    while ((bp < &cp[oid_str->length]) &&
-           (isspace(*bp) || *bp == '.'))
-        bp++;
-    nbytes++;
-    while (isdigit(*bp)) {
-        if (sscanf((char *)bp, "%ld", &numbuf) != 1) {
-            return(GSS_S_FAILURE);
-        }
-        do {
-            nbytes++;
-            numbuf >>= 7;
-        } while (numbuf);
-        while ((bp < &cp[oid_str->length]) && isdigit(*bp))
-            bp++;
-        while ((bp < &cp[oid_str->length]) &&
-               (isspace(*bp) || *bp == '.'))
-            bp++;
-    }
-    if (brace && (*bp != '}')) {
-        return(GSS_S_FAILURE);
-    }
-
-    /*
-     * Phew!  We've come this far, so the syntax is good.
-     */
-    if ((*oid = (gss_OID) malloc(sizeof(gss_OID_desc)))) {
-        if (((*oid)->elements = (void *) malloc(nbytes))) {
-            (*oid)->length = nbytes;
-            op = (unsigned char *) (*oid)->elements;
-            bp = startp;
-            (void) sscanf((char *)bp, "%ld", &numbuf);
-            while (isdigit(*bp))
-                bp++;
-            while (isspace(*bp) || *bp == '.')
-                bp++;
-            onumbuf = 40*numbuf;
-            (void) sscanf((char *)bp, "%ld", &numbuf);
-            onumbuf += numbuf;
-            *op = (unsigned char) onumbuf;
-            op++;
-            while (isdigit(*bp))
-                bp++;
-            while (isspace(*bp) || *bp == '.')
-                bp++;
-            while (isdigit(*bp)) {
-                (void) sscanf((char *)bp, "%ld", &numbuf);
-                nbytes = 0;
-                /* Have to fill in the bytes msb-first */
-                onumbuf = numbuf;
-                do {
-                    nbytes++;
-                    numbuf >>= 7;
-                } while (numbuf);
-                numbuf = onumbuf;
-                op += nbytes;
-                i = -1;
-                do {
-                    op[i] = (unsigned char) numbuf & 0x7f;
-                    if (i != -1)
-                        op[i] |= 0x80;
-                    i--;
-                    numbuf >>= 7;
-                } while (numbuf);
-                while (isdigit(*bp))
-                    bp++;
-                while (isspace(*bp) || *bp == '.')
-                    bp++;
-            }
-            return(GSS_S_COMPLETE);
-        }
-        else {
-            free(*oid);
-            *oid = GSS_C_NO_OID;
-        }
+    while (p < end && isspace(*p))
+        p++;
+
+    /* Get the first two arc values, to be encoded as one subidentifier. */
+    if (!get_arc(&p, end, &arc1) || !get_arc(&p, end, &arc2))
+        return (GSS_S_FAILURE);
+    if (arc1 > 2 || (arc1 < 2 && arc2 > 39) || arc2 > ULONG_MAX - 80)
+        return (GSS_S_FAILURE);
+    arc3_start = p;
+
+    /* Compute the total length of the encoding while checking syntax. */
+    nbytes = arc_encoded_length(arc1 * 40 + arc2);
+    while (get_arc(&p, end, &arc))
+        nbytes += arc_encoded_length(arc);
+    if (brace && (p == end || *p != '}'))
+        return (GSS_S_FAILURE);
+
+    /* Allocate an oid structure. */
+    oid = malloc(sizeof(*oid));
+    if (oid == NULL)
+        return (GSS_S_FAILURE);
+    oid->elements = malloc(nbytes);
+    if (oid->elements == NULL) {
+        free(oid);
+        return (GSS_S_FAILURE);
     }
-    return(GSS_S_FAILURE);
+    oid->length = nbytes;
+
+    out = oid->elements;
+    arc_encode(arc1 * 40 + arc2, &out);
+    p = arc3_start;
+    while (get_arc(&p, end, &arc))
+        arc_encode(arc, &out);
+    assert(out == oid->elements + nbytes);
+    *oid_out = oid;
+    return(GSS_S_COMPLETE);
 }
 
 /* Compose an OID of a prefix and an integer suffix */