Blob Blame History Raw
From 9ef423d4e7c85629772131b3216b98e17d7b8d7e Mon Sep 17 00:00:00 2001
From: Michael Stahl <michael.stahl@allotropia.de>
Date: Thu, 18 Feb 2021 19:22:31 +0100
Subject: [PATCH 4/6] CVE-2021-25634
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

xmlsecurity: XSecParser confused about multiple timestamps

LO writes timestamp both to dc:date and xades:SigningTime elements.

The parser tries to avoid reading multiple dc:date, preferring the first
one, but doesn't care about multiple xades:SigningTime, for undocumented
reasons.

Ideally something should check all read values for consistency.

Reviewed-on: https://gerrit.libreoffice.org/c/core/+/111160
Tested-by: Jenkins
Reviewed-by: Michael Stahl <michael.stahl@allotropia.de>
(cherry picked from commit 4ab8d9c09a5873ca0aea56dafa1ab34758d52ef7)

xmlsecurity: remove XSecController::setPropertyId()

Reviewed-on: https://gerrit.libreoffice.org/c/core/+/111252
Tested-by: Jenkins
Reviewed-by: Michael Stahl <michael.stahl@allotropia.de>
(cherry picked from commit d2a345e1163616fe3201ef1d6c758e2e819214e0)

Reviewed-on: https://gerrit.libreoffice.org/c/core/+/111908
Tested-by: Jenkins
Reviewed-by: Caolán McNamara <caolanm@redhat.com>
(cherry picked from commit abe77c4fcb9ea97d9fff07eaea6d8863bcba5b02)

Change-Id: Ic018ee89797a1c8a4f870ae102af48006de930ef
---
 include/svl/sigstruct.hxx                    |  7 +-
 xmlsecurity/inc/xsecctl.hxx                  |  5 +-
 xmlsecurity/source/helper/ooxmlsecparser.cxx |  4 +-
 xmlsecurity/source/helper/xsecctl.cxx        |  2 +-
 xmlsecurity/source/helper/xsecparser.cxx     | 81 ++++++++++----------
 xmlsecurity/source/helper/xsecparser.hxx     |  6 --
 xmlsecurity/source/helper/xsecsign.cxx       |  4 +-
 xmlsecurity/source/helper/xsecverify.cxx     | 39 ++++------
 8 files changed, 68 insertions(+), 80 deletions(-)

diff --git a/include/svl/sigstruct.hxx b/include/svl/sigstruct.hxx
index f6ee242c84d1..7a0296fa9fae 100644
--- a/include/svl/sigstruct.hxx
+++ b/include/svl/sigstruct.hxx
@@ -103,6 +103,9 @@ struct SignatureInformation
     // XAdES EncapsulatedX509Certificate values
     std::set<OUString> maEncapsulatedX509Certificates;
 
+    OUString ouSignatureId;
+    // signature may contain multiple time stamps - check they're consistent
+    bool hasInconsistentSigningTime = false;
     //We also keep the date and time as string. This is done when this
     //structure is created as a result of a XML signature being read.
     //When then a signature is added or another removed, then the original
@@ -115,8 +118,8 @@ struct SignatureInformation
     //and the converted time is written back, then the string looks different
     //and the signature is broken.
     OUString ouDateTime;
-    OUString ouSignatureId;
-    OUString ouPropertyId;
+    /// The Id attribute of the <SignatureProperty> element that contains the <dc:date>.
+    OUString ouDateTimePropertyId;
     /// Characters of the <dc:description> element inside the signature.
     OUString ouDescription;
     /// The Id attribute of the <SignatureProperty> element that contains the <dc:description>.
diff --git a/xmlsecurity/inc/xsecctl.hxx b/xmlsecurity/inc/xsecctl.hxx
index 351c94a2a3e6..7baa219fb13c 100644
--- a/xmlsecurity/inc/xsecctl.hxx
+++ b/xmlsecurity/inc/xsecctl.hxx
@@ -271,8 +271,8 @@ private:
     void setGpgCertificate( OUString const & ouGpgCert );
     void setGpgOwner( OUString const & ouGpgOwner );
 
-    void setDate( OUString const & ouDate );
-    void setDescription(const OUString& rDescription);
+    void setDate(OUString const& rId, OUString const& ouDate);
+    void setDescription(OUString const& rId, OUString const& rDescription);
     void setCertDigest(const OUString& rCertDigest);
     void setValidSignatureImage(const OUString& rValidSigImg);
     void setInvalidSignatureImage(const OUString& rInvalidSigImg);
