From: Greg Hudson <ghudson@mit.edu>
Date: Thu, 6 Sep 2018 15:47:56 +0000 (-0400)
Subject: Fix null deref on some invalid PKINIT identities
X-Git-Tag: krb5-1.17-beta1~40
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=23cd8e41d335027ff2e2a586345a570f97a926d4;p=thirdparty%2Fkrb5.git

Fix null deref on some invalid PKINIT identities

pkinit_identity.c:parse_fs_options() could crash if the first
strtok_r() call returns NULL, which happens when the residual string
begins with ','.  Fix this bug by checking for a leading comma and
checking the strtok_r() result, and add a test case.  Reported by Bean
Zhang.

Also return EINVAL rather than 0 on invalid input, and don't leave an
allocated value in idopts->cert_filename if we fail to copy the key
filename.

ticket: 8726
---

diff --git a/src/plugins/preauth/pkinit/pkinit_identity.c b/src/plugins/preauth/pkinit/pkinit_identity.c
index fa754e3fa6..8cd3fc6405 100644
--- a/src/plugins/preauth/pkinit/pkinit_identity.c
+++ b/src/plugins/preauth/pkinit/pkinit_identity.c
@@ -317,29 +317,38 @@ parse_fs_options(krb5_context context,
                  const char *residual)
 {
     char *certname, *keyname, *save;
+    char *cert_filename = NULL, *key_filename = NULL;
     krb5_error_code retval = ENOMEM;
 
-    if (residual == NULL || residual[0] == '\0')
-        return 0;
+    if (residual == NULL || residual[0] == '\0' || residual[0] == ',')
+        return EINVAL;
 
     certname = strdup(residual);
     if (certname == NULL)
         goto cleanup;
 
     certname = strtok_r(certname, ",", &save);
+    if (certname == NULL)
+        goto cleanup;
     keyname = strtok_r(NULL, ",", &save);
 
-    idopts->cert_filename = strdup(certname);
-    if (idopts->cert_filename == NULL)
+    cert_filename = strdup(certname);
+    if (cert_filename == NULL)
         goto cleanup;
 
-    idopts->key_filename = strdup(keyname ? keyname : certname);
-    if (idopts->key_filename == NULL)
+    key_filename = strdup((keyname != NULL) ? keyname : certname);
+    if (key_filename == NULL)
         goto cleanup;
 
+    idopts->cert_filename = cert_filename;
+    idopts->key_filename = key_filename;
+    cert_filename = key_filename = NULL;
     retval = 0;
+
 cleanup:
     free(certname);
+    free(cert_filename);
+    free(key_filename);
     return retval;
 }
 
diff --git a/src/tests/t_pkinit.py b/src/tests/t_pkinit.py
index 6ea294c8ab..1dadb1b967 100755
--- a/src/tests/t_pkinit.py
+++ b/src/tests/t_pkinit.py
@@ -393,6 +393,11 @@ realm.kinit(realm.user_princ,
             flags=['-X', 'X509_user_identity=%s' % p12_generic_identity])
 realm.klist(realm.user_princ)
 
+# Regression test for #8726: null deref when parsing a FILE residual
+# beginning with a comma.
+realm.kinit(realm.user_princ, flags=['-X', 'X509_user_identity=,'],
+            expected_code=1, expected_msg='Preauthentication failed while')
+
 if not have_soft_pkcs11:
     skip_rest('PKINIT PKCS11 tests', 'soft-pkcs11.so not found')