Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add optional setting to set a ceiling on how old a SAML response is allowed to be #577

Merged
merged 10 commits into from Apr 28, 2021

Conversation

nickiepucel
Copy link
Contributor

@nickiepucel nickiepucel commented Mar 26, 2021

Summary

For security reasons, we want to ensure that there is an upper limit as to how long
a SAML response is valid for even if the IdP is configured to have extremely long (or indefinite) validity
times.

Normally an IdP would be configured to make the SAML documents short-lived, but
since in our case we run the SP and don't control the IdP, we would still like some minimal guarantees that
leaked or logged SAML documents are not dangerous after some fixed amount of time.

This change was added to the Asana fork in 2015; this PR emulates https://github.com/Asana/node-saml-lib/pull/15.

Checklist:

  • Issue Addressed: N/A
  • Link to SAML spec: N/A
  • Tests included? [X]
  • Documentation updated? [X]

@nickiepucel nickiepucel force-pushed the nickie/add-max-assertion-age-ms branch from da34986 to a8ed2ba Compare March 26, 2021 16:26
@nickiepucel nickiepucel force-pushed the nickie/add-max-assertion-age-ms branch from a8ed2ba to 352e059 Compare March 26, 2021 16:29
Copy link
Contributor

@markstos markstos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a nice security feature to me, reasonable implemented.

The SAML spec does not appear to be involved here. Rather, this code allows the service provider to decide for itself how old of an response it will accept.

Adding this this feature would allow the window for replay attacks to be narrowed, improving security.

@nickiepucel nickiepucel force-pushed the nickie/add-max-assertion-age-ms branch from c3c433d to 6405dc8 Compare March 26, 2021 21:15
@cjbarth
Copy link
Collaborator

cjbarth commented Mar 26, 2021

@nickiepucel Thank you for submitting this. We are trying to get things ready for a 3.x release in a few weeks and we'd like to get this code included in that. However, we need to wait for #574 to land first, which might cause some merge conflicts for you. Thank you for your understanding.

@cjbarth cjbarth added the semver-minor This PR requires a semver-minor version bump label Mar 27, 2021
@cjbarth cjbarth added this to the 3.0.0 milestone Apr 4, 2021
@cjbarth
Copy link
Collaborator

cjbarth commented Apr 5, 2021

@nickiepucel We're ready for this to land. If you'd like to resolve the merge conflicts, we'll get this in for the 3.x release.

@nickiepucel
Copy link
Contributor Author

great! i'll try to get that done today

@nickiepucel
Copy link
Contributor Author

@cjbarth all set!

Copy link
Collaborator

@cjbarth cjbarth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good; very clean implementation. I'm concerned about possibly parsing into NaN. I see that we don't have tests for this condition, but I believe it is possible. If I'm missing something, please let me know.

src/node-saml/types.ts Show resolved Hide resolved
src/node-saml/saml.ts Outdated Show resolved Hide resolved
src/node-saml/saml.ts Show resolved Hide resolved
@cjbarth cjbarth requested a review from markstos April 9, 2021 19:38
src/node-saml/saml.ts Outdated Show resolved Hide resolved
@nickiepucel
Copy link
Contributor Author

I feel like there should be some checks for NaN here (along with a test or two).

@cjbarth I took a stab at addressing the NaN issues, but unfortunately I haven't been able to get tests to pass and I'm not sure why.

  • Looks like the new tests I added are returning Invalid signuature errors instead of my new error; I'm not sure why
  • I thought I caught some malformed dates in some of the response stubs (see this commit), but after fixing them the related tests are also seeing Invalid signature errors.
    • Before making that change, my new error was being hit when parsing this malformed date (see this test run)

Do you have any ideas?
Also, do you know how I can run these tests locally? That would make it much easier for me to debug.

@cjbarth
Copy link
Collaborator

cjbarth commented Apr 10, 2021

I'll have a look later, but to unblock you, you can test locally with npm run test. You may have to npm install first.

@nickiepucel
Copy link
Contributor Author

Ah I think I figured it out.

I believe that because I changed the SAMLResponses' XML (by changing the NotBefore/NotOnOrAfter values), the signature is now invalid, since the signature is based on the old values for the dates.

Does that sound plausible? If so, do you know how how I can regenerate signatures for those SAMLResponses?

@cjbarth
Copy link
Collaborator

cjbarth commented Apr 10, 2021

That not only sounds plausible, that is entirely correct. The XML is cryptographically signed, and we check for that, so it is good that it failed :) You can use https://www.samltool.com/online_tools.php to re-sign XML.

