Mailing-List: contact security-dev-help@xml.apache.org; run by ezmlm
Precedence: bulk
Reply-To: security-dev@xml.apache.org
Received-SPF: pass (athena.apache.org: local policy)
Message-ID: <4A31139A.70200@metanate.com>
Date: Thu, 11 Jun 2009 15:24:26 +0100
From: John Keeping <john@metanate.com>
User-Agent: Thunderbird 2.0.0.21 (X11/20090429)
MIME-Version: 1.0
To: security-dev@xml.apache.org
Subject: [PATCH] xml-security-c: Potential bug in canonicalization from an
 XPathNodeList
Content-Type: multipart/mixed;
 boundary="------------090907020002070703080702"
DomainKey-Status: no signature ()

This is a multi-part message in MIME format.
--------------090907020002070703080702
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit

Hi,

We've recently started using xml-security-c and while trying to validate
signatures in some test files we noticed some digest failures.

Using the "2 out of 3" rule, the digests validate in xmlsec
(http://www.aleksey.com/xmlsec/) and using a validator based on the Java
6 libraries but not xml-security-c, so I believe xml-security-c is at
fault. By dumping out the byte stream at the point of digest generation,
I think I've narrowed this down to the XSECC14n20010315 canonicalization
in the case where it's fed by a XPathNodeList as provided by an
enveloped signature transform.

For this bug to be exposed, the referenced section of the file must be
using no namespace prefix but be in a namespace defined on a parent
element not in the node set.

I'm attaching an example file which illustrates this structure (although
I've removed some of the signature block and the digest is definitely
invalid, so it won't validate) and a test program through which the file
can be run to demonstrate the problem.

The test program can be run as:

./test test.xml data1 test.out

expected.xml contains the expected output.

I've created a patch (fix_c14n.patch) which fixes this for me, but it's
not extensively tested and I'm not intimately familiar with the source
so there may well be a better way of doing this. It's also quite
possible that I've broken something else, so please don't apply this
blindly!


Regards,

John

-- 
John Keeping
Metanate Ltd
www.metanate.com (Software consultancy)
www.schemus.com (Data synchronisation)

This e-mail and all attachments it may contain is confidential and
intended solely for the use of the individual to whom it is addressed.
Any views or opinions presented are those of the author and do not
necessarily represent those of Metanate Ltd.  If you are not the
intended recipient, be advised that you have received this e-mail in
error and that any use, dissemination, printing, forwarding or copying
of this e-mail is strictly prohibited.  Please contact the sender if
you have received this e-mail in error.


--------------090907020002070703080702
Content-Type: text/xml;
 name="expected.xml"
Content-Transfer-Encoding: base64
Content-Disposition: inline;
 filename="expected.xml"

PERhdGEgeG1sbnM9Imh0dHA6Ly93d3cuZXhhbXBsZS5jb20vZG9jdW1lbnQiIGlkPSJkYXRh
MSI+CiAgICA8RmlsZU5hbWU+aW1hZ2UuanBlZzwvRmlsZU5hbWU+CiAgICA8RmlsZVR5cGU+
aW1hZ2UvanBlZzwvRmlsZVR5cGU+CiAgPC9EYXRhPg==
--------------090907020002070703080702
Content-Type: text/x-c++src;
 name="test.cpp"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="test.cpp"

#include <cstdlib>

#include <iostream>

#include <xsec/utils/XSECPlatformUtils.hpp>
#include <xsec/framework/XSECProvider.hpp>
#include <xsec/framework/XSECException.hpp>
#include <xsec/transformers/TXFMChain.hpp>
#include <xsec/transformers/TXFMC14n.hpp>
#include <xsec/transformers/TXFMOutputFile.hpp>
#include <xsec/transformers/TXFMDocObject.hpp>
#include <xsec/transformers/TXFMEnvelope.hpp>
#include <xsec/utils/XSECDOMUtils.hpp>
#include <xsec/framework/XSECError.hpp>

#include <xercesc/util/PlatformUtils.hpp>
#include <xercesc/util/XMLString.hpp>

#include <xercesc/dom/DOM.hpp>
#include <xercesc/parsers/XercesDOMParser.hpp>
#include <xercesc/util/XMLException.hpp>

XERCES_CPP_NAMESPACE_USE

#ifndef XSEC_NO_XALAN

#include <xalanc/XalanTransformer/XalanTransformer.hpp>
#include <xalanc/XPath/XPathEvaluator.hpp>
#include <xalanc/XalanDOM/XalanDOMString.hpp>

XALAN_USING_XALAN(XPathEvaluator)
XALAN_USING_XALAN(XalanTransformer)
XALAN_USING_XALAN(XalanDOMString)

#endif // XSEC_NO_XALAN

void usage(char *name) {
	std::cout << "Usage: " << name << " <xml-file> <element-id> <output-file>" << std::endl;
}

int main(int argc, char *argv[]) {
	if(4 != argc) {
		usage(argv[0]);
		exit(1);
	}

	try {
		XMLPlatformUtils::Initialize();
#ifndef XSEC_NO_XALAN
		XPathEvaluator::initialize();
		XalanTransformer::initialize();
#endif
		XSECPlatformUtils::Initialise();
	} catch (const XMLException &e) {
		std::cerr << "Error during initialization of Xerces: " << e.getMessage() << std::endl;
		exit(2);
	}

	const char *xmlFile(argv[1]);
	const char *elementId(argv[2]);
	char *outFile(argv[3]);

	int errorCount = 0;

	{
		XercesDOMParser parser;
		parser.setDoNamespaces(true);
		parser.setCreateEntityReferenceNodes(true);

		try {
			parser.parse(xmlFile);
			errorCount = parser.getErrorCount();
		} catch (const XMLException &e) {
			std::cerr << "An error occurred during parsing: " << e.getMessage() << std::endl;
			errorCount = -1;
		} catch (const DOMException &e) {
			std::cerr << "A DOM error occurred during parsing: " << e.getMessage() << std::endl;
			errorCount = -1;
		}

		if(0 == errorCount) {
			DOMDocument *doc(parser.getDocument());

			DOMTreeWalker *walker(((DOMDocumentTraversal *) doc)->createTreeWalker(doc->getDocumentElement(), DOMNode::ELEMENT_NODE, NULL, false));

			// Mark attributes with name id or Id as ID attributes
			XMLCh Id[] = { chLatin_I, chLatin_d, chNull };
			XMLCh id[] = { chLatin_i, chLatin_d, chNull };
			DOMNode *node;
			while(NULL != (node = walker->nextNode())) {
				if(DOMNode::ELEMENT_NODE == node->getNodeType()) {
					DOMElement *elem((DOMElement *) node);
					DOMAttr *attr(elem->getAttributeNode(id));
					if(NULL == attr)
						attr = elem->getAttributeNode(Id);
					if(NULL != attr) {
						elem->setIdAttributeNode(attr, true);
					}
				}
			}

			try {
				// We need to find a transform node so that the envelope transform will work correctly
				DOMNodeList *txfmNodes(doc->getElementsByTagNameNS(DSIGConstants::s_unicodeStrURIDSIG, XalanDOMString("Transform").data()));
				if(1 > txfmNodes->getLength()) {
					throw XSECException(XSECException::UnknownError, "No Transform node found");
				}
				DOMNode *txfmNode(txfmNodes->item(0));

				// Now set up our transforms, we need a subset of the document to expose the bug
				TXFMDocObject *docTxfm;
				XSECnew(docTxfm, TXFMDocObject(doc));
				docTxfm->setInput(doc, XMLString::transcode(elementId));
				docTxfm->stripComments();

				TXFMChain chain(docTxfm);

				// The envelope transform feeds an XPath node list to the c14n transform
				TXFMEnvelope *envTxfm;
				XSECnew(envTxfm, TXFMEnvelope(doc));
				chain.appendTxfm(envTxfm);
				envTxfm->evaluateEnvelope(txfmNode);

				TXFMC14n *c14nTxfm;
				XSECnew(c14nTxfm, TXFMC14n(doc));
				chain.appendTxfm(c14nTxfm);

				TXFMOutputFile *fileTxfm;
				XSECnew(fileTxfm, TXFMOutputFile(doc));
				chain.appendTxfm(fileTxfm);
				if(!fileTxfm->setFile(outFile))
					throw XSECException(XSECException::UnknownError, "Failed to open output file");

				XMLByte toFill[1024];
				XMLSize_t size;
				while(0 != (size = chain.getLastTxfm()->readBytes(toFill, 1024)))
					;
			} catch (XSECException &e) {
				std::cerr << "An error occurred while processing: " << XMLString::transcode(e.getMsg()) << std::endl;
				errorCount = -1;
			}
		}
	}

	XSECPlatformUtils::Terminate();
#ifndef XSEC_NO_XALAN
	XalanTransformer::terminate();
	XPathEvaluator::terminate();
#endif
	XMLPlatformUtils::Terminate();

	return 0 == errorCount ? 0 : 2;
}

--------------090907020002070703080702
Content-Type: text/xml;
 name="test.xml"
Content-Transfer-Encoding: 8bit
Content-Disposition: inline;
 filename="test.xml"

﻿<?xml version="1.0" encoding="UTF-8"?>
<Document xmlns="http://www.example.com/document">
  <Signature xmlns="http://www.w3.org/2000/09/xmldsig#" Id="signature1">
    <SignedInfo>
      <CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#WithComments"/>
      <SignatureMethod Algorithm="http://www.w3.org/2000/09/xmldsig#rsa-sha1"/>
      <Reference URI="#data1">
        <Transforms>
          <Transform Algorithm="http://www.w3.org/2000/09/xmldsig#enveloped-signature"/>
        </Transforms>
        <DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/>
        <DigestValue>XIpTOJt677eAqa02BT3OwZMjIA0=</DigestValue>
      </Reference>
    </SignedInfo>
  </Signature>
  <Data id="data1">
    <FileName>image.jpeg</FileName>
    <FileType>image/jpeg</FileType>
  </Data>
</Document>

--------------090907020002070703080702
Content-Type: text/plain;
 name="fix_c14n.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="fix_c14n.patch"

--- src/canon/XSECC14n20010315.cpp	2009/04/17 15:51:27	1.2
+++ src/canon/XSECC14n20010315.cpp	2009/06/11 13:56:13
@@ -842,9 +842,11 @@
 		// in question
 
 		parent = e->getParentNode();
+		bool intermediateNodeFound(false);
 		while (parent != NULL) {
 
-			if (!m_XPathSelection || m_XPathMap.hasNode(parent)) {
+			if (!m_XPathSelection || intermediateNodeFound || m_XPathMap.hasNode(parent)) {
+				intermediateNodeFound = true;
 				DOMNamedNodeMap *pmap = parent->getAttributes();
 				DOMNode *pns;
 				if (pmap)

--------------090907020002070703080702
Content-Type: text/xml;
 name="actual.xml"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="actual.xml"

<Data xmlns="http://www.example.com/document" id="data1">
    <FileName xmlns="http://www.example.com/document">image.jpeg</FileName>
    <FileType xmlns="http://www.example.com/document">image/jpeg</FileType>
  </Data>

--------------090907020002070703080702--