krb5 commit: Fix null deref on some invalid PKINIT identities

[Greg Hudson]

https://github.com/krb5/krb5/commit/23cd8e41d335027ff2e2a586345a570f97a926d4
commit 23cd8e41d335027ff2e2a586345a570f97a926d4
Author: Greg Hudson <ghudson at mit.edu>
Date:   Thu Sep 6 11:47:56 2018 -0400

    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

 src/plugins/preauth/pkinit/pkinit_identity.c |   21 +++++++++++++++------
 src/tests/t_pkinit.py                        |    5 +++++
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/src/plugins/preauth/pkinit/pkinit_identity.c b/src/plugins/preauth/pkinit/pkinit_identity.c
index fa754e3..8cd3fc6 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 6ea294c..1dadb1b 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')