Add implicit C14N when signing if no transforms specified

[roc13x]

When signing, the library currently does not apply any canonicalization to the referenced document being signed, unless explicitly specified in the transforms config property.

This behaviour differs from many other XML signature libraries such as javax.xml.crypto.dsig and signxml, who implicitly apply http://www.w3.org/TR/2001/REC-xml-c14n-20010315 if the user does not specify any canonicalization transforms.

This PR proposes adding the same implicit canonicalization behaviour, to make the output more consistent with other implementations.

Related issues: #210 #212 #484 #486

[markstos]

It seems the SAML spec does support this as an option:

http://docs.oasis-open.org/security/saml/v2.0/saml-core-2.0-os.pdf at line 2941:

Signatures in SAML messages SHOULD NOT contain transforms other than the enveloped
signature transform (with the identifier http://www.w3.org/2000/09/xmldsig#enveloped-signature) > or the exclusive canonicalization transforms (with the identifier http://www.w3.org/2001/10/xml-exc-> c14n# or http://www.w3.org/2001/10/xml-exc-c14n#WithComments).

My concern is that while this may improve things for some users, it will break them for others, as it changes a default behavior. There are reports-- I'm not sure if they are current-- that ADFS may have problems and some Java-based servers may have problems as well.

That concern could be addressed if the new behavior was opt-in, or if there was evidence that would help in more cases then it might break.

if (this.options.enableImplicitCanonicalisation !== false) {
  // Apply implicit transform
}

[markstos]

Wouldn't exclusive C14N be more compatible?

transforms.push("http://www.w3.org/2001/10/xml-exc-c14n#");

[srd90]

First of all: I'm not XMLSIG expert.

@markstos Lets put case SAML aside for a moment and investigate XMLSIG... xml-crypto already applies implicit http://www.w3.org/TR/2001/REC-xml-c14n-20010315 C14N automatically while validating signature but it does not do such thing while generating signature.

Algorithm seems to be that:

1. if transforms list is empty add http://www.w3.org/TR/2001/REC-xml-c14n-20010315 C14N algorithm or
2. if http://www.w3.org/2000/09/xmldsig#enveloped-signature is last element in transforms list append http://www.w3.org/TR/2001/REC-xml-c14n-20010315 C14N after that (NOTE: if last algorithm at the tranforms list is NOT envenloped signature then http://www.w3.org/TR/2001/REC-xml-c14n-20010315 C14N is not added to transforms list)

Other tools seems to apply symmetrically similar automatic C14N See e.g. #212 (comment) and #212 (comment) .

Side note: xml-crypto provides functionality to introduce one or more "API user defined" implicit transforms prior to validating signature

xml-crypto/lib/signed-xml.js

Lines 649 to 654 in d295ecc

	var hasImplicitTransforms = (Array.isArray(this.implicitTransforms) && this.implicitTransforms.length > 0);
	if(hasImplicitTransforms){
	this.implicitTransforms.forEach(function(t){
	transforms.push(t);
	});
	}

Those are applied to the end of transforms list and after that code proceeds to check whether "overall list of transformas at validation phase" should contain also http://www.w3.org/TR/2001/REC-xml-c14n-20010315 C14N:

xml-crypto/lib/signed-xml.js

Lines 656 to 667 in d295ecc

	/**
	* DigestMethods take an octet stream rather than a node set. If the output of the last transform is a node set, we
	* need to canonicalize the node set to an octet stream using non-exclusive canonicalization. If there are no
	* transforms, we need to canonicalize because URI dereferencing for a same-document reference will return a node-set.
	* See:
	* https://www.w3.org/TR/xmldsig-core1/#sec-DigestMethod
	* https://www.w3.org/TR/xmldsig-core1/#sec-ReferenceProcessingModel
	* https://www.w3.org/TR/xmldsig-core1/#sec-Same-Document
	*/
	if (transforms.length === 0 || transforms[transforms.length - 1] === "http://www.w3.org/2000/09/xmldsig#enveloped-signature") {
	transforms.push("http://www.w3.org/TR/2001/REC-xml-c14n-20010315")
	}

Signing (with xml-crypto) requires always explicitly defined transforms (i.e. visible at signature element's transforms structure).

About SAML aspect:

e.g. @node-saml/node-saml sets following tranforms explicitly (unless otherwise configured at node-saml options) during signing (link refers to version 5.0.1):
https://github.com/node-saml/node-saml/blob/668cbb67b74f810e80bd090d9c7804a1fdb7b80b/src/xml.ts#L150-L159
which

1. seems to be inline with SAML spec you referred
2. those transforms ends up to produced XML documents <transforms> element
3. and because enveloped signature is followed by http://www.w3.org/2001/10/xml-exc-c14n# C14N addition of implicit http://www.w3.org/TR/2001/REC-xml-c14n-20010315 C14N is not trigged by xml-crypto (nor by e.g. xmlsec1 / xmlsec lib ... based on experiments)

So...it seems that at least SAML case is covered even if automatic implicit http://www.w3.org/TR/2001/REC-xml-c14n-20010315 C14N would be added also to signing path. Another possibility would be to document with bold text that xml-crypto does not apply any implicit transforms (like some other tools) under any circumstances while signing and that it would be perfectly a-ok to add e.g. http://www.w3.org/TR/2001/REC-xml-c14n-20010315 C14N explicitly to transforms list. There is at least one case where user of xml-crypto api ended up to quite "interesting" (not in a good way interesting) solution to have only one element at transforms list (maybe user valued structure of some example document at some internal requirement over anything else) while still applying also http://www.w3.org/TR/2001/REC-xml-c14n-20010315 C14N see #484 (comment)

My point is that evidence shows that lack of implicit http://www.w3.org/TR/2001/REC-xml-c14n-20010315 C14N (applied under same circumstances when validating signature) seems to be constant source of confusion and some may end up forking xml-crypto to introduce quick hack (and probably never merge bug fixes from upstream project to fork).

Maybe @ahacker1-securesaml has some insight to share to this PR/discussion especially if work with new validation / signing functionality is still in active phase.

random sidenote: interesting piece of history at this issue lsh123/xmlsec#841 (comment)

[ahacker1-securesaml]

a) We should apply the http://www.w3.org/TR/2001/REC-xml-c14n-20010315 transform if output of last transform is a node-set. For both signing and verification.

However, I disagree with the method that this is implemented in the PR and library.
First of all, we should follow as closely as possible to the Java (Santuario) library for how they generate signatures, since they are most familiar with XML spec.

We should avoid adding implicit transforms (like in the PR and how it was previously implemented). Instead, we should:
Right before hashing the input, check the XML input:

• If it is a node-set: then set hash input as apply c14n canonicalization
• If it is an octets, hash input is the octets

This is same method as how Santuario does it. Internally, they create a function for each transform that accepts XMLSignatureInput: octets | Node-Set, then transforms depending on it.

Conclusion:
In a future release, v7.x (security improvement release), I will probably also add these improvements