With both response and assertion are signed, we fail to catch that one of the two signatures is invalid.

[BerserkerGaruk]

I am testing passport saml using some of the lower level functionality in order to verify saml tokens. I've run into an issue with okta saml tokens when both the saml response, and saml assertion are signed and the saml assertion is encrypted.

When I pass the saml token to validatePostResponse with private decryption key cert(decryptionPvk) (cert) I get a correct response, but If I modify the signature of the saml response header (either id or the digest value), which should cause the saml response to fail the library returns the decoded saml token without error.

Stepping through the code showed some additional information When validating the root level signature here: https://github.com/bergie/passport-saml/blob/2de3528f308f2103625b191c5b32432636f1592e/lib/passport-saml/saml.js#L575-L577

the first signature it finds before decrypting the assertion returns that the signature is not valid. At this point it will continue to decrypt the assertion and check the assertion's signature at 616: https://github.com/bergie/passport-saml/blob/2de3528f308f2103625b191c5b32432636f1592e/lib/passport-saml/saml.js#L612-L617

If this is able to successfully check the signature then the entire token will be accepted even though the signature failed on the response. The same will be true in the reverse if it was able to decrypt the root signature but not assertion signature.

I have also tested this section of the code without an encrypted assertion with a invalidly signed response and assertion and the code will fail due to the below. https://github.com/bergie/passport-saml/blob/2de3528f308f2103625b191c5b32432636f1592e/lib/passport-saml/saml.js#L497

In this case validateSignature() is able to see both signature at this point with signature.length is equal to 2 and will not check the either of these signatures. When the code later checks the assertion it will call validateSignature() with only the assertion xml element and it will see that there is a valid assertion signature and succeed.

Even though only one of the saml signatures is required it is possible to have a signature on both the response and assertion (okta by default does both) and in this case this library will fail at catching that one of the 2 signatures are invalid.

[markstos]

It would be helpful if you referenced the section of the SAML specification details how signature verification should work thin this case. If it appears we are not spec-compliant here, feel free to submit a pull request to improve that. Thanks.

[markstos]

Is this the same as #168 ?

[markstos]

@cjbarth I think this one may have a security implication. Would you be willing to help with it? #168 appears to the same issue, explained a different way.

[cjbarth]

I'll see if I can buy out some time to look into this. It would be nice to have a spec saying what should happen. Even better is if @BerserkerGaruk or @timotm has a test case that fails that I can work against.

[BerserkerGaruk]

@cjbarth This was the function that I was using for verification purposes. I was testing this through AWS lambda combined with API Gateway so I wasn't using the entire passport framework (some of my code might also be off due to that) and was instead using a lower level method of the library called validatePostResponse, which when I was originally testing was called to verify the SAML token that was received. As I haven't touched anything with this library in almost 4-5 months what I have below might not work exactly, but I hope it's able to help.

I was testing using an issued OKTA token with an encrypted assertion. I would have to reissue this token during testing as it expired after a certain amount of time. As such I'm not able to provide a token that works with this code, but it should be sufficient to take a token that is issued in a similar way (with a signature on both the assertion and top level message) to make sure that it correctly verifies and then to decode it mess with the signature in someway(could just delete a couple characters as that would cause the signature to become invalid).

I was using some of the tools provided by https://www.samltool.com/online_tools.php to do this and verify that another tool would catch that one of the signatures was incorrect which this did. Hope this helps in someway.

`

var SAML = require('passport-saml').SAML;
var fs = require('fs');
const path = require('path')

function samlDecoderPassport(event,context,callback){
    var samlResponse = event.data.SAMLResponse
    var relayState = event.data.RelayState

    var saml_options = {
        host: "localhost",
        issuer: "issuer.org",//entity id
        identifierFormat:"urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress",
        authnContext:"urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport",
        acceptedClockSkewMs:-1,
        validateInResponseTo:"endpoint_recieved_at",
        requestIdExpirationPeriodMs: 28800000,  // 8 hours
        cert:fs.readFileSync(path.join(__dirname, './certs')+"/derp_okta.crt").toString(),
        // logouturl:"",
        signatureAlgorithm:'sha256', //sha1
        decryptionPvk:fs.readFileSync(path.join(__dirname, './certs')+"/derp_custom.pem").toString(),
        // protocol:"",
        // callbackUrl:"",
        // path:"",
        privateCert:fs.readFileSync(path.join(__dirname, './certs')+"/derp_custom.crt").toString(),
        // forceAuthn:"",
        // entryPoint:"",
        disableRequestedAuthnContext:"",
        attributeConsumingServiceIndex:"",
        // providerName:"",
        // additionalParams:{},
        audience:"audience.org",
        // AudienceRestriction:""
    };

    var saml = new SAML(saml_options);

    fullXml = samlResponse;
    // saml.validateSignature(fullXml,currentNode,certs);
    // saml.validateSignatureForCert(signature, cert, fullXml, currentNode);
    // var SamlStrategy = require('passport-saml').Strategy;
    //

    //callback(null);
    /*
    SAML.prototype.validatePostReponse
     */
    var check_validation = function(member1,member2,member3,member4){
        /*
        member1:Is error if there is an error
        member2:Is the decoded value if it decodes
        member3:is true or false for some reason?
         This check the validity of the saml2 decoder
         */
        console.log(JSON.stringify(member1));
    };

    var container = {
        SAMLResponse:samlResponse
    };

    saml.validatePostResponse(container,check_validation);

    var container = {
        SAMLResponse:samlResponse
    };
    // var xmlresponse = new Buffer(container.SAMLResponse, 'base64').toString('utf8');
    // var doc = new xmldom.DOMParser({
    // }).parseFromString(xmlresponse);
    // // var dom.documentElement;
    // // var xmlresponse =xml
    // var returnValue = saml.validateSignature(xmlresponse,"",saml.certsToCheck());
    //console.log(returnValue);
}

if (require.main === module) {
    console.log('called directly');
    var context = "local";
    var xmlSaml =fs.readFileSync(path.join(__dirname, './')+"/test_saml.xml").toString('base64');
    //This is an example event that is sent through
    event= {
        data:{
            SAMLResponse:xmlSaml,
            relayState:"/"
        }
    };
    samlDecoderPassport(event,context,function(state){
        console.log(state)
    });
}

`

edit: formatting, code error

[markstos]

Thanks for the follow-up. We would like to more formally support using this library outside of Passport. A number of people are using it that way.

[ghost]

@markstos Can you tell me how is this security vulnerability, as suppose response signature is invalid, but assertion signature is valid, the main data is anyway in assertion, so how is it impacting security?

[mcwhittemore]

The issuer is duplicated between the assertion and the response so if an SP is using the issuer from the response to do some action they can be using a value that was not provided by the IDP. Any other field that is in both places could have this problem as well.

[srd90]

There is an upcoming feature to passport-saml 4.x which allows developer to configure passport-saml to fail hard if top level signature is not valid. I.e. if developer knows that his/her IdP signs whole message or signs whole message and assertion it shall be possible to throw exception if top level signature didn't match instead of proceeding to see whether there is atleast validly signed assertion.

Discussion of < 4.0 version's behaviour: https://github.com/node-saml/passport-saml/discussions/671#discussioncomment-2136937

Related issue: https://github.com/node-saml/node-saml/issues/58 node-saml/node-saml because internals of node-saml/passport-saml has been moved to separate module which shall be introduced as dependency to upcoming passport-saml 4.x

Related pull request: https://github.com/node-saml/node-saml/pull/83

[cjbarth]

passport-saml@4 has been released.