Upgrade to latest version of xml-crypto

[cjbarth]

The latest version of xml-crypto has many breaking changes and a few new patterns that we can leverage. This PR makes use of this newly updated library.

Note: a parameter that was optional is no longer so: signatureAlgorithm

[cjbarth]

@LoneRifle , I'd like to see what you have to say about this since you've been involved in xml-crypto and is-dom-node.

[codecov]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (e691ccf) 82.05% compared to head (a042f0c) 82.82%.
Report is 1 commits behind head on master.

Files	Patch %	Lines
src/xml.ts	90.90%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #341      +/-   ##
==========================================
+ Coverage   82.05%   82.82%   +0.77%     
==========================================
  Files          11       11              
  Lines         819      821       +2     
  Branches      253      250       -3     
==========================================
+ Hits          672      680       +8     
+ Misses         62       58       -4     
+ Partials       85       83       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[srd90]

I wasn't able to find any test case which would test a case where attacker re-signs message (after e.g. modifying it) with his/her own private key and places corresponding certificate into key info element of XML document to be consumed by SP.

I wanted to test this due to xml-crypto 4.x vulnerability which preferred by default and always certificate from untrusted source (i.e. source controlled by attacker). For more information about that from node-saml/xml-crypto#399 ( aka node-saml/xml-crypto#403 aka node-saml/xml-crypto#404 ... actual issue was described at node-saml/xml-crypto#399 ). xml-crypto side PR addressing node-saml/xml-crypto#399 was node-saml/xml-crypto#411 (PR was released at xml-crypto 5.0.0).

Following patch contains testcase which must fail due invalid document signature because SAML response is signed with https://raw.githubusercontent.com/node-saml/xml-crypto/ecbedd9e01a05e9340c18e217efbd1cd891b7f58/test/static/client.pem key (that key's certificate https://raw.githubusercontent.com/node-saml/xml-crypto/ecbedd9e01a05e9340c18e217efbd1cd891b7f58/test/static/client_public.pem is placed to keyinfo) node-saml stack is configured to use https://raw.githubusercontent.com/node-saml/node-saml/f35191dd532301a6b1802ba40396ee9d7fe203df/test/static/cert.pem as IdP's certificate.

Instead of failing due invalid document signature node-saml returns parsed SAML profile. I.e. instead of using IdP's certificate for signature verification it seems that this PR's current version uses certificate controlled by attacker.