@nickiepucel
Copy link
Contributor Author

The tool is requiring that I pass in a Private Key to sign the response, but it's not clear to me in looking at the tests what I should use for this value. Any ideas?

image

@vandernorth
Copy link
Contributor

The tool is requiring that I pass in a Private Key to sign the response, but it's not clear to me in looking at the tests what I should use for this value. Any ideas?

If I remember correctly that should be key.pem in test/static.

@nickiepucel
Copy link
Contributor Author

nickiepucel commented Apr 12, 2021

If I remember correctly that should be key.pem in test/static.

Sadly, using key.pem is not working for me 😕 I'm still getting an Invalid signature error in my tests.

any other ideas?

image

@cjbarth
Copy link
Collaborator

cjbarth commented Apr 12, 2021

All you have to do is get any key and cert that match and use that. You're modifying the test anyway, so you can just change which cert you pass. See test.spec.ts:814 as an example.

@nickiepucel
Copy link
Contributor Author

nickiepucel commented Apr 12, 2021

How can I sign the Assertions present within a <saml:Advice>?

I tried only pasting the Assertion in the samltool, but that didn't work. It looks like the SAML tool only supports signing the Response and the top-level Assertion.

@vandernorth
Copy link
Contributor

This is the code I used to create all the different test scenario's. Hope that helps, let me know if you need more info. The key.pem and cert.pem are in the location I mentioned earlier.

const _      = require('lodash'),
      fs     = require('fs'),
      select = require('xml-crypto').xpath,
      Dom    = require('xmldom').DOMParser,
      xCrypt = require("xml-crypto");

class Sign {

    constructor( xmlPath, outPath, signRefs = [], verifyOnly = false ) {

        this.key  = fs.readFileSync('./key.pem').toString('utf-8');
        this.cert = fs.readFileSync('./cert.pem').toString('utf-8');

        if ( verifyOnly ) {
            this.rawXml = fs.readFileSync(xmlPath).toString('utf-8');
            this.verify(this.rawXml);
            return;
        }

        this.rawXml = fs.readFileSync(xmlPath).toString('utf-8');
        this.xml    = new Dom().parseFromString(this.rawXml);
        const o     = {
            reference: ''
        };

        this.signed = this.rawXml;

        signRefs.forEach(ref => {
            o.reference = ref;
            this.signed = this.signXml(this.signed, o.reference, o);
        });

        //== Done
        console.info('Writing', outPath);
        fs.writeFileSync(outPath, this.signed.toString());
        this.verify(this.signed);
    }

    signXml( xmlToSign, pathToSign = "//*[local-name(.)='EntityDescriptor']", location = {}, prefix = 'ds' ) {
        try {
            const signedXml = new xCrypt.SignedXml();
            signedXml.addReference(
                pathToSign,
                ['http://www.w3.org/2000/09/xmldsig#enveloped-signature', 'http://www.w3.org/2001/10/xml-exc-c14n#'],
                'http://www.w3.org/2001/04/xmlenc#sha256',
                null, null, '', false);
            signedXml.signingKey                = this.key;
            signedXml.signatureAlgorithm        = "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256";
            signedXml.canonicalizationAlgorithm = "http://www.w3.org/2001/10/xml-exc-c14n#";
            signedXml.computeSignature(xmlToSign, {
                prefix:   prefix,
                location: location
            });
            return signedXml.getSignedXml();
        }
        catch ( ex ) {
            console.error('Unable to sign XML', ex);
            return xmlToSign;
        }
    }

    verify( xml ) {
        const doc = new Dom().parseFromString(xml);
        select(doc, "//*[local-name(.)='Signature']").forEach(signature => {
            const URI        = select(signature, ".//*[local-name(.)='Reference']")[0].getAttribute('URI');
            const parent     = select(signature, "./parent::*")[0];
            const findOnId   = `.//*[@ID='${URI.replace('#', '')}']`;
            const onID       = select(doc, findOnId)[0];
            const validation = this.validateXmlSignature(signature, parent, xml);
            console.info('  - CHECK SIGNATURE', URI, onID.getAttribute('ID'), ' =>', validation.result, validation.errors.length, validation.errors[0]);
        });
        console.info('Verify done');
    }