@@ -283,7 +283,6 @@ public:
 
 private:
     void setId( OUString const & ouId );
-    void setPropertyId( OUString const & ouPropertyId );
 
     css::uno::Reference< css::xml::crypto::sax::XReferenceResolvedListener > prepareSignatureToRead(
         sal_Int32 nSecurityId );
diff --git a/xmlsecurity/source/helper/ooxmlsecparser.cxx b/xmlsecurity/source/helper/ooxmlsecparser.cxx
index c22e8c2261bf..a200de60c07a 100644
--- a/xmlsecurity/source/helper/ooxmlsecparser.cxx
+++ b/xmlsecurity/source/helper/ooxmlsecparser.cxx
@@ -192,12 +192,12 @@ void SAL_CALL OOXMLSecParser::endElement(const OUString& rName)
     }
     else if (rName == "mdssi:Value")
     {
-        m_pXSecController->setDate(m_aMdssiValue);
+        m_pXSecController->setDate("", m_aMdssiValue);
         m_bInMdssiValue = false;
     }
     else if (rName == "SignatureComments")
     {
-        m_pXSecController->setDescription(m_aSignatureComments);
+        m_pXSecController->setDescription("", m_aSignatureComments);
         m_bInSignatureComments = false;
     }
     else if (rName == "X509IssuerName")
diff --git a/xmlsecurity/source/helper/xsecctl.cxx b/xmlsecurity/source/helper/xsecctl.cxx
index ab108d13c224..8d5ea68c768b 100644
--- a/xmlsecurity/source/helper/xsecctl.cxx
+++ b/xmlsecurity/source/helper/xsecctl.cxx
@@ -819,7 +819,7 @@ void XSecController::exportSignature(
                 pAttributeList = new SvXMLAttributeList();
                 pAttributeList->AddAttribute(
                     "Id",
-                    signatureInfo.ouPropertyId);
+                    signatureInfo.ouDateTimePropertyId);
                 pAttributeList->AddAttribute(
                     "Target",
                     "#" + signatureInfo.ouSignatureId);
diff --git a/xmlsecurity/source/helper/xsecparser.cxx b/xmlsecurity/source/helper/xsecparser.cxx
index 5c92e5efa104..9cc9312b4d9f 100644
--- a/xmlsecurity/source/helper/xsecparser.cxx
+++ b/xmlsecurity/source/helper/xsecparser.cxx
@@ -978,6 +978,9 @@ class XSecParser::XadesSigningCertificateContext
 class XSecParser::XadesSigningTimeContext
     : public XSecParser::Context
 {
+    private:
+        OUString m_Value;
+
     public:
         XadesSigningTimeContext(XSecParser & rParser,
                 std::unique_ptr<SvXMLNamespaceMap> pOldNamespaceMap)
@@ -985,20 +988,14 @@ class XSecParser::XadesSigningTimeContext
         {
         }
 
-        virtual void StartElement(
-            css::uno::Reference<css::xml::sax::XAttributeList> const& /*xAttrs*/) override
-        {
-            m_rParser.m_ouDate.clear();
-        }
-
         virtual void EndElement() override
         {
-            m_rParser.m_pXSecController->setDate( m_rParser.m_ouDate );
+            m_rParser.m_pXSecController->setDate("", m_Value);
         }
 
         virtual void Characters(OUString const& rChars) override
         {
-            m_rParser.m_ouDate += rChars;
+            m_Value += rChars;
         }
 };
 
@@ -1104,35 +1101,20 @@ class XSecParser::DcDateContext
     : public XSecParser::Context
 {
     private:
-        bool m_isIgnore = false;
+        OUString & m_rValue;
 
     public:
         DcDateContext(XSecParser & rParser,
-                std::unique_ptr<SvXMLNamespaceMap> pOldNamespaceMap)
+                std::unique_ptr<SvXMLNamespaceMap> pOldNamespaceMap,
+                OUString & rValue)
             : XSecParser::Context(rParser, std::move(pOldNamespaceMap))
+            , m_rValue(rValue)
         {
         }
 
