xmlsecurity/CppunitTest_xmlsecurity_signing.mk | 1 xmlsecurity/inc/biginteger.hxx | 11 + xmlsecurity/qa/unit/signing/signing.cxx | 18 + xmlsecurity/source/component/documentdigitalsignatures.cxx | 2 xmlsecurity/source/helper/xmlsignaturehelper.cxx | 6 xmlsecurity/source/helper/xsecverify.cxx | 6 xmlsecurity/source/xmlsec/mscrypt/x509certificate_mscryptimpl.cxx | 92 ++++++++- xmlsecurity/source/xmlsec/nss/x509certificate_nssimpl.cxx | 95 +++++++++- 8 files changed, 211 insertions(+), 20 deletions(-)
New commits: commit f77ecfff1963aea4e5903824a304fa902ac4be35 Author: Michael Stahl <[email protected]> AuthorDate: Wed Oct 27 15:28:11 2021 +0200 Commit: Michael Stahl <[email protected]> CommitDate: Fri Oct 29 12:35:51 2021 +0200 xmlsecurity: some Distinguished Names are less equal than others It turns out that the 2 backends NSS and MS CryptoAPI generate different string representations of the same Distinguished Name in at least one corner case, when a value contains a quote " U+0022. The CryptoAPI function to generate the strings is: CertNameToStr(..., CERT_X500_NAME_STR | CERT_NAME_STR_REVERSE_FLAG, ...) This is documented on MSDN: https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-certnametostra#CERT_X500_NAME_STR NSS appears to implement RFC 1485, at least that's what the internal function is named after, or perhaps one of its several successor RFCs (not clear currently if there's a relevant difference). This is now causing trouble if a certificate with such a DN is used in a signature, created on WNT but then verified on another platform, because commit 5af5ea893bcb8a8eb472ac11133da10e5a604e66 introduced consistency checks that compare the DNs that occur as strings in META-INF/documentsignatures.xml: xmlsecurity/source/helper/xmlsignaturehelper.cxx:672: X509Data cannot be parsed The reason is that in XSecController::setX509Data() the value read from the X509IssuerSerial element (a string generated by CryptoAPI) doesn't match the value generated by NSS from the certificate parsed from the X509Certificate element, so these are erroneously interpreted as 2 distinct certificates. Try to make the EqualDistinguishedNames() more flexible so that it can try also a converted variant of the DN. (libxmlsec's NSS backend also complains that it cannot parse the DN: x509vfy.c:607: xmlSecNssX509NameRead() '' '' 12 'invalid data for 'char': actual=34 and expected comma ','' but it manages to validate the signature despite this.) Reviewed-on: https://gerrit.libreoffice.org/c/core/+/124287 Tested-by: Jenkins Reviewed-by: Michael Stahl <[email protected]> (cherry picked from commit e63611fabd38c757809b510fbb71c077880b1081) Reviewed-on: https://gerrit.libreoffice.org/c/core/+/124196 Reviewed-by: Thorsten Behrens <[email protected]> (cherry picked from commit 3dfe381032fc61ea31106f103dee9db8277d4d25) Change-Id: I4f72900738d1f5313146bbda7320a8f44319ebc8 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/124421 Tested-by: Michael Stahl <[email protected]> Reviewed-by: Michael Stahl <[email protected]> diff --git a/xmlsecurity/CppunitTest_xmlsecurity_signing.mk b/xmlsecurity/CppunitTest_xmlsecurity_signing.mk index 4b3c4787ea73..030becd36a37 100644 --- a/xmlsecurity/CppunitTest_xmlsecurity_signing.mk +++ b/xmlsecurity/CppunitTest_xmlsecurity_signing.mk @@ -27,6 +27,7 @@ $(eval $(call gb_CppunitTest_use_libraries,xmlsecurity_signing, \ unotest \ utl \ xmlsecurity \ + xsec_xmlsec \ )) $(eval $(call gb_CppunitTest_use_externals,xmlsecurity_signing,\ diff --git a/xmlsecurity/inc/biginteger.hxx b/xmlsecurity/inc/biginteger.hxx index 8b4d8a9143b5..1e6b3f4a876e 100644 --- a/xmlsecurity/inc/biginteger.hxx +++ b/xmlsecurity/inc/biginteger.hxx @@ -32,8 +32,17 @@ namespace xmlsecurity XSECXMLSEC_DLLPUBLIC OUString bigIntegerToNumericString( const css::uno::Sequence< sal_Int8 >& serial ); XSECXMLSEC_DLLPUBLIC css::uno::Sequence< sal_Int8 > numericStringToBigInteger ( const OUString& serialNumber ); +// DNs read as strings from XML files may need to be mangled for compatibility +// as NSS and MS CryptoAPI have different string serialisations; if the DN is +// from an XCertificate it's "native" already and doesn't need to be mangled. +enum EqualMode +{ + NOCOMPAT, + COMPAT_2ND, + COMPAT_BOTH +}; XSECXMLSEC_DLLPUBLIC bool EqualDistinguishedNames(OUString const& rName1, - OUString const& rName2); + OUString const& rName2, EqualMode eMode); } #endif diff --git a/xmlsecurity/qa/unit/signing/signing.cxx b/xmlsecurity/qa/unit/signing/signing.cxx index 781c555fbd3f..85aacdd12cf3 100644 --- a/xmlsecurity/qa/unit/signing/signing.cxx +++ b/xmlsecurity/qa/unit/signing/signing.cxx @@ -49,6 +49,7 @@ #include <documentsignaturehelper.hxx> #include <xmlsignaturehelper.hxx> #include <documentsignaturemanager.hxx> +#include <biginteger.hxx> #include <certificate.hxx> #include <sfx2/docfile.hxx> #include <sfx2/docfilt.hxx> @@ -91,6 +92,7 @@ public: void testODFTripleX509Data(); void testODFMacroDoubleX509Data(); void testODFDoubleX509Certificate(); + void testDNCompatibility(); /// Test a typical OOXML where a number of (but not all) streams are signed. void testOOXMLPartial(); /// Test a typical broken OOXML signature where one stream is corrupted. @@ -154,6 +156,7 @@ public: CPPUNIT_TEST(testODFTripleX509Data); CPPUNIT_TEST(testODFMacroDoubleX509Data); CPPUNIT_TEST(testODFDoubleX509Certificate); + CPPUNIT_TEST(testDNCompatibility); CPPUNIT_TEST(testOOXMLPartial); CPPUNIT_TEST(testOOXMLBroken); CPPUNIT_TEST(testOOXMLDescription); @@ -752,6 +755,21 @@ void SigningTest::testODFDoubleX509Certificate() CPPUNIT_ASSERT(!infos[0].Signer.is()); } +void SigningTest::testDNCompatibility() +{ + OUString const msDN("CN=\"\"\"ABC\"\".\", O=\"Enterprise \"\"ABC\"\"\""); + OUString const nssDN("CN=\\\"ABC\\\".,O=Enterprise \\\"ABC\\\""); + // this is just the status quo, possibly either NSS or CryptoAPI might change + CPPUNIT_ASSERT(!xmlsecurity::EqualDistinguishedNames(msDN, nssDN, xmlsecurity::NOCOMPAT)); + CPPUNIT_ASSERT(!xmlsecurity::EqualDistinguishedNames(nssDN, msDN, xmlsecurity::NOCOMPAT)); + // with compat flag it should work, with the string one 2nd and the native one 1st +#ifdef _WIN32 + CPPUNIT_ASSERT(xmlsecurity::EqualDistinguishedNames(msDN, nssDN, xmlsecurity::COMPAT_2ND)); +#else + CPPUNIT_ASSERT(xmlsecurity::EqualDistinguishedNames(nssDN, msDN, xmlsecurity::COMPAT_2ND)); +#endif +} + void SigningTest::testOOXMLPartial() { createDoc(m_directories.getURLFromSrc(DATA_DIRECTORY) + "partial.docx"); diff --git a/xmlsecurity/source/component/documentdigitalsignatures.cxx b/xmlsecurity/source/component/documentdigitalsignatures.cxx index bedc07e261df..dc8d93ec8ec7 100644 --- a/xmlsecurity/source/component/documentdigitalsignatures.cxx +++ b/xmlsecurity/source/component/documentdigitalsignatures.cxx @@ -616,7 +616,7 @@ sal_Bool DocumentDigitalSignatures::isAuthorTrusted( for ( ; pAuthors != pAuthorsEnd; ++pAuthors ) { SvtSecurityOptions::Certificate aAuthor = *pAuthors; - if (xmlsecurity::EqualDistinguishedNames(aAuthor[0], xAuthor->getIssuerName()) + if (xmlsecurity::EqualDistinguishedNames(aAuthor[0], xAuthor->getIssuerName(), xmlsecurity::NOCOMPAT) && (aAuthor[1] == sSerialNum)) { bFound = true; diff --git a/xmlsecurity/source/helper/xmlsignaturehelper.cxx b/xmlsecurity/source/helper/xmlsignaturehelper.cxx index 86982f03dacf..538d4c696760 100644 --- a/xmlsecurity/source/helper/xmlsignaturehelper.cxx +++ b/xmlsecurity/source/helper/xmlsignaturehelper.cxx @@ -48,6 +48,8 @@ #include <comphelper/ofopxmlhelper.hxx> #include <comphelper/sequence.hxx> #include <tools/diagnose_ex.h> +#include <rtl/ustrbuf.hxx> +#include <sal/log.hxx> #include <boost/optional.hpp> @@ -633,7 +635,7 @@ static auto CheckX509Data( start = i; // issuer isn't in the list break; } - if (xmlsecurity::EqualDistinguishedNames(certs[i]->getIssuerName(), certs[j]->getSubjectName())) + if (xmlsecurity::EqualDistinguishedNames(certs[i]->getIssuerName(), certs[j]->getSubjectName(), xmlsecurity::NOCOMPAT)) { if (i == j) // self signed { @@ -666,7 +668,7 @@ static auto CheckX509Data( if (chain[i] != j) { if (xmlsecurity::EqualDistinguishedNames( - certs[chain[i]]->getSubjectName(), certs[j]->getIssuerName())) + certs[chain[i]]->getSubjectName(), certs[j]->getIssuerName(), xmlsecurity::NOCOMPAT)) { if (chain.size() != i + 1) // already found issuee? { diff --git a/xmlsecurity/source/helper/xsecverify.cxx b/xmlsecurity/source/helper/xsecverify.cxx index 503bae24d379..8d96433fd6cf 100644 --- a/xmlsecurity/source/helper/xsecverify.cxx +++ b/xmlsecurity/source/helper/xsecverify.cxx @@ -273,7 +273,7 @@ void XSecController::setX509Data( OUString const serialNumber(xmlsecurity::bigIntegerToNumericString(xCert->getSerialNumber())); auto const iter = std::find_if(rX509IssuerSerials.begin(), rX509IssuerSerials.end(), [&](auto const& rX509IssuerSerial) { - return xmlsecurity::EqualDistinguishedNames(issuerName, rX509IssuerSerial.first) + return xmlsecurity::EqualDistinguishedNames(issuerName, rX509IssuerSerial.first, xmlsecurity::COMPAT_2ND) && serialNumber == rX509IssuerSerial.second; }); if (iter != rX509IssuerSerials.end()) @@ -420,7 +420,7 @@ void XSecController::setX509CertDigest( { for (auto & it : rData) { - if (xmlsecurity::EqualDistinguishedNames(it.X509IssuerName, rX509IssuerName) + if (xmlsecurity::EqualDistinguishedNames(it.X509IssuerName, rX509IssuerName, xmlsecurity::COMPAT_BOTH) && it.X509SerialNumber == rX509SerialNumber) { it.CertDigest = rCertDigest; @@ -443,7 +443,7 @@ void XSecController::setX509CertDigest( { SAL_INFO("xmlsecurity.helper", "cannot parse X509Certificate"); } - else if (xmlsecurity::EqualDistinguishedNames(xCert->getIssuerName(),rX509IssuerName) + else if (xmlsecurity::EqualDistinguishedNames(xCert->getIssuerName(), rX509IssuerName, xmlsecurity::COMPAT_2ND) && xmlsecurity::bigIntegerToNumericString(xCert->getSerialNumber()) == rX509SerialNumber) { it.CertDigest = rCertDigest; diff --git a/xmlsecurity/source/xmlsec/mscrypt/x509certificate_mscryptimpl.cxx b/xmlsecurity/source/xmlsec/mscrypt/x509certificate_mscryptimpl.cxx index 062b91387cba..da0504addbc7 100644 --- a/xmlsecurity/source/xmlsec/mscrypt/x509certificate_mscryptimpl.cxx +++ b/xmlsecurity/source/xmlsec/mscrypt/x509certificate_mscryptimpl.cxx @@ -31,6 +31,7 @@ #include "oid.hxx" #include <rtl/locale.h> +#include <rtl/ustrbuf.hxx> #include <osl/nlsupport.h> #include <osl/process.h> #include <o3tl/char16_t2wchar_t.hxx> @@ -671,6 +672,67 @@ Sequence<OUString> SAL_CALL X509Certificate_MSCryptImpl::getSupportedServiceName namespace xmlsecurity { +// based on some guesswork and: +// https://datatracker.ietf.org/doc/html/rfc1485 +// https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-certnametostra#CERT_X500_NAME_STR +// the main problem appears to be that in values NSS uses \ escapes but CryptoAPI requires " quotes around value +static OUString CompatDNNSS(OUString const& rDN) +{ + OUStringBuffer buf(rDN.getLength()); + enum { DEFAULT, INVALUE, INQUOTE } state(DEFAULT); + for (sal_Int32 i = 0; i < rDN.getLength(); ++i) + { + if (state == DEFAULT) + { + buf.append(rDN[i]); + if (rDN[i] == '=') + { + if (rDN.getLength() == i+1) + { + break; // invalid? + } + else + { + buf.append('"'); + state = INVALUE; + } + } + } + else if (state == INVALUE) + { + if (rDN[i] == '+' || rDN[i] == ',' || rDN[i] == ';') + { + buf.append('"'); + buf.append(rDN[i]); + state = DEFAULT; + } + else if (rDN[i] == '\\') + { + if (rDN.getLength() == i+1) + { + break; // invalid? + } + if (rDN[i+1] == '"') + { + buf.append('"'); + } + buf.append(rDN[i+1]); + ++i; + } + else + { + buf.append(rDN[i]); + } + if (i+1 == rDN.getLength()) + { + buf.append('"'); + state = DEFAULT; + } + } + } + return buf.makeStringAndClear(); +} + static bool EncodeDistinguishedName(OUString const& rName, CERT_NAME_BLOB & rBlob) { LPCWSTR pszError; @@ -693,22 +755,38 @@ static bool EncodeDistinguishedName(OUString const& rName, CERT_NAME_BLOB & rBlo } bool EqualDistinguishedNames( - OUString const& rName1, OUString const& rName2) + OUString const& rName1, OUString const& rName2, + EqualMode const eMode) { + if (eMode == COMPAT_BOTH && !rName1.isEmpty() && rName1 == rName2) + { // handle case where both need to be converted + return true; + } CERT_NAME_BLOB blob1; if (!EncodeDistinguishedName(rName1, blob1)) { return false; } CERT_NAME_BLOB blob2; - if (!EncodeDistinguishedName(rName2, blob2)) + bool ret(false); + if (!!EncodeDistinguishedName(rName2, blob2)) { - delete[] blob1.pbData; - return false; + ret = CertCompareCertificateName(X509_ASN_ENCODING, + &blob1, &blob2) == TRUE; + delete[] blob2.pbData; + } + if (!ret && eMode == COMPAT_2ND) + { + CERT_NAME_BLOB blob2compat; + if (!EncodeDistinguishedName(CompatDNNSS(rName2), blob2compat)) + { + delete[] blob1.pbData; + return false; + } + ret = CertCompareCertificateName(X509_ASN_ENCODING, + &blob1, &blob2compat) == TRUE; + delete[] blob2compat.pbData; } - bool const ret(CertCompareCertificateName(X509_ASN_ENCODING, - &blob1, &blob2) == TRUE); - delete[] blob2.pbData; delete[] blob1.pbData; return ret; } diff --git a/xmlsecurity/source/xmlsec/nss/x509certificate_nssimpl.cxx b/xmlsecurity/source/xmlsec/nss/x509certificate_nssimpl.cxx index 332f9363f4dd..9e954fb91cca 100644 --- a/xmlsecurity/source/xmlsec/nss/x509certificate_nssimpl.cxx +++ b/xmlsecurity/source/xmlsec/nss/x509certificate_nssimpl.cxx @@ -30,6 +30,8 @@ #include <comphelper/servicehelper.hxx> #include <cppuhelper/supportsservice.hxx> #include <rtl/ref.hxx> +#include <rtl/ustrbuf.hxx> +#include <sal/log.hxx> #include "x509certificate_nssimpl.hxx" #include <biginteger.hxx> @@ -526,22 +528,103 @@ Sequence<OUString> SAL_CALL X509Certificate_NssImpl::getSupportedServiceNames() namespace xmlsecurity { +// based on some guesswork and: +// https://datatracker.ietf.org/doc/html/rfc1485 +// https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-certnametostra#CERT_X500_NAME_STR +// the main problem appears to be that in values " is escaped as "" vs. \" +static OUString CompatDNCryptoAPI(OUString const& rDN) +{ + OUStringBuffer buf(rDN.getLength()); + enum { DEFAULT, INVALUE, INQUOTE } state(DEFAULT); + for (sal_Int32 i = 0; i < rDN.getLength(); ++i) + { + if (state == DEFAULT) + { + buf.append(rDN[i]); + if (rDN[i] == '=') + { + if (rDN.getLength() == i+1) + { + break; // invalid? + } + else if (rDN[i+1] == '"') + { + buf.append(rDN[i+1]); + ++i; + state = INQUOTE; + } + else + { + state = INVALUE; + } + } + } + else if (state == INVALUE) + { + if (rDN[i] == '+' || rDN[i] == ',' || rDN[i] == ';') + { + state = DEFAULT; + } + buf.append(rDN[i]); + } + else + { + assert(state == INQUOTE); + if (rDN[i] == '"') + { + if (rDN.getLength() != i+1 && rDN[i+1] == '"') + { + buf.append('\\'); + buf.append(rDN[i+1]); + ++i; + } + else + { + buf.append(rDN[i]); + state = DEFAULT; + } + } + else + { + buf.append(rDN[i]); + } + } + } + return buf.makeStringAndClear(); +} + bool EqualDistinguishedNames( - OUString const& rName1, OUString const& rName2) + OUString const& rName1, OUString const& rName2, + EqualMode const eMode) { + if (eMode == COMPAT_BOTH && !rName1.isEmpty() && rName1 == rName2) + { // handle case where both need to be converted + return true; + } CERTName *const pName1(CERT_AsciiToName(OUStringToOString(rName1, RTL_TEXTENCODING_UTF8).getStr())); if (pName1 == nullptr) { return false; } CERTName *const pName2(CERT_AsciiToName(OUStringToOString(rName2, RTL_TEXTENCODING_UTF8).getStr())); - if (pName2 == nullptr) + bool ret(false); + if (pName2) { - CERT_DestroyName(pName1); - return false; + ret = (CERT_CompareName(pName1, pName2) == SECEqual); + CERT_DestroyName(pName2); + } + if (!ret && eMode == COMPAT_2ND) + { + CERTName *const pName2Compat(CERT_AsciiToName(OUStringToOString( + CompatDNCryptoAPI(rName2), RTL_TEXTENCODING_UTF8).getStr())); + if (pName2Compat == nullptr) + { + CERT_DestroyName(pName1); + return false; + } + ret = CERT_CompareName(pName1, pName2Compat) == SECEqual; + CERT_DestroyName(pName2Compat); } - bool const ret(CERT_CompareName(pName1, pName2) == SECEqual); - CERT_DestroyName(pName2); CERT_DestroyName(pName1); return ret; }
