Bypass Vulnerability if entryPoint misconfigured

[cintolas]

We received this report when our product went through pen testing

The empty default configuration of the SAML authentication workflow would break due to the lack of an entry point URL, causing an infinite redirect loop. However, a valid SAML Request was still generated with the default configuration values.

The SAMLRequest parameter shown above was generated by the product and was intended to be sent to an SAML endpoint. Since the authentication method is not fully configured with a certificate, any SAML SSO service can respond to the request. Atredis set up a simple SSO service to validate this finding. The SSO service handled the request and posted a valid SAML response to the F callback URL, which was considered a successful login attempt.

The following snippet shows that a saml request is still generated.

var express = require ('express');
var passport = require ('passport')
var SamlStrategy = require('passport-saml').Strategy;

passport.use(new SamlStrategy({
    path:'/',
    entryPoint:''	
},
function verify (req,profile, done) {
    return done(null,profile);
}));

var app = express ();

app.route ('/')
.get (passport.authenticate ('saml'))
.post (passport.authenticate('saml', function(err,user,info){ 
    console.log (err, user, info);
}));

app.route ('/callback', passport.authenticate('saml', function(err,user,info){
    console.log (err, user, info);
}));
app.listen ({
    port: 8000
});

[markstos]

@cintolas Thanks for the report.

What we call entryPoint is used for the Destination attribute in AuthnRequest. According to the SAML spec, Destination is required to be spec-compliant when the request is signed:

https://support.pingidentity.com/Destination-attribute-in-SAML-Request

So if we are failing to require the Destination attribute when the spec requires it, that's a bug.

On the other hand, if your case involves un-signed requests with a missing Destination attribute, it appears that' a weakness in the SAML spec itself, this project is compliant, developers need to understand to avoid that case to be safe.

Help with updated code or docs to deal with this case in a spec-compliant way is welcome.

[markstos]

Addressed by #299. Closing.