diff --git a/test/static/signatures/invalid/response.root-resigned-by-attacker-assertion-unsigned-attackers-cert-at-keyinfo.xml b/test/static/signatures/invalid/response.root-resigned-by-attacker-assertion-unsigned-attackers-cert-at-keyinfo.xml
new file mode 100644
index 0000000..4bf3963
--- /dev/null
+++ b/test/static/signatures/invalid/response.root-resigned-by-attacker-assertion-unsigned-attackers-cert-at-keyinfo.xml
@@ -0,0 +1,57 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<samlp:Response xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol" xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" Destination="https://evil-corp.madness.com/sso/callback" ID="pfxea164cc1-96ac-af95-85e8-058c9d279cc5" InResponseTo="_e8df3fe5f04237d25670" IssueInstant="2020-09-25T16:00:00Z" Version="2.0">
+  <saml:Issuer>https://evil-corp.com</saml:Issuer>
+  <ds:Signature xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
+    <ds:SignedInfo>
+      <ds:CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
+      <ds:SignatureMethod Algorithm="http://www.w3.org/2000/09/xmldsig#rsa-sha1"/>
+      <ds:Reference URI="#pfxea164cc1-96ac-af95-85e8-058c9d279cc5">
+        <ds:Transforms>
+          <ds:Transform Algorithm="http://www.w3.org/2000/09/xmldsig#enveloped-signature"/>
+         <ds:Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
+       </ds:Transforms>
+       <ds:DigestMethod Algorithm="http://www.w3.org/2000/09/xmldsig#sha1"/>
+        <ds:DigestValue>lJCJZkryq2l8hf02Rw23nl1eGOo=</ds:DigestValue>
+      </ds:Reference>
+    </ds:SignedInfo>
+    <ds:SignatureValue>Y1zxmbC3oXtXws+ea7o3C+8Hx7EuBg+6nU+mWPqg6bUvuhhV6IzdNYBfW6QLZcQv
+dfKr4RyIfCqlk4GQEVL7iafj397leUzeAUXruAebaK8QXZidByDvuC+TrjLS+uZB
+K3kfuGMWBqCDx8gmP5H9WEanvi4x6MQ9wHNK3UFc8AE=</ds:SignatureValue>
+    <ds:KeyInfo>
+      <ds:X509Data>
+        <ds:X509Certificate>MIIBxDCCAW6gAwIBAgIQxUSXFzWJYYtOZnmmuOMKkjANBgkqhkiG9w0BAQQFADAW
+MRQwEgYDVQQDEwtSb290IEFnZW5jeTAeFw0wMzA3MDgxODQ3NTlaFw0zOTEyMzEy
+MzU5NTlaMB8xHTAbBgNVBAMTFFdTRTJRdWlja1N0YXJ0Q2xpZW50MIGfMA0GCSqG
+SIb3DQEBAQUAA4GNADCBiQKBgQC+L6aB9x928noY4+0QBsXnxkQE4quJl7c3PUPd
+Vu7k9A02hRG481XIfWhrDY5i7OEB7KGW7qFJotLLeMec/UkKUwCgv3VvJrs2nE9x
+O3SSWIdNzADukYh+Cxt+FUU6tUkDeqg7dqwivOXhuOTRyOI3HqbWTbumaLdc8juf
+z2LhaQIDAQABo0swSTBHBgNVHQEEQDA+gBAS5AktBh0dTwCNYSHcFmRjoRgwFjEU
+MBIGA1UEAxMLUm9vdCBBZ2VuY3mCEAY3bACqAGSKEc+41KpcNfQwDQYJKoZIhvcN
+AQEEBQADQQAfIbnMPVYkNNfX1tG1F+qfLhHwJdfDUZuPyRPucWF5qkh6sSdWVBY5
+sT/txBnVJGziyO8DPYdu2fPMER8ajJfl</ds:X509Certificate>
+      </ds:X509Data>
+    </ds:KeyInfo>
+  </ds:Signature>
+  <samlp:Status>
+    <samlp:StatusCode Value="urn:oasis:names:tc:SAML:2.0:status:Success"/>
+  </samlp:Status>
+  <saml:Assertion ID="_bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" IssueInstant="2020-09-25T16:00:00Z" Version="2.0">
+    <saml:Issuer>https://evil-corp.com</saml:Issuer>
+    <saml:Subject>
+      <saml:NameID Format="urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress">vincent.vega@evil-corp.com</saml:NameID>
+      <saml:SubjectConfirmation Method="urn:oasis:names:tc:SAML:2.0:cm:bearer">
+        <saml:SubjectConfirmationData InResponseTo="_e8df3fe5f04237d25670" NotOnOrAfter="2020-09-25T17:00:00Z" Recipient="https://evil-corp.madness.com/sso/callback"/>
+      </saml:SubjectConfirmation>
+    </saml:Subject>
+    <saml:Conditions NotBefore="2020-09-25T16:00:00Z" NotOnOrAfter="2020-09-25T17:00:00Z">
+      <saml:AudienceRestriction>
+        <saml:Audience>audience</saml:Audience>
+      </saml:AudienceRestriction>
+    </saml:Conditions>
+    <saml:AuthnStatement AuthnInstant="2020-09-25T16:00:00Z" SessionIndex="_9e315bdf7b1b6732be33c377cf6f5c4f">
+      <saml:AuthnContext>
+        <saml:AuthnContextClassRef>urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport</saml:AuthnContextClassRef>
+      </saml:AuthnContext>
+    </saml:AuthnStatement>
+  </saml:Assertion>
+</samlp:Response>
diff --git a/test/test-signatures.spec.ts b/test/test-signatures.spec.ts
index f8f1af1..d5b0540 100644
--- a/test/test-signatures.spec.ts
+++ b/test/test-signatures.spec.ts
@@ -229,6 +229,40 @@ describe("Signatures", function () {
         },
       ),
     );
