xml-crypto icon indicating copy to clipboard operation
xml-crypto copied to clipboard

New line and whitespace normalisation in canonicalisation operation

Open dventurait opened this issue 3 years ago • 3 comments

I have and error related to unmatching digests. After debugging some signature validation errors on SAML responses, I would like to discuss with you a couple of questions.

The SAML response we have is like this:


<bar>\r\n
  <foo>Something Here</foo>\r\n
</bar>\r\n

In the Canonical process the \r chars are replaced by &#xD; ( see this function: https://github.com/yaronn/xml-crypto/blob/master/lib/utils.js#L66)

This code will modify the SAML response generating this output:


<bar>&#xD;\n
  <foo>Something Here</foo>&#xD;\n
</bar>&#xD;\n

The digest is calculated on this modified XML which generates a digest different from the one in the original SAML response. The consequence is the validation process fails.

I have a couple of questions:

  • This reference https://www.w3.org/TR/xml-c14n/#ProcessingModel says “all #xD characters are replaced by &#xD;”. So why you are replacing \r with &#xD in this line https://github.com/yaronn/xml-crypto/blob/master/lib/utils.js#L53
  • This code .replace(/\r\n?/g, '\n') was removed in this PR https://github.com/yaronn/xml-crypto/pull/196/files - but it looks to me it’s the right code in order to prevent the issue I have being having and it will make the operation to calculate the digest be successful. Can this code be re-introduced? Otherwise what should be the right approach to follow?

dventurait avatar Oct 08 '21 15:10 dventurait

  • This reference https://www.w3.org/TR/xml-c14n/#ProcessingModel says “all #xD characters are replaced by &#xD;”. So why you are replacing \r with &#xD in this line https://github.com/yaronn/xml-crypto/blob/master/lib/utils.js#L53

Unless I'm missing something, that replacement is only for Attribute Nodes.

cjbarth avatar Jul 24 '22 23:07 cjbarth

  • Otherwise what should be the right approach to follow?

Have you looked at #195 for thoughts?

cjbarth avatar Jul 24 '22 23:07 cjbarth

We'd welcome a PR that includes some tests of what the problem is. It would then be easier to determine what the appropriate fix would be. Having said that, you might also want to look at #286 where we have several broken tests. Perhaps one of those addresses your issue and you could submit a PR to fix the test.

cjbarth avatar May 29 '23 18:05 cjbarth