Invalid signature after passport-saml upgraded xml-crypto to ^1.0.2: but solved

[huineng]

I was using passport-saml since a long time and all was working fine until they upgraded crypto-xml to ^1.0.2. (from xml-crypto@0.10.1)

After 2 days banging my head on this i solved it, though i like to know if the changes from xml-crypto@0.10.1

it's all about the rsaml response attributes (examples below taken from various copies)

attributes on the incoming saml

<samlp:Response 
    Destination="blablabla"
    ID="FIMRSP_50b75184-0168-1d14-b75b-951f7c61a75a" 
    InResponseTo="_78262119f68b5424897b"
    IssueInstant="2019-01-15T08:53:37Z" Version="2.0" 
    xmlns:ds="http://www.w3.org/2000/09/xmldsig#"
    xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion"
    xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol" 
    xmlns:xs="http://www.w3.org/2001/XMLSchema"
    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">

using crypto-xml 0.10.1 in the function to get the canonXML this produced

<saml:Assertion
	xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion"
	xmlns:xs="http://www.w3.org/2001/XMLSchema"
	xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" 
        ID="Assertion-uuid505be4ab-0168-12f2-b56e-e5decbef3937"
        IssueInstant="2019-01-15T07:13:46Z" 
        Version="2.0">

with ^1.0.2 i suddenly got invalid signature, but the reason was that the digest no longer matched (invalid signature: for uri....)

the Assertion now produced

<saml:Assertion
	xmlns:ds="http://www.w3.org/2000/09/xmldsig#"
	xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion"
	xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol"
	xmlns:xs="http://www.w3.org/2001/XMLSchema"
	xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" 
        ID="Assertion-uuid50634b8a-0168-1c5e-b8fc-e5decbef3937" 
        IssueInstant="2019-01-15T07:21:51Z" 
        Version="2.0">

so 2 attributes more xmlns:samlp and xmlns:ds
these are taken from the top level as i suppose but they are now generating me this invalid digest
(as i could understand , the incoming digest is calculated without those 2 attributes)

I fixed this by patching my saml reponse deleting the attributes xmlns:samlp and xmlns:ds and adding xmlns:ds to the Signature attribute not not mess up with the reference function

shortly the change from

var c14nOptions = {
    ancestorNamespaces: ancestorNamespaces
  };

to

var c14nOptions = {
      inclusiveNamespacesPrefixList: ref.inclusiveNamespacesPrefixList,
      ancestorNamespaces: ref.ancestorNamespaces
    };

caused all the pain

is this what i did an acceptable solution, is there something that i'm missing. Of course i cannot change how our company generates the saml and calculates the digest

thanks

[LoneRifle]

Thanks for raising this. This is likely caused by #163, which would add attributes to the root node of a document for non-exclusive canonicalization in compliance with the SAML spec. This was shortly released as 1.0.2.

@huineng - could you try using 1.0.1 of xml-crypto and see if that works for you? I just want to confirm if #163 is indeed the cause. Can I also confirm that you are saying that changing lib/signed-xml.js from:

var c14nOptions = {
    ancestorNamespaces: ancestorNamespaces
  };

to:

var c14nOptions = {
      inclusiveNamespacesPrefixList: ref.inclusiveNamespacesPrefixList,
      ancestorNamespaces: ref.ancestorNamespaces
    };

would resolve the issue in the library?

[christiaanwesterbeek]

I looked into whether applying the proposed diff would help in my case.

https://github.com/yaronn/xml-crypto/blob/v1.1.1/lib/signed-xml.js#L379-L381

^^^ Here a ref object is introduced that isn't defined earlier.

https://github.com/yaronn/xml-crypto/blob/v1.1.1/lib/signed-xml.js#L460-L463

^^^ Here it seems that the desired end result is already so.

[LoneRifle]

Thanks. Given that 1.0.1 is known to work, I have a feeling that it is due to #163 and the way the changes in the canonicalization work with the main SignedXml class. The PR basically adds namespaces to the root node (ie, what huineng saw), in adherence to what @gcharnock understood of the spec.

Somehow the signature value being computed does not take the additions into account. It might have to do with the createSignature method, which uses a dummy signature wrapper to create the signature value rather than a clone of the signature element.

I hence propose that...

• we revert Bugfix: a namespace in the inclusive namespace list should be treated… #163 and release it as 1.1.2 so that people have non-exclusive canonicalization working again, in a fashion, then;
• reapply the PR with the extra change to createSignature described above, then testing to ensure that it works

[LoneRifle]

1.1.2 published, please test this

[christiaanwesterbeek]

I justed tested 1.1.2 and it solved my case.

[phof]

1.1.2 also solves the issue for me

[tunetheweb]

@huineng, @christiaanwesterbeek, @phof I'm proposing a similar change in #179 and want to ensure we don't reintroduce this issue that caused you pain.

Would any of you be able to test this change to confirm this? Basically only the signed-xml.js has changed from master so you should be able to download that (https://github.com/yaronn/xml-crypto/blob/679d8b666bd9d4b29476d156eeceafd2f4d2d9ef/lib/signed-xml.js) and drop it in to your copy to test.

Ideally we would have a test case to test for this, so if any of you can help provide that then happy to add that to help avoid any future issues.

[tunetheweb]

Actually I've refactored the code slightly so you need both these files now:

1. signed-xml.js: https://github.com/yaronn/xml-crypto/blob/08e4eff3d5a2e61c93d62f46faa80038bb6f23c2/lib/signed-xml.js
2. exclusive-canonicalization.js: https://github.com/yaronn/xml-crypto/blob/08e4eff3d5a2e61c93d62f46faa80038bb6f23c2/lib/exclusive-canonicalization.js

[christiaanwesterbeek]

@bazzadp My test ran just fine with the files signed-xml.js and exclusive-canonicalization.js you mentioned. I also see they're already part of release 1.3.0.

Maybe to conclude I ran my tests against all releases of xml-crypto. Here's my outcome.

v1.3.0 - OK
v1.2.0 - OK
v1.1.4 - OK
v1.1.3 - OK
v1.1.2 - OK
v1.1.1 - NOT OK
v1.1.0 - NOT OK
v1.0.2 - NOT OK
v1.0.1 - OK
v1.0.0 - OK

I suggest to close this issue.

[tunetheweb]

Thanks @christiaanwesterbeek for the comprehensive tests! That all sounds great.

@huineng if you're happy too can you close this as you originally raised this?

[LoneRifle]

I'll close this for now.. if this is still a problem we can just re-open the issue

[pks90]

@bazzadp @LoneRifle , I am getting this same issue , I checked that the xml-crypto version being used by the passport-saml in my case is ^1.1.4

[LoneRifle]

The issue has been fixed properly in later versions. Could you find a way to pick that up in your project please?