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)

3 files changed, 349 insertions, 127 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 */
diff --git a/src/tests/gssapi/Makefile.in b/src/tests/gssapi/Makefile.in
index e20cbac1fe..36e9d53c90 100644
--- a/src/tests/gssapi/Makefile.in
+++ b/src/tests/gssapi/Makefile.in
@@ -23,7 +23,10 @@ COMMON_LIBS= common.o $(GSS_LIBS) $(KRB5_BASE_LIBS)
 all:: ccinit ccrefresh t_accname t_ccselect t_credstore t_export_cred \
 	t_export_name t_gssexts t_imp_cred t_imp_name t_inq_cred \
 	t_inq_mechs_name t_namingexts t_s4u t_s4u2proxy_krb5 t_saslname \
-	t_spnego
+	t_spnego t_oid
+
+check-unix:: t_oid
+	$(KRB5_RUN_ENV) $(VALGRIND) ./t_oid
 
 check-pytests:: ccinit ccrefresh t_accname t_ccselect t_credstore \
 	t_export_cred t_export_name t_imp_cred t_inq_cred t_inq_mechs_name \
@@ -68,9 +71,11 @@ t_saslname: t_saslname.o $(COMMON_DEPLIBS)
 	$(CC_LINK) -o $@ t_saslname.o $(COMMON_LIBS)
 t_spnego: t_spnego.o $(COMMON_DEPS)
 	$(CC_LINK) -o $@ t_spnego.o $(COMMON_LIBS)
+t_oid: t_oid.o $(COMMON_DEPS)
+	$(CC_LINK) -o $@ t_oid.o $(COMMON_LIBS)
 
 clean::
 	$(RM) ccinit ccrefresh t_accname t_ccselect t_credstore t_export_cred \
 	$(RM) t_export_name t_gssexts t_imp_cred t_imp_name t_inq_cred
 	$(RM) t_inq_mechs_name t_namingexts t_s4u t_s4u2proxy_krb5 t_saslname
