From baea5dcb27fe2db53ddaa26ca9c4522fbd7f5d30 Mon Sep 17 00:00:00 2001 From: Aleksey Sanin Date: Wed, 30 Sep 2026 11:24:32 -0400 Subject: [PATCH] Safety fixes for key / cert parsing; X509 validation; and error paths --- .gitignore | 1 + src/bn.c | 53 ++++- src/buffer.c | 9 + src/gnutls/x509vfy.c | 487 ++++++++++++++++++++++++++++++++++++++++- src/keys.c | 2 +- src/keysmngr.c | 4 + src/mscng/certkeys.c | 18 +- src/mscng/hmac.c | 31 ++- src/mscng/x509vfy.c | 317 +++++++++++++++++++-------- src/mscrypto/symkeys.c | 4 +- src/mscrypto/x509vfy.c | 328 +++++++++++++++++++-------- src/nss/app.c | 1 + src/nss/pkikeys.c | 76 +++++-- src/nss/x509.c | 27 ++- src/nss/x509vfy.c | 31 ++- src/openssl/x509vfy.c | 6 +- src/soap.c | 2 +- src/xmltree.c | 7 +- 18 files changed, 1140 insertions(+), 264 deletions(-) diff --git a/.gitignore b/.gitignore index 8daeaf3e5..789eda68d 100644 --- a/.gitignore +++ b/.gitignore @@ -68,4 +68,5 @@ docs/api/*.bak docs/api/*.types docs/api/sgml.stamp docs/html.stamp +build-all/ diff --git a/src/bn.c b/src/bn.c index 8e3817e23..63c6a4ea1 100644 --- a/src/bn.c +++ b/src/bn.c @@ -20,6 +20,7 @@ #include #include #include +#include #include @@ -581,9 +582,10 @@ xmlSecBnDiv(xmlSecBnPtr bn, int divider, int* mod) { */ int xmlSecBnAdd(xmlSecBnPtr bn, int delta) { - int over, tmp; + unsigned int over, byteDelta; + unsigned int tmp; xmlSecByte* data; - xmlSecSize ii; + xmlSecSize ii, size; xmlSecByte ch; int ret; @@ -595,10 +597,10 @@ xmlSecBnAdd(xmlSecBnPtr bn, int delta) { data = xmlSecBufferGetData(bn); if(delta > 0) { - for(over = delta, ii = xmlSecBufferGetSize(bn); (ii > 0) && (over > 0) ;) { + for(over = (unsigned int)delta, ii = xmlSecBufferGetSize(bn); (ii > 0) && (over > 0) ;) { xmlSecAssert2(data != NULL, -1); tmp = data[--ii]; - over += tmp; + over += (unsigned int)tmp; data[ii] = (xmlSecByte)(over % 256); over = over / 256; } @@ -614,16 +616,47 @@ xmlSecBnAdd(xmlSecBnPtr bn, int delta) { } } } else { - for(over = -delta, ii = xmlSecBufferGetSize(bn); (ii > 0) && (over > 0);) { + unsigned int absDelta; + + /* avoid undefined behavior from negating INT_MIN */ + absDelta = (delta == INT_MIN) ? (((unsigned int)INT_MAX) + 1U) : (unsigned int)(-delta); + + size = xmlSecBufferGetSize(bn); + + /* subtract |delta| from the least significant bytes; a borrow that runs + * past the most significant byte (over still non-zero when ii reaches 0) + * means the value was smaller than |delta|, i.e. the result would go below + * zero which is not representable for an unsigned BN */ + over = absDelta; + for(ii = size; (ii > 0) && (over > 0);) { xmlSecAssert2(data != NULL, -1); tmp = data[--ii]; - if(tmp < over) { - data[ii] = 0; - over = (over - tmp) / 256; + byteDelta = over % 256; + over = over / 256; + if(tmp < byteDelta) { + data[ii] = (xmlSecByte)((tmp + 256U) - byteDelta); + ++over; } else { - data[ii] = (xmlSecByte)(tmp - over); - over = 0; + data[ii] = (xmlSecByte)(tmp - byteDelta); + } + } + + if(over > 0) { + xmlSecInvalidIntegerDataError("delta", delta, "value >= |delta| (result must not go below zero)", NULL); + return (-1); + } + + /* trim leading zeros to keep the canonical form, keeping at least one byte */ + size = xmlSecBufferGetSize(bn); + data = xmlSecBufferGetData(bn); + while((size > 1) && (data != NULL) && (data[0] == 0x00)) { + ret = xmlSecBufferRemoveHead(bn, 1); + if(ret < 0) { + xmlSecInternalError("xmlSecBufferRemoveHead(1)", NULL); + return (-1); } + size = xmlSecBufferGetSize(bn); + data = xmlSecBufferGetData(bn); } } return(0); diff --git a/src/buffer.c b/src/buffer.c index 3cb926d57..4888b2c87 100644 --- a/src/buffer.c +++ b/src/buffer.c @@ -19,6 +19,7 @@ #include #include #include +#include #include @@ -282,9 +283,17 @@ xmlSecBufferSetMaxSize(xmlSecBufferPtr buf, xmlSecSize size) { switch(buf->allocMode) { case xmlSecAllocModeExact: + if(size > XMLSEC_SIZE_MAX - 8) { + xmlSecInvalidSizeMoreThanError("size", size, (XMLSEC_SIZE_MAX - 8), NULL); + return(-1); + } newSize = size + 8; break; case xmlSecAllocModeDouble: + if(size > ((XMLSEC_SIZE_MAX - 32) / 2)) { + xmlSecInvalidSizeMoreThanError("size", size, ((XMLSEC_SIZE_MAX - 32) / 2), NULL); + return(-1); + } newSize = 2 * size + 32; break; } diff --git a/src/gnutls/x509vfy.c b/src/gnutls/x509vfy.c index d5c5e0464..7ed2a4b8b 100644 --- a/src/gnutls/x509vfy.c +++ b/src/gnutls/x509vfy.c @@ -92,6 +92,10 @@ static gnutls_x509_crt_t xmlSecGnuTLSX509FindSignerCert (xmlSecP static int xmlSecGnuTLSX509CertCompareSKI (gnutls_x509_crt_t cert, const xmlSecByte * ski, xmlSecSize skiSize); +static int xmlSecGnuTLSX509StoreVerifyCrlInternal (xmlSecKeyDataStorePtr store, + gnutls_x509_crl_t crl, + xmlSecPtrListPtr extra_certs, + const xmlSecKeyInfoCtx* keyInfoCtx); /** * xmlSecGnuTLSX509StoreGetKlass: * @@ -245,6 +249,29 @@ xmlSecGnuTLSX509CheckTime(const gnutls_x509_crt_t * cert_list, return(1); } +static int +xmlSecGnuTLSX509GetVerificationFlags(const xmlSecKeyInfoCtx* keyInfoCtx, + unsigned int* flags) { + xmlSecAssert2(keyInfoCtx != NULL, -1); + xmlSecAssert2(flags != NULL, -1); + + (*flags) = 0; + + /* gnutls doesn't allow to specify "verification" timestamp so + we have to do it ourselves */ + (*flags) |= GNUTLS_VERIFY_DISABLE_TIME_CHECKS; + + if((keyInfoCtx->flags & XMLSEC_KEYINFO_FLAGS_X509DATA_SKIP_STRICT_CHECKS) != 0) { + (*flags) |= GNUTLS_VERIFY_ALLOW_SIGN_RSA_MD2; + (*flags) |= GNUTLS_VERIFY_ALLOW_SIGN_RSA_MD5; +#if GNUTLS_VERSION_NUMBER >= 0x030600 + (*flags) |= GNUTLS_VERIFY_ALLOW_SIGN_WITH_SHA1; +#endif + } + + return(0); +} + /** * xmlSecGnuTLSX509StoreVerify: * @store: the pointer to X509 key data store klass. @@ -268,6 +295,7 @@ xmlSecGnuTLSX509StoreVerify(xmlSecKeyDataStorePtr store, xmlSecSize cert_list_size; gnutls_x509_crl_t * crl_list = NULL; xmlSecSize crl_list_size; + xmlSecSize crl_list_used = 0; gnutls_x509_crt_t * ca_list = NULL; xmlSecSize ca_list_size; time_t verification_time; @@ -308,14 +336,31 @@ xmlSecGnuTLSX509StoreVerify(xmlSecKeyDataStorePtr store, xmlSecKeyDataStoreGetName(store)); goto done; } + /* verify caller-supplied crls before use; fail closed on invalid CRLs */ for(ii = 0; ii < crl_list_size; ++ii) { - crl_list[ii] = xmlSecPtrListGetItem(crls, ii); - if(crl_list[ii] == NULL) { + gnutls_x509_crl_t crl; + int crl_ret; + + crl = xmlSecPtrListGetItem(crls, ii); + if(crl == NULL) { xmlSecInternalError("xmlSecPtrListGetItem(crls)", xmlSecKeyDataStoreGetName(store)); goto done; } + + crl_ret = xmlSecGnuTLSX509StoreVerifyCrlInternal(store, crl, certs, keyInfoCtx); + if(crl_ret < 0) { + xmlSecInternalError("xmlSecGnuTLSX509StoreVerifyCrlInternal", + xmlSecKeyDataStoreGetName(store)); + goto done; + } else if(crl_ret != 1) { + /* crl failed verification: this is a hard failure because we expect CRLs to be valid */ + goto done; + } + crl_list[crl_list_used] = crl; + ++crl_list_used; } + crl_list_size = crl_list_used; } ca_list_size = xmlSecPtrListGetSize(&(ctx->certsTrusted)); @@ -341,14 +386,11 @@ xmlSecGnuTLSX509StoreVerify(xmlSecKeyDataStorePtr store, verification_time = (keyInfoCtx->certsVerificationTime > 0) ? keyInfoCtx->certsVerificationTime : time(0); - flags |= GNUTLS_VERIFY_DISABLE_TIME_CHECKS; - - if((keyInfoCtx->flags & XMLSEC_KEYINFO_FLAGS_X509DATA_SKIP_STRICT_CHECKS) != 0) { - flags |= GNUTLS_VERIFY_ALLOW_SIGN_RSA_MD2; - flags |= GNUTLS_VERIFY_ALLOW_SIGN_RSA_MD5; -#if GNUTLS_VERSION_NUMBER >= 0x030600 - flags |= GNUTLS_VERIFY_ALLOW_SIGN_WITH_SHA1; -#endif + ret = xmlSecGnuTLSX509GetVerificationFlags(keyInfoCtx, &flags); + if(ret < 0) { + xmlSecInternalError("xmlSecGnuTLSX509GetVerificationFlags", + xmlSecKeyDataStoreGetName(store)); + goto done; } /* We are going to build all possible cert chains and try to verify them */ @@ -452,6 +494,431 @@ xmlSecGnuTLSX509StoreVerify(xmlSecKeyDataStorePtr store, return(res); } +/* Verify that the @p issuer_cert chain (built from the store's untrusted certs) + * reaches a trusted cert: 1 if verified, 0 if not verified, < 0 if error */ +static int +xmlSecGnuTLSX509StoreVerifyIssuerCert(xmlSecGnuTLSX509StoreCtxPtr ctx, + gnutls_x509_crt_t issuer_cert, + const xmlSecKeyInfoCtx* keyInfoCtx) { + gnutls_x509_crt_t* chain = NULL; + xmlSecSize chain_size, chain_cur_size = 0; + gnutls_x509_crt_t* ca_list = NULL; + xmlSecSize ca_list_size = 0; + time_t verification_time; + unsigned int flags = 0; + unsigned int verify = 0; + xmlSecSize ii; + int err; + int ret; + int res = -1; + + xmlSecAssert2(ctx != NULL, -1); + xmlSecAssert2(issuer_cert != NULL, -1); + xmlSecAssert2(keyInfoCtx != NULL, -1); + + /* do we even need to verify the cert? */ + if((keyInfoCtx->flags & XMLSEC_KEYINFO_FLAGS_X509DATA_DONT_VERIFY_CERTS) != 0) { + return(1); + } + + ca_list_size = xmlSecPtrListGetSize(&(ctx->certsTrusted)); + if(ca_list_size <= 0) { + res = 0; + goto done; + } + ca_list = (gnutls_x509_crt_t *)xmlMalloc(sizeof(gnutls_x509_crt_t) * ca_list_size); + if(ca_list == NULL) { + xmlSecMallocError(sizeof(gnutls_x509_crt_t) * ca_list_size, NULL); + goto done; + } + for(ii = 0; ii < ca_list_size; ++ii) { + ca_list[ii] = xmlSecPtrListGetItem(&(ctx->certsTrusted), ii); + if(ca_list[ii] == NULL) { + xmlSecInternalError("xmlSecPtrListGetItem(certsTrusted)", NULL); + goto done; + } + } + + /* build the issuer cert chain using the store's untrusted certs */ + chain_size = xmlSecPtrListGetSize(&(ctx->certsUntrusted)) + 1; + chain = (gnutls_x509_crt_t*)xmlMalloc(sizeof(gnutls_x509_crt_t) * chain_size); + if(chain == NULL) { + xmlSecMallocError(sizeof(gnutls_x509_crt_t) * chain_size, NULL); + goto done; + } + chain[0] = issuer_cert; + chain_cur_size = 1; + if((xmlSecGnuTLSX509CertIsSelfSigned(issuer_cert) != 1) && (chain_size > 1)) { + gnutls_x509_crt_t cert = issuer_cert; + + for(ii = 1; ii < chain_size; ++ii) { + gnutls_x509_crt_t tmp; + + tmp = xmlSecGnuTLSX509FindSignerCert(&(ctx->certsUntrusted), cert); + if((tmp == NULL) || (tmp == cert)) { + break; + } + chain[ii] = tmp; + chain_cur_size = ii + 1; + cert = tmp; + } + } + + ret = xmlSecGnuTLSX509GetVerificationFlags(keyInfoCtx, &flags); + if(ret < 0) { + xmlSecInternalError("xmlSecGnuTLSX509GetVerificationFlags", NULL); + goto done; + } + + { + unsigned int chain_len, ca_list_len; + + XMLSEC_SAFE_CAST_SIZE_TO_UINT(chain_cur_size, chain_len, goto done, NULL); + XMLSEC_SAFE_CAST_SIZE_TO_UINT(ca_list_size, ca_list_len, goto done, NULL); + + err = gnutls_x509_crt_list_verify( + chain, chain_len, + ca_list, ca_list_len, + NULL, 0, + flags, + &verify); + } + if(err != GNUTLS_E_SUCCESS) { + xmlSecGnuTLSError("gnutls_x509_crt_list_verify", err, NULL); + goto done; + } else if(verify != 0) { + xmlSecOtherError2(XMLSEC_ERRORS_R_CERT_VERIFY_FAILED, NULL, + "gnutls_x509_crt_list_verify: verification failed: status=%u", verify); + res = 0; + goto done; + } + + /* gnutls doesn't allow to specify "verification" timestamp so + we have to do it ourselves */ + verification_time = (keyInfoCtx->certsVerificationTime > 0) ? + keyInfoCtx->certsVerificationTime : + time(0); + ret = xmlSecGnuTLSX509CheckTime(chain, chain_cur_size, verification_time); + if(ret < 0) { + xmlSecInternalError("xmlSecGnuTLSX509CheckTime", NULL); + goto done; + } else if(ret != 1) { + /* issuer cert not valid at the verification time */ + res = 0; + goto done; + } + + /* done! */ + res = 1; + +done: + /* cleanup */ + if(chain != NULL) { + xmlFree(chain); + } + if(ca_list != NULL) { + xmlFree(ca_list); + } + return(res); +} + +/* Verify CRL time validity: 1 if valid, 0 if not valid, < 0 if error */ +static int +xmlSecGnuTLSX509StoreVerifyCrlTimeValidity(gnutls_x509_crl_t crl, + const xmlSecKeyInfoCtx* keyInfoCtx, + const xmlChar* storeName) { + time_t this_update, next_update, verification_time; + int err; + + xmlSecAssert2(crl != NULL, -1); + xmlSecAssert2(keyInfoCtx != NULL, -1); + + /* Get verification time */ + if(keyInfoCtx->certsVerificationTime > 0) { + verification_time = keyInfoCtx->certsVerificationTime; + } else { + verification_time = time(NULL); + if(verification_time == (time_t)-1) { + xmlSecInternalError("time", storeName); + return(-1); + } + } + + /* Verify this_update */ + this_update = gnutls_x509_crl_get_this_update(crl); + if(this_update == (time_t)-1) { + xmlSecInternalError("gnutls_x509_crl_get_this_update (failed to get CRL thisUpdate time)", + storeName); + return(-1); + } + + if(this_update > verification_time) { + /* CRL not yet valid */ + size_t dn_size = 256; + char dn_buf[256]; + + err = gnutls_x509_crl_get_issuer_dn(crl, dn_buf, &dn_size); + if(err == GNUTLS_E_SUCCESS) { + xmlSecOtherError2(XMLSEC_ERRORS_R_CERT_NOT_YET_VALID, storeName, + "issuer=%s", dn_buf); + } else { + xmlSecOtherError(XMLSEC_ERRORS_R_CERT_NOT_YET_VALID, storeName, NULL); + } + return(0); + } + + /* Verify next_update + * gnutls_x509_crl_get_next_update() returns (time_t)-1 both on error and when the + * nextUpdate field is absent; a missing nextUpdate means the CRL has no expiration + * (RFC 5280), so the expiration check is skipped in that case. */ + next_update = gnutls_x509_crl_get_next_update(crl); + if(next_update != (time_t)-1) { + if(next_update < verification_time) { + /* CRL expired */ + size_t dn_size = 256; + char dn_buf[256]; + + err = gnutls_x509_crl_get_issuer_dn(crl, dn_buf, &dn_size); + if(err == GNUTLS_E_SUCCESS) { + xmlSecOtherError2(XMLSEC_ERRORS_R_CERT_HAS_EXPIRED, storeName, + "issuer=%s", dn_buf); + } else { + xmlSecOtherError(XMLSEC_ERRORS_R_CERT_HAS_EXPIRED, storeName, NULL); + } + return(0); + } + } + + /* Success */ + return(1); +} + +/* + * Find the certificate in @p certs that issued @p crl (checked with + * gnutls_x509_crl_check_issuer). If @p verify_issuer is non-zero, a found + * certificate is only accepted when its own chain verifies via + * xmlSecGnuTLSX509StoreVerifyIssuerCert. + * + * Returns: 1 if an issuer cert is found (stored in @p issuer_cert), 0 if no + * cert in @p certs issued @p crl, < 0 on error. + */ +static int +xmlSecGnuTLSX509StoreFindCrlIssuerCert(xmlSecGnuTLSX509StoreCtxPtr ctx, + xmlSecPtrListPtr certs, + gnutls_x509_crl_t crl, + int verify_issuer, + const xmlSecKeyInfoCtx* keyInfoCtx, + const xmlChar* storeName, + gnutls_x509_crt_t* issuer_cert) { + xmlSecSize certs_size, ii; + gnutls_x509_crt_t cert; + unsigned int is_issuer; + int ret; + + xmlSecAssert2(certs != NULL, -1); + xmlSecAssert2(crl != NULL, -1); + xmlSecAssert2(keyInfoCtx != NULL, -1); + xmlSecAssert2(issuer_cert != NULL, -1); + + certs_size = xmlSecPtrListGetSize(certs); + for(ii = 0; ii < certs_size; ++ii) { + cert = xmlSecPtrListGetItem(certs, ii); + if(cert == NULL) { + continue; + } + + is_issuer = gnutls_x509_crl_check_issuer(crl, cert); + if(is_issuer == 0) { + continue; + } + + if(verify_issuer != 0) { + /* the issuer cert's chain must verify before the cert can be used */ + ret = xmlSecGnuTLSX509StoreVerifyIssuerCert(ctx, cert, keyInfoCtx); + if(ret < 0) { + xmlSecInternalError("xmlSecGnuTLSX509StoreVerifyIssuerCert", storeName); + return(-1); + } else if(ret != 1) { + continue; + } + } + + (*issuer_cert) = cert; + return(1); + } + + return(0); +} + +/* Verify CRL signature: 1 if verified, 0 if not verified, < 0 if error */ +static int +xmlSecGnuTLSX509StoreVerifyCrlSignature(xmlSecGnuTLSX509StoreCtxPtr ctx, + gnutls_x509_crl_t crl, + xmlSecPtrListPtr extra_certs, + const xmlSecKeyInfoCtx* keyInfoCtx, + const xmlChar* storeName) { + gnutls_x509_crt_t issuer_cert = NULL; + xmlChar *issuer_dn = NULL; + unsigned int verify_result = 0; + unsigned int flags = 0; + int err; + int res = -1; + int ret; + + xmlSecAssert2(ctx != NULL, -1); + xmlSecAssert2(crl != NULL, -1); + xmlSecAssert2(keyInfoCtx != NULL, -1); + + /* Find the issuer certificate: search trusted certs first (no need to verify trusted issuer certs). */ + if(issuer_cert == NULL) { + ret = xmlSecGnuTLSX509StoreFindCrlIssuerCert(ctx, &(ctx->certsTrusted), crl, 0, + keyInfoCtx, storeName, &issuer_cert); + if(ret < 0) { + xmlSecInternalError("xmlSecGnuTLSX509StoreFindCrlIssuerCert(trusted)", storeName); + goto done; + } + } + + /* Then untrusted certs and make sure their chain verifies. */ + if(issuer_cert == NULL) { + ret = xmlSecGnuTLSX509StoreFindCrlIssuerCert(ctx, &(ctx->certsUntrusted), crl, 1, + keyInfoCtx, storeName, &issuer_cert); + if(ret < 0) { + xmlSecInternalError("xmlSecGnuTLSX509StoreFindCrlIssuerCert(untrusted)", storeName); + goto done; + } + } + + /* And finally caller-supplied (e.g. KeyInfo) certs and make sure their chain verifies. */ + if((issuer_cert == NULL) && (extra_certs != NULL)) { + ret = xmlSecGnuTLSX509StoreFindCrlIssuerCert(ctx, extra_certs, crl, 1, + keyInfoCtx, storeName, &issuer_cert); + if(ret < 0) { + xmlSecInternalError("xmlSecGnuTLSX509StoreFindCrlIssuerCert(extra_certs)", storeName); + goto done; + } + } + + if(issuer_cert == NULL) { + /* Try to get issuer DN for error message */ + issuer_dn = xmlSecGnuTLSX509CrlGetIssuerDN(crl); + if(issuer_dn != NULL) { + xmlSecOtherError2(XMLSEC_ERRORS_R_CERT_NOT_FOUND, storeName, + "issuer=%s", xmlSecErrorsSafeString(issuer_dn)); + } else { + xmlSecOtherError(XMLSEC_ERRORS_R_CERT_NOT_FOUND, storeName, NULL); + } + res = 0; + goto done; + } + + ret = xmlSecGnuTLSX509GetVerificationFlags(keyInfoCtx, &flags); + if(ret < 0) { + xmlSecInternalError("xmlSecGnuTLSX509GetVerificationFlags", storeName); + goto done; + } + + err = gnutls_x509_crl_verify(crl, &issuer_cert, 1, flags, &verify_result); + if(err != GNUTLS_E_SUCCESS) { + xmlSecGnuTLSError("gnutls_x509_crl_verify", err, storeName); + goto done; + } + + /* gnutls_x509_crl_verify always compares the CRL's thisUpdate/nextUpdate + * against the current time (not gated by GNUTLS_VERIFY_DISABLE_TIME_CHECKS). + * When a verification timestamp is set, time validity was already checked + * against that timestamp by xmlSecGnuTLSX509StoreVerifyCrlTimeValidity, so + * ignore the time-based failure flags here. + */ +#if GNUTLS_VERSION_NUMBER >= 0x030200 + if(keyInfoCtx->certsVerificationTime > 0) { + const unsigned int ignored_verify_result = + (unsigned int)(GNUTLS_CERT_REVOCATION_DATA_ISSUED_IN_FUTURE | + GNUTLS_CERT_REVOCATION_DATA_SUPERSEDED); + if((verify_result & ignored_verify_result) != 0) { + /* + * gnutls_x509_crl_verify() also sets GNUTLS_CERT_INVALID when any + * specific status flag is present. If we want to ignore all specific + * flags above, then GNUTLS_CERT_INVALID must be ignored too. + */ + verify_result &= ~(ignored_verify_result | (unsigned int)GNUTLS_CERT_INVALID); + } + } +#endif /* GNUTLS_VERSION_NUMBER >= 0x030200 */ + + /* Check if verification failed */ + if(verify_result != 0) { + /* CRL verification failed - try to get issuer DN for error message */ + if(issuer_dn == NULL) { + issuer_dn = xmlSecGnuTLSX509CrlGetIssuerDN(crl); + } + if(issuer_dn != NULL) { + xmlSecOtherError3(XMLSEC_ERRORS_R_CRL_VERIFY_FAILED, storeName, + "verify_result=%u; issuer=%s", verify_result, xmlSecErrorsSafeString(issuer_dn)); + } else { + xmlSecOtherError2(XMLSEC_ERRORS_R_CRL_VERIFY_FAILED, storeName, + "verify_result=%u", verify_result); + } + res = 0; + goto done; + } + + /* Success */ + res = 1; + +done: + if(issuer_dn != NULL) { + xmlFree(issuer_dn); + } + return(res); +} + +/* Verifies a CRL (time validity first, then signature), searching @p extra_certs + * for the CRL issuer: 1 if verified, 0 if not verified, < 0 if error */ +static int +xmlSecGnuTLSX509StoreVerifyCrlInternal(xmlSecKeyDataStorePtr store, + gnutls_x509_crl_t crl, + xmlSecPtrListPtr extra_certs, + const xmlSecKeyInfoCtx* keyInfoCtx) { + xmlSecGnuTLSX509StoreCtxPtr ctx; + int ret; + + xmlSecAssert2(xmlSecKeyDataStoreCheckId(store, xmlSecGnuTLSX509StoreId), -1); + xmlSecAssert2(crl != NULL, -1); + xmlSecAssert2(keyInfoCtx != NULL, -1); + + /* do we even need to verify the CRL? */ + if((keyInfoCtx->flags & XMLSEC_KEYINFO_FLAGS_X509DATA_DONT_VERIFY_CERTS) != 0) { + return(1); + } + + ctx = xmlSecGnuTLSX509StoreGetCtx(store); + xmlSecAssert2(ctx != NULL, -1); + + /* Verify time validity first (fast check) */ + ret = xmlSecGnuTLSX509StoreVerifyCrlTimeValidity(crl, keyInfoCtx, xmlSecKeyDataStoreGetName(store)); + if(ret < 0) { + xmlSecInternalError("xmlSecGnuTLSX509StoreVerifyCrlTimeValidity", xmlSecKeyDataStoreGetName(store)); + return(-1); + } else if(ret != 1) { + /* Time validity check failed */ + return(0); + } + + /* Verify CRL signature (slower check) */ + ret = xmlSecGnuTLSX509StoreVerifyCrlSignature(ctx, crl, extra_certs, keyInfoCtx, xmlSecKeyDataStoreGetName(store)); + if(ret < 0) { + xmlSecInternalError("xmlSecGnuTLSX509StoreVerifyCrlSignature", xmlSecKeyDataStoreGetName(store)); + return(-1); + } else if(ret != 1) { + /* Signature verification failed */ + return(0); + } + + /* Success: verified */ + return(1); +} + /** * xmlSecGnuTLSX509StoreAdoptCert: * @store: the pointer to X509 key data store klass. diff --git a/src/keys.c b/src/keys.c index 4633bd90b..eca88573c 100644 --- a/src/keys.c +++ b/src/keys.c @@ -163,7 +163,7 @@ xmlSecKeyUseWithDuplicate(xmlSecKeyUseWithPtr keyUseWith) { ret = xmlSecKeyUseWithCopy(newKeyUseWith, keyUseWith); if(ret < 0) { xmlSecInternalError("xmlSecKeyUseWithCopy", NULL); - xmlSecKeyUseWithDestroy(keyUseWith); + xmlSecKeyUseWithDestroy(newKeyUseWith); return(NULL); } diff --git a/src/keysmngr.c b/src/keysmngr.c index f58905ab8..4ce4a7033 100644 --- a/src/keysmngr.c +++ b/src/keysmngr.c @@ -133,6 +133,10 @@ xmlSecKeysMngrAdoptKeysStore(xmlSecKeysMngrPtr mngr, xmlSecKeyStorePtr store) { xmlSecAssert2(mngr != NULL, -1); xmlSecAssert2(xmlSecKeyStoreIsValid(store), -1); + if(mngr->keysStore == store) { + /* already adopted, nothing to do */ + return(0); + } if(mngr->keysStore != NULL) { xmlSecKeyStoreDestroy(mngr->keysStore); } diff --git a/src/mscng/certkeys.c b/src/mscng/certkeys.c index d9412696b..d3aeb085b 100644 --- a/src/mscng/certkeys.c +++ b/src/mscng/certkeys.c @@ -48,6 +48,7 @@ struct _xmlSecMSCngKeyDataCtx { PCCERT_CONTEXT cert; NCRYPT_KEY_HANDLE privkey; BCRYPT_KEY_HANDLE pubkey; + BOOL privkeyNeedsFree; }; XMLSEC_KEY_DATA_DECLARE(MSCngKeyData, xmlSecMSCngKeyDataCtx) @@ -73,11 +74,13 @@ xmlSecMSCngKeyDataCertGetPubkey(PCCERT_CONTEXT cert, BCRYPT_KEY_HANDLE* key) { } static int -xmlSecMSCngKeyDataCertGetPrivkey(PCCERT_CONTEXT cert, NCRYPT_KEY_HANDLE* key) { +xmlSecMSCngKeyDataCertGetPrivkey(PCCERT_CONTEXT cert, NCRYPT_KEY_HANDLE* key, + BOOL* needsFree) { int ret; xmlSecAssert2(cert != NULL, -1); xmlSecAssert2(key != NULL, -1); + xmlSecAssert2(needsFree != NULL, -1); DWORD keySpec = 0; BOOL callerFree = FALSE; @@ -94,6 +97,8 @@ xmlSecMSCngKeyDataCertGetPrivkey(PCCERT_CONTEXT cert, NCRYPT_KEY_HANDLE* key) { return(-1); } + (*needsFree) = callerFree; + return(0); } @@ -125,15 +130,17 @@ xmlSecMSCngKeyDataAdoptCert(xmlSecKeyDataPtr data, PCCERT_CONTEXT cert, xmlSecKe /* acquire the CNG key handle from the certificate */ if((type & xmlSecKeyDataTypePrivate) != 0) { - NCRYPT_KEY_HANDLE hPrivKey; + NCRYPT_KEY_HANDLE hPrivKey = 0; + BOOL needsFree = TRUE; - ret = xmlSecMSCngKeyDataCertGetPrivkey(cert, &hPrivKey); + ret = xmlSecMSCngKeyDataCertGetPrivkey(cert, &hPrivKey, &needsFree); if(ret < 0) { xmlSecInternalError("xmlSecMSCngKeyDataCertGetPrivkey", NULL); return(-1); } ctx->privkey = hPrivKey; + ctx->privkeyNeedsFree = needsFree; } ret = xmlSecMSCngKeyDataCertGetPubkey(cert, &hPubKey); @@ -301,7 +308,7 @@ xmlSecMSCngKeyDataFinalize(xmlSecKeyDataPtr data) { ctx = xmlSecMSCngKeyDataGetCtx(data); xmlSecAssert(ctx != NULL); - if(ctx->privkey != 0) { + if((ctx->privkey != 0) && (ctx->privkeyNeedsFree == TRUE)) { status = NCryptFreeObject(ctx->privkey); if(status != STATUS_SUCCESS) { xmlSecMSCngNtError("BCryptDestroyKey", NULL, status); @@ -358,7 +365,8 @@ xmlSecMSCngKeyDataDuplicate(xmlSecKeyDataPtr dst, xmlSecKeyDataPtr src) { } if(srcCtx->privkey != 0) { - ret = xmlSecMSCngKeyDataCertGetPrivkey(dstCtx->cert, &dstCtx->privkey); + ret = xmlSecMSCngKeyDataCertGetPrivkey(dstCtx->cert, &dstCtx->privkey, + &dstCtx->privkeyNeedsFree); if(ret < 0) { xmlSecInternalError("xmlSecMSCngKeyDataCertGetPrivkey", NULL); return(-1); diff --git a/src/mscng/hmac.c b/src/mscng/hmac.c index ddbc598d8..c76d480b3 100644 --- a/src/mscng/hmac.c +++ b/src/mscng/hmac.c @@ -255,6 +255,9 @@ xmlSecMSCngHmacSetKey(xmlSecTransformPtr transform, xmlSecKeyPtr key) { BCRYPT_ALG_HANDLE_HMAC_FLAG); if(status != STATUS_SUCCESS) { xmlSecMSCngNtError("BCryptOpenAlgorithmProvider", xmlSecTransformGetName(transform), status); + /* the out-handle is not guaranteed to be zeroed on failure; reset it so + * finalize() does not attempt to close an indeterminate handle */ + ctx->hAlg = NULL; return(-1); } @@ -266,17 +269,17 @@ xmlSecMSCngHmacSetKey(xmlSecTransformPtr transform, xmlSecKeyPtr key) { 0); if(status != STATUS_SUCCESS) { xmlSecMSCngNtError("BCryptGetProperty", xmlSecTransformGetName(transform), status); - return(-1); + goto done; } ctx->hash = (PBYTE)xmlMalloc(ctx->hashLength); if(ctx->hash == NULL) { xmlSecMallocError(ctx->hashLength, NULL); - return(-1); + goto done; } bufSize = xmlSecBufferGetSize(buffer); - XMLSEC_SAFE_CAST_SIZE_TO_ULONG(bufSize, dwBufSize, return(-1), xmlSecTransformGetName(transform)); + XMLSEC_SAFE_CAST_SIZE_TO_ULONG(bufSize, dwBufSize, goto done, xmlSecTransformGetName(transform)); status = BCryptCreateHash(ctx->hAlg, &ctx->hHash, NULL, @@ -286,16 +289,36 @@ xmlSecMSCngHmacSetKey(xmlSecTransformPtr transform, xmlSecKeyPtr key) { 0); if(status != STATUS_SUCCESS) { xmlSecMSCngNtError("BCryptCreateHash", xmlSecTransformGetName(transform), status); - return(-1); + goto done; } if (ctx->dgstSize == 0) { /* no custom value is requested, then default to the full length */ ctx->dgstSize = ctx->hashLength * 8; + } else if (ctx->dgstSize > ((xmlSecSize)ctx->hashLength * 8)) { + /* reject oversized values: they would cause out-of-bounds reads when + the truncated digest buffer is accessed in verify/sign paths */ + xmlSecInvalidSizeMoreThanError("HMAC digest size (bits)", + ctx->dgstSize, ((xmlSecSize)ctx->hashLength * 8), + xmlSecTransformGetName(transform)); + goto done; } ctx->initialized = 1; return(0); + +done: + if(ctx->hash != NULL) { + xmlFree(ctx->hash); + } + if(ctx->hHash != NULL) { + BCryptDestroyHash(ctx->hHash); + } + if(ctx->hAlg != NULL) { + BCryptCloseAlgorithmProvider(ctx->hAlg, 0); + } + memset(ctx, 0, sizeof(xmlSecMSCngHmacCtx)); + return(-1); } static int diff --git a/src/mscng/x509vfy.c b/src/mscng/x509vfy.c index 90dd25991..3e577cb3d 100644 --- a/src/mscng/x509vfy.c +++ b/src/mscng/x509vfy.c @@ -501,12 +501,77 @@ xmlSecMSCngVerifyCertTime(PCCERT_CONTEXT cert, LPFILETIME time) { return(0); } +static PCCERT_CONTEXT +xmlSecMSCngX509StoreFindIssuer(HCERTSTORE store, PCCERT_CONTEXT cert, + xmlSecKeyDataStorePtr keyDataStore) { + PCCERT_CONTEXT issuerCert = NULL; + int ret; + + xmlSecAssert2(store != NULL, NULL); + xmlSecAssert2(cert != NULL, NULL); + xmlSecAssert2(keyDataStore != NULL, NULL); + + while (TRUE) { + /* CertFindCertificateInStore automatically frees the previous certificate context (see + * https://learn.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-certfindcertificateinstore) */ + issuerCert = CertFindCertificateInStore(store, + X509_ASN_ENCODING | PKCS_7_ASN_ENCODING, + 0, + CERT_FIND_SUBJECT_NAME, + &(cert->pCertInfo->Issuer), + issuerCert); + if (issuerCert == NULL) { + return(NULL); + } + + ret = xmlSecMSCngX509StoreVerifySubject(cert, issuerCert); + if (ret < 0) { + xmlSecInternalError("xmlSecMSCngX509StoreVerifySubject", NULL); + continue; + } else if (ret == 0) { + xmlSecOtherError(XMLSEC_ERRORS_R_CERT_VERIFY_FAILED, + xmlSecKeyDataStoreGetName(keyDataStore), + "xmlSecMSCngX509StoreVerifySubject"); + continue; + } + + /* success */ + return(issuerCert); + } +} + +struct xmlSecMSCngX509StoreVerifyCertificateChainStep { + PCCERT_CONTEXT cert; + BOOL freeCert; +}; +#define XMLSEC_MSCNG_X509_STORE_VERIFY_CERTIFICATE_CHAIN_STEP_SIZE 32 +#define XMLSEC_MSCNG_X509_STORE_VERIFY_CERTIFICATE_CHAIN_MAX_DEPTH 100 +#define XMLSEC_MSCNG_X509_CERT_HASH_SIZE 20 + +/* Returns the SHA1 hash of @p pCert in @p pHash. Returns 0 on success, -1 on error. */ +static int +xmlSecMSCngX509GetCertHash(PCCERT_CONTEXT pCert, BYTE* pHash, DWORD* hashSize) { + BOOL ret; + + xmlSecAssert2(pCert != NULL, -1); + xmlSecAssert2(pHash != NULL, -1); + xmlSecAssert2(hashSize != NULL, -1); + + ret = CertGetCertificateContextProperty(pCert, CERT_HASH_PROP_ID, pHash, hashSize); + if((ret == FALSE) || (*hashSize != (DWORD)XMLSEC_MSCNG_X509_CERT_HASH_SIZE)) { + xmlSecMSCngLastError("CertGetCertificateContextProperty(CERT_HASH_PROP_ID)", NULL); + return(-1); + } + return(0); +} + /** * xmlSecMSCngX509StoreVerifyCertificateOwn: * @cert: the certificate to verify. * @time: pointer to FILETIME that we are interested in * @trustedStore: trusted certificates added via xmlSecMSCngX509StoreAdoptCert(). - * @certStore: the untrusted certificates stack. + * @untrustedStore: the untrusted certificates stack. + * @certStore: the certificates stack from the document. * @store: key data store, name used for error reporting only. * * Verifies @cert based on trustedStore (ignoring system trusted certificates). @@ -517,7 +582,15 @@ static int xmlSecMSCngX509StoreVerifyCertificateOwn(PCCERT_CONTEXT cert, FILETIME* time, HCERTSTORE trustedStore, HCERTSTORE untrustedStore, HCERTSTORE certStore, xmlSecKeyDataStorePtr store) { - PCCERT_CONTEXT issuerCert = NULL; + struct xmlSecMSCngX509StoreVerifyCertificateChainStep * queue = NULL; + xmlSecSize queueSize = 0, queueMaxSize = 0; + BYTE seenHashes[XMLSEC_MSCNG_X509_STORE_VERIFY_CERTIFICATE_CHAIN_MAX_DEPTH][XMLSEC_MSCNG_X509_CERT_HASH_SIZE]; + xmlSecSize seenSize = 0; + BYTE hash[XMLSEC_MSCNG_X509_CERT_HASH_SIZE]; + DWORD hashSize; + PCCERT_CONTEXT currentCert = NULL; + BOOL freeCurrentCert = FALSE; + int res = -1; int ret; xmlSecAssert2(cert != NULL, -1); @@ -525,120 +598,172 @@ xmlSecMSCngX509StoreVerifyCertificateOwn(PCCERT_CONTEXT cert, xmlSecAssert2(certStore != NULL, -1); xmlSecAssert2(store != NULL, -1); - /* check certificate validity and revokation */ - ret = xmlSecMSCngVerifyCertTime(cert, time); - if(ret < 0) { - xmlSecInternalError("xmlSecMSCngVerifyCertTime", - xmlSecKeyDataStoreGetName(store)); + /* setup queue */ + queue = (struct xmlSecMSCngX509StoreVerifyCertificateChainStep*)xmlMalloc( + sizeof(struct xmlSecMSCngX509StoreVerifyCertificateChainStep) * XMLSEC_MSCNG_X509_STORE_VERIFY_CERTIFICATE_CHAIN_STEP_SIZE); + if(queue == NULL) { + xmlSecMallocError( + sizeof(struct xmlSecMSCngX509StoreVerifyCertificateChainStep) * XMLSEC_MSCNG_X509_STORE_VERIFY_CERTIFICATE_CHAIN_STEP_SIZE, NULL); return(-1); } + queueMaxSize = XMLSEC_MSCNG_X509_STORE_VERIFY_CERTIFICATE_CHAIN_STEP_SIZE; - ret = xmlSecMSCngCheckRevocation(certStore, cert); - if(ret < 0) { - xmlSecInternalError("xmlSecMSCngCheckRevocation", - xmlSecKeyDataStoreGetName(store)); - return(-1); - } + queue[0].cert = cert; + queue[0].freeCert = FALSE; + queueSize = 1; - /* does trustedStore contain cert directly? */ - ret = xmlSecMSCngX509StoreContainsCert(trustedStore, - &(cert->pCertInfo->Subject), cert); - if(ret < 0) { - xmlSecInternalError("xmlSecMSCngX509StoreContainsCert", - xmlSecKeyDataStoreGetName(store)); - return(-1); - } else if(ret == 1) { - /* success */ - return(0); - } + while(queueSize > 0) { + PCCERT_CONTEXT issuerCert = NULL; + xmlSecSize ii; + BOOL alreadySeen = FALSE; - /* does trustedStore contain the issuer cert? */ - ret = xmlSecMSCngX509StoreContainsCert(trustedStore, - &(cert->pCertInfo->Issuer), cert); - if(ret < 0) { - xmlSecInternalError("xmlSecMSCngX509StoreContainsCert", - xmlSecKeyDataStoreGetName(store)); - return(-1); - } else if(ret == 1) { - /* success */ - return(0); - } + currentCert = queue[queueSize - 1].cert; + freeCurrentCert = queue[queueSize - 1].freeCert; + --queueSize; - /* is cert self-signed? no recursion in that case */ - if(CertCompareCertificateName(X509_ASN_ENCODING | PKCS_7_ASN_ENCODING, - &(cert->pCertInfo->Subject), - &(cert->pCertInfo->Issuer))) { - /* not verified */ - return(-1); - } + /* limit the chain depth to avoid excessive work on crafted inputs */ + if(seenSize >= XMLSEC_MSCNG_X509_STORE_VERIFY_CERTIFICATE_CHAIN_MAX_DEPTH) { + xmlSecOtherError(XMLSEC_ERRORS_R_CERT_VERIFY_FAILED, + xmlSecKeyDataStoreGetName(store), + "certificate chain is too deep"); + goto done; + } - /* the same checks recursively for the issuer cert in certStore */ - issuerCert = CertFindCertificateInStore(certStore, - X509_ASN_ENCODING | PKCS_7_ASN_ENCODING, - 0, - CERT_FIND_SUBJECT_NAME, - &(cert->pCertInfo->Issuer), - NULL); - if(issuerCert != NULL) { - ret = xmlSecMSCngX509StoreVerifySubject(cert, issuerCert); - if (ret < 0) { - xmlSecInternalError("xmlSecMSCngX509StoreVerifySubject", NULL); - CertFreeCertificateContext(issuerCert); - return(-1); + /* cycle detection: make sure we have not seen this certificate before */ + hashSize = sizeof(hash); + ret = xmlSecMSCngX509GetCertHash(currentCert, hash, &hashSize); + if((ret < 0) || (hashSize != XMLSEC_MSCNG_X509_CERT_HASH_SIZE)) { + xmlSecInternalError("xmlSecMSCngX509GetCertHash", NULL); + goto done; } - else if (ret == 0) { - xmlSecOtherError(XMLSEC_ERRORS_R_CERT_VERIFY_FAILED, - NULL, - "xmlSecMSCngX509StoreVerifySubject"); - CertFreeCertificateContext(issuerCert); - return(-1); + for(ii = 0; ii < seenSize; ++ii) { + if(memcmp(&seenHashes[ii], hash, XMLSEC_MSCNG_X509_CERT_HASH_SIZE) == 0) { + alreadySeen = TRUE; + break; + } + } + if(alreadySeen) { + /* The same certificate can be reached through multiple stores/branches; + * we only need to process each cert once. */ + if(freeCurrentCert == TRUE) { + CertFreeCertificateContext(currentCert); + } + currentCert = NULL; + freeCurrentCert = FALSE; + continue; } - ret = xmlSecMSCngX509StoreVerifyCertificateOwn(issuerCert, time, - trustedStore, untrustedStore, certStore, store); + /* remember this certificate */ + memcpy(&seenHashes[seenSize], hash, XMLSEC_MSCNG_X509_CERT_HASH_SIZE); + ++seenSize; + + /* check certificate validity and revokation */ + ret = xmlSecMSCngVerifyCertTime(currentCert, time); if(ret < 0) { - xmlSecInternalError("xmlSecMSCngX509StoreVerifyCertificateOwn", xmlSecKeyDataStoreGetName(store)); - CertFreeCertificateContext(issuerCert); - return(-1); + xmlSecInternalError("xmlSecMSCngVerifyCertTime", + xmlSecKeyDataStoreGetName(store)); + goto done; } - CertFreeCertificateContext(issuerCert); - return(0); - } - /* the same checks recursively for the issuer cert in untrustedStore */ - issuerCert = CertFindCertificateInStore(untrustedStore, - X509_ASN_ENCODING | PKCS_7_ASN_ENCODING, - 0, - CERT_FIND_SUBJECT_NAME, - &(cert->pCertInfo->Issuer), - NULL); - if(issuerCert != NULL) { - ret = xmlSecMSCngX509StoreVerifySubject(cert, issuerCert); - if (ret < 0) { - xmlSecInternalError("xmlSecMSCngX509StoreVerifySubject", NULL); - CertFreeCertificateContext(issuerCert); - return(-1); + ret = xmlSecMSCngCheckRevocation(certStore, currentCert); + if(ret < 0) { + xmlSecInternalError("xmlSecMSCngCheckRevocation", + xmlSecKeyDataStoreGetName(store)); + goto done; } - else if (ret == 0) { - xmlSecOtherError(XMLSEC_ERRORS_R_CERT_VERIFY_FAILED, - NULL, - "xmlSecMSCngX509StoreVerifySubject"); - CertFreeCertificateContext(issuerCert); - return(-1); + + /* does trustedStore contain cert directly? */ + ret = xmlSecMSCngX509StoreContainsCert(trustedStore, + &(currentCert->pCertInfo->Subject), currentCert); + if(ret < 0) { + xmlSecInternalError("xmlSecMSCngX509StoreContainsCert", + xmlSecKeyDataStoreGetName(store)); + goto done; + } else if(ret == 1) { + /* success */ + res = 0; + goto done; } - ret = xmlSecMSCngX509StoreVerifyCertificateOwn(issuerCert, time, - trustedStore, untrustedStore, certStore, store); + /* does trustedStore contain the issuer cert? */ + ret = xmlSecMSCngX509StoreContainsCert(trustedStore, + &(currentCert->pCertInfo->Issuer), currentCert); if(ret < 0) { - xmlSecInternalError("xmlSecMSCngX509StoreVerifyCertificateOwn", xmlSecKeyDataStoreGetName(store)); - CertFreeCertificateContext(issuerCert); - return(-1); + xmlSecInternalError("xmlSecMSCngX509StoreContainsCert", + xmlSecKeyDataStoreGetName(store)); + goto done; + } else if(ret == 1) { + /* success */ + res = 0; + goto done; } - CertFreeCertificateContext(issuerCert); - return(0); + + /* is cert self-signed? no further chain building in that case */ + if(CertCompareCertificateName(X509_ASN_ENCODING | PKCS_7_ASN_ENCODING, + &(currentCert->pCertInfo->Subject), + &(currentCert->pCertInfo->Issuer)) == FALSE + ) { + /* we need space for at most 2 certificates */ + if(queueSize + 2 > queueMaxSize) { + struct xmlSecMSCngX509StoreVerifyCertificateChainStep * newQueue; + xmlSecSize newQueueMaxSize = queueMaxSize + XMLSEC_MSCNG_X509_STORE_VERIFY_CERTIFICATE_CHAIN_STEP_SIZE; + + newQueue = (struct xmlSecMSCngX509StoreVerifyCertificateChainStep*)xmlRealloc(queue, + sizeof(struct xmlSecMSCngX509StoreVerifyCertificateChainStep) * newQueueMaxSize); + if(newQueue == NULL) { + xmlSecMallocError( + sizeof(struct xmlSecMSCngX509StoreVerifyCertificateChainStep) * newQueueMaxSize, NULL); + goto done; + } + queue = newQueue; + queueMaxSize = newQueueMaxSize; + } + + /* try the issuer cert in certStore */ + issuerCert = xmlSecMSCngX509StoreFindIssuer(certStore, currentCert, store); + if(issuerCert != NULL) { + queue[queueSize].cert = issuerCert; + queue[queueSize].freeCert = TRUE; + ++queueSize; + } + + /* try the issuer cert in untrustedStore */ + issuerCert = xmlSecMSCngX509StoreFindIssuer(untrustedStore, currentCert, store); + if(issuerCert != NULL) { + if(queueSize >= queueMaxSize) { + /* can't happen: the queue was resized above to fit two more entries */ + CertFreeCertificateContext(issuerCert); + xmlSecInternalError("queue is full", NULL); + goto done; + } + queue[queueSize].cert = issuerCert; + queue[queueSize].freeCert = TRUE; + ++queueSize; + } + } + + if(freeCurrentCert == TRUE) { + CertFreeCertificateContext(currentCert); + } + currentCert = NULL; + freeCurrentCert = FALSE; } - return(-1); + /* not verified */ +done: + if((currentCert != NULL) && (freeCurrentCert == TRUE)) { + CertFreeCertificateContext(currentCert); + } + if(queue != NULL) { + xmlSecSize ii; + for(ii = 0; ii < queueSize; ++ii) { + if((queue[ii].cert != NULL) && (queue[ii].freeCert == TRUE)) { + CertFreeCertificateContext(queue[ii].cert); + } + } + xmlFree(queue); + } + return(res); } /** diff --git a/src/mscrypto/symkeys.c b/src/mscrypto/symkeys.c index 4ff94dbdd..615d49af6 100644 --- a/src/mscrypto/symkeys.c +++ b/src/mscrypto/symkeys.c @@ -340,8 +340,8 @@ xmlSecMSCryptoCreatePrivateExponentOneKey(HCRYPTPROV hProv, HCRYPTKEY *hPrivateK /* Skip coefficient */ ptr += bitLen / 16; - /* Convert privateExponent to 1 */ - for (n = 0; n < (bitLen / 16); n++) { + /* Convert privateExponent to 1 (the field is bitLen/8 bytes long) */ + for (n = 0; n < (bitLen / 8); n++) { if (n == 0) ptr[n] = 1; else ptr[n] = 0; } diff --git a/src/mscrypto/x509vfy.c b/src/mscrypto/x509vfy.c index 1a4b96855..6e3589ca6 100644 --- a/src/mscrypto/x509vfy.c +++ b/src/mscrypto/x509vfy.c @@ -395,9 +395,73 @@ xmlSecMSCryptoX509StoreContainsCert(HCERTSTORE store, CERT_NAME_BLOB* name, } +static PCCERT_CONTEXT +xmlSecMSCryptoX509StoreFindIssuer(HCERTSTORE store, PCCERT_CONTEXT cert, + xmlSecKeyDataStorePtr keyDataStore) { + PCCERT_CONTEXT issuerCert = NULL; + int ret; + + xmlSecAssert2(store != NULL, NULL); + xmlSecAssert2(cert != NULL, NULL); + xmlSecAssert2(keyDataStore != NULL, NULL); + + while (TRUE) { + /* CertFindCertificateInStore automatically frees the previous certificate context (see + * https://learn.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-certfindcertificateinstore) */ + issuerCert = CertFindCertificateInStore(store, + X509_ASN_ENCODING | PKCS_7_ASN_ENCODING, + 0, + CERT_FIND_SUBJECT_NAME, + &(cert->pCertInfo->Issuer), + issuerCert); + if (issuerCert == NULL) { + return(NULL); + } + + ret = xmlSecMSCryptoX509StoreVerifySubject(keyDataStore, cert, issuerCert); + if (ret < 0) { + xmlSecInternalError("xmlSecMSCryptoX509StoreVerifySubject", NULL); + continue; + } else if (ret == 0) { + xmlSecOtherError(XMLSEC_ERRORS_R_CERT_VERIFY_FAILED, + xmlSecKeyDataStoreGetName(keyDataStore), + "xmlSecMSCryptoX509StoreVerifySubject"); + continue; + } + + /* success */ + return(issuerCert); + } +} + +struct xmlSecMSCryptoBuildCertChainStep { + PCCERT_CONTEXT cert; + BOOL freeCert; +}; +#define XMLSEC_MSCRYPTO_BUILD_CERT_CHAIN_STEP_SIZE 32 +#define XMLSEC_MSCRYPTO_BUILD_CERT_CHAIN_MAX_DEPTH 100 +#define XMLSEC_MSCRYPTO_X509_CERT_HASH_SIZE 20 + +/* Returns the SHA1 hash of @p pCert in @p pHash. Returns 0 on success, -1 on error. */ +static int +xmlSecMSCryptoX509GetCertHash(PCCERT_CONTEXT pCert, BYTE* pHash, DWORD* hashSize) { + BOOL ret; + + xmlSecAssert2(pCert != NULL, -1); + xmlSecAssert2(pHash != NULL, -1); + xmlSecAssert2(hashSize != NULL, -1); + + ret = CertGetCertificateContextProperty(pCert, CERT_HASH_PROP_ID, pHash, hashSize); + if((ret == FALSE) || (*hashSize != (DWORD)XMLSEC_MSCRYPTO_X509_CERT_HASH_SIZE)) { + xmlSecMSCryptoError("CertGetCertificateContextProperty(CERT_HASH_PROP_ID)", NULL); + return(-1); + } + return(0); +} + /** * xmlSecMSCryptoBuildCertChainManually: - * @cert: the certificate we check + * @theCert: the certificate we check * @pfTime: pointer to FILETIME that we are interested in * @store_trusted: trusted certificates added via API * @store_untrusted: untrusted certificates added via API @@ -409,125 +473,199 @@ xmlSecMSCryptoX509StoreContainsCert(HCERTSTORE store, CERT_NAME_BLOB* name, * Returns: TRUE on success or FALSE otherwise. */ static BOOL -xmlSecMSCryptoBuildCertChainManually (PCCERT_CONTEXT cert, LPFILETIME pfTime, +xmlSecMSCryptoBuildCertChainManually (PCCERT_CONTEXT theCert, LPFILETIME pfTime, HCERTSTORE store_trusted, HCERTSTORE store_untrusted, HCERTSTORE certs, xmlSecKeyDataStorePtr store) { - PCCERT_CONTEXT issuerCert = NULL; + struct xmlSecMSCryptoBuildCertChainStep * queue = NULL; + xmlSecSize queueSize = 0, queueMaxSize = 0; + BYTE seenHashes[XMLSEC_MSCRYPTO_BUILD_CERT_CHAIN_MAX_DEPTH][XMLSEC_MSCRYPTO_X509_CERT_HASH_SIZE]; + xmlSecSize seenSize = 0; + BYTE hash[XMLSEC_MSCRYPTO_X509_CERT_HASH_SIZE]; + DWORD hashSize; + PCCERT_CONTEXT currentCert = NULL; + BOOL freeCurrentCert = FALSE; + BOOL res = FALSE; int ret; - /* check certificate validity and revokation */ - if (!xmlSecMSCryptoVerifyCertTime(cert, pfTime)) { - xmlSecOtherError(XMLSEC_ERRORS_R_CERT_HAS_EXPIRED, - xmlSecKeyDataStoreGetName(store), - "certificate expired"); + xmlSecAssert2(theCert != NULL, FALSE); + xmlSecAssert2(pfTime != NULL, FALSE); + xmlSecAssert2(store_trusted != NULL, FALSE); + xmlSecAssert2(store_untrusted != NULL, FALSE); + xmlSecAssert2(certs != NULL, FALSE); + xmlSecAssert2(store != NULL, FALSE); + + /* setup queue */ + queue = (struct xmlSecMSCryptoBuildCertChainStep*)xmlMalloc( + sizeof(struct xmlSecMSCryptoBuildCertChainStep) * XMLSEC_MSCRYPTO_BUILD_CERT_CHAIN_STEP_SIZE); + if(queue == NULL) { + xmlSecMallocError( + sizeof(struct xmlSecMSCryptoBuildCertChainStep) * XMLSEC_MSCRYPTO_BUILD_CERT_CHAIN_STEP_SIZE, NULL); return(FALSE); } + queueMaxSize = XMLSEC_MSCRYPTO_BUILD_CERT_CHAIN_STEP_SIZE; - if (!xmlSecMSCryptoCheckRevocation(certs, cert)) { - xmlSecOtherError(XMLSEC_ERRORS_R_CRL_VERIFY_FAILED, - xmlSecKeyDataStoreGetName(store), - "certificate revoked");; - return(FALSE); - } + queue[0].cert = theCert; + queue[0].freeCert = FALSE; + queueSize = 1; - /* does trustedStore contain cert directly? */ - ret = xmlSecMSCryptoX509StoreContainsCert(store_trusted, - &(cert->pCertInfo->Subject), cert, store); - if (ret < 0) { - xmlSecInternalError("xmlSecMSCryptoX509StoreContainsCert", NULL); - return(FALSE); - } else if (ret == 1) { - /* success */ - return(TRUE); - } + while(queueSize > 0) { + PCCERT_CONTEXT issuerCert = NULL; + xmlSecSize ii; + BOOL alreadySeen = FALSE; - /* does trustedStore contain the issuer cert? */ - ret = xmlSecMSCryptoX509StoreContainsCert(store_trusted, - &(cert->pCertInfo->Issuer), cert, store); - if (ret < 0) { - xmlSecInternalError("xmlSecMSCryptoX509StoreContainsCert", NULL); - return(FALSE); - } else if (ret == 1) { - /* success */ - return(TRUE); - } + currentCert = queue[queueSize - 1].cert; + freeCurrentCert = queue[queueSize - 1].freeCert; + --queueSize; - /* is cert self-signed? no recursion in that case */ - if (CertCompareCertificateName(X509_ASN_ENCODING | PKCS_7_ASN_ENCODING, - &(cert->pCertInfo->Subject), - &(cert->pCertInfo->Issuer))) { - /* not verified */ - return(FALSE); - } + /* limit the chain depth to avoid excessive work on crafted inputs */ + if(seenSize >= XMLSEC_MSCRYPTO_BUILD_CERT_CHAIN_MAX_DEPTH) { + xmlSecOtherError(XMLSEC_ERRORS_R_CERT_VERIFY_FAILED, + xmlSecKeyDataStoreGetName(store), + "certificate chain is too deep"); + goto done; + } - /* try the untrusted certs in the chain */ - issuerCert = CertFindCertificateInStore(certs, - X509_ASN_ENCODING | PKCS_7_ASN_ENCODING, - 0, - CERT_FIND_SUBJECT_NAME, - &(cert->pCertInfo->Issuer), - NULL); - if(issuerCert != NULL) { - ret = xmlSecMSCryptoX509StoreVerifySubject(store, cert, issuerCert); - if (ret < 0) { - xmlSecInternalError("xmlSecMSCryptoX509StoreVerifySubject", NULL); - CertFreeCertificateContext(issuerCert); - return(FALSE); + /* cycle detection: make sure we have not seen this certificate before */ + hashSize = sizeof(hash); + ret = xmlSecMSCryptoX509GetCertHash(currentCert, hash, &hashSize); + if((ret < 0) || (hashSize != XMLSEC_MSCRYPTO_X509_CERT_HASH_SIZE)) { + xmlSecInternalError("xmlSecMSCryptoX509GetCertHash", NULL); + goto done; } - else if (ret == 0) { - xmlSecOtherError(XMLSEC_ERRORS_R_CERT_VERIFY_FAILED, - NULL, - "xmlSecMSCryptoX509StoreVerifySubject"); - CertFreeCertificateContext(issuerCert); - return(FALSE); + for(ii = 0; ii < seenSize; ++ii) { + if(memcmp(&seenHashes[ii], hash, XMLSEC_MSCRYPTO_X509_CERT_HASH_SIZE) == 0) { + alreadySeen = TRUE; + break; + } + } + if(alreadySeen) { + /* The same certificate can be reached through multiple stores/branches; + * we only need to process each cert once. */ + if(freeCurrentCert == TRUE) { + CertFreeCertificateContext(currentCert); + } + currentCert = NULL; + freeCurrentCert = FALSE; + continue; } - if (!xmlSecMSCryptoBuildCertChainManually(issuerCert, pfTime, store_trusted, store_untrusted, certs, store)) { - xmlSecInternalError("xmlSecMSCryptoBuildCertChainManually", NULL); - CertFreeCertificateContext(issuerCert); - return(FALSE); + /* remember this certificate */ + memcpy(&seenHashes[seenSize], hash, XMLSEC_MSCRYPTO_X509_CERT_HASH_SIZE); + ++seenSize; + + /* check certificate validity and revocation; an expired/revoked cert + * cannot be part of a valid chain, so skip this branch (and its issuer) + * and continue searching the other branches in the queue */ + if (!xmlSecMSCryptoVerifyCertTime(currentCert, pfTime)) { + xmlSecOtherError(XMLSEC_ERRORS_R_CERT_HAS_EXPIRED, + xmlSecKeyDataStoreGetName(store), + "certificate expired"); + if(freeCurrentCert == TRUE) { + CertFreeCertificateContext(currentCert); + } + currentCert = NULL; + freeCurrentCert = FALSE; + continue; } - /* success */ - CertFreeCertificateContext(issuerCert); - return(TRUE); - } + if (!xmlSecMSCryptoCheckRevocation(certs, currentCert)) { + xmlSecOtherError(XMLSEC_ERRORS_R_CRL_VERIFY_FAILED, + xmlSecKeyDataStoreGetName(store), + "certificate revoked"); + if(freeCurrentCert == TRUE) { + CertFreeCertificateContext(currentCert); + } + currentCert = NULL; + freeCurrentCert = FALSE; + continue; + } - /* try the untrusted certs in the store */ - issuerCert = CertFindCertificateInStore(store_untrusted, - X509_ASN_ENCODING | PKCS_7_ASN_ENCODING, - 0, - CERT_FIND_SUBJECT_NAME, - &(cert->pCertInfo->Issuer), - NULL); - if(issuerCert != NULL) { - ret = xmlSecMSCryptoX509StoreVerifySubject(store, cert, issuerCert); + /* does trustedStore contain cert directly? */ + ret = xmlSecMSCryptoX509StoreContainsCert(store_trusted, + &(currentCert->pCertInfo->Subject), currentCert, store); if (ret < 0) { - xmlSecInternalError("xmlSecMSCryptoX509StoreVerifySubject", NULL); - CertFreeCertificateContext(issuerCert); - return(FALSE); + xmlSecInternalError("xmlSecMSCryptoX509StoreContainsCert", NULL); + goto done; + } else if (ret == 1) { + /* success */ + res = TRUE; + goto done; } - else if (ret == 0) { - xmlSecOtherError(XMLSEC_ERRORS_R_CERT_VERIFY_FAILED, - NULL, - "xmlSecMSCryptoX509StoreVerifySubject"); - CertFreeCertificateContext(issuerCert); - return(FALSE); + + /* does trustedStore contain the issuer cert? */ + ret = xmlSecMSCryptoX509StoreContainsCert(store_trusted, + &(currentCert->pCertInfo->Issuer), currentCert, store); + if (ret < 0) { + xmlSecInternalError("xmlSecMSCryptoX509StoreContainsCert", NULL); + goto done; + } else if (ret == 1) { + /* success */ + res = TRUE; + goto done; } - if (!xmlSecMSCryptoBuildCertChainManually(issuerCert, pfTime, store_trusted, store_untrusted, certs, store)) { - xmlSecInternalError("xmlSecMSCryptoBuildCertChainManually", NULL); - CertFreeCertificateContext(issuerCert); - return(FALSE); + /* is cert self-signed? no further chain building in that case */ + if (CertCompareCertificateName(X509_ASN_ENCODING | PKCS_7_ASN_ENCODING, + &(currentCert->pCertInfo->Subject), + &(currentCert->pCertInfo->Issuer)) == FALSE + ) { + /* we need space for at most 2 certificates */ + if(queueSize + 2 > queueMaxSize) { + struct xmlSecMSCryptoBuildCertChainStep * newQueue; + xmlSecSize newQueueMaxSize = queueMaxSize + XMLSEC_MSCRYPTO_BUILD_CERT_CHAIN_STEP_SIZE; + + newQueue = (struct xmlSecMSCryptoBuildCertChainStep*)xmlRealloc(queue, + sizeof(struct xmlSecMSCryptoBuildCertChainStep) * newQueueMaxSize); + if(newQueue == NULL) { + xmlSecMallocError( + sizeof(struct xmlSecMSCryptoBuildCertChainStep) * newQueueMaxSize, NULL); + goto done; + } + queue = newQueue; + queueMaxSize = newQueueMaxSize; + } + + /* try the untrusted certs in the chain */ + issuerCert = xmlSecMSCryptoX509StoreFindIssuer(certs, currentCert, store); + if(issuerCert != NULL) { + xmlSecAssert2(queueSize < queueMaxSize, FALSE); + queue[queueSize].cert = issuerCert; + queue[queueSize].freeCert = TRUE; + ++queueSize; + } + + /* try the untrusted certs in the store */ + issuerCert = xmlSecMSCryptoX509StoreFindIssuer(store_untrusted, currentCert, store); + if(issuerCert != NULL) { + xmlSecAssert2(queueSize < queueMaxSize, FALSE); + queue[queueSize].cert = issuerCert; + queue[queueSize].freeCert = TRUE; + ++queueSize; + } } - /* success */ - CertFreeCertificateContext(issuerCert); - return(TRUE); + if(freeCurrentCert == TRUE) { + CertFreeCertificateContext(currentCert); + } + currentCert = NULL; + freeCurrentCert = FALSE; } - /* no luck */ - return(FALSE); + /* not verified */ +done: + if((currentCert != NULL) && (freeCurrentCert == TRUE)) { + CertFreeCertificateContext(currentCert); + } + if(queue != NULL) { + xmlSecSize ii; + for(ii = 0; ii < queueSize; ++ii) { + if((queue[ii].cert != NULL) && (queue[ii].freeCert == TRUE)) { + CertFreeCertificateContext(queue[ii].cert); + } + } + xmlFree(queue); + } + return(res); } static BOOL diff --git a/src/nss/app.c b/src/nss/app.c index 6b0fcf661..00aa39945 100644 --- a/src/nss/app.c +++ b/src/nss/app.c @@ -427,6 +427,7 @@ xmlSecNssAppDerKeyLoadSECItem(SECItem* secItem) { spki = SECKEY_DecodeDERSubjectPublicKeyInfo(secItem); if (spki == NULL) { xmlSecNssError("SECKEY_DecodeDERSubjectPublicKeyInfo", NULL); + goto done; } pubkey = SECKEY_ExtractPublicKey(spki); diff --git a/src/nss/pkikeys.c b/src/nss/pkikeys.c index 627a18863..2b5a1c156 100644 --- a/src/nss/pkikeys.c +++ b/src/nss/pkikeys.c @@ -147,41 +147,56 @@ xmlSecNssPKIKeyDataAdoptKey(xmlSecKeyDataPtr data, SECKEYPublicKey *pubkey) { xmlSecNssPKIKeyDataCtxPtr ctx; + SECKEYPublicKey *pubkey2 = NULL; KeyType pubType = nullKey; KeyType priType = nullKey; xmlSecAssert2(xmlSecKeyDataIsValid(data), -1); xmlSecAssert2(xmlSecKeyDataCheckSize(data, xmlSecNssPKIKeyDataSize), -1); - if(privkey != NULL) { - priType = SECKEY_GetPrivateKeyType(privkey); + ctx = xmlSecNssPKIKeyDataGetCtx(data); + xmlSecAssert2(ctx != NULL, -1); + + /* get public key if needed from private */ + if ((pubkey == NULL) && (privkey != NULL)) { + pubkey2 = SECKEY_ConvertToPublicKey(privkey); + if(pubkey2 == NULL) { + xmlSecNssError("SECKEY_ConvertToPublicKey", NULL); + return(-1); + } } - if(pubkey != NULL) { - pubType = SECKEY_GetPublicKeyType(pubkey); + /* ensure key types match */ + if (privkey != NULL) { + priType = SECKEY_GetPrivateKeyType(privkey); } - if(priType != nullKey && pubType != nullKey) { - if(pubType != priType) { - xmlSecNssError3("SECKEY_GetPrivateKeyType/SECKEY_GetPublicKeyType", NULL, - "pubType=%u; priType=%u", pubType, priType); - return -1; + if (pubkey != NULL) { + pubType = SECKEY_GetPublicKeyType(pubkey); + } else if (pubkey2 != NULL) { + pubType = SECKEY_GetPublicKeyType(pubkey2); + } + if ((priType != nullKey) && (pubType != priType)) { + xmlSecNssError3("SECKEY_GetPrivateKeyType/SECKEY_GetPublicKeyType", NULL, + "pubType=%u; priType=%u", pubType, priType); + if (pubkey2 != NULL) { + SECKEY_DestroyPublicKey(pubkey2); } + return(-1); } - ctx = xmlSecNssPKIKeyDataGetCtx(data); - xmlSecAssert2(ctx != NULL, -1); - - if (ctx->privkey) { + /* destroy old keys (if needed) and set new ones */ + if (ctx->privkey != NULL) { SECKEY_DestroyPrivateKey(ctx->privkey); } ctx->privkey = privkey; - if (ctx->pubkey) { + if (ctx->pubkey != NULL) { SECKEY_DestroyPublicKey(ctx->pubkey); } - ctx->pubkey = pubkey; + ctx->pubkey = (pubkey != NULL) ? pubkey : pubkey2; + /* done */ return(0); } @@ -325,7 +340,7 @@ xmlSecNssPKIKeyDataGetPrivKey(xmlSecKeyDataPtr data) { KeyType xmlSecNssPKIKeyDataGetKeyType(xmlSecKeyDataPtr data) { xmlSecNssPKIKeyDataCtxPtr ctx; - KeyType kt; + KeyType kt = nullKey; xmlSecAssert2(xmlSecKeyDataIsValid(data), nullKey); xmlSecAssert2(xmlSecKeyDataCheckSize(data, xmlSecNssPKIKeyDataSize), nullKey); @@ -335,7 +350,7 @@ xmlSecNssPKIKeyDataGetKeyType(xmlSecKeyDataPtr data) { if (ctx->pubkey != NULL) { kt = SECKEY_GetPublicKeyType(ctx->pubkey); - } else { + } else if (ctx->privkey != NULL) { kt = SECKEY_GetPrivateKeyType(ctx->privkey); } return(kt); @@ -718,6 +733,9 @@ xmlSecNssKeyDataDsaGetType(xmlSecKeyDataPtr data) { ctx = xmlSecNssPKIKeyDataGetCtx(data); xmlSecAssert2(ctx != NULL, xmlSecKeyDataTypeUnknown); + if (ctx->pubkey == NULL) { + return(xmlSecKeyDataTypeUnknown); + } xmlSecAssert2(SECKEY_GetPublicKeyType(ctx->pubkey) == dsaKey, xmlSecKeyDataTypeUnknown); if (ctx->privkey != NULL) { @@ -737,7 +755,9 @@ xmlSecNssKeyDataDsaGetSize(xmlSecKeyDataPtr data) { ctx = xmlSecNssPKIKeyDataGetCtx(data); xmlSecAssert2(ctx != NULL, 0); - xmlSecAssert2(ctx->pubkey != NULL, 0); + if (ctx->pubkey == NULL) { + return(0); + } xmlSecAssert2(SECKEY_GetPublicKeyType(ctx->pubkey) == dsaKey, 0); return(8 * SECKEY_PublicKeyStrength(ctx->pubkey)); @@ -891,6 +911,7 @@ xmlSecNssKeyDataDsaWrite(xmlSecKeyDataId id, xmlSecKeyDataPtr data, ctx = xmlSecNssPKIKeyDataGetCtx(data); xmlSecAssert2(ctx != NULL, -1); + xmlSecAssert2(ctx->pubkey != NULL, -1); xmlSecAssert2(SECKEY_GetPublicKeyType(ctx->pubkey) == dsaKey, -1); /*** p ***/ @@ -1104,7 +1125,10 @@ xmlSecNssKeyDataRsaGetType(xmlSecKeyDataPtr data) { ctx = xmlSecNssPKIKeyDataGetCtx(data); xmlSecAssert2(ctx != NULL, xmlSecKeyDataTypeUnknown); - xmlSecAssert2(ctx->pubkey == NULL || SECKEY_GetPublicKeyType(ctx->pubkey) == rsaKey, xmlSecKeyDataTypeUnknown); + if (ctx->pubkey == NULL) { + return(xmlSecKeyDataTypeUnknown); + } + xmlSecAssert2(SECKEY_GetPublicKeyType(ctx->pubkey) == rsaKey, xmlSecKeyDataTypeUnknown); if (ctx->privkey != NULL) { return(xmlSecKeyDataTypePrivate | xmlSecKeyDataTypePublic); @@ -1123,7 +1147,9 @@ xmlSecNssKeyDataRsaGetSize(xmlSecKeyDataPtr data) { ctx = xmlSecNssPKIKeyDataGetCtx(data); xmlSecAssert2(ctx != NULL, 0); - xmlSecAssert2(ctx->pubkey != NULL, 0); + if (ctx->pubkey == NULL) { + return(0); + } xmlSecAssert2(SECKEY_GetPublicKeyType(ctx->pubkey) == rsaKey, 0); return(8 * SECKEY_PublicKeyStrength(ctx->pubkey)); @@ -1252,6 +1278,7 @@ xmlSecNssKeyDataRsaWrite(xmlSecKeyDataId id,xmlSecKeyDataPtr data, ctx = xmlSecNssPKIKeyDataGetCtx(data); xmlSecAssert2(ctx != NULL, -1); + xmlSecAssert2(ctx->pubkey != NULL, -1); xmlSecAssert2(SECKEY_GetPublicKeyType(ctx->pubkey) == rsaKey, -1); /*** Modulus ***/ @@ -1431,7 +1458,10 @@ xmlSecNssKeyDataEcdsaGetType(xmlSecKeyDataPtr data) { ctx = xmlSecNssPKIKeyDataGetCtx(data); xmlSecAssert2(ctx != NULL, xmlSecKeyDataTypeUnknown); - xmlSecAssert2(ctx->pubkey == NULL || SECKEY_GetPublicKeyType(ctx->pubkey) == ecKey, xmlSecKeyDataTypeUnknown); + if (ctx->pubkey == NULL) { + return(xmlSecKeyDataTypeUnknown); + } + xmlSecAssert2(SECKEY_GetPublicKeyType(ctx->pubkey) == ecKey, xmlSecKeyDataTypeUnknown); if (ctx->privkey != NULL) { return(xmlSecKeyDataTypePrivate | xmlSecKeyDataTypePublic); @@ -1448,7 +1478,9 @@ xmlSecNssKeyDataEcdsaGetSize(xmlSecKeyDataPtr data) { ctx = xmlSecNssPKIKeyDataGetCtx(data); xmlSecAssert2(ctx != NULL, 0); - xmlSecAssert2(ctx->pubkey != NULL, 0); + if (ctx->pubkey == NULL) { + return(0); + } xmlSecAssert2(SECKEY_GetPublicKeyType(ctx->pubkey) == ecKey, 0); return(SECKEY_SignatureLen(ctx->pubkey)); diff --git a/src/nss/x509.c b/src/nss/x509.c index 2543fcef9..c6f877b76 100644 --- a/src/nss/x509.c +++ b/src/nss/x509.c @@ -66,7 +66,8 @@ static int xmlSecNssKeyDataX509VerifyAndExtractKey(xmlSecKeyDataPtr static int xmlSecNssX509SECItemWrite (SECItem * secItem, xmlSecBufferPtr buf); -static CERTCertificate* xmlSecNssX509CertDerRead (xmlSecByte* buf, +static CERTCertificate* xmlSecNssX509CertDerRead (CERTCertDBHandle *handle, + xmlSecByte* buf, xmlSecSize size); static CERTSignedCrl* xmlSecNssX509CrlDerRead (xmlSecByte* buf, xmlSecSize size, @@ -714,6 +715,7 @@ xmlSecNssKeyDataX509DebugXmlDump(xmlSecKeyDataPtr data, FILE* output) { static int xmlSecNssKeyDataX509Read(xmlSecKeyDataPtr data, xmlSecKeyValueX509Ptr x509Value, xmlSecKeysMngrPtr keysMngr, unsigned int flags) { + CERTCertDBHandle *certDb; xmlSecKeyDataStorePtr x509Store; CERTCertificate* cert = NULL; CERTSignedCrl* crl = NULL; @@ -726,6 +728,12 @@ xmlSecNssKeyDataX509Read(xmlSecKeyDataPtr data, xmlSecKeyValueX509Ptr x509Value, xmlSecAssert2(x509Value != NULL, -1); xmlSecAssert2(keysMngr != NULL, -1); + certDb = CERT_GetDefaultCertDB(); + if(certDb == NULL) { + xmlSecNssError("CERT_GetDefaultCertDB", xmlSecKeyDataGetName(data)); + goto done; + } + x509Store = xmlSecKeysMngrGetDataStore(keysMngr, xmlSecNssX509StoreId); if(x509Store == NULL) { xmlSecInternalError("xmlSecKeysMngrGetDataStore", xmlSecKeyDataGetName(data)); @@ -738,7 +746,8 @@ xmlSecNssKeyDataX509Read(xmlSecKeyDataPtr data, xmlSecKeyValueX509Ptr x509Value, } if(xmlSecBufferGetSize(&(x509Value->cert)) > 0) { - cert = xmlSecNssX509CertDerRead(xmlSecBufferGetData(&(x509Value->cert)), + cert = xmlSecNssX509CertDerRead(certDb, + xmlSecBufferGetData(&(x509Value->cert)), xmlSecBufferGetSize(&(x509Value->cert))); if(cert == NULL) { xmlSecInternalError("xmlSecNssX509CertDerRead", xmlSecKeyDataGetName(data)); @@ -1084,10 +1093,11 @@ xmlSecNssX509SECItemWrite(SECItem* secItem, xmlSecBufferPtr buf) { } static CERTCertificate* -xmlSecNssX509CertDerRead(xmlSecByte* buf, xmlSecSize size) { +xmlSecNssX509CertDerRead(CERTCertDBHandle *handle, xmlSecByte* buf, xmlSecSize size) { CERTCertificate *cert; SECItem derCert; + xmlSecAssert2(handle != NULL, NULL); xmlSecAssert2(buf != NULL, NULL); xmlSecAssert2(size > 0, NULL); @@ -1095,7 +1105,7 @@ xmlSecNssX509CertDerRead(xmlSecByte* buf, xmlSecSize size) { XMLSEC_SAFE_CAST_SIZE_TO_UINT(size, derCert.len, return(NULL), NULL); /* decode cert and import to temporary cert db */ - cert = __CERT_NewTempCertificate(CERT_GetDefaultCertDB(), &derCert, + cert = __CERT_NewTempCertificate(handle, &derCert, NULL, PR_FALSE, PR_TRUE); if(cert == NULL) { xmlSecNssError("__CERT_NewTempCertificate", NULL); @@ -1321,6 +1331,7 @@ static int xmlSecNssKeyDataRawX509CertBinRead(xmlSecKeyDataId id, xmlSecKeyPtr key, const xmlSecByte* buf, xmlSecSize bufSize, xmlSecKeyInfoCtxPtr keyInfoCtx) { + CERTCertDBHandle *certDb; xmlSecKeyDataPtr data; CERTCertificate* cert; int ret; @@ -1331,7 +1342,13 @@ xmlSecNssKeyDataRawX509CertBinRead(xmlSecKeyDataId id, xmlSecKeyPtr key, xmlSecAssert2(bufSize > 0, -1); xmlSecAssert2(keyInfoCtx != NULL, -1); - cert = xmlSecNssX509CertDerRead((xmlSecByte*)buf, bufSize); + certDb = CERT_GetDefaultCertDB(); + if(certDb == NULL) { + xmlSecNssError("CERT_GetDefaultCertDB", NULL); + return(-1); + } + + cert = xmlSecNssX509CertDerRead(certDb, (xmlSecByte*)buf, bufSize); if(cert == NULL) { xmlSecInternalError("xmlSecNssX509CertDerRead", NULL); return(-1); diff --git a/src/nss/x509vfy.c b/src/nss/x509vfy.c index 52d9d3ac9..9766c99d2 100644 --- a/src/nss/x509vfy.c +++ b/src/nss/x509vfy.c @@ -59,6 +59,7 @@ struct _xmlSecNssX509StoreCtx { */ CERTCertList* certsList; /* just keeping a reference to destroy later */ + CERTCertDBHandle *certDb; }; /**************************************************************************** @@ -105,7 +106,8 @@ static CERTCertificate* xmlSecNssX509FindCert(CERTCertList* certsList, const xmlChar *issuerName, const xmlChar *issuerSerial, xmlSecByte * ski, - xmlSecSize skiSize); + xmlSecSize skiSize, + CERTCertDBHandle *certDb); /** @@ -187,10 +189,11 @@ xmlSecNssX509StoreFindCert_ex(xmlSecKeyDataStorePtr store, xmlChar *subjectName, ctx = xmlSecNssX509StoreGetCtx(store); xmlSecAssert2(ctx != NULL, NULL); + xmlSecAssert2(ctx->certDb != NULL, NULL); return xmlSecNssX509FindCert(ctx->certsList, subjectName, issuerName, issuerSerial, - ski, skiSize); + ski, skiSize, ctx->certDb); } @@ -223,6 +226,7 @@ xmlSecNssX509StoreVerify(xmlSecKeyDataStorePtr store, CERTCertList* certs, ctx = xmlSecNssX509StoreGetCtx(store); xmlSecAssert2(ctx != NULL, NULL); + xmlSecAssert2(ctx->certDb != NULL, NULL); if(keyInfoCtx->certsVerificationTime > 0) { /* convert the time since epoch in seconds to microseconds */ @@ -263,7 +267,7 @@ xmlSecNssX509StoreVerify(xmlSecKeyDataStorePtr store, CERTCertList* certs, if((keyInfoCtx->flags & XMLSEC_KEYINFO_FLAGS_X509DATA_DONT_VERIFY_CERTS) == 0) { /* it's important to set the usage here, otherwise no real verification * is performed. */ - status = CERT_VerifyCertificate(CERT_GetDefaultCertDB(), + status = CERT_VerifyCertificate(ctx->certDb, cert, PR_FALSE, certificateUsageEmailSigner, timeboundary , NULL, NULL, NULL); @@ -334,6 +338,7 @@ xmlSecNssX509StoreAdoptCert(xmlSecKeyDataStorePtr store, CERTCertificate* cert, ctx = xmlSecNssX509StoreGetCtx(store); xmlSecAssert2(ctx != NULL, -1); + xmlSecAssert2(ctx->certDb != NULL, -1); if(ctx->certsList == NULL) { ctx->certsList = CERT_NewCertList(); @@ -359,7 +364,7 @@ xmlSecNssX509StoreAdoptCert(xmlSecKeyDataStorePtr store, CERTCertificate* cert, xmlSecNssError("CERT_DecodeTrustString", xmlSecKeyDataStoreGetName(store)); return(-1); } - CERT_ChangeCertTrust(CERT_GetDefaultCertDB(), cert, &trust); + status = CERT_ChangeCertTrust(ctx->certDb, cert, &trust); if(status != SECSuccess) { xmlSecNssError("CERT_ChangeCertTrust", xmlSecKeyDataStoreGetName(store)); return(-1); @@ -379,6 +384,13 @@ xmlSecNssX509StoreInitialize(xmlSecKeyDataStorePtr store) { memset(ctx, 0, sizeof(xmlSecNssX509StoreCtx)); + ctx->certDb = CERT_GetDefaultCertDB(); + if(ctx->certDb == NULL) { + xmlSecNssError("CERT_GetDefaultCertDB", xmlSecKeyDataStoreGetName(store)); + return(-1); + } + + /* success */ return(0); } @@ -453,7 +465,8 @@ xmlSecNssGetCertName(const xmlChar * name) { static CERTCertificate* xmlSecNssX509FindCert(CERTCertList* certsList, const xmlChar *subjectName, const xmlChar *issuerName, const xmlChar *issuerSerial, - xmlSecByte * ski, xmlSecSize skiSize) { + xmlSecByte * ski, xmlSecSize skiSize, + CERTCertDBHandle *certDb) { CERTCertificate *cert = NULL; CERTName *name = NULL; SECItem *nameitem = NULL; @@ -463,6 +476,8 @@ xmlSecNssX509FindCert(CERTCertList* certsList, const xmlChar *subjectName, PRArenaPool *arena = NULL; int rv; + xmlSecAssert2(certDb != NULL, NULL); + /* certsList can be NULL */ /* search by subject name if available */ @@ -490,7 +505,7 @@ xmlSecNssX509FindCert(CERTCertList* certsList, const xmlChar *subjectName, goto done; } - cert = CERT_FindCertByName(CERT_GetDefaultCertDB(), nameitem); + cert = CERT_FindCertByName(certDb, nameitem); } /* search by issuer name+serial if available */ @@ -540,7 +555,7 @@ xmlSecNssX509FindCert(CERTCertList* certsList, const xmlChar *subjectName, goto done; } - cert = CERT_FindCertByIssuerAndSN(CERT_GetDefaultCertDB(), &issuerAndSN); + cert = CERT_FindCertByIssuerAndSN(certDb, &issuerAndSN); SECITEM_FreeItem(&issuerAndSN.serialNumber, PR_FALSE); } @@ -552,7 +567,7 @@ xmlSecNssX509FindCert(CERTCertList* certsList, const xmlChar *subjectName, subjKeyID.data = ski; XMLSEC_SAFE_CAST_SIZE_TO_UINT(skiSize, subjKeyID.len, goto done, NULL); - cert = CERT_FindCertBySubjectKeyID(CERT_GetDefaultCertDB(), + cert = CERT_FindCertBySubjectKeyID(certDb, &subjKeyID); /* try to search in our list - NSS doesn't update it's cache correctly diff --git a/src/openssl/x509vfy.c b/src/openssl/x509vfy.c index da0e0a605..cc858bfd9 100644 --- a/src/openssl/x509vfy.c +++ b/src/openssl/x509vfy.c @@ -271,7 +271,11 @@ xmlSecOpenSSLX509StoreVerify(xmlSecKeyDataStorePtr store, XMLSEC_STACK_OF_X509* if(ret == 1) { ++ii; } else if(ret == 0) { - (void)sk_X509_CRL_delete(verified_crls, ii); + /* crl failed verification: this is a hard failure because we expect CRLs to be valid */ + xmlSecOtherError(XMLSEC_ERRORS_R_CRL_VERIFY_FAILED, + xmlSecKeyDataStoreGetName(store), + "xmlSecOpenSSLX509VerifyCRL"); + goto done; } else { xmlSecInternalError("xmlSecOpenSSLX509VerifyCRL", xmlSecKeyDataStoreGetName(store)); goto done; diff --git a/src/soap.c b/src/soap.c index f1498f09f..88c57c64e 100644 --- a/src/soap.c +++ b/src/soap.c @@ -865,7 +865,7 @@ xmlSecSoap12AddFaultSubcode(xmlNodePtr faultNode, const xmlChar* subCodeHref, co } /* set result qname in Value node */ - xmlNodeSetContent(cur, qname); + xmlNodeSetContent(valueNode, qname); if(qname != subCodeName) { xmlFree(qname); } diff --git a/src/xmltree.c b/src/xmltree.c index 0e246a6f3..eabbeb810 100644 --- a/src/xmltree.c +++ b/src/xmltree.c @@ -1600,14 +1600,13 @@ xmlSecQName2BitMaskNodesRead(xmlSecQName2BitMaskInfoConstPtr info, xmlNodePtr* n xmlFree(content); return(-1); } - xmlFree(content); if((stopOnUnknown != 0) && (tmp == 0)) { - /* todo: better error */ - xmlSecInternalError2("xmlSecQName2BitMaskGetBitMaskFromString", NULL, - "value=%s", xmlSecErrorsSafeString(content)); + xmlSecInvalidNodeContentError(cur, NULL, "unknown value"); + xmlFree(content); return(-1); } + xmlFree(content); (*mask) |= tmp; cur = xmlSecGetNextElementNode(cur->next);