From: Scott Cantor Date: Wed, 1 Aug 2018 13:02:21 +0000 Subject: Default KeyInfo resolver doesn't check for empty element content The Apache Santuario XML Security for C++ library contained a number of code paths at risk of dereferencing null pointers when processing various kinds of malformed KeyInfo hints typically found in signed or encrypted XML. The usual effect is a crash, and in the case of the Shibboleth SP software, a crash in the shibd daemon. This is a combination of two upstream commits with some unrelated whitespace changes removed. Quite much of it is still indentation change because of the added conditionals. Best viewed with tabstop=4, for example with 'LESS=Rx4 git show --ignore-space-changes'. Thanks: Scott Cantor Upstream bug: https://issues.apache.org/jira/projects/SANTUARIO/issues/SANTUARIO-491 CVE: not assigned yet Closes: #905332 --- xsec/dsig/DSIGKeyInfoValue.hpp | 8 +-- xsec/enc/XSECKeyInfoResolverDefault.cpp | 108 +++++++++++++++++++------------- 2 files changed, 69 insertions(+), 47 deletions(-) diff --git a/xsec/dsig/DSIGKeyInfoValue.hpp b/xsec/dsig/DSIGKeyInfoValue.hpp index f652679..d6da5d5 100644 --- a/xsec/dsig/DSIGKeyInfoValue.hpp +++ b/xsec/dsig/DSIGKeyInfoValue.hpp @@ -117,7 +117,7 @@ public: * @returns a pointer to the DSA P string value. */ - const XMLCh * getDSAP(void) const {return mp_PTextNode->getNodeValue();} + const XMLCh * getDSAP() const {return mp_PTextNode ? mp_PTextNode->getNodeValue() : NULL;} /** * \brief Get Q value @@ -125,7 +125,7 @@ public: * @returns a pointer to the DSA Q string value. */ - const XMLCh * getDSAQ(void) const {return mp_QTextNode->getNodeValue();} + const XMLCh * getDSAQ() const {return mp_QTextNode ? mp_QTextNode->getNodeValue() : NULL;} /** * \brief Get G value @@ -133,7 +133,7 @@ public: * @returns a pointer to the DSA G string value. */ - const XMLCh * getDSAG(void) const {return mp_GTextNode->getNodeValue();} + const XMLCh * getDSAG() const {return mp_GTextNode ? mp_GTextNode->getNodeValue() : NULL;} /** * \brief Get Y value @@ -141,7 +141,7 @@ public: * @returns a pointer to the DSA Y string value. */ - const XMLCh * getDSAY(void) const {return mp_YTextNode->getNodeValue();} + const XMLCh * getDSAY() const {return mp_YTextNode ? mp_YTextNode->getNodeValue() : NULL;} /** * \brief Get Modulus diff --git a/xsec/enc/XSECKeyInfoResolverDefault.cpp b/xsec/enc/XSECKeyInfoResolverDefault.cpp index 52cdfe3..c4c81cb 100644 --- a/xsec/enc/XSECKeyInfoResolverDefault.cpp +++ b/xsec/enc/XSECKeyInfoResolverDefault.cpp @@ -79,13 +79,11 @@ XSECCryptoKey * XSECKeyInfoResolverDefault::resolveKey(DSIGKeyInfoList * lst) { case (DSIGKeyInfo::KEYINFO_X509) : { ret = NULL; - const XMLCh * x509Str; - XSECCryptoX509 * x509 = XSECPlatformUtils::g_cryptoProvider->X509(); - Janitor j_x509(x509); - - x509Str = ((DSIGKeyInfoX509 *) lst->item(i))->getCertificateItem(0); + const XMLCh* x509Str = ((const DSIGKeyInfoX509 *) lst->item(i))->getCertificateItem(0); - if (x509Str != 0) { + if (x509Str) { + XSECCryptoX509 * x509 = XSECPlatformUtils::g_cryptoProvider->X509(); + Janitor j_x509(x509); // The crypto interface classes work UTF-8 safeBuffer transX509; @@ -104,66 +102,90 @@ XSECCryptoKey * XSECKeyInfoResolverDefault::resolveKey(DSIGKeyInfoList * lst) { case (DSIGKeyInfo::KEYINFO_VALUE_DSA) : { - XSECCryptoKeyDSA * dsa = XSECPlatformUtils::g_cryptoProvider->keyDSA(); - Janitor j_dsa(dsa); - - safeBuffer value; - - value << (*mp_formatter << ((DSIGKeyInfoValue *) lst->item(i))->getDSAP()); - dsa->loadPBase64BigNums(value.rawCharBuffer(), (unsigned int) strlen(value.rawCharBuffer())); - value << (*mp_formatter << ((DSIGKeyInfoValue *) lst->item(i))->getDSAQ()); - dsa->loadQBase64BigNums(value.rawCharBuffer(), (unsigned int) strlen(value.rawCharBuffer())); - value << (*mp_formatter << ((DSIGKeyInfoValue *) lst->item(i))->getDSAG()); - dsa->loadGBase64BigNums(value.rawCharBuffer(), (unsigned int) strlen(value.rawCharBuffer())); - value << (*mp_formatter << ((DSIGKeyInfoValue *) lst->item(i))->getDSAY()); - dsa->loadYBase64BigNums(value.rawCharBuffer(), (unsigned int) strlen(value.rawCharBuffer())); - - j_dsa.release(); - return dsa; + const DSIGKeyInfoValue* dsaval = (const DSIGKeyInfoValue *) lst->item(i); + if (dsaval->getDSAP() || dsaval->getDSAQ() || dsaval->getDSAG() || dsaval->getDSAY()) { + + XSECCryptoKeyDSA * dsa = XSECPlatformUtils::g_cryptoProvider->keyDSA(); + Janitor j_dsa(dsa); + + safeBuffer value; + + if (dsaval->getDSAP()) { + value << (*mp_formatter << dsaval->getDSAP()); + dsa->loadPBase64BigNums(value.rawCharBuffer(), (unsigned int) strlen(value.rawCharBuffer())); + } + if (dsaval->getDSAQ()) { + value << (*mp_formatter << dsaval->getDSAQ()); + dsa->loadQBase64BigNums(value.rawCharBuffer(), (unsigned int) strlen(value.rawCharBuffer())); + } + if (dsaval->getDSAG()) { + value << (*mp_formatter << dsaval->getDSAG()); + dsa->loadGBase64BigNums(value.rawCharBuffer(), (unsigned int) strlen(value.rawCharBuffer())); + } + if (dsaval->getDSAY()) { + value << (*mp_formatter << dsaval->getDSAY()); + dsa->loadYBase64BigNums(value.rawCharBuffer(), (unsigned int) strlen(value.rawCharBuffer())); + } + + j_dsa.release(); + return dsa; + } } break; case (DSIGKeyInfo::KEYINFO_VALUE_RSA) : { + const DSIGKeyInfoValue* rsaval = (const DSIGKeyInfoValue *) lst->item(i); + if (rsaval->getRSAModulus() && rsaval->getRSAExponent()) { - XSECCryptoKeyRSA * rsa = XSECPlatformUtils::g_cryptoProvider->keyRSA(); - Janitor j_rsa(rsa); + XSECCryptoKeyRSA* rsa = XSECPlatformUtils::g_cryptoProvider->keyRSA(); + Janitor j_rsa(rsa); - safeBuffer value; + safeBuffer value; - value << (*mp_formatter << ((DSIGKeyInfoValue *) lst->item(i))->getRSAModulus()); - rsa->loadPublicModulusBase64BigNums(value.rawCharBuffer(), (unsigned int) strlen(value.rawCharBuffer())); - value << (*mp_formatter << ((DSIGKeyInfoValue *) lst->item(i))->getRSAExponent()); - rsa->loadPublicExponentBase64BigNums(value.rawCharBuffer(), (unsigned int) strlen(value.rawCharBuffer())); + value << (*mp_formatter << rsaval->getRSAModulus()); + rsa->loadPublicModulusBase64BigNums(value.rawCharBuffer(), (unsigned int) strlen(value.rawCharBuffer())); + value << (*mp_formatter << rsaval->getRSAExponent()); + rsa->loadPublicExponentBase64BigNums(value.rawCharBuffer(), (unsigned int) strlen(value.rawCharBuffer())); - j_rsa.release(); - return rsa; + j_rsa.release(); + return rsa; + } } break; case (DSIGKeyInfo::KEYINFO_VALUE_EC) : { + const DSIGKeyInfoValue* ecval = (const DSIGKeyInfoValue *) lst->item(i); + if (ecval->getECPublicKey() && ecval->getECNamedCurve()) { - XSECCryptoKeyEC* ec = XSECPlatformUtils::g_cryptoProvider->keyEC(); - Janitor j_ec(ec); + XSECCryptoKeyEC* ec = XSECPlatformUtils::g_cryptoProvider->keyEC(); + Janitor j_ec(ec); - safeBuffer value; - value << (*mp_formatter << ((DSIGKeyInfoValue *) lst->item(i))->getECPublicKey()); - XSECAutoPtrChar curve(((DSIGKeyInfoValue *) lst->item(i))->getECNamedCurve()); - if (curve.get()) { - ec->loadPublicKeyBase64(curve.get(), value.rawCharBuffer(), (unsigned int) strlen(value.rawCharBuffer())); - j_ec.release(); - return ec; + safeBuffer value; + + value << (*mp_formatter << ecval->getECPublicKey()); + XSECAutoPtrChar curve(ecval->getECNamedCurve()); + if (curve.get()) { + ec->loadPublicKeyBase64(curve.get(), value.rawCharBuffer(), (unsigned int) strlen(value.rawCharBuffer())); + j_ec.release(); + return ec; + } } } break; case (DSIGKeyInfo::KEYINFO_DERENCODED) : { - safeBuffer value; - value << (*mp_formatter << ((DSIGKeyInfoDEREncoded *) lst->item(i))->getData()); - return XSECPlatformUtils::g_cryptoProvider->keyDER(value.rawCharBuffer(), (unsigned int)strlen(value.rawCharBuffer()), true); + const DSIGKeyInfoDEREncoded* derval = (const DSIGKeyInfoDEREncoded *) lst->item(i); + if (derval->getData()) { + + safeBuffer value; + + value << (*mp_formatter << derval->getData()); + return XSECPlatformUtils::g_cryptoProvider->keyDER(value.rawCharBuffer(), (unsigned int)strlen(value.rawCharBuffer()), true); + } } break;