+    // following test is added to test that if saml response contains
+    // key info field with certificate that it is NOT used blindly for
+    // signature validation. I.e. if attacker resigns response (after
+    // altering content) using his/her own private key and places corresponding
+    // certificate to key info that particular certificate MUST NOT be used
+    // for signature validation. Instead IdP's certificate fetched via
+    // trusted side channel and configured as 'cert' parameter must be
+    // only source for response signature validation
+    // This test was triggered due to xml-crypto vulnerability at
+    // xml-crypto 4.x release which preferred by default attacker controlled
+    // certificate from input XML document's keyinfo element.
+    // see https://github.com/node-saml/xml-crypto/discussions/399
+    // It was supposed to be fixed at xml-crypto 5.x
+    // I.e. purpose was to validate that @node-saml/node-saml which
+    // jumped from xml-crypto 3.x to 5.x shall behave correctly in casse
+    // of re-sign case.
+    // for the record test material saml response was signed with
+    // https://raw.githubusercontent.com/node-saml/xml-crypto/ecbedd9e01a05e9340c18e217efbd1cd891b7f58/test/static/client.pem
+    // and corresponding certificate is
+    // https://raw.githubusercontent.com/node-saml/xml-crypto/ecbedd9e01a05e9340c18e217efbd1cd891b7f58/test/static/client_public.pem
+    // the only cetificate which should be used for validation is the one configured
+    // at the beginning of this testcases file (meaning /static/cert.pem
+    // https://raw.githubusercontent.com/node-saml/node-saml/f35191dd532301a6b1802ba40396ee9d7fe203df/test/static/cert.pem )
+    it(
+      "R1A - root re-signed by attackers own private key and attacker's certificate placed to keyinfo",
+      testOneResponse(
+        "/invalid/response.root-resigned-by-attacker-assertion-unsigned-attackers-cert-at-keyinfo.xml",
+        INVALID_DOCUMENT_SIGNATURE,
+        1,
+        {
+          wantAssertionsSigned: false,
+        },
+      ),
+    );
   });
 
   describe("Signatures on saml:Response - 1 saml:Assertion + 1 saml:Advice containing 1 saml:Assertion", () => {

Aforementioned patch was written on top of version eaaae9f of this PR branch.

FWIW, test/static/signatures/invalid/response.root-resigned-by-attacker-assertion-unsigned-attackers-cert-at-keyinfo.xml was validated with https://www.samltool.com/validate_response.php (with ignore timing issues). samltool reported invalid signature when it was configured to use "test IdP's" certificate. I.e. samltool did not pick certificate from saml response.

[srd90]

When testcase provided at patch at #341 (comment) was introduced to (on top of) @node-saml/node-saml 4.0.5 it - testcase - reported success. I.e. @node-saml/node-saml 4.0.5 rejected SAML response due invalid document signature. @node-saml/node-saml 4.0.5 uses xml-crypto ^3.0.1.

So...root cause for issue described at #341 (comment) seems/might be xml-crypto >= 4.0.0's "dangerous/confusing" new API related to signature verification (especially to source of certificate to be used to verification) at least in case of migrations from < 4.0.0 versions of xml-crypto. I did not have time to dig this thoroughly I just assume that possible migration threats described at node-saml/xml-crypto#399 materialized in this PR (another possiblity is that xml-crypto 5.x contains some new bug).

[cjbarth]

@srd90 , I've replicated your findings and appreciate your comment. I guess, in harmony with the comments in #399, your concern here is that, while we "fixed" this issue in xml-crypto@5 by making sure there is no KeyInfo checking by default, we undid that here by setting a default. Does that sound correct?

[cjbarth]

@srd90 After more closely reviewing the code and your comments, it does seem like, per the SAML spec, which uses out-of-band key exchange as part of the metadata, we shouldn't be leveraging the <KeyInfo /> element. So, while appropriate for xml-crypto, it should be turned off for node-saml.

I'd like to see what @LoneRifle and @markstos have to say about this before I land this.

[srd90]

...while we "fixed" this issue in xml-crypto@5 by making sure there is no KeyInfo checking by default, we undid that here by setting a default. Does that sound correct?

@cjbarth I just noticed that your fix at b502c96#diff-7d35df84f844a55e65c60e12cd2ec98f375574df4a5075bf7d2e45af25fa616a was to set getCertFromKeyInfo to null which raised my eye browses because I thought that it was already set to function which return null by default. It turned out that xml-crypto side PR which was supposed to address underlying "trust by default to attacker's cert and signature" issue might have set incorrect function to return null see additional comments from node-saml/xml-crypto#411 (comment)

[cjbarth]

@srd90 , @LoneRifle , I've incorporated all your suggestions and am ready to land this. @markstos , did you want to give this a second set of eyes?

[markstos]

This looks good to me. Thanks for working on this!

I presume there will be a major version bump due to making an optional argument required. Some folks may be prefer we default to a newer algorithm instead, but I think there's also some value in requiring this to be explicit.

The commit history could a bit cleaner by merging the wipcommit with another and merging a couple of the node compat commits as well.

[cjbarth]

@markstos , thanks for your review. I'll do a squash merge anyway, so the commits don't matter. And, yes, this is in preparation for a (long overdue) major version bump.

[markstos]

I found more of @cjbarth rationale in the xml-crypto bug tracker:

... Where we can't choose a default that is secure and works for everyone, we choose no default and make things throw if the consumer doesn't make a decision.

Makes sense to me and is aligned with the general approach here of trying to be "secure by default".