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

[nickiepucel]

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]

[markstos]

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.

[cjbarth]

@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]

@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]

great! i'll try to get that done today

[nickiepucel]

@cjbarth all set!

[cjbarth]

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.

[nickiepucel]

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]

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]

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]

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]

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?

[vandernorth]

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]

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?

[cjbarth]

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]

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]

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]

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]

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 :)

[cjbarth]

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 :)

[nickiepucel]

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

[cjbarth]

@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.