krb5 commit: Fix null deref in KDC when decoding invalid NDR

[ghudson at mit.edu]

https://github.com/krb5/krb5/commit/fa62bd33a0c0889c083999c0289ffa81a5d51e7b
commit fa62bd33a0c0889c083999c0289ffa81a5d51e7b
Author: Greg Hudson <ghudson at mit.edu>
Date:   Wed Oct 12 00:46:52 2022 -0400

    Fix null deref in KDC when decoding invalid NDR
    
    In ndr_dec_delegation_info(), keep di->transited_services_length valid
    by incrementing it as we add entries.  Otherwise
    ndr_free_delegation_info() could dereference a null
    di->transited_services field.  Also bound nservices using data->length
    to prevent inordinately large memory allocations.  Credit to OSS-Fuzz
    for discovering the issues.
    
    ticket: 9073 (new)
    tags: pullup
    target_version: 1.20-next

 src/kdc/ndr.c   | 14 +++++++-------
 src/kdc/t_ndr.c | 32 +++++++++++++++++++++++++++++---
 2 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/src/kdc/ndr.c b/src/kdc/ndr.c
index 2d16e6a5b..48395abe5 100644
--- a/src/kdc/ndr.c
+++ b/src/kdc/ndr.c
@@ -142,7 +142,7 @@ ndr_dec_delegation_info(krb5_data *data, struct pac_s4u_delegation_info **out)
     krb5_error_code ret;
     struct pac_s4u_delegation_info *di = NULL;
     struct k5input in;
-    uint32_t i, object_buffer_length;
+    uint32_t i, object_buffer_length, nservices;
     uint8_t version, endianness, common_header_length;
 
     *out = NULL;
@@ -193,26 +193,26 @@ ndr_dec_delegation_info(krb5_data *data, struct pac_s4u_delegation_info **out)
     ret = dec_wchar_pointer(&in, &di->proxy_target);
     if (ret)
         goto error;
-    di->transited_services_length = k5_input_get_uint32_le(&in);
+    nservices = k5_input_get_uint32_le(&in);
 
     /* Here, we have encoded 2 bytes of length, 2 bytes of (length + 2), and 4
      * bytes of pointer, for each element (deferred pointers). */
-    if (di->transited_services_length > UINT32_MAX / 8) {
+    if (nservices > data->length / 8) {
         ret = ERANGE;
         goto error;
     }
-    (void)k5_input_get_bytes(&in, 8 * di->transited_services_length);
+    (void)k5_input_get_bytes(&in, 8 * nservices);
 
     /* Since we're likely to add another entry, leave a blank at the end. */
-    di->transited_services = k5calloc(di->transited_services_length + 1,
-                                      sizeof(char *), &ret);
+    di->transited_services = k5calloc(nservices + 1, sizeof(char *), &ret);
     if (di->transited_services == NULL)
         goto error;
 
-    for (i = 0; i < di->transited_services_length; i++) {
+    for (i = 0; i < nservices; i++) {
         ret = dec_wchar_pointer(&in, &di->transited_services[i]);
         if (ret)
             goto error;
+        di->transited_services_length++;
     }
 
     ret = in.status;
diff --git a/src/kdc/t_ndr.c b/src/kdc/t_ndr.c
index 6f1173f47..378066044 100644
--- a/src/kdc/t_ndr.c
+++ b/src/kdc/t_ndr.c
@@ -107,8 +107,24 @@ static uint8_t s4u_di_double[] = {
     0x45, 0x00, 0x53, 0x00, 0x54, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
 };
 
+static uint8_t fuzz1[] = {
+    0x01, 0x10, 0x08, 0x20, 0x20, 0x20, 0x20, 0x20, 0x24, 0x00, 0x00, 0x00,
+    0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20,
+    0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0xff, 0xff, 0xff,
+    0xff, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x20, 0x00, 0x00, 0x00, 0x00,
+    0x20, 0x20, 0x20, 0x20
+};
+
+static uint8_t fuzz2[] = {
+    0x01, 0x10, 0x08, 0x00, 0x00, 0xff, 0xff, 0xff, 0x24, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x1e, 0x00, 0x1e, 0x00, 0x00, 0x00, 0x00, 0x1e, 0x16, 0x00, 0x00,
+    0x1e, 0x00, 0x00, 0x00, 0x00, 0x1e, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+    0x00, 0x00, 0x00, 0x1e
+};
+
 static void
-test_dec_enc(uint8_t *blob, size_t len, char *name)
+test_dec_enc(uint8_t *blob, size_t len, char *name, int fail)
 {
     krb5_data data_in, data_out;
     struct pac_s4u_delegation_info *di = NULL;
@@ -120,7 +136,14 @@ test_dec_enc(uint8_t *blob, size_t len, char *name)
 
     data_in = make_data(blob, len);
     ret = ndr_dec_delegation_info(&data_in, &di);
-    if (ret) {
+    if (fail) {
+        if (!ret) {
+            printf("%s: unexpected decode success\n", name);
+            exit(1);
+        }
+        printf("%s: failed as expected\n", name);
+        return;
+    } else if (ret) {
         printf("%s: bad decode (%d): %s\n", name, ret, strerror(ret));
         exit(1);
     }
@@ -146,7 +169,8 @@ test_dec_enc(uint8_t *blob, size_t len, char *name)
     printf("%s matched\n\n", name);
 }
 
-#define RUN_TEST(blob) test_dec_enc(blob, sizeof(blob), #blob)
+#define RUN_TEST(blob) test_dec_enc(blob, sizeof(blob), #blob, 0)
+#define RUN_TEST_FAIL(blob) test_dec_enc(blob, sizeof(blob), #blob, 1)
 
 int
 main()
@@ -156,6 +180,8 @@ main()
     RUN_TEST(s4u_di_short);
     RUN_TEST(s4u_di_long);
     RUN_TEST(s4u_di_double);
+    RUN_TEST_FAIL(fuzz1);
+    RUN_TEST_FAIL(fuzz2);
 
     printf("Passed NDR tests\n");
 }