    validateXmlSignature( signature, xmlElement, fullXML ) {
        const SignedXml     = require('xml-crypto').SignedXml,
              FileKeyInfo   = require('xml-crypto').FileKeyInfo,
              sig           = new SignedXml();
        sig.keyInfoProvider = new FileKeyInfo('./cert.pem');
        sig.loadSignature(signature);

        const res = sig.checkSignature(fullXML);

        return {
            result: res,
            errors: sig.validationErrors || []
        };
    }
}

const Advice    = "//*[local-name(.)='Response']/*[local-name(.)='Assertion']/*[local-name(.)='Advice']/*[local-name(.)='Assertion']";
const Assertion = "//*[local-name(.)='Response']/*[local-name(.)='Assertion']";
const Response  = "//*[local-name(.)='Response']";

new Sign('./signme.xml', './signed.xml', [Advice, Assertion, Response]);
new Sign('./response_root_1_assertion_1.xml', './response.root-unsigned.assertion-unsigned.xml', []);
new Sign('./response_root_1_assertion_1.xml', './response.root-signed.assertion-signed.xml', [Assertion, Response]);
new Sign('./response_root_1_assertion_1.xml', './response.root-unsigned.assertion-signed.xml', [Assertion]);
new Sign('./response_root_1_assertion_1.xml', './response.root-signed.assertion-unsigned.xml', [ Response]);

new Sign('./response_root_1_assertion_1_advice_1.xml', './response.root-signed.assertion-signed.1advice-signed.xml', [Advice, Assertion, Response]);
new Sign('./response_root_1_assertion_1_advice_1.xml', './response.root-signed.assertion-signed.1advice-unsigned.xml', [Assertion, Response]);
new Sign('./response_root_1_assertion_1_advice_1.xml', './response.root-signed.assertion-unsigned.1advice-unsigned.xml', [Response]);
new Sign('./response_root_1_assertion_1_advice_1.xml', './response.root-unsigned.assertion-unsigned.1advice-unsigned.xml', []);
new Sign('./response_root_1_assertion_1_advice_1.xml', './response.root-unsigned.assertion-signed.1advice-unsigned.xml', [Assertion]);
new Sign('./response_root_1_assertion_1_advice_1.xml', './response.root-unsigned.assertion-signed.1advice-signed.xml', [Advice, Assertion]);


new Sign('./response_root_1_assertion_1_advice_2.xml', './response.root-signed.assertion-signed.2advice-signed.xml', [Advice, Assertion, Response]);
new Sign('./response_root_1_assertion_1_advice_2.xml', './response.root-signed.assertion-signed.2advice-unsigned.xml', [Assertion, Response]);
new Sign('./response_root_1_assertion_1_advice_2.xml', './response.root-signed.assertion-unsigned.2advice-unsigned.xml', [Response]);
new Sign('./response_root_1_assertion_1_advice_2.xml', './response.root-unsigned.assertion-unsigned.2advice-unsigned.xml', []);

@cjbarth
Copy link
Collaborator

cjbarth commented Apr 13, 2021

I tried only pasting the Assertion in the samltool, but that didn't work. It looks like the SAML tool only supports signing the Response and the top-level Assertion.

There is a "Mode" setting on that page that specifies "Assertion". Does that work?

@nickiepucel
Copy link
Contributor Author

There is a "Mode" setting on that page that specifies "Assertion". Does that work?

Unfortunately, this does not work for signing the Assertions within an Advice, as I mentioned here.

This is the code I used to create all the different test scenario's.

TYSM @vandernorth , this worked!

All the tests are fixed now @cjbarth :)

Copy link
Collaborator

@cjbarth cjbarth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this; this is some very nice work. Thank you for contributing to this project. I really didn't get a chance to go over all the changed XML file carefully. I think I might pretty-print them and then check them over with a careful eye to match the name to the contents. I assume you already did that... but that is the concept of code review :)

src/node-saml/saml.ts Show resolved Hide resolved
test/node-saml/test-signatures.spec.ts Show resolved Hide resolved
test/node-saml/tests.spec.ts Show resolved Hide resolved
@nickiepucel
Copy link
Contributor Author

@cjbarth just to clarify, there isn't any further action needed from me on this PR as of now?

@cjbarth
Copy link
Collaborator

cjbarth commented Apr 14, 2021

@nickiepucel You are correct. I need to buy out some time to give this a little more thurough review, but it looks good the way it is and I don't expect I'll find anything else. I might push another commit or two to pretty some XML files. I'll keep you posted.

@cjbarth cjbarth merged commit 54a1e04 into node-saml:master Apr 28, 2021
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor This PR requires a semver-minor version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants