krb5 commit [krb5-1.16]: Length check when parsing GSS token encapsulation

[Greg Hudson]

https://github.com/krb5/krb5/commit/8a49abbbdd23f0eb77d7258676ecb8fd93454a25
commit 8a49abbbdd23f0eb77d7258676ecb8fd93454a25
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.
    
    (cherry picked from commit f949e990f930f48df1f108fe311c58ae3da18b24)
    
    ticket: 8620
    version_fixed: 1.16

 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;
 }