From 55aba2ba0f8960a5e281441c230596f25b19b687 Mon Sep 17 00:00:00 2001 From: Aleksey Sanin Date: Tue, 29 Sep 2026 13:43:31 -0400 Subject: [PATCH] (xmlsec-openssl) Enabled password entry for PKCS12 files import; fixed error / cleanup paths; added comments --- include/xmlsec/openssl/crypto.h | 13 ++++++ scripts/build_memcheck.sh | 5 +-- src/openssl/app.c | 35 +++++++++++---- src/openssl/crypto.c | 20 ++++++--- src/openssl/evp.c | 17 ++++++-- src/openssl/globals.h | 1 + src/openssl/hmac.c | 8 ++++ src/openssl/kdf.c | 21 +++++++++ src/openssl/key_agrmnt.c | 5 +++ src/openssl/kt_rsa.c | 13 +++++- src/openssl/symkeys.c | 2 +- src/openssl/x509.c | 77 ++++++++++++++++++++++++++++++--- src/openssl/x509vfy.c | 34 +++++++++------ 13 files changed, 206 insertions(+), 45 deletions(-) diff --git a/include/xmlsec/openssl/crypto.h b/include/xmlsec/openssl/crypto.h index d1741d290..bddfa4b23 100644 --- a/include/xmlsec/openssl/crypto.h +++ b/include/xmlsec/openssl/crypto.h @@ -61,12 +61,21 @@ * *****************************************************************************/ #if defined(OPENSSL_IS_BORINGSSL) || defined(OPENSSL_IS_AWSLC) +/* + * BoringSSL and AWS-LC do not provide a usable opensslconf.h, so the + * OPENSSL_NO_* capability mappings below are not available for them and the + * missing features have to be listed here explicitly (their opensslconf.h + * defines OPENSSL_NO_CAMELLIA, OPENSSL_NO_GOST and OPENSSL_NO_RMD160). + */ #define XMLSEC_NO_RSA_OAEP 1 #define XMLSEC_NO_DH 1 #define XMLSEC_NO_DSA 1 #define XMLSEC_NO_SHA3 1 #define XMLSEC_NO_CAMELLIA 1 #define XMLSEC_NO_CHACHA20 1 +#define XMLSEC_NO_GOST 1 +#define XMLSEC_NO_GOST2012 1 +#define XMLSEC_NO_RIPEMD160 1 #endif /* defined(OPENSSL_IS_BORINGSSL) || defined(OPENSSL_IS_AWSLC) */ #if defined(LIBRESSL_VERSION_NUMBER) @@ -205,6 +214,10 @@ XMLSEC_CRYPTO_EXPORT BIO* xmlSecOpenSSLCreateReadFileBio (const ch #define XMLSEC_NO_DES 1 #endif /* OPENSSL_NO_DES */ +#ifdef OPENSSL_NO_DH +#define XMLSEC_NO_DH 1 +#endif /* OPENSSL_NO_DH */ + #ifdef OPENSSL_NO_DSA #define XMLSEC_NO_DSA 1 #endif /* OPENSSL_NO_DSA */ diff --git a/scripts/build_memcheck.sh b/scripts/build_memcheck.sh index 49e3fbc82..b2277f80c 100755 --- a/scripts/build_memcheck.sh +++ b/scripts/build_memcheck.sh @@ -22,12 +22,11 @@ parallel_jobs=${PARALLEL_JOBS:-12} tmp_folder=${TMPFOLDER:-/tmp} echo "============== Starting memcheck for ${crypto} using source root '${top_dir}'" -cd "$top_dir" rm -rf "${tmp_folder}"/xmlsec-test* if [ -f Makefile ]; then make distclean fi -autoreconf -i -f -./configure --enable-development --enable-legacy-features --with-default-crypto=${crypto} "$@" +autoreconf -i -f "${top_dir}" +${top_dir}/configure --enable-development --enable-legacy-features --with-default-crypto=${crypto} "$@" make -j${parallel_jobs} make memcheck-crypto-${crypto} diff --git a/src/openssl/app.c b/src/openssl/app.c index a25d84243..94606504a 100644 --- a/src/openssl/app.c +++ b/src/openssl/app.c @@ -1377,31 +1377,29 @@ xmlSecOpenSSLAppPkcs12LoadMemory( * in format=xmlSecKeyDataFormatPkcs12. * * @param bio the PKCS12 key bio. - * @param pwd the PKCS12 file password. - * @param pwdCallback the password callback. + * @param pwd the PKCS12 file password. If NULL, the password is obtained + * from the password callback. + * @param pwdCallback the password callback, used when pwd is NULL. * @param pwdCallbackCtx the user context for password callback. * @return pointer to the key or NULL if an error occurs. */ xmlSecKeyPtr xmlSecOpenSSLAppPkcs12LoadBIO(BIO* bio, const char *pwd, - void* pwdCallback XMLSEC_ATTRIBUTE_UNUSED, - void* pwdCallbackCtx XMLSEC_ATTRIBUTE_UNUSED) { + void* pwdCallback, + void* pwdCallbackCtx) { PKCS12 *p12 = NULL; EVP_PKEY * pKey = NULL; X509 * keyCert = NULL; STACK_OF(X509) * chain = NULL; xmlSecKeyPtr res = NULL; + pem_password_cb* pwdCb = NULL; + char pwdBuf[2048]; size_t pwdSize; int pwdLen; int ret; xmlSecAssert2(bio != NULL, NULL); - XMLSEC_UNREFERENCED(pwdCallback); - XMLSEC_UNREFERENCED(pwdCallbackCtx); - - pwdSize = (pwd != NULL) ? strlen(pwd) : 0; - XMLSEC_SAFE_CAST_SIZE_T_TO_INT(pwdSize, pwdLen, return(NULL), NULL); XMLSEC_OPENSSL_PUSH_LIB_CTX(goto done); p12 = d2i_PKCS12_bio(bio, NULL); @@ -1411,6 +1409,24 @@ xmlSecOpenSSLAppPkcs12LoadBIO(BIO* bio, const char *pwd, goto done; } + /* + * If the password is not given explicitly, try to obtain it + * from the password callback. + */ + if((pwd == NULL) && (pwdCallback != NULL)) { + pwdCb = XMLSEC_PTR_TO_FUNC(pem_password_cb, pwdCallback); + ret = pwdCb(pwdBuf, (int)sizeof(pwdBuf), 0, pwdCallbackCtx); + if((ret < 0) || (ret >= (int)sizeof(pwdBuf))) { + xmlSecOpenSSLError("pwdCallback", NULL); + goto done; + } + pwdBuf[ret] = '\0'; + pwd = pwdBuf; + } + + pwdSize = (pwd != NULL) ? strlen(pwd) : 0; + XMLSEC_SAFE_CAST_SIZE_T_TO_INT(pwdSize, pwdLen, goto done, NULL); + XMLSEC_OPENSSL_PUSH_LIB_CTX(goto done); ret = PKCS12_verify_mac(p12, pwd, ((pwd != NULL) ? pwdLen : 0)); XMLSEC_OPENSSL_POP_LIB_CTX(); @@ -1450,6 +1466,7 @@ xmlSecOpenSSLAppPkcs12LoadBIO(BIO* bio, const char *pwd, /* success! */ done: + OPENSSL_cleanse(pwdBuf, sizeof(pwdBuf)); if(chain != NULL) { sk_X509_pop_free(chain, X509_free); } diff --git a/src/openssl/crypto.c b/src/openssl/crypto.c index 73e3a6a1c..cd894b037 100644 --- a/src/openssl/crypto.c +++ b/src/openssl/crypto.c @@ -767,18 +767,26 @@ xmlSecOpenSSLErrorsShutdown(void) { */ int xmlSecOpenSSLSetDefaultTrustedCertsFolder(const xmlChar* path) { - if(gXmlSecOpenSSLTrustedCertsFolder != NULL) { - xmlFree(gXmlSecOpenSSLTrustedCertsFolder); - gXmlSecOpenSSLTrustedCertsFolder = NULL; - } + xmlChar* newPath; + /* + * Duplicate the new path first so that the previously set folder + * is preserved if the allocation fails. + */ if(path != NULL) { - gXmlSecOpenSSLTrustedCertsFolder = xmlStrdup(BAD_CAST path); - if(gXmlSecOpenSSLTrustedCertsFolder == NULL) { + newPath = xmlStrdup(BAD_CAST path); + if(newPath == NULL) { xmlSecStrdupError(BAD_CAST path, NULL); return(-1); } + } else { + newPath = NULL; + } + + if(gXmlSecOpenSSLTrustedCertsFolder != NULL) { + xmlFree(gXmlSecOpenSSLTrustedCertsFolder); } + gXmlSecOpenSSLTrustedCertsFolder = newPath; return(0); } diff --git a/src/openssl/evp.c b/src/openssl/evp.c index cd20b15f0..f321b99ef 100644 --- a/src/openssl/evp.c +++ b/src/openssl/evp.c @@ -336,6 +336,7 @@ xmlSecOpenSSLEvpKeyDataGetKeySize(xmlSecKeyDataPtr data) { #endif /* XMLSEC_NO_DSA */ #ifndef XMLSEC_NO_DH + case EVP_PKEY_DH: case EVP_PKEY_DHX: { DH* dh = NULL; @@ -502,6 +503,7 @@ xmlSecOpenSSLEvpKeyDataGetType(xmlSecKeyDataPtr data) { #endif /* XMLSEC_NO_DSA */ #ifndef XMLSEC_NO_DH + case EVP_PKEY_DH: case EVP_PKEY_DHX: { DH* dh = NULL; @@ -700,6 +702,7 @@ xmlSecOpenSSLEvpKeyGetKeyDataId(EVP_PKEY *pKey) { switch(xmlSecOpenSSLEvpKeyGetId(pKey)) { #ifndef XMLSEC_NO_DH + case EVP_PKEY_DH: case EVP_PKEY_DHX: return (xmlSecOpenSSLKeyDataDhId); #endif /* XMLSEC_NO_DH */ @@ -1710,6 +1713,8 @@ typedef struct _xmlSecOpenSSLKeyValueDh { * * The DH key type uses PKCS#3 format which saves p and g, but not the 'q' value. The DHX key * type uses X9.42 format which saves the value of 'q' and this must be used for FIPS186-4. + * Keys in both formats are accepted; the 'DHX' name is used to create new keys because it + * supports the 'q' value. */ #define XMLSEC_OPENSSL_DH_EVP_NAME "DHX" @@ -1795,7 +1800,11 @@ int xmlSecOpenSSLKeyDataDhAdoptEvp(xmlSecKeyDataPtr data, EVP_PKEY* pKey) { xmlSecAssert2(xmlSecKeyDataCheckId(data, xmlSecOpenSSLKeyDataDhId), -1); xmlSecAssert2(pKey != NULL, -1); - xmlSecAssert2(EVP_PKEY_base_id(pKey) == EVP_PKEY_DHX, -1); + /* + * DH keys can have two types: EVP_PKEY_DH (PKCS#3 form, no 'q' value) and + * EVP_PKEY_DHX (X9.42 form, with 'q' value). + */ + xmlSecAssert2((EVP_PKEY_base_id(pKey) == EVP_PKEY_DH) || (EVP_PKEY_base_id(pKey) == EVP_PKEY_DHX), -1); return(xmlSecOpenSSLEvpKeyDataAdoptEvp(data, pKey)); } @@ -1879,7 +1888,7 @@ xmlSecOpenSSLKeyDataDhGetDh(xmlSecKeyDataPtr data) { xmlSecAssert2(xmlSecKeyDataCheckId(data, xmlSecOpenSSLKeyDataDhId), NULL); pKey = xmlSecOpenSSLKeyDataDhGetEvp(data); - xmlSecAssert2((pKey == NULL) || (EVP_PKEY_base_id(pKey) == EVP_PKEY_DHX), NULL); + xmlSecAssert2((pKey == NULL) || (EVP_PKEY_base_id(pKey) == EVP_PKEY_DH) || (EVP_PKEY_base_id(pKey) == EVP_PKEY_DHX), NULL); return((pKey != NULL) ? EVP_PKEY_get0_DH(pKey) : NULL); } @@ -2046,8 +2055,7 @@ xmlSecOpenSSLKeyDataDhGetValue(xmlSecKeyDataPtr data, xmlSecOpenSSLKeyValueDhPtr } ret = EVP_PKEY_get_bn_param(pKey, OSSL_PKEY_PARAM_FFC_Q, &(dhKeyValue->q)); if((ret != 1) || (dhKeyValue->q == NULL)) { - xmlSecOpenSSLError("EVP_PKEY_get_bn_param(q)", xmlSecKeyDataGetName(data)); - return(-1); + /* ignore the error since q is optional for DH keys in the PKCS#3 form */ } ret = EVP_PKEY_get_bn_param(pKey, OSSL_PKEY_PARAM_FFC_G, &(dhKeyValue->generator)); if((ret != 1) || (dhKeyValue->generator == NULL)) { @@ -2839,6 +2847,7 @@ xmlSecOpenSSLKeyDataEcSetValue(xmlSecKeyDataPtr data, const xmlChar* curveOid, x xmlSecOpenSSLError("EC_POINT_new", xmlSecKeyDataGetName(data)); goto done; } + /* EC_POINT_oct2point() validates that the point is on the curve */ ret = EC_POINT_oct2point(group, point, pubkeyData, pubkeySize, NULL); if(ret != 1) { xmlSecOpenSSLError("EC_POINT_oct2point", xmlSecKeyDataGetName(data)); diff --git a/src/openssl/globals.h b/src/openssl/globals.h index 92d899e14..eff277eda 100644 --- a/src/openssl/globals.h +++ b/src/openssl/globals.h @@ -21,6 +21,7 @@ #include #include +#include #ifndef IN_XMLSEC_CRYPTO #define IN_XMLSEC_CRYPTO diff --git a/src/openssl/hmac.c b/src/openssl/hmac.c index fb0496720..088ccec7a 100644 --- a/src/openssl/hmac.c +++ b/src/openssl/hmac.c @@ -458,6 +458,14 @@ xmlSecOpenSSLHmacVerify(xmlSecTransformPtr transform, return(-1); } + /* the digest to verify must not be empty */ + if(dataSize <= 0) { + xmlSecInvalidSizeError("HMAC digest", dataSize, + XMLSEC_TRANSFORM_HMAC_BITS_TO_BYTES(ctx->dgstSizeInBits), + xmlSecTransformGetName(transform)); + return(-1); + } + /* Returns 1 for match, 0 for no match, <0 for errors. */ ret = xmlSecTransformHmacVerify(data, dataSize, ctx->dgst, ctx->dgstSizeInBits, EVP_MAX_MD_SIZE); if(ret < 0) { diff --git a/src/openssl/kdf.c b/src/openssl/kdf.c index 27c28e471..84cfa5804 100644 --- a/src/openssl/kdf.c +++ b/src/openssl/kdf.c @@ -67,6 +67,7 @@ struct _xmlSecOpenSSLKdfCtx { xmlSecBuffer buffer; xmlSecBuffer buffer2; + xmlSecBuffer keyBuffer; unsigned int param1; }; XMLSEC_TRANSFORM_DECLARE(OpenSSLKdf, xmlSecOpenSSLKdfCtx) @@ -190,6 +191,14 @@ xmlSecOpenSSLKdfInitialize(xmlSecTransformPtr transform) { } ctx->buffer2.flags |= XMLSEC_BUFFER_FLAG_SECURE; + ret = xmlSecBufferInitialize(&(ctx->keyBuffer), 0); + if(ret < 0) { + xmlSecInternalError("xmlSecBufferInitialize(keyBuffer)", NULL); + xmlSecOpenSSLKdfFinalize(transform); + return(-1); + } + ctx->keyBuffer.flags |= XMLSEC_BUFFER_FLAG_SECURE; + /* done */ return(0); } @@ -218,6 +227,7 @@ xmlSecOpenSSLKdfFinalize(xmlSecTransformPtr transform) { xmlSecBufferFinalize(&(ctx->buffer)); xmlSecBufferFinalize(&(ctx->buffer2)); + xmlSecBufferFinalize(&(ctx->keyBuffer)); OPENSSL_cleanse(ctx, sizeof(xmlSecOpenSSLKdfCtx)); } @@ -247,6 +257,7 @@ xmlSecOpenSSLKdfSetKey(xmlSecTransformPtr transform, xmlSecKeyPtr key) { xmlSecBufferPtr buffer; xmlSecByte * keyData; xmlSecSize keySize; + int ret; xmlSecAssert2(xmlSecOpenSSLKdfCheckId(transform), -1); xmlSecAssert2(((transform->operation == xmlSecTransformOperationEncrypt) || (transform->operation == xmlSecTransformOperationDecrypt)), -1); @@ -273,6 +284,16 @@ xmlSecOpenSSLKdfSetKey(xmlSecTransformPtr transform, xmlSecKeyPtr key) { return(-1); } + /* copy the key into a ctx-owned buffer so the OSSL_PARAM pointer remains + * valid until EVP_KDF_derive() is called */ + ret = xmlSecBufferSetData(&(ctx->keyBuffer), keyData, keySize); + if(ret < 0) { + xmlSecInternalError("xmlSecBufferSetData(key)", xmlSecTransformGetName(transform)); + return(-1); + } + keyData = xmlSecBufferGetData(&(ctx->keyBuffer)); + xmlSecAssert2(keyData != NULL, -1); + /* set key */ if(ctx->paramsPos >= XMLSEC_OPENSSL_KDF_MAX_PARAMS) { xmlSecInvalidSizeDataError("Kdf Params Number", ctx->paramsPos, "too big", xmlSecTransformGetName(transform)); diff --git a/src/openssl/key_agrmnt.c b/src/openssl/key_agrmnt.c index a1d715309..cce873417 100644 --- a/src/openssl/key_agrmnt.c +++ b/src/openssl/key_agrmnt.c @@ -385,6 +385,11 @@ xmlSecOpenSSLKeyAgreementGenerateSecret(xmlSecOpenSSLKeyAgreementCtxPtr ctx, xml xmlSecOpenSSLError("EVP_PKEY_derive_init", NULL); goto done; } + /* + * EVP_PKEY_derive_set_peer() validates the peer key: it checks that the peer + * has the same key type and matching parameters (OpenSSL < 3.0), and performs + * a public key check (OpenSSL 3.0+). + */ ret = EVP_PKEY_derive_set_peer(pKeyCtx, otherPubKey); if(ret != 1) { xmlSecOpenSSLError("EVP_PKEY_derive_set_peer", NULL); diff --git a/src/openssl/kt_rsa.c b/src/openssl/kt_rsa.c index 1f0b301c5..f8a4fd8f0 100644 --- a/src/openssl/kt_rsa.c +++ b/src/openssl/kt_rsa.c @@ -749,6 +749,7 @@ xmlSecOpenSSLRsaOaepProcessImpl(xmlSecOpenSSLRsaOaepCtxPtr ctx, const xmlSecByte } else { /* decrypt */ BIGNUM * bn; + xmlSecSize outResSize; int outLen; ret = RSA_private_decrypt(inLen, inBuf, outBuf, rsa, RSA_NO_PADDING); @@ -797,9 +798,19 @@ xmlSecOpenSSLRsaOaepProcessImpl(xmlSecOpenSSLRsaOaepCtxPtr ctx, const xmlSecByte OPENSSL_cleanse(outBuf, (*outSize)); return(-1); } + XMLSEC_SAFE_CAST_INT_TO_SIZE(ret, outResSize, return(-1), NULL); + + /* + * cleanse the tail of the output buffer that held the raw + * OAEP-encoded message; only the first 'ret' bytes contain the + * recovered plaintext + */ + if((*outSize) > outResSize) { + OPENSSL_cleanse(outBuf + outResSize, ((*outSize) - outResSize)); + } /* success */ - XMLSEC_SAFE_CAST_INT_TO_SIZE(ret, (*outSize), return(-1), NULL); + (*outSize) = outResSize; } /* success */ diff --git a/src/openssl/symkeys.c b/src/openssl/symkeys.c index 2e87ab68d..bb187045f 100644 --- a/src/openssl/symkeys.c +++ b/src/openssl/symkeys.c @@ -140,7 +140,7 @@ xmlSecOpenSSLSymKeyDataGenerate(xmlSecKeyDataPtr data, xmlSecSize sizeBits, xmlS buffer = xmlSecKeyDataBinaryValueGetBuffer(data); xmlSecAssert2(buffer != NULL, -1); - size = (sizeBits + 7) / 8; + size = (sizeBits - 1) / 8 + 1; ret = xmlSecBufferSetSize(buffer, size); if(ret < 0) { xmlSecInternalError2("xmlSecBufferSetSize", NULL, "size=" XMLSEC_SIZE_FMT, size); diff --git a/src/openssl/x509.c b/src/openssl/x509.c index eac0855f0..1988958f4 100644 --- a/src/openssl/x509.c +++ b/src/openssl/x509.c @@ -254,9 +254,15 @@ xmlSecOpenSSLKeyDataX509AdoptKeyCert(xmlSecKeyDataPtr data, X509* cert) { /* check if for some reasons same cert is used */ - if((ctx->keyCert != NULL) && ((cert == ctx->keyCert) || (X509_cmp(cert, ctx->keyCert) == 0))) { - X509_free(cert); /* caller expects data to own the cert on success. */ - return(0); + if(ctx->keyCert != NULL) { + if(cert == ctx->keyCert) { + X509_free(cert); /* caller expects data to own the cert on success, this is required for refcounted objects. */ + return(0); + } + if(X509_cmp(cert, ctx->keyCert) == 0) { + X509_free(cert); /* caller expects data to own the cert on success. */ + return(0); + } } xmlSecAssert2(ctx->keyCert == NULL, -1); @@ -1420,6 +1426,15 @@ xmlSecOpenSSLX509Asn1TimeToTime(const ASN1_TIME * t, time_t * res) { } (*res) = timegm(&tm); + if((*res) == (time_t)-1) { + /* + * timegm() returns (time_t)-1 on failure. A (time_t)-1 is also a valid + * time (1969-12-31 23:59:59 UTC), but a certificate date is never in + * 1969, so treat it as a conversion error. + */ + xmlSecOpenSSLError("timegm", NULL); + return(-1); + } return (0); } @@ -1429,6 +1444,7 @@ int xmlSecOpenSSLX509Asn1TimeToTime(const ASN1_TIME * t, time_t * res) { struct tm tm; int offset; + time_t utcTime; xmlSecAssert2(t != NULL, -1); xmlSecAssert2(res != NULL, -1); @@ -1498,7 +1514,32 @@ xmlSecOpenSSLX509Asn1TimeToTime(const ASN1_TIME * t, time_t * res) { } #undef g2 - (*res) = (timegm(&tm) - offset * 60); + /* + * validate the parsed fields; out-of-range values make timegm() fail and + * that failure is not reliably detectable from its return value on all + * platforms (e.g. Windows) + */ + if((tm.tm_mon < 0) || (tm.tm_mon > 11) || + (tm.tm_mday < 1) || (tm.tm_mday > 31) || + (tm.tm_hour < 0) || (tm.tm_hour > 23) || + (tm.tm_min < 0) || (tm.tm_min > 59) || + (tm.tm_sec < 0) || (tm.tm_sec > 61) + ) { + xmlSecOpenSSLError("invalid ASN1_TIME fields", NULL); + return(-1); + } + + utcTime = timegm(&tm); + if(utcTime == (time_t)-1) { + /* + * timegm() returns (time_t)-1 on failure. A (time_t)-1 is also a valid + * time (1969-12-31 23:59:59 UTC), but a certificate date is never in + * 1969, so treat it as a conversion error. + */ + xmlSecOpenSSLError("timegm", NULL); + return(-1); + } + (*res) = utcTime - (time_t)(offset * 60); return (0); } #endif /* XMLSEC_OPENSSL_NO_ASN1_TIME_TO_TM */ @@ -1511,6 +1552,8 @@ xmlSecOpenSSLVerifyAndAdoptX509KeyData(xmlSecKeyPtr key, xmlSecKeyDataPtr data, xmlSecKeyDataPtr keyValue; X509* cert; X509* keyCert; + time_t origNotValidBefore; + time_t origNotValidAfter; int ret; xmlSecAssert2(xmlSecKeyDataCheckId(data, xmlSecOpenSSLKeyDataX509Id), -1); @@ -1570,6 +1613,15 @@ xmlSecOpenSSLVerifyAndAdoptX509KeyData(xmlSecKeyPtr key, xmlSecKeyDataPtr data, xmlSecKeyDataDestroy(keyValue); return(-1); } + /* + * from this point on we mutate the caller's key (set its value and validity + * times). save the original validity times so we can restore the key if a + * later step fails; the key value is guaranteed to be NULL here (see the + * check at the beginning of this function) + */ + origNotValidBefore = key->notValidBefore; + origNotValidAfter = key->notValidAfter; + ret = xmlSecKeySetValue(key, keyValue); if(ret < 0) { xmlSecInternalError("xmlSecKeySetValue", xmlSecKeyDataGetName(data)); @@ -1583,7 +1635,7 @@ xmlSecOpenSSLVerifyAndAdoptX509KeyData(xmlSecKeyPtr key, xmlSecKeyDataPtr data, ret = xmlSecOpenSSLX509Asn1TimeToTime(X509_get0_notBefore(ctx->keyCert), &(key->notValidBefore)); if(ret < 0) { xmlSecInternalError("xmlSecOpenSSLX509Asn1TimeToTime(notValidBefore)", xmlSecKeyDataGetName(data)); - return(-1); + goto restore; } } else { key->notValidBefore = 0; @@ -1592,7 +1644,7 @@ xmlSecOpenSSLVerifyAndAdoptX509KeyData(xmlSecKeyPtr key, xmlSecKeyDataPtr data, ret = xmlSecOpenSSLX509Asn1TimeToTime(X509_get0_notAfter(ctx->keyCert), &(key->notValidAfter)); if(ret < 0) { xmlSecInternalError("xmlSecOpenSSLX509Asn1TimeToTime(notValidAfter)", xmlSecKeyDataGetName(data)); - return(-1); + goto restore; } } else { key->notValidAfter = 0; @@ -1604,11 +1656,22 @@ xmlSecOpenSSLVerifyAndAdoptX509KeyData(xmlSecKeyPtr key, xmlSecKeyDataPtr data, ret = xmlSecKeyAdoptData(key, data); if(ret < 0) { xmlSecInternalError("xmlSecKeyAdoptData", xmlSecKeyDataGetName(data)); - return(-1); + goto restore; } /* success: cert found and data was adopted */ return(1); + +restore: + /* restore the key to its original state */ + ret = xmlSecKeySetValue(key, NULL); + if(ret < 0) { + xmlSecInternalError("xmlSecKeySetValue", xmlSecKeyDataGetName(data)); + /* continue restoring the original state even if setting the value to NULL failed */ + } + key->notValidBefore = origNotValidBefore; + key->notValidAfter = origNotValidAfter; + return(-1); } /** diff --git a/src/openssl/x509vfy.c b/src/openssl/x509vfy.c index fc2fb8992..6b378a83e 100644 --- a/src/openssl/x509vfy.c +++ b/src/openssl/x509vfy.c @@ -685,6 +685,7 @@ static int xmlSecOpenSSLX509StoreSetCtx(X509_STORE_CTX* xsc, xmlSecKeyInfoCtx* keyInfoCtx) { X509_VERIFY_PARAM * vpm = NULL; unsigned long vpm_flags = 0; + int ret; xmlSecAssert2(xsc != NULL, -1); xmlSecAssert2(keyInfoCtx != NULL, -1); @@ -705,7 +706,14 @@ xmlSecOpenSSLX509StoreSetCtx(X509_STORE_CTX* xsc, xmlSecKeyInfoCtx* keyInfoCtx) vpm_flags |= X509_V_FLAG_NO_CHECK_TIME; } - X509_VERIFY_PARAM_set_flags(vpm, vpm_flags); + /* X509_VERIFY_PARAM_set_time() and X509_VERIFY_PARAM_set_depth() return void + * (they cannot fail), so only set_flags() needs a return value check */ + ret = X509_VERIFY_PARAM_set_flags(vpm, vpm_flags); + if(ret != 1) { + xmlSecOpenSSLError("X509_VERIFY_PARAM_set_flags", NULL); + X509_VERIFY_PARAM_free(vpm); + return(-1); + } X509_VERIFY_PARAM_set_depth(vpm, keyInfoCtx->certsVerificationDepth); X509_STORE_CTX_set0_param(xsc, vpm); @@ -1361,15 +1369,13 @@ xmlSecOpenSSLX509StoreInitialize(xmlSecKeyDataStorePtr store) { ctx->xst = X509_STORE_new(); if(ctx->xst == NULL) { - xmlSecOpenSSLError("X509_STORE_new", - xmlSecKeyDataStoreGetName(store)); + xmlSecOpenSSLError("X509_STORE_new", xmlSecKeyDataStoreGetName(store)); return(-1); } ret = X509_STORE_set_default_paths_ex(ctx->xst, xmlSecOpenSSLGetLibCtx(), NULL); if(ret != 1) { - xmlSecOpenSSLError("X509_STORE_set_default_paths_ex", - xmlSecKeyDataStoreGetName(store)); + xmlSecOpenSSLError("X509_STORE_set_default_paths_ex", xmlSecKeyDataStoreGetName(store)); return(-1); } @@ -1392,34 +1398,34 @@ xmlSecOpenSSLX509StoreInitialize(xmlSecKeyDataStorePtr store) { } } else { if(!X509_LOOKUP_add_dir(lookup, NULL, X509_FILETYPE_DEFAULT)) { - xmlSecOpenSSLError("X509_LOOKUP_add_dir", - xmlSecKeyDataStoreGetName(store)); + xmlSecOpenSSLError("X509_LOOKUP_add_dir", xmlSecKeyDataStoreGetName(store)); return(-1); } } ctx->untrusted = sk_X509_new_null(); if(ctx->untrusted == NULL) { - xmlSecOpenSSLError("sk_X509_new_null", - xmlSecKeyDataStoreGetName(store)); + xmlSecOpenSSLError("sk_X509_new_null", xmlSecKeyDataStoreGetName(store)); return(-1); } ctx->crls = sk_X509_CRL_new_null(); if(ctx->crls == NULL) { - xmlSecOpenSSLError("sk_X509_CRL_new_null", - xmlSecKeyDataStoreGetName(store)); + xmlSecOpenSSLError("sk_X509_CRL_new_null", xmlSecKeyDataStoreGetName(store)); return(-1); } ctx->vpm = X509_VERIFY_PARAM_new(); if(ctx->vpm == NULL) { - xmlSecOpenSSLError("X509_VERIFY_PARAM_new", - xmlSecKeyDataStoreGetName(store)); + xmlSecOpenSSLError("X509_VERIFY_PARAM_new", xmlSecKeyDataStoreGetName(store)); return(-1); } X509_VERIFY_PARAM_set_depth(ctx->vpm, 9); /* the default maximum cert verification depth in xmlsec (see XMLSEC_KEYINFO_DEFAULT_MAX_CERT_VERIFICATION_DEPTH) */ - X509_STORE_set1_param(ctx->xst, ctx->vpm); + ret = X509_STORE_set1_param(ctx->xst, ctx->vpm); + if(ret != 1) { + xmlSecOpenSSLError("X509_STORE_set1_param", xmlSecKeyDataStoreGetName(store)); + return(-1); + } return(0);