Blob Blame History Raw
From 54c5bec8deb2d4e972795e37273ad17a0b1e2f4f Mon Sep 17 00:00:00 2001
From: Greg Hudson <ghudson@mit.edu>
Date: Wed, 4 Jan 2017 11:33:57 -0500
Subject: [PATCH] Deindent crypto_retrieve_X509_sans()

Fix some long lines in crypto_retrieve_X509_sans() by returning early
if X509_get_ext_by_NID() returns a negative result.  Also ensure that
return parameters are always initialized.

(cherry picked from commit c6b772523db9d7791ee1c56eb512c4626556a4e7)
(cherry picked from commit 23086ac768a32db1e40a9b63684dbcfd76aba033)
---
 .../preauth/pkinit/pkinit_crypto_openssl.c    | 224 +++++++++---------
 1 file changed, 114 insertions(+), 110 deletions(-)

diff --git a/src/plugins/preauth/pkinit/pkinit_crypto_openssl.c b/src/plugins/preauth/pkinit/pkinit_crypto_openssl.c
index bc6e7662e..8def8c542 100644
--- a/src/plugins/preauth/pkinit/pkinit_crypto_openssl.c
+++ b/src/plugins/preauth/pkinit/pkinit_crypto_openssl.c
@@ -2101,11 +2101,21 @@ crypto_retrieve_X509_sans(krb5_context context,
 {
     krb5_error_code retval = EINVAL;
     char buf[DN_BUF_LEN];
-    int p = 0, u = 0, d = 0, l;
+    int p = 0, u = 0, d = 0, ret = 0, l;
     krb5_principal *princs = NULL;
     krb5_principal *upns = NULL;
     unsigned char **dnss = NULL;
-    unsigned int i, num_found = 0;
+    unsigned int i, num_found = 0, num_sans = 0;
+    X509_EXTENSION *ext = NULL;
+    GENERAL_NAMES *ialt = NULL;
+    GENERAL_NAME *gen = NULL;
+
+    if (princs_ret != NULL)
+        *princs_ret = NULL;
+    if (upn_ret != NULL)
+        *upn_ret = NULL;
+    if (dns_ret != NULL)
+        *dns_ret = NULL;
 
     if (princs_ret == NULL && upn_ret == NULL && dns_ret == NULL) {
         pkiDebug("%s: nowhere to return any values!\n", __FUNCTION__);
@@ -2121,118 +2131,112 @@ crypto_retrieve_X509_sans(krb5_context context,
                       buf, sizeof(buf));
     pkiDebug("%s: looking for SANs in cert = %s\n", __FUNCTION__, buf);
 
-    if ((l = X509_get_ext_by_NID(cert, NID_subject_alt_name, -1)) >= 0) {
-        X509_EXTENSION *ext = NULL;
-        GENERAL_NAMES *ialt = NULL;
-        GENERAL_NAME *gen = NULL;
-        int ret = 0;
-        unsigned int num_sans = 0;
+    l = X509_get_ext_by_NID(cert, NID_subject_alt_name, -1);
+    if (l < 0)
+        return 0;
 
-        if (!(ext = X509_get_ext(cert, l)) || !(ialt = X509V3_EXT_d2i(ext))) {
-            pkiDebug("%s: found no subject alt name extensions\n",
-                     __FUNCTION__);
+    if (!(ext = X509_get_ext(cert, l)) || !(ialt = X509V3_EXT_d2i(ext))) {
+        pkiDebug("%s: found no subject alt name extensions\n", __FUNCTION__);
+        goto cleanup;
+    }
+    num_sans = sk_GENERAL_NAME_num(ialt);
+
+    pkiDebug("%s: found %d subject alt name extension(s)\n", __FUNCTION__,
+             num_sans);
+
+    /* OK, we're likely returning something. Allocate return values */
+    if (princs_ret != NULL) {
+        princs = calloc(num_sans + 1, sizeof(krb5_principal));
+        if (princs == NULL) {
+            retval = ENOMEM;
             goto cleanup;
         }
-        num_sans = sk_GENERAL_NAME_num(ialt);
-
-        pkiDebug("%s: found %d subject alt name extension(s)\n",
-                 __FUNCTION__, num_sans);
-
-        /* OK, we're likely returning something. Allocate return values */
-        if (princs_ret != NULL) {
-            princs = calloc(num_sans + 1, sizeof(krb5_principal));
-            if (princs == NULL) {
-                retval = ENOMEM;
-                goto cleanup;
-            }
-        }
-        if (upn_ret != NULL) {
-            upns = calloc(num_sans + 1, sizeof(krb5_principal));
-            if (upns == NULL) {
-                retval = ENOMEM;
-                goto cleanup;
-            }
-        }
-        if (dns_ret != NULL) {
-            dnss = calloc(num_sans + 1, sizeof(*dnss));
-            if (dnss == NULL) {
-                retval = ENOMEM;
-                goto cleanup;
-            }
-        }
-
-        for (i = 0; i < num_sans; i++) {
-            krb5_data name = { 0, 0, NULL };
-
-            gen = sk_GENERAL_NAME_value(ialt, i);
-            switch (gen->type) {
-            case GEN_OTHERNAME:
-                name.length = gen->d.otherName->value->value.sequence->length;
-                name.data = (char *)gen->d.otherName->value->value.sequence->data;
-                if (princs != NULL
-                    && OBJ_cmp(plgctx->id_pkinit_san,
-                               gen->d.otherName->type_id) == 0) {
-#ifdef DEBUG_ASN1
-                    print_buffer_bin((unsigned char *)name.data, name.length,
-                                     "/tmp/pkinit_san");
-#endif
-                    ret = k5int_decode_krb5_principal_name(&name, &princs[p]);
-                    if (ret) {
-                        pkiDebug("%s: failed decoding pkinit san value\n",
-                                 __FUNCTION__);
-                    } else {
-                        p++;
-                        num_found++;
-                    }
-                } else if (upns != NULL
-                           && OBJ_cmp(plgctx->id_ms_san_upn,
-                                      gen->d.otherName->type_id) == 0) {
-                    /* Prevent abuse of embedded null characters. */
-                    if (memchr(name.data, '\0', name.length))
-                        break;
-                    ret = krb5_parse_name_flags(context, name.data,
-                                                KRB5_PRINCIPAL_PARSE_ENTERPRISE,
-                                                &upns[u]);
-                    if (ret) {
-                        pkiDebug("%s: failed parsing ms-upn san value\n",
-                                 __FUNCTION__);
-                    } else {
-                        u++;
-                        num_found++;
-                    }
-                } else {
-                    pkiDebug("%s: unrecognized othername oid in SAN\n",
-                             __FUNCTION__);
-                    continue;
-                }
-
-                break;
-            case GEN_DNS:
-                if (dnss != NULL) {
-                    /* Prevent abuse of embedded null characters. */
-                    if (memchr(gen->d.dNSName->data, '\0',
-                               gen->d.dNSName->length))
-                        break;
-                    pkiDebug("%s: found dns name = %s\n",
-                             __FUNCTION__, gen->d.dNSName->data);
-                    dnss[d] = (unsigned char *)
-                        strdup((char *)gen->d.dNSName->data);
-                    if (dnss[d] == NULL) {
-                        pkiDebug("%s: failed to duplicate dns name\n",
-                                 __FUNCTION__);
-                    } else {
-                        d++;
-                        num_found++;
-                    }
-                }
-                break;
-            default:
-                pkiDebug("%s: SAN type = %d expecting %d\n",
-                         __FUNCTION__, gen->type, GEN_OTHERNAME);
-            }
-        }
-        sk_GENERAL_NAME_pop_free(ialt, GENERAL_NAME_free);
     }
+    if (upn_ret != NULL) {
+        upns = calloc(num_sans + 1, sizeof(krb5_principal));
+        if (upns == NULL) {
+            retval = ENOMEM;
+            goto cleanup;
+        }
+    }
+    if (dns_ret != NULL) {
+        dnss = calloc(num_sans + 1, sizeof(*dnss));
+        if (dnss == NULL) {
+            retval = ENOMEM;
+            goto cleanup;
+        }
+    }
+
+    for (i = 0; i < num_sans; i++) {
+        krb5_data name = { 0, 0, NULL };
+
+        gen = sk_GENERAL_NAME_value(ialt, i);
+        switch (gen->type) {
+        case GEN_OTHERNAME:
+            name.length = gen->d.otherName->value->value.sequence->length;
+            name.data = (char *)gen->d.otherName->value->value.sequence->data;
+            if (princs != NULL &&
+                OBJ_cmp(plgctx->id_pkinit_san,
+                        gen->d.otherName->type_id) == 0) {
+#ifdef DEBUG_ASN1
+                print_buffer_bin((unsigned char *)name.data, name.length,
+                                 "/tmp/pkinit_san");
+#endif
+                ret = k5int_decode_krb5_principal_name(&name, &princs[p]);
+                if (ret) {
+                    pkiDebug("%s: failed decoding pkinit san value\n",
+                             __FUNCTION__);
+                } else {
+                    p++;
+                    num_found++;
+                }
+            } else if (upns != NULL &&
+                       OBJ_cmp(plgctx->id_ms_san_upn,
+                               gen->d.otherName->type_id) == 0) {
+                /* Prevent abuse of embedded null characters. */
+                if (memchr(name.data, '\0', name.length))
+                    break;
+                ret = krb5_parse_name_flags(context, name.data,
+                                            KRB5_PRINCIPAL_PARSE_ENTERPRISE,
+                                            &upns[u]);
+                if (ret) {
+                    pkiDebug("%s: failed parsing ms-upn san value\n",
+                             __FUNCTION__);
+                } else {
+                    u++;
+                    num_found++;
+                }
+            } else {
+                pkiDebug("%s: unrecognized othername oid in SAN\n",
+                         __FUNCTION__);
+                continue;
+            }
+
+            break;
+        case GEN_DNS:
+            if (dnss != NULL) {
+                /* Prevent abuse of embedded null characters. */
+                if (memchr(gen->d.dNSName->data, '\0', gen->d.dNSName->length))
+                    break;
+                pkiDebug("%s: found dns name = %s\n", __FUNCTION__,
+                         gen->d.dNSName->data);
+                dnss[d] = (unsigned char *)
+                    strdup((char *)gen->d.dNSName->data);
+                if (dnss[d] == NULL) {
+                    pkiDebug("%s: failed to duplicate dns name\n",
+                             __FUNCTION__);
+                } else {
+                    d++;
+                    num_found++;
+                }
+            }
+            break;
+        default:
+            pkiDebug("%s: SAN type = %d expecting %d\n", __FUNCTION__,
+                     gen->type, GEN_OTHERNAME);
+        }
+    }
+    sk_GENERAL_NAME_pop_free(ialt, GENERAL_NAME_free);
 
     retval = 0;
     if (princs)