krb5 commit: Length check when parsing GSS token encapsulation

[Greg Hudson]

https://github.com/krb5/krb5/commit/f949e990f930f48df1f108fe311c58ae3da18b24
commit f949e990f930f48df1f108fe311c58ae3da18b24
Author: Greg Hudson <ghudson at mit.edu>
Date:   Sat Nov 11 13:42:28 2017 -0500

    Length check when parsing GSS token encapsulation
    
    gssint_get_mech_type_oid() is used by gss_accept_sec_context() to
    determine the mechanism of the token.  Without length checking, it
    might read a few bytes past the end of the input token buffer.  Add
    length checking as well as test cases for truncated encapsulations.
    Reported by Bar Katz.
    
    ticket: 8620 (new)
    target_version: 1.16
    target_version: 1.15-next
    target_version: 1.14-next
    tags: pullup

 src/lib/gssapi/mechglue/g_glue.c |   20 +++++++++----
 src/tests/gssapi/t_invalid.c     |   57 ++++++++++++++++++++++++++++++++++---
 2 files changed, 66 insertions(+), 11 deletions(-)

diff --git a/src/lib/gssapi/mechglue/g_glue.c b/src/lib/gssapi/mechglue/g_glue.c
index 4aa3591..4cd2e8f 100644
--- a/src/lib/gssapi/mechglue/g_glue.c
+++ b/src/lib/gssapi/mechglue/g_glue.c
@@ -189,7 +189,7 @@ OM_uint32 gssint_get_mech_type_oid(OID, token)
     gss_buffer_t	token;
 {
     unsigned char * buffer_ptr;
-    int length;
+    size_t buflen, lenbytes, length, oidlen;
 
     /*
      * This routine reads the prefix of "token" in order to determine
@@ -223,25 +223,33 @@ OM_uint32 gssint_get_mech_type_oid(OID, token)
     /* Skip past the APP/Sequnce byte and the token length */
 
     buffer_ptr = (unsigned char *) token->value;
+    buflen = token->length;
 
-    if (*(buffer_ptr++) != 0x60)
+    if (buflen < 2 || *buffer_ptr++ != 0x60)
 	return (GSS_S_DEFECTIVE_TOKEN);
     length = *buffer_ptr++;
+    buflen -= 2;
 
 	/* check if token length is null */
 	if (length == 0)
 	    return (GSS_S_DEFECTIVE_TOKEN);
 
     if (length & 0x80) {
-	if ((length & 0x7f) > 4)
+	lenbytes = length & 0x7f;
+	if (lenbytes > 4 || lenbytes > buflen)
 	    return (GSS_S_DEFECTIVE_TOKEN);
-	buffer_ptr += length & 0x7f;
+	buffer_ptr += lenbytes;
+	buflen -= lenbytes;
     }
 
-    if (*(buffer_ptr++) != 0x06)
+    if (buflen < 2 || *buffer_ptr++ != 0x06)
+	return (GSS_S_DEFECTIVE_TOKEN);
+    oidlen = *buffer_ptr++;
+    buflen -= 2;
+    if (oidlen > 0x7f || oidlen > buflen)
 	return (GSS_S_DEFECTIVE_TOKEN);
 
-    OID->length = (OM_uint32) *(buffer_ptr++);
+    OID->length = oidlen;
     OID->elements = (void *) buffer_ptr;
     return (GSS_S_COMPLETE);
 }
diff --git a/src/tests/gssapi/t_invalid.c b/src/tests/gssapi/t_invalid.c
index 5c8ddac..2a332a8 100644
--- a/src/tests/gssapi/t_invalid.c
+++ b/src/tests/gssapi/t_invalid.c
@@ -31,8 +31,8 @@
  */
 
 /*
- * This file contains regression tests for some GSSAPI krb5 invalid per-message
- * token vulnerabilities.
+ * This file contains regression tests for some GSSAPI invalid token
+ * vulnerabilities.
  *
  * 1. A pre-CFX wrap or MIC token processed with a CFX-only context causes a
  *    null pointer dereference.  (The token must use SEAL_ALG_NONE or it will
@@ -54,10 +54,13 @@
  *    causes an integer underflow when computing the original message length,
  *    leading to an allocation error.
  *
+ * 5. In the mechglue, truncated encapsulation in the initial context token can
+ *    cause input buffer overruns in gss_accept_sec_context().
+ *
  * Vulnerabilities #1 and #2 also apply to IOV unwrap, although tokens with
- * fewer than 16 bytes after the ASN.1 header will be rejected.  Vulnerability
- * #2 can only be robustly detected using a memory-checking environment such as
- * valgrind.
+ * fewer than 16 bytes after the ASN.1 header will be rejected.
+ * Vulnerabilities #2 and #5 can only be robustly detected using a
+ * memory-checking environment such as valgrind.
  */
 
 #include "k5-int.h"
@@ -406,6 +409,48 @@ test_bad_pad(gss_ctx_id_t ctx, const struct test *test)
     (void)gss_release_buffer(&minor, &out);
 }
 
+static void
+try_accept(void *value, size_t len)
+{
+    OM_uint32 minor;
+    gss_buffer_desc in, out;
+    gss_ctx_id_t ctx = GSS_C_NO_CONTEXT;
+
+    /* Copy the provided value to make input overruns more obvious. */
+    in.value = malloc(len);
+    if (in.value == NULL)
+        abort();
+    memcpy(in.value, value, len);
+    in.length = len;
+    (void)gss_accept_sec_context(&minor, &ctx, GSS_C_NO_CREDENTIAL, &in,
+                                 GSS_C_NO_CHANNEL_BINDINGS, NULL, NULL,
+                                 &out, NULL, NULL, NULL);
+    gss_release_buffer(&minor, &out);
+    gss_delete_sec_context(&minor, &ctx, GSS_C_NO_BUFFER);
+    free(in.value);
+}
+
+/* Accept contexts using superficially valid but truncated encapsulations. */
+static void
+test_short_encapsulation()
+{
+    /* Include just the initial application tag, to see if we overrun reading
+     * the sequence length. */
+    try_accept("\x60", 1);
+
+    /* Indicate four additional sequence length bytes, to see if we overrun
+     * reading them (or skipping them and reading the next byte). */
+    try_accept("\x60\x84", 2);
+
+    /* Include an object identifier tag but no length, to see if we overrun
+     * reading the length. */
+    try_accept("\x60\x40\x06", 3);
+
+    /* Include an object identifier tag with a length matching the krb5 mech,
+     * but no OID bytes, to see if we overrun comparing against mechs. */
+    try_accept("\x60\x40\x06\x09", 4);
+}
+
 int
 main(int argc, char **argv)
 {
@@ -425,5 +470,7 @@ main(int argc, char **argv)
         free_fake_context(ctx);
     }
 
+    test_short_encapsulation();
+
     return 0;
 }