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 fa754e3fa6bccce397794da940d5ccd8233b901b..8cd3fc64059e4cf741fc7ef2c012e85cbf0bfe8d 100644 (file)
--- 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 6ea294c8abff46083a2721545e77d4ce4956a911..1dadb1b9672cea29648c882bd1c01139310df970 100755 (executable)
--- 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')