-	$(RM) t_spnego
+	$(RM) t_spnego t_oid
diff --git a/src/tests/gssapi/t_oid.c b/src/tests/gssapi/t_oid.c
new file mode 100644
index 0000000000..8bfd2df5a2
--- /dev/null
+++ b/src/tests/gssapi/t_oid.c
@@ -0,0 +1,221 @@
+/* -*- mode: c; c-basic-offset: 4; indent-tabs-mode: nil -*- */
+/* tests/gssapi/t_oid.c - Test OID manipulation functions */
+/*
+ * Copyright (C) 2012 by the Massachusetts Institute of Technology.
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ *   notice, this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright
+ *   notice, this list of conditions and the following disclaimer in
+ *   the documentation and/or other materials provided with the
+ *   distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
+ * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
+ * COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
+ * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+ * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
+ * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
+ * OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include "common.h"
+
+static struct {
+    char *canonical;
+    char *variant;
+    gss_OID_desc oid;
+} tests[] = {
+    /* GSS_C_NT_USER_NAME */
+    { "{ 1 2 840 113554 1 2 1 1 }", "1.2.840.113554.1.2.1.1",
+      { 10, "\x2A\x86\x48\x86\xF7\x12\x01\x02\x01\x01" } },
+    /* GSS_C_NT_MACHINE_UID_NAME */
+    { "{ 1 2 840 113554 1 2 1 2 }", "1 2 840 113554 1 2 1 2",
+      { 10, "\x2A\x86\x48\x86\xF7\x12\x01\x02\x01\x02" } },
+    /* GSS_C_NT_STRING_UID_NAME */
+    { "{ 1 2 840 113554 1 2 1 3 }", "{1 2 840 113554 1 2 1 3}",
+      { 10, "\x2A\x86\x48\x86\xF7\x12\x01\x02\x01\x03" } },
+    /* GSS_C_NT_HOSTBASED_SERVICE_X */
+    { "{ 1 3 6 1 5 6 2 }", "{  1  3  6  1  5  6  2  }",
+      { 6, "\x2B\x06\x01\x05\x06\x02" } },
+    /* GSS_C_NT_ANONYMOUS */
+    { "{ 1 3 6 1 5 6 3 }", "{ 01 03 06 01 05 06 03 }",
+      { 6, "\x2B\x06\x01\x05\x06\x03" } },
+    /* GSS_KRB5_NT_PRINCIPAL_NAME */
+    { "{ 1 2 840 113554 1 2 2 1 }", " {01 2 840 113554 1 2 2 1  } ",
+      { 10, "\x2A\x86\x48\x86\xF7\x12\x01\x02\x02\x01" } },
+    /* gss_krb5_nt_principal */
+    { "{ 1 2 840 113554 1 2 2 2 }", "{1.2.840.113554.1.2.2.2}",
+      { 10, "\x2A\x86\x48\x86\xF7\x12\x01\x02\x02\x02" } },
+    /* gss_mech_krb5 */
+    { "{ 1 2 840 113554 1 2 2 }", "{ 1.2.840.113554.1.2.2 }",
+      { 9, "\x2A\x86\x48\x86\xF7\x12\x01\x02\x02" } },
+    /* gss_mech_krb5_old */
+    { "{ 1 3 5 1 5 2 }", "001 . 003 . 005 . 001 . 005 . 002",
+      { 5, "\x2B\x05\x01\x05\x02" } },
+    /* gss_mech_krb5_wrong */
+    { "{ 1 2 840 48018 1 2 2 }", "1.2.840.48018.1.2.2 trailing garbage",
+      { 9, "\x2A\x86\x48\x82\xF7\x12\x01\x02\x02" } },
+    /* gss_mech_iakerb */
+    { "{ 1 3 6 1 5 2 5 }", "{ 1 3 6 1 5 2 5 } trailing garbage",
+      { 6, "\x2B\x06\x01\x05\x02\x05" } },
+    /* SPNEGO */
+    { "{ 1 3 6 1 5 5 2 }", "{1 3 6 1 5 5 2} trailing garbage",
+      { 6, "\x2B\x06\x01\x05\x05\x02" } },
+    /* Edge cases for the first two arcs */
+    { "{ 0 0 }", NULL, { 1, "\x00" } },
+    { "{ 0 39 }", NULL, { 1, "\x27" } },
+    { "{ 1 0 }", NULL, { 1, "\x28" } },
+    { "{ 1 39 }", NULL, { 1, "\x4F" } },
+    { "{ 2 0 }", NULL, { 1, "\x50" } },
+    { "{ 2 40 }", NULL, { 1, "\x78" } },
+    { "{ 2 47 }", NULL, { 1, "\x7F" } },
+    { "{ 2 48 }", NULL, { 2, "\x81\x00" } },
+    { "{ 2 16304 }", NULL, { 3, "\x81\x80\x00" } },
+    /* Zero-valued arcs */
+    { "{ 0 0 0 }", NULL, { 2, "\x00\x00" } },
+    { "{ 0 0 1 0 }", NULL, { 3, "\x00\x01\x00" } },
+    { "{ 0 0 128 0 }", NULL, { 4, "\x00\x81\x00\x00 " } },
+    { "{ 0 0 0 1 }", NULL, { 3, "\x00\x00\x01" } },
+    { "{ 0 0 128 0 1 0 128 }", NULL,
+      { 8, "\x00\x81\x00\x00\x01\x00\x81\x00 " } }
+};
+
+static char *invalid_strings[] = {
+    "",
+    "{}",
+    "{",
+    "}",
+    "  ",
+    " { } ",
+    "x",
+    "+1 1",
+    "-1.1",
+    "1.+0",
+    "+0.1",
+    "{ 1 garbage }",
+    "{ 1 }"
+    "{ 0 40 }",
+    "{ 1 40 }",
+    "{ 1 128 }",
+    "{ 1 1",
+    "{ 1 2 3 4 +5 }",
+    "{ 1.2.-3.4.5 }"
+};
+
+static int
+oid_equal(gss_OID o1, gss_OID o2)
+{
+    return o1->length == o2->length &&
+        memcmp(o1->elements, o2->elements, o1->length) == 0;
+}
+
+int
+main()
+{
+    size_t i;
+    OM_uint32 major, minor;
+    gss_buffer_desc buf;
+    gss_OID oid;
+    gss_OID_set set;
+    int status = 0, present;
+
+    for (i = 0; i < sizeof(tests) / sizeof(*tests); i++) {
+        /* Check that this test's OID converts to its canonical string form. */
+        major = gss_oid_to_str(&minor, &tests[i].oid, &buf);
+        check_gsserr("gss_oid_to_str", major, minor);
+        if (buf.length != strlen(tests[i].canonical) + 1 ||
+            memcmp(buf.value, tests[i].canonical, buf.length) != 0) {
+            status = 1;
+            printf("test %d: OID converts to %.*s, wanted %s\n", (int)i,
+                   (int)buf.length, (char *)buf.value, tests[i].canonical);
+        }
+        (void)gss_release_buffer(&minor, &buf);
+
+        /* Check that this test's canonical string form converts to its OID. */
+        buf.value = tests[i].canonical;
+        buf.length = strlen(tests[i].canonical);
+        major = gss_str_to_oid(&minor, &buf, &oid);
+        check_gsserr("gss_str_to_oid", major, minor);
+        if (!oid_equal(oid, &tests[i].oid)) {
+            status = 1;
+            printf("test %d: %s converts to wrong OID\n", (int)i,
+                   tests[i].canonical);
+            display_oid("wanted", &tests[i].oid);
+            display_oid("actual", oid);
+        }
+        (void)gss_release_oid(&minor, &oid);
+
+        /* Check that this test's variant string form converts to its OID. */
+        if (tests[i].variant == NULL)
+            continue;
+        buf.value = tests[i].variant;
+        buf.length = strlen(tests[i].variant);
+        major = gss_str_to_oid(&minor, &buf, &oid);
+        check_gsserr("gss_str_to_oid", major, minor);
+        if (!oid_equal(oid, &tests[i].oid)) {
+            status = 1;
+            printf("test %d: %s converts to wrong OID\n", (int)i,
+                   tests[i].variant);
+            display_oid("wanted", &tests[i].oid);
+            display_oid("actual", oid);
+        }
+        (void)gss_release_oid(&minor, &oid);
+    }
+
+    for (i = 0; i < sizeof(invalid_strings) / sizeof(*invalid_strings); i++) {
+        buf.value = invalid_strings[i];
+        buf.length = strlen(invalid_strings[i]);
+        major = gss_str_to_oid(&minor, &buf, &oid);
+        if (major == GSS_S_COMPLETE) {
+            status = 1;
+            printf("invalid %d: %s converted when it should not have\n",
+                   (int)i, invalid_strings[i]);
+            (void)gss_release_oid(&minor, &oid);
+        }
+    }
+
+    major = gss_create_empty_oid_set(&minor, &set);
+    check_gsserr("gss_create_empty_oid_set", major, minor);
+    for (i = 0; i < sizeof(tests) / sizeof(*tests); i++) {
+        major = gss_add_oid_set_member(&minor, &tests[i].oid, &set);
+        check_gsserr("gss_add_oid_set_member", major, minor);
+    }
+    if (set->count != i) {
+        status = 1;
+        printf("oid set has wrong size: wanted %d, actual %d\n", (int)i,
+               (int)set->count);
+    }
+    for (i = 0; i < set->count; i++) {
+        if (!oid_equal(&set->elements[i], &tests[i].oid)) {
+            status = 1;
+            printf("oid set has wrong element %d\n", (int)i);
+            display_oid("wanted", &tests[i].oid);
+            display_oid("actual", &set->elements[i]);
+        }
+        major = gss_test_oid_set_member(&minor, &tests[i].oid, set, &present);
+        check_gsserr("gss_test_oid_set_member", major, minor);
+        if (!present) {
+            status = 1;
+            printf("oid set does not contain OID %d\n", (int)i);
+            display_oid("wanted", &tests[i].oid);
+        }
+    }
+    (void)gss_release_oid_set(&minor, &set);
+    return status;
+}