You can not select more than 25 topics
Topics must start with a letter or number, can include dashes ('-') and can be up to 35 characters long.
398 lines
15 KiB
398 lines
15 KiB
2 years ago
|
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
|
||
|
|
|