-        virtual void StartElement(
-            css::uno::Reference<css::xml::sax::XAttributeList> const& /*xAttrs*/) override
-        {
-            m_isIgnore = !m_rParser.m_ouDate.isEmpty();
-        }
-
-        virtual void EndElement() override
-        {
-            if (!m_isIgnore)
-            {
-                m_rParser.m_pXSecController->setDate( m_rParser.m_ouDate );
-            }
-        }
-
         virtual void Characters(OUString const& rChars) override
         {
-            if (!m_isIgnore)
-            {
-                m_rParser.m_ouDate += rChars;
-            }
+            m_rValue += rChars;
         }
 };
 
@@ -1140,29 +1122,32 @@ class XSecParser::DcDescriptionContext
     : public XSecParser::Context
 {
     private:
-        OUString m_Value;
+        OUString & m_rValue;
 
     public:
         DcDescriptionContext(XSecParser & rParser,
-                std::unique_ptr<SvXMLNamespaceMap> pOldNamespaceMap)
+                std::unique_ptr<SvXMLNamespaceMap> pOldNamespaceMap,
+                OUString & rValue)
             : XSecParser::Context(rParser, std::move(pOldNamespaceMap))
+            , m_rValue(rValue)
         {
         }
 
-        virtual void EndElement() override
-        {
-            m_rParser.m_pXSecController->setDescription(m_Value);
-        }
-
         virtual void Characters(OUString const& rChars) override
         {
-            m_Value += rChars;
+            m_rValue += rChars;
         }
 };
 
 class XSecParser::DsSignaturePropertyContext
     : public XSecParser::Context
 {
+    private:
+        enum class SignatureProperty { Unknown, Date, Description };
+        SignatureProperty m_Property = SignatureProperty::Unknown;
+        OUString m_Id;
+        OUString m_Value;
+
     public:
         DsSignaturePropertyContext(XSecParser & rParser,
                 std::unique_ptr<SvXMLNamespaceMap> pOldNamespaceMap)
@@ -1173,10 +1158,22 @@ class XSecParser::DsSignaturePropertyContext
         virtual void StartElement(
             css::uno::Reference<css::xml::sax::XAttributeList> const& xAttrs) override
         {
-            OUString const ouIdAttr(m_rParser.HandleIdAttr(xAttrs));
-            if (!ouIdAttr.isEmpty())
+            m_Id = m_rParser.HandleIdAttr(xAttrs);
+        }
+
+        virtual void EndElement() override
+        {
+            switch (m_Property)
             {
-                m_rParser.m_pXSecController->setPropertyId( ouIdAttr );
+                case SignatureProperty::Unknown:
+                    SAL_INFO("xmlsecurity.helper", "Unknown property in ds:Object ignored");
+                    break;
+                case SignatureProperty::Date:
+                    m_rParser.m_pXSecController->setDate(m_Id, m_Value);
+                    break;
+                case SignatureProperty::Description:
+                    m_rParser.m_pXSecController->setDescription(m_Id, m_Value);
+                    break;
             }
         }
 
@@ -1186,11 +1183,13 @@ class XSecParser::DsSignaturePropertyContext
         {
             if (nNamespace == XML_NAMESPACE_DC && rName == "date")
             {
-                return std::make_unique<DcDateContext>(m_rParser, std::move(pOldNamespaceMap));
+                m_Property = SignatureProperty::Date;
+                return std::make_unique<DcDateContext>(m_rParser, std::move(pOldNamespaceMap), m_Value);
             }
             if (nNamespace == XML_NAMESPACE_DC && rName == "description")
             {
-                return std::make_unique<DcDescriptionContext>(m_rParser, std::move(pOldNamespaceMap));
+                m_Property = SignatureProperty::Description;
+                return std::make_unique<DcDescriptionContext>(m_rParser, std::move(pOldNamespaceMap), m_Value);
             }
             return XSecParser::Context::CreateChildContext(std::move(pOldNamespaceMap), nNamespace, rName);
         }
diff --git a/xmlsecurity/source/helper/xsecparser.hxx b/xmlsecurity/source/helper/xsecparser.hxx
index 93efcb766e3e..7a0eb08bca28 100644
--- a/xmlsecurity/source/helper/xsecparser.hxx
+++ b/xmlsecurity/source/helper/xsecparser.hxx
@@ -97,12 +97,6 @@ private:
     class DsSignatureContext;
     class DsigSignaturesContext;
 
-    /*
-     * the following members are used to reserve the signature information,
-     * including X509IssuerName, X509SerialNumber, and X509Certificate,etc.
-     */
-    OUString m_ouDate;
-
     std::stack<std::unique_ptr<Context>> m_ContextStack;
     std::unique_ptr<SvXMLNamespaceMap> m_pNamespaceMap;
 
diff --git a/xmlsecurity/source/helper/xsecsign.cxx b/xmlsecurity/source/helper/xsecsign.cxx
index 4d1b89949feb..5ed23281f083 100644
--- a/xmlsecurity/source/helper/xsecsign.cxx
+++ b/xmlsecurity/source/helper/xsecsign.cxx
@@ -132,8 +132,8 @@ cssu::Reference< cssxc::sax::XReferenceResolvedListener > XSecController::prepar
     if (nStorageFormat != embed::StorageFormats::OFOPXML)
     {
         internalSignatureInfor.signatureInfor.ouSignatureId = createId();
-        internalSignatureInfor.signatureInfor.ouPropertyId = createId();
-        internalSignatureInfor.addReference(SignatureReferenceType::SAMEDOCUMENT, digestID, internalSignatureInfor.signatureInfor.ouPropertyId, -1, OUString() );
+        internalSignatureInfor.signatureInfor.ouDateTimePropertyId = createId();
+        internalSignatureInfor.addReference(SignatureReferenceType::SAMEDOCUMENT, digestID, internalSignatureInfor.signatureInfor.ouDateTimePropertyId, -1, OUString() );
         size++;
 
         if (bXAdESCompliantIfODF)
diff --git a/xmlsecurity/source/helper/xsecverify.cxx b/xmlsecurity/source/helper/xsecverify.cxx
index 1f7fa9ac8ca8..5f5840334254 100644
--- a/xmlsecurity/source/helper/xsecverify.cxx
+++ b/xmlsecurity/source/helper/xsecverify.cxx
@@ -321,7 +321,7 @@ void XSecController::setGpgOwner( OUString const & ouGpgOwner )
     isi.signatureInfor.ouGpgOwner = ouGpgOwner;
 }
 
-void XSecController::setDate( OUString const & ouDate )
+void XSecController::setDate(OUString const& rId, OUString const& ouDate)
 {
     if (m_vInternalSignatureInformations.empty())
     {
@@ -329,17 +329,31 @@ void XSecController::setDate( OUString const & ouDate )
         return;
     }
     InternalSignatureInformation &isi = m_vInternalSignatureInformations.back();
+    // there may be multiple timestamps in a signature - check them for consistency
+    if (!isi.signatureInfor.ouDateTime.isEmpty()
+        && isi.signatureInfor.ouDateTime != ouDate)
+    {
+        isi.signatureInfor.hasInconsistentSigningTime = true;
+    }
     (void)utl::ISO8601parseDateTime( ouDate, isi.signatureInfor.stDateTime);
     isi.signatureInfor.ouDateTime = ouDate;
+    if (!rId.isEmpty())
+    {
+        isi.signatureInfor.ouDateTimePropertyId = rId;
+    }
 }
 
-void XSecController::setDescription(const OUString& rDescription)
+void XSecController::setDescription(OUString const& rId, OUString const& rDescription)
 {
     if (m_vInternalSignatureInformations.empty())
         return;
 
     InternalSignatureInformation& rInformation = m_vInternalSignatureInformations.back();
     rInformation.signatureInfor.ouDescription = rDescription;
+    if (!rId.isEmpty())
+    {
+        rInformation.signatureInfor.ouDescriptionPropertyId = rId;
+    }
 }
 
 void XSecController::setSignatureBytes(const uno::Sequence<sal_Int8>& rBytes)
@@ -433,27 +447,6 @@ void XSecController::setId( OUString const & ouId )
     isi.signatureInfor.ouSignatureId = ouId;
 }
 
-void XSecController::setPropertyId( OUString const & ouPropertyId )
-{
-    if (m_vInternalSignatureInformations.empty())
-    {
-        SAL_INFO("xmlsecurity.helper","XSecController::setPropertyId: no signature");
-        return;
-    }
-    InternalSignatureInformation &isi = m_vInternalSignatureInformations.back();
-
-    if (isi.signatureInfor.ouPropertyId.isEmpty())
-    {
-        // <SignatureProperty> ID attribute is for the date.
-        isi.signatureInfor.ouPropertyId = ouPropertyId;
-    }
-    else
-    {
-        // <SignatureProperty> ID attribute is for the description.
-        isi.signatureInfor.ouDescriptionPropertyId = ouPropertyId;
-    }
-}
-
 /* public: for signature verify */
 void XSecController::collectToVerify( const OUString& referenceId )
 {
-- 
2.32.0