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.)

Change-Id: I4f72900738d1f5313146bbda7320a8f44319ebc8
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/124287
Tested-by: Jenkins
Reviewed-by: Michael Stahl <michael.stahl@allotropia.de>
(cherry picked from commit e63611fabd38c757809b510fbb71c077880b1081)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/124196
Reviewed-by: Thorsten Behrens <thorsten.behrens@allotropia.de>

8 files changed, 208 insertions, 20 deletions

diff --git a/xmlsecurity/CppunitTest_xmlsecurity_signing.mk b/xmlsecurity/CppunitTest_xmlsecurity_signing.mk
index 2d8d92c37300..bf746b022de7 100644
--- a/xmlsecurity/CppunitTest_xmlsecurity_signing.mk
+++ b/xmlsecurity/CppunitTest_xmlsecurity_signing.mk
@@ -30,6 +30,7 @@ $(eval $(call gb_CppunitTest_use_libraries,xmlsecurity_signing, \
 	utl \
 	vcl \
 	xmlsecurity \
+	xsec_xmlsec \
 ))
 
 $(eval $(call gb_CppunitTest_use_externals,xmlsecurity_signing,\
diff --git a/xmlsecurity/inc/biginteger.hxx b/xmlsecurity/inc/biginteger.hxx
index c15b54de6229..fc99320c7ea2 100644
--- a/xmlsecurity/inc/biginteger.hxx
+++ b/xmlsecurity/inc/biginteger.hxx
@@ -35,8 +35,17 @@ XSECXMLSEC_DLLPUBLIC OUString bigIntegerToNumericString(const css::uno::Sequence
 XSECXMLSEC_DLLPUBLIC css::uno::Sequence<sal_Int8>
 numericStringToBigInteger(std::u16string_view 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(std::u16string_view rName1,
-                                                  std::u16string_view rName2);
+                                                  std::u16string_view rName2, EqualMode eMode);
 }
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/xmlsecurity/qa/unit/signing/signing.cxx b/xmlsecurity/qa/unit/signing/signing.cxx
index b6f76d69034d..4a726e0d7a2f 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 <xsecctl.hxx>
 #include <sfx2/docfile.hxx>
@@ -662,6 +663,21 @@ CPPUNIT_TEST_FIXTURE(SigningTest, testODFDoubleX509Certificate)
     CPPUNIT_ASSERT(!infos[0].Signer.is());
 }
 
+CPPUNIT_TEST_FIXTURE(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
+}
+
 /// Test a typical OOXML where a number of (but not all) streams are signed.
 CPPUNIT_TEST_FIXTURE(SigningTest, testOOXMLPartial)
 {
diff --git a/xmlsecurity/source/component/documentdigitalsignatures.cxx b/xmlsecurity/source/component/documentdigitalsignatures.cxx
index 5a353c39924b..58fff6003e9a 100644
--- a/xmlsecurity/source/component/documentdigitalsignatures.cxx
+++ b/xmlsecurity/source/component/documentdigitalsignatures.cxx
@@ -667,7 +667,7 @@ sal_Bool DocumentDigitalSignatures::isAuthorTrusted(
 
     return std::any_of(aTrustedAuthors.begin(), aTrustedAuthors.end(),
         [&xAuthor, &sSerialNum](const SvtSecurityOptions::Certificate& rAuthor) {
-            return xmlsecurity::EqualDistinguishedNames(rAuthor.SubjectName, xAuthor->getIssuerName())
+            return xmlsecurity::EqualDistinguishedNames(rAuthor.SubjectName, xAuthor->getIssuerName(), xmlsecurity::NOCOMPAT)
                 && ( rAuthor.SerialNumber == sSerialNum );
         });
 }
diff --git a/xmlsecurity/source/helper/xmlsignaturehelper.cxx b/xmlsecurity/source/helper/xmlsignaturehelper.cxx
index 3dda90fa2b02..5f3cd7b7038f 100644
--- a/xmlsecurity/source/helper/xmlsignaturehelper.cxx
+++ b/xmlsecurity/source/helper/xmlsignaturehelper.cxx
@@ -44,6 +44,7 @@
 #include <comphelper/ofopxmlhelper.hxx>
 #include <comphelper/sequence.hxx>
 #include <tools/diagnose_ex.h>
+#include <rtl/ustrbuf.hxx>
 #include <sal/log.hxx>
 
 #include <optional>
@@ -607,7 +608,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
                 {
@@ -640,7 +641,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 issue?
                     {
diff --git a/xmlsecurity/source/helper/xsecverify.cxx b/xmlsecurity/source/helper/xsecverify.cxx
index b5b4af76b1cf..170346ba5936 100644
--- a/xmlsecurity/source/helper/xsecverify.cxx
+++ b/xmlsecurity/source/helper/xsecverify.cxx
@@ -271,7 +271,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())
@@ -418,7 +418,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;
@@ -441,7 +441,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 b40639f586d8..97648f3b85cd 100644
--- a/xmlsecurity/source/xmlsec/mscrypt/x509certificate_mscryptimpl.cxx
+++ b/xmlsecurity/source/xmlsec/mscrypt/x509certificate_mscryptimpl.cxx
@@ -33,6 +33,7 @@
 
 #include <rtl/locale.h>
 #include <rtl/ref.hxx>
+#include <rtl/ustrbuf.hxx>
 #include <osl/nlsupport.h>
 #include <osl/process.h>
 #include <o3tl/char16_t2wchar_t.hxx>
@@ -654,6 +655,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(std::u16string_view const rName, CERT_NAME_BLOB & rBlob)
 {
     LPCWSTR pszError;
@@ -676,22 +738,38 @@ static bool EncodeDistinguishedName(std::u16string_view const rName, CERT_NAME_B
 }
 
 bool EqualDistinguishedNames(
-        std::u16string_view const rName1, std::u16string_view const rName2)
+        std::u16string_view const rName1, std::u16string_view const rName2,
+        EqualMode const eMode)
 {
+    if (eMode == COMPAT_BOTH && !rName1.empty() && 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(OUString(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 bd3ad0174273..695888d89c27 100644
--- a/xmlsecurity/source/xmlsec/nss/x509certificate_nssimpl.cxx
+++ b/xmlsecurity/source/xmlsec/nss/x509certificate_nssimpl.cxx
@@ -29,6 +29,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>
@@ -532,22 +534,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(
-        std::u16string_view const rName1, std::u16string_view const rName2)
+        std::u16string_view const rName1, std::u16string_view const rName2,
+        EqualMode const eMode)
 {
+    if (eMode == COMPAT_BOTH && !rName1.empty() && 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(OUString(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;
 }