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