From 54c5bec8deb2d4e972795e37273ad17a0b1e2f4f Mon Sep 17 00:00:00 2001 From: Greg Hudson 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) --- src/plugins/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)