tdf#123747 xmlsecurity, ODF sign roundtrip: preserve invalid reference type

Only add the correct type to new signatures to avoid breaking the hash
of old ones.

Change-Id: I30f892b292f84a0575a3d4ef5ccf3eddbe0090ca
Reviewed-on: https://gerrit.libreoffice.org/70424
Tested-by: Jenkins
Reviewed-by: Miklos Vajna <vmiklos@collabora.com>

include/svl/sigstruct.hxx

@@ -48,6 +48,8 @@ struct SignatureReferenceInformation
     // For ODF: XAdES digests (SHA256) or the old SHA1, from css::xml::crypto::DigestID
     sal_Int32  nDigestID;
     OUString   ouDigestValue;
+    /// Type of the reference: an URI (newer idSignedProperties references) or empty.
+    OUString ouType;
 
     SignatureReferenceInformation() :
         nType(SignatureReferenceType::SAMEDOCUMENT),
@@ -57,12 +59,13 @@ struct SignatureReferenceInformation
     {
     }
 
-    SignatureReferenceInformation( SignatureReferenceType type, sal_Int32 digestID, const OUString& uri ) :
+    SignatureReferenceInformation( SignatureReferenceType type, sal_Int32 digestID, const OUString& uri, const OUString& rType ) :
         SignatureReferenceInformation()
     {
         nType = type;
         nDigestID = digestID;
         ouURI = uri;
+        ouType = rType;
     }
 };
 

xmlsecurity/inc/xsecctl.hxx

@@ -89,10 +89,10 @@ public:
         xReferenceResolvedListener = xListener;
     }
 
-    void addReference( SignatureReferenceType type, sal_Int32 digestID, const OUString& uri, sal_Int32 keeperId )
+    void addReference( SignatureReferenceType type, sal_Int32 digestID, const OUString& uri, sal_Int32 keeperId, const OUString& rType )
     {
         signatureInfor.vSignatureReferenceInfors.push_back(
-                SignatureReferenceInformation(type, digestID, uri));
+                SignatureReferenceInformation(type, digestID, uri, rType));
         vKeeperIds.push_back( keeperId );
     }
 };
@@ -260,7 +260,8 @@ private:
     void switchGpgSignature();
     void addReference(
         const OUString& ouUri,
-        sal_Int32 nDigestID );
+        sal_Int32 nDigestID,
+        const OUString& ouType );
     void addStreamReference(
         const OUString& ouUri,
         bool isBinary,

xmlsecurity/qa/unit/signing/signing.cxx

@@ -697,6 +697,65 @@ CPPUNIT_TEST_FIXTURE(SigningTest, test96097Doc)
     }
 }
 
+CPPUNIT_TEST_FIXTURE(SigningTest, testXAdESNotype)
+{
+    // Create a working copy.
+    utl::TempFile aTempFile;
+    aTempFile.EnableKillingFile();
+    OUString aURL = aTempFile.GetURL();
+    CPPUNIT_ASSERT_EQUAL(
+        osl::File::RC::E_None,
+        osl::File::copy(m_directories.getURLFromSrc(DATA_DIRECTORY) + "notype-xades.odt", aURL));
+
+    // Read existing signature.
+    DocumentSignatureManager aManager(mxComponentContext, DocumentSignatureMode::Content);
+    CPPUNIT_ASSERT(aManager.init());
+    uno::Reference<embed::XStorage> xStorage
+        = comphelper::OStorageHelper::GetStorageOfFormatFromURL(
+            ZIP_STORAGE_FORMAT_STRING, aTempFile.GetURL(), embed::ElementModes::READWRITE);
+    CPPUNIT_ASSERT(xStorage.is());
+    aManager.mxStore = xStorage;
+    aManager.maSignatureHelper.SetStorage(xStorage, "1.2");
+    aManager.read(/*bUseTempStream=*/false);
+
+    // Create a new signature.
+    uno::Reference<security::XCertificate> xCertificate
+        = getCertificate(aManager, svl::crypto::SignatureMethodAlgorithm::RSA);
+    if (!xCertificate.is())
+        return;
+    sal_Int32 nSecurityId;
+    aManager.add(xCertificate, mxSecurityContext, /*rDescription=*/OUString(), nSecurityId,
+                 /*bAdESCompliant=*/true);
+
+    // Write to storage.
+    aManager.read(/*bUseTempStream=*/true);
+    aManager.write(/*bXAdESCompliantIfODF=*/true);
+    uno::Reference<embed::XTransactedObject> xTransactedObject(xStorage, uno::UNO_QUERY);
+    xTransactedObject->commit();
+
+    // Parse the resulting XML.
+    uno::Reference<embed::XStorage> xMetaInf
+        = xStorage->openStorageElement("META-INF", embed::ElementModes::READ);
+    uno::Reference<io::XInputStream> xInputStream(
+        xMetaInf->openStreamElement("documentsignatures.xml", embed::ElementModes::READ),
+        uno::UNO_QUERY);
+    std::shared_ptr<SvStream> pStream(utl::UcbStreamHelper::CreateStream(xInputStream, true));
+    xmlDocPtr pXmlDoc = parseXmlStream(pStream.get());
+
+    // Without the accompanying fix in place, this test would have failed with "unexpected 'Type'
+    // attribute", i.e. the signature without such an attribute was not preserved correctly.
+    assertXPathNoAttribute(pXmlDoc,
+                           "/odfds:document-signatures/dsig:Signature[1]/dsig:SignedInfo/"
+                           "dsig:Reference[@URI='#idSignedProperties']",
+                           "Type");
+
+    // New signature always has the Type attribute.
+    assertXPath(pXmlDoc,
+                "/odfds:document-signatures/dsig:Signature[2]/dsig:SignedInfo/"
+                "dsig:Reference[@URI='#idSignedProperties']",
+                "Type", "http://uri.etsi.org/01903#SignedProperties");
+}
+
 /// Creates a XAdES signature from scratch.
 CPPUNIT_TEST_FIXTURE(SigningTest, testXAdES)
 {

xmlsecurity/source/helper/ooxmlsecparser.cxx

@@ -72,7 +72,7 @@ void SAL_CALL OOXMLSecParser::startElement(const OUString& rName, const uno::Ref
     {
         OUString aURI = xAttribs->getValueByName("URI");
         if (aURI.startsWith("#"))
-            m_pXSecController->addReference(aURI.copy(1), xml::crypto::DigestID::SHA1);
+            m_pXSecController->addReference(aURI.copy(1), xml::crypto::DigestID::SHA1, OUString());
         else
         {
             m_aReferenceURI = aURI;

xmlsecurity/source/helper/xsecctl.cxx

@@ -662,12 +662,12 @@ void XSecController::exportSignature(
                         "URI",
                         "#" + refInfor.ouURI);
 
-                    if (bXAdESCompliantIfODF && refInfor.ouURI == "idSignedProperties")
+                    if (bXAdESCompliantIfODF && refInfor.ouURI == "idSignedProperties" && !refInfor.ouType.isEmpty())
                     {
                         // The reference which points to the SignedProperties
                         // shall have this specific type.
                         pAttributeList->AddAttribute("Type",
-                                                     "http://uri.etsi.org/01903#SignedProperties");
+                                                     refInfor.ouType);
                     }
                 }
 

xmlsecurity/source/helper/xsecparser.cxx

@@ -129,12 +129,14 @@ void SAL_CALL XSecParser::startElement(
         {
             OUString ouUri = xAttribs->getValueByName("URI");
             SAL_WARN_IF( ouUri.isEmpty(), "xmlsecurity.helper", "URI is empty" );
+            // Remember the type of this reference.
+            OUString ouType = xAttribs->getValueByName("Type");
             if (ouUri.startsWith("#"))
             {
                 /*
                 * remove the first character '#' from the attribute value
                 */
-                m_pXSecController->addReference( ouUri.copy(1), m_nReferenceDigestID );
+                m_pXSecController->addReference( ouUri.copy(1), m_nReferenceDigestID, ouType );
             }
             else
             {

xmlsecurity/source/helper/xsecsign.cxx

@@ -138,12 +138,13 @@ cssu::Reference< cssxc::sax::XReferenceResolvedListener > XSecController::prepar
     {
         internalSignatureInfor.signatureInfor.ouSignatureId = createId();
         internalSignatureInfor.signatureInfor.ouPropertyId = createId();
-        internalSignatureInfor.addReference(SignatureReferenceType::SAMEDOCUMENT, digestID, internalSignatureInfor.signatureInfor.ouPropertyId, -1 );
+        internalSignatureInfor.addReference(SignatureReferenceType::SAMEDOCUMENT, digestID, internalSignatureInfor.signatureInfor.ouPropertyId, -1, OUString() );
         size++;
 
         if (bXAdESCompliantIfODF)
         {
-            internalSignatureInfor.addReference(SignatureReferenceType::SAMEDOCUMENT, digestID, "idSignedProperties", -1);
+            // We write a new reference, so it's possible to use the correct type URI.
+            internalSignatureInfor.addReference(SignatureReferenceType::SAMEDOCUMENT, digestID, "idSignedProperties", -1, "http://uri.etsi.org/01903#SignedProperties");
             size++;
         }
 
@@ -151,17 +152,17 @@ cssu::Reference< cssxc::sax::XReferenceResolvedListener > XSecController::prepar
         {
             // Only mention the hash of the description in the signature if it's non-empty.
             internalSignatureInfor.signatureInfor.ouDescriptionPropertyId = createId();
-            internalSignatureInfor.addReference(SignatureReferenceType::SAMEDOCUMENT, digestID, internalSignatureInfor.signatureInfor.ouDescriptionPropertyId, -1);
+            internalSignatureInfor.addReference(SignatureReferenceType::SAMEDOCUMENT, digestID, internalSignatureInfor.signatureInfor.ouDescriptionPropertyId, -1, OUString());
             size++;
         }
     }
     else
     {
-        internalSignatureInfor.addReference(SignatureReferenceType::SAMEDOCUMENT, digestID, "idPackageObject", -1);
+        internalSignatureInfor.addReference(SignatureReferenceType::SAMEDOCUMENT, digestID, "idPackageObject", -1, OUString());
         size++;
-        internalSignatureInfor.addReference(SignatureReferenceType::SAMEDOCUMENT, digestID, "idOfficeObject", -1);
+        internalSignatureInfor.addReference(SignatureReferenceType::SAMEDOCUMENT, digestID, "idOfficeObject", -1, OUString());
         size++;
-        internalSignatureInfor.addReference(SignatureReferenceType::SAMEDOCUMENT, digestID, "idSignedProperties", -1);
+        internalSignatureInfor.addReference(SignatureReferenceType::SAMEDOCUMENT, digestID, "idSignedProperties", -1, OUString());
         size++;
     }
 
@@ -189,7 +190,7 @@ void XSecController::signAStream( sal_Int32 securityId, const OUString& uri, boo
     if (index == -1)
     {
         InternalSignatureInformation isi(securityId, nullptr);
-        isi.addReference(type, digestID, uri, -1);
+        isi.addReference(type, digestID, uri, -1, OUString());
         m_vInternalSignatureInformations.push_back( isi );
     }
     else
@@ -197,7 +198,7 @@ void XSecController::signAStream( sal_Int32 securityId, const OUString& uri, boo
         // use sha512 for gpg signing unconditionally
         if (!m_vInternalSignatureInformations[index].signatureInfor.ouGpgCertificate.isEmpty())
             digestID = cssxc::DigestID::SHA512;
-        m_vInternalSignatureInformations[index].addReference(type, digestID, uri, -1);
+        m_vInternalSignatureInformations[index].addReference(type, digestID, uri, -1, OUString());
     }
 }
 

xmlsecurity/source/helper/xsecverify.cxx

@@ -148,7 +148,7 @@ void XSecController::switchGpgSignature()
 #endif
 }
 
-void XSecController::addReference( const OUString& ouUri, sal_Int32 nDigestID )
+void XSecController::addReference( const OUString& ouUri, sal_Int32 nDigestID, const OUString& ouType )
 {
     if (m_vInternalSignatureInformations.empty())
     {
@@ -156,7 +156,7 @@ void XSecController::addReference( const OUString& ouUri, sal_Int32 nDigestID )
         return;
     }
     InternalSignatureInformation &isi = m_vInternalSignatureInformations.back();
-    isi.addReference(SignatureReferenceType::SAMEDOCUMENT, nDigestID, ouUri, -1 );
+    isi.addReference(SignatureReferenceType::SAMEDOCUMENT, nDigestID, ouUri, -1, ouType );
 }
 
 void XSecController::addStreamReference(
@@ -189,7 +189,7 @@ void XSecController::addStreamReference(
         }
     }
 
-    isi.addReference(type, nDigestID, ouUri, -1);
+    isi.addReference(type, nDigestID, ouUri, -1, OUString());
 }
 
 void XSecController::setReferenceCount() const