Missing cert configuration option disables signature validation silenty/without huge warning messages

[srd90]

There is/was issue #180 which addressed this particular case.

There is no need to repeat reasons why passport-saml MUST have safe defaults for signature validation configuration ( read @pitbulk 's and @alexstuart 's comments from #180 ). In fact one should not be able disable it at any circumstances.

It seems that there was some sort of consensus at the end of #180 that

Certificate must be required and validation for assertions must fail if signature is missing.

but content of commit ( f6b1c88 ) which closed issue #180 allows completely missing or misspelled cert configuration option which is effectively same thing as cert with falsy value (signature validation is completely disabled allowing anyone to alter login response any way he/she chooses to).

---

Following examples are executed against passport-saml version https://github.com/node-saml/passport-saml/tree/932da9d09a018fed4cb830e67090bb994f8539c1 ( v2.0.4 ) using this particular test case (with various modifications) to demonstrate effects of missing cert configuration option:

passport-saml/test/tests.js

Lines 1623 to 1643 in 932da9d

	var samlConfig = {
	entryPoint: 'https://app.onelogin.com/trust/saml2/http-post/sso/371755',
	cert: TEST_CERT,
	};
	it( 'valid onelogin xml document should validate', function( done ) {
	var xml = '<samlp:Response xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol" ID="R689b0733bccca22a137e3654830312332940b1be" Version="2.0" IssueInstant="2014-05-28T00:16:08Z" Destination="{recipient}" InResponseTo="_a6fc46be84e1e3cf3c50"><saml:Issuer>https://app.onelogin.com/saml/metadata/371755</saml:Issuer><samlp:Status><samlp:StatusCode Value="urn:oasis:names:tc:SAML:2.0:status:Success"/></samlp:Status>' +
	'<saml:Assertion xmlns:xs="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" Version="2.0" ID="pfx3b63c7be-fe86-62fd-8cb5-16ab6273efaa" IssueInstant="2014-05-28T00:16:08Z"><saml:Issuer>https://app.onelogin.com/saml/metadata/371755</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="#pfx3b63c7be-fe86-62fd-8cb5-16ab6273efaa"><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>DCnPTQYBb1hKspbe6fg1U3q8xn4=</ds:DigestValue></ds:Reference></ds:SignedInfo><ds:SignatureValue>e0+aFomA0+JAY0f9tKqzIuqIVSSw7LiFUsneEDKPBWdiTz1sMdgr/2y1e9+rjaS2mRmCi/vSQLY3zTYz0hp6nJNU19+TWoXo9kHQyWT4KkeQL4Xs/gZ/AoKC20iHVKtpPps0IQ0Ml/qRoouSitt6Sf/WDz2LV/pWcH2hx5tv3xSw36hK2NQc7qw7r1mEXnvcjXReYo8rrVf7XHGGxNoRIEICUIi110uvsWemSXf0Z0dyb0FVYOWuSsQMDlzNpheADBifFO4UTfSEhFZvn8kVCGZUIwrbOhZ2d/+YEtgyuTg+qtslgfy4dwd4TvEcfuRzQTazeefprSFyiQckAXOjcw==</ds:SignatureValue><ds:KeyInfo><ds:X509Data><ds:X509Certificate>'+TEST_CERT+'</ds:X509Certificate></ds:X509Data></ds:KeyInfo></ds:Signature><saml:Subject><saml:NameID Format="urn:oasis:names:tc:SAML:2.0:nameid-format:transient">ploer@subspacesw.com</saml:NameID><saml:SubjectConfirmation Method="urn:oasis:names:tc:SAML:2.0:cm:bearer"><saml:SubjectConfirmationData NotOnOrAfter="2014-05-28T00:19:08Z" Recipient="{recipient}" InResponseTo="_a6fc46be84e1e3cf3c50"/></saml:SubjectConfirmation></saml:Subject><saml:Conditions NotBefore="2014-05-28T00:13:08Z" NotOnOrAfter="2014-05-28T00:19:08Z"><saml:AudienceRestriction><saml:Audience>{audience}</saml:Audience></saml:AudienceRestriction></saml:Conditions><saml:AuthnStatement AuthnInstant="2014-05-28T00:16:07Z" SessionNotOnOrAfter="2014-05-29T00:16:08Z" SessionIndex="_30a4af50-c82b-0131-f8b5-782bcb56fcaa"><saml:AuthnContext><saml:AuthnContextClassRef>urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport</saml:AuthnContextClassRef></saml:AuthnContext></saml:AuthnStatement></saml:Assertion>' +
	'</samlp:Response>';
	var base64xml = Buffer.from( xml ).toString('base64');
	var container = { SAMLResponse: base64xml };
	var samlObj = new SAML( samlConfig );
	samlObj.validatePostResponse( container, function( err, profile, logout ) {
	try {
	should.not.exist( err );
	profile.nameID.should.startWith( 'ploer' );
	done();
	} catch (err2) {
	done(err2);
	}
	});
	});

---

Reference run (version 932da9d without any modifications):

passport-saml$ npm run tsc && node_modules/mocha/bin/mocha -g 'valid onelogin xml document should validate'

  passport-saml /
    saml.js / 
      validatePostResponse checks /
        validatePostResponse xml signature checks /
          ✓ valid onelogin xml document should validate (81ms)

  1 passing (90ms)

---

Reference run 2 ( 932da9d with altered response):

Value of response's nameID is altered to aaaaaaaa@bbbbb.local

diff --git a/test/tests.js b/test/tests.js
index 045db3c..5f7da83 100644
--- a/test/tests.js
+++ b/test/tests.js
@@ -1626,7 +1626,7 @@ describe( 'passport-saml /', function() {
         };
         it( 'valid onelogin xml document should validate', function( done ) {
           var xml = '<samlp:Response xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol" ID="R689b0733bccca22a137e3654830312332940b1be" Version="2.0" IssueInstant="2014-05-28T00:16:08Z" Destination="{recipient}" InResponseTo="_a6fc46be84e1e3cf3c50"><saml:Issuer>https://app.onelogin.com/saml/metadata/371755</saml:Issuer><samlp:Status><samlp:StatusCode Value="urn:oasis:names:tc:SAML:2.0:status:Success"/></samlp:Status>' +
-            '<saml:Assertion xmlns:xs="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" Version="2.0" ID="pfx3b63c7be-fe86-62fd-8cb5-16ab6273efaa" IssueInstant="2014-05-28T00:16:08Z"><saml:Issuer>https://app.onelogin.com/saml/metadata/371755</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="#pfx3b63c7be-fe86-62fd-8cb5-16ab6273efaa"><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>DCnPTQYBb1hKspbe6fg1U3q8xn4=</ds:DigestValue></ds:Reference></ds:SignedInfo><ds:SignatureValue>e0+aFomA0+JAY0f9tKqzIuqIVSSw7LiFUsneEDKPBWdiTz1sMdgr/2y1e9+rjaS2mRmCi/vSQLY3zTYz0hp6nJNU19+TWoXo9kHQyWT4KkeQL4Xs/gZ/AoKC20iHVKtpPps0IQ0Ml/qRoouSitt6Sf/WDz2LV/pWcH2hx5tv3xSw36hK2NQc7qw7r1mEXnvcjXReYo8rrVf7XHGGxNoRIEICUIi110uvsWemSXf0Z0dyb0FVYOWuSsQMDlzNpheADBifFO4UTfSEhFZvn8kVCGZUIwrbOhZ2d/+YEtgyuTg+qtslgfy4dwd4TvEcfuRzQTazeefprSFyiQckAXOjcw==</ds:SignatureValue><ds:KeyInfo><ds:X509Data><ds:X509Certificate>'+TEST_CERT+'</ds:X509Certificate></ds:X509Data></ds:KeyInfo></ds:Signature><saml:Subject><saml:NameID Format="urn:oasis:names:tc:SAML:2.0:nameid-format:transient">ploer@subspacesw.com</saml:NameID><saml:SubjectConfirmation Method="urn:oasis:names:tc:SAML:2.0:cm:bearer"><saml:SubjectConfirmationData NotOnOrAfter="2014-05-28T00:19:08Z" Recipient="{recipient}" InResponseTo="_a6fc46be84e1e3cf3c50"/></saml:SubjectConfirmation></saml:Subject><saml:Conditions NotBefore="2014-05-28T00:13:08Z" NotOnOrAfter="2014-05-28T00:19:08Z"><saml:AudienceRestriction><saml:Audience>{audience}</saml:Audience></saml:AudienceRestriction></saml:Conditions><saml:AuthnStatement AuthnInstant="2014-05-28T00:16:07Z" SessionNotOnOrAfter="2014-05-29T00:16:08Z" SessionIndex="_30a4af50-c82b-0131-f8b5-782bcb56fcaa"><saml:AuthnContext><saml:AuthnContextClassRef>urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport</saml:AuthnContextClassRef></saml:AuthnContext></saml:AuthnStatement></saml:Assertion>' +
+            '<saml:Assertion xmlns:xs="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" Version="2.0" ID="pfx3b63c7be-fe86-62fd-8cb5-16ab6273efaa" IssueInstant="2014-05-28T00:16:08Z"><saml:Issuer>https://app.onelogin.com/saml/metadata/371755</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="#pfx3b63c7be-fe86-62fd-8cb5-16ab6273efaa"><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>DCnPTQYBb1hKspbe6fg1U3q8xn4=</ds:DigestValue></ds:Reference></ds:SignedInfo><ds:SignatureValue>e0+aFomA0+JAY0f9tKqzIuqIVSSw7LiFUsneEDKPBWdiTz1sMdgr/2y1e9+rjaS2mRmCi/vSQLY3zTYz0hp6nJNU19+TWoXo9kHQyWT4KkeQL4Xs/gZ/AoKC20iHVKtpPps0IQ0Ml/qRoouSitt6Sf/WDz2LV/pWcH2hx5tv3xSw36hK2NQc7qw7r1mEXnvcjXReYo8rrVf7XHGGxNoRIEICUIi110uvsWemSXf0Z0dyb0FVYOWuSsQMDlzNpheADBifFO4UTfSEhFZvn8kVCGZUIwrbOhZ2d/+YEtgyuTg+qtslgfy4dwd4TvEcfuRzQTazeefprSFyiQckAXOjcw==</ds:SignatureValue><ds:KeyInfo><ds:X509Data><ds:X509Certificate>'+TEST_CERT+'</ds:X509Certificate></ds:X509Data></ds:KeyInfo></ds:Signature><saml:Subject><saml:NameID Format="urn:oasis:names:tc:SAML:2.0:nameid-format:transient">aaaaaaaa@bbbbb.local</saml:NameID><saml:SubjectConfirmation Method="urn:oasis:names:tc:SAML:2.0:cm:bearer"><saml:SubjectConfirmationData NotOnOrAfter="2014-05-28T00:19:08Z" Recipient="{recipient}" InResponseTo="_a6fc46be84e1e3cf3c50"/></saml:SubjectConfirmation></saml:Subject><saml:Conditions NotBefore="2014-05-28T00:13:08Z" NotOnOrAfter="2014-05-28T00:19:08Z"><saml:AudienceRestriction><saml:Audience>{audience}</saml:Audience></saml:AudienceRestriction></saml:Conditions><saml:AuthnStatement AuthnInstant="2014-05-28T00:16:07Z" SessionNotOnOrAfter="2014-05-29T00:16:08Z" SessionIndex="_30a4af50-c82b-0131-f8b5-782bcb56fcaa"><saml:AuthnContext><saml:AuthnContextClassRef>urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport</saml:AuthnContextClassRef></saml:AuthnContext></saml:AuthnStatement></saml:Assertion>' +
             '</samlp:Response>';
           var base64xml = Buffer.from( xml ).toString('base64');
           var container = { SAMLResponse: base64xml };

passport-saml$ npm run tsc && node_modules/mocha/bin/mocha -g 'valid onelogin xml document should validate'

  passport-saml /
    saml.js / 
      validatePostResponse checks /
        validatePostResponse xml signature checks /
          1) valid onelogin xml document should validate

  0 passing (85ms)
  1 failing

  1) passport-saml /
       saml.js / 
         validatePostResponse checks /
           validatePostResponse xml signature checks /
             valid onelogin xml document should validate:
     AssertionError: expected Error { message: 'Invalid signature' } to not exist

Test failed as expected due invalid signature.

---

Case: cert configuration option is not provided at all.

This could happen

• accidentally or
• due lack of understanding how essential part of SAML's security the signature check is or
• due lack of integration testing and someone pushes temporary debug version into production codebase (or maybe developer is just happy that he/she finally got SAML login to work and does not pay attention to all configuaration parameters, especially to cert)

Test case is modified so that SAML is configured without cert option and NameID is altered to aaaaaaaa@bbbbb.local.

Test should fail due invalid signature or passport-saml should fail fast due missing cert configuration option value.

It SHOULD NOT pass so that passport-saml's client application receives altered nameid value...

diff --git a/test/tests.js b/test/tests.js
index 045db3c..eefaf05 100644
--- a/test/tests.js
+++ b/test/tests.js
@@ -1622,11 +1622,10 @@ describe( 'passport-saml /', function() {
 
         var samlConfig = {
           entryPoint: 'https://app.onelogin.com/trust/saml2/http-post/sso/371755',
-          cert: TEST_CERT,
         };
         it( 'valid onelogin xml document should validate', function( done ) {
           var xml = '<samlp:Response xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol" ID="R689b0733bccca22a137e3654830312332940b1be" Version="2.0" IssueInstant="2014-05-28T00:16:08Z" Destination="{recipient}" InResponseTo="_a6fc46be84e1e3cf3c50"><saml:Issuer>https://app.onelogin.com/saml/metadata/371755</saml:Issuer><samlp:Status><samlp:StatusCode Value="urn:oasis:names:tc:SAML:2.0:status:Success"/></samlp:Status>' +
-            '<saml:Assertion xmlns:xs="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" Version="2.0" ID="pfx3b63c7be-fe86-62fd-8cb5-16ab6273efaa" IssueInstant="2014-05-28T00:16:08Z"><saml:Issuer>https://app.onelogin.com/saml/metadata/371755</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="#pfx3b63c7be-fe86-62fd-8cb5-16ab6273efaa"><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>DCnPTQYBb1hKspbe6fg1U3q8xn4=</ds:DigestValue></ds:Reference></ds:SignedInfo><ds:SignatureValue>e0+aFomA0+JAY0f9tKqzIuqIVSSw7LiFUsneEDKPBWdiTz1sMdgr/2y1e9+rjaS2mRmCi/vSQLY3zTYz0hp6nJNU19+TWoXo9kHQyWT4KkeQL4Xs/gZ/AoKC20iHVKtpPps0IQ0Ml/qRoouSitt6Sf/WDz2LV/pWcH2hx5tv3xSw36hK2NQc7qw7r1mEXnvcjXReYo8rrVf7XHGGxNoRIEICUIi110uvsWemSXf0Z0dyb0FVYOWuSsQMDlzNpheADBifFO4UTfSEhFZvn8kVCGZUIwrbOhZ2d/+YEtgyuTg+qtslgfy4dwd4TvEcfuRzQTazeefprSFyiQckAXOjcw==</ds:SignatureValue><ds:KeyInfo><ds:X509Data><ds:X509Certificate>'+TEST_CERT+'</ds:X509Certificate></ds:X509Data></ds:KeyInfo></ds:Signature><saml:Subject><saml:NameID Format="urn:oasis:names:tc:SAML:2.0:nameid-format:transient">ploer@subspacesw.com</saml:NameID><saml:SubjectConfirmation Method="urn:oasis:names:tc:SAML:2.0:cm:bearer"><saml:SubjectConfirmationData NotOnOrAfter="2014-05-28T00:19:08Z" Recipient="{recipient}" InResponseTo="_a6fc46be84e1e3cf3c50"/></saml:SubjectConfirmation></saml:Subject><saml:Conditions NotBefore="2014-05-28T00:13:08Z" NotOnOrAfter="2014-05-28T00:19:08Z"><saml:AudienceRestriction><saml:Audience>{audience}</saml:Audience></saml:AudienceRestriction></saml:Conditions><saml:AuthnStatement AuthnInstant="2014-05-28T00:16:07Z" SessionNotOnOrAfter="2014-05-29T00:16:08Z" SessionIndex="_30a4af50-c82b-0131-f8b5-782bcb56fcaa"><saml:AuthnContext><saml:AuthnContextClassRef>urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport</saml:AuthnContextClassRef></saml:AuthnContext></saml:AuthnStatement></saml:Assertion>' +
+            '<saml:Assertion xmlns:xs="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" Version="2.0" ID="pfx3b63c7be-fe86-62fd-8cb5-16ab6273efaa" IssueInstant="2014-05-28T00:16:08Z"><saml:Issuer>https://app.onelogin.com/saml/metadata/371755</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="#pfx3b63c7be-fe86-62fd-8cb5-16ab6273efaa"><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>DCnPTQYBb1hKspbe6fg1U3q8xn4=</ds:DigestValue></ds:Reference></ds:SignedInfo><ds:SignatureValue>e0+aFomA0+JAY0f9tKqzIuqIVSSw7LiFUsneEDKPBWdiTz1sMdgr/2y1e9+rjaS2mRmCi/vSQLY3zTYz0hp6nJNU19+TWoXo9kHQyWT4KkeQL4Xs/gZ/AoKC20iHVKtpPps0IQ0Ml/qRoouSitt6Sf/WDz2LV/pWcH2hx5tv3xSw36hK2NQc7qw7r1mEXnvcjXReYo8rrVf7XHGGxNoRIEICUIi110uvsWemSXf0Z0dyb0FVYOWuSsQMDlzNpheADBifFO4UTfSEhFZvn8kVCGZUIwrbOhZ2d/+YEtgyuTg+qtslgfy4dwd4TvEcfuRzQTazeefprSFyiQckAXOjcw==</ds:SignatureValue><ds:KeyInfo><ds:X509Data><ds:X509Certificate>'+TEST_CERT+'</ds:X509Certificate></ds:X509Data></ds:KeyInfo></ds:Signature><saml:Subject><saml:NameID Format="urn:oasis:names:tc:SAML:2.0:nameid-format:transient">aaaaaaaa@bbbbb.local</saml:NameID><saml:SubjectConfirmation Method="urn:oasis:names:tc:SAML:2.0:cm:bearer"><saml:SubjectConfirmationData NotOnOrAfter="2014-05-28T00:19:08Z" Recipient="{recipient}" InResponseTo="_a6fc46be84e1e3cf3c50"/></saml:SubjectConfirmation></saml:Subject><saml:Conditions NotBefore="2014-05-28T00:13:08Z" NotOnOrAfter="2014-05-28T00:19:08Z"><saml:AudienceRestriction><saml:Audience>{audience}</saml:Audience></saml:AudienceRestriction></saml:Conditions><saml:AuthnStatement AuthnInstant="2014-05-28T00:16:07Z" SessionNotOnOrAfter="2014-05-29T00:16:08Z" SessionIndex="_30a4af50-c82b-0131-f8b5-782bcb56fcaa"><saml:AuthnContext><saml:AuthnContextClassRef>urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport</saml:AuthnContextClassRef></saml:AuthnContext></saml:AuthnStatement></saml:Assertion>' +
             '</samlp:Response>';
           var base64xml = Buffer.from( xml ).toString('base64');
           var container = { SAMLResponse: base64xml };
@@ -1634,7 +1633,7 @@ describe( 'passport-saml /', function() {
           samlObj.validatePostResponse( container, function( err, profile, logout ) {
             try {
               should.not.exist( err );
-              profile.nameID.should.startWith( 'ploer' );
+              profile.nameID.should.startWith( 'aaaaaaaa@bbbbb.local' );
               done();
             } catch (err2) {
               done(err2);

passport-saml$ npm run tsc && node_modules/mocha/bin/mocha -g 'valid onelogin xml document should validate'

  passport-saml /
    saml.js / 
      validatePostResponse checks /
        validatePostResponse xml signature checks /
          ✓ valid onelogin xml document should validate

  1 passing (32ms)

...but instead of failing it passed altered nameid to passport-saml's cient application (even though signature didn't match anymore).

---

Case: misspelled cert configuration option name.

Basically same as missing cert configuration value.

diff --git a/test/tests.js b/test/tests.js
index 045db3c..03231ff 100644
--- a/test/tests.js
+++ b/test/tests.js
@@ -1622,11 +1622,11 @@ describe( 'passport-saml /', function() {
 
         var samlConfig = {
           entryPoint: 'https://app.onelogin.com/trust/saml2/http-post/sso/371755',
-          cert: TEST_CERT,
+          crt: TEST_CERT,
         };
         it( 'valid onelogin xml document should validate', function( done ) {
           var xml = '<samlp:Response xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion" xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol" ID="R689b0733bccca22a137e3654830312332940b1be" Version="2.0" IssueInstant="2014-05-28T00:16:08Z" Destination="{recipient}" InResponseTo="_a6fc46be84e1e3cf3c50"><saml:Issuer>https://app.onelogin.com/saml/metadata/371755</saml:Issuer><samlp:Status><samlp:StatusCode Value="urn:oasis:names:tc:SAML:2.0:status:Success"/></samlp:Status>' +
-            '<saml:Assertion xmlns:xs="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" Version="2.0" ID="pfx3b63c7be-fe86-62fd-8cb5-16ab6273efaa" IssueInstant="2014-05-28T00:16:08Z"><saml:Issuer>https://app.onelogin.com/saml/metadata/371755</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="#pfx3b63c7be-fe86-62fd-8cb5-16ab6273efaa"><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>DCnPTQYBb1hKspbe6fg1U3q8xn4=</ds:DigestValue></ds:Reference></ds:SignedInfo><ds:SignatureValue>e0+aFomA0+JAY0f9tKqzIuqIVSSw7LiFUsneEDKPBWdiTz1sMdgr/2y1e9+rjaS2mRmCi/vSQLY3zTYz0hp6nJNU19+TWoXo9kHQyWT4KkeQL4Xs/gZ/AoKC20iHVKtpPps0IQ0Ml/qRoouSitt6Sf/WDz2LV/pWcH2hx5tv3xSw36hK2NQc7qw7r1mEXnvcjXReYo8rrVf7XHGGxNoRIEICUIi110uvsWemSXf0Z0dyb0FVYOWuSsQMDlzNpheADBifFO4UTfSEhFZvn8kVCGZUIwrbOhZ2d/+YEtgyuTg+qtslgfy4dwd4TvEcfuRzQTazeefprSFyiQckAXOjcw==</ds:SignatureValue><ds:KeyInfo><ds:X509Data><ds:X509Certificate>'+TEST_CERT+'</ds:X509Certificate></ds:X509Data></ds:KeyInfo></ds:Signature><saml:Subject><saml:NameID Format="urn:oasis:names:tc:SAML:2.0:nameid-format:transient">ploer@subspacesw.com</saml:NameID><saml:SubjectConfirmation Method="urn:oasis:names:tc:SAML:2.0:cm:bearer"><saml:SubjectConfirmationData NotOnOrAfter="2014-05-28T00:19:08Z" Recipient="{recipient}" InResponseTo="_a6fc46be84e1e3cf3c50"/></saml:SubjectConfirmation></saml:Subject><saml:Conditions NotBefore="2014-05-28T00:13:08Z" NotOnOrAfter="2014-05-28T00:19:08Z"><saml:AudienceRestriction><saml:Audience>{audience}</saml:Audience></saml:AudienceRestriction></saml:Conditions><saml:AuthnStatement AuthnInstant="2014-05-28T00:16:07Z" SessionNotOnOrAfter="2014-05-29T00:16:08Z" SessionIndex="_30a4af50-c82b-0131-f8b5-782bcb56fcaa"><saml:AuthnContext><saml:AuthnContextClassRef>urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport</saml:AuthnContextClassRef></saml:AuthnContext></saml:AuthnStatement></saml:Assertion>' +
+            '<saml:Assertion xmlns:xs="http://www.w3.org/2001/XMLSchema" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" Version="2.0" ID="pfx3b63c7be-fe86-62fd-8cb5-16ab6273efaa" IssueInstant="2014-05-28T00:16:08Z"><saml:Issuer>https://app.onelogin.com/saml/metadata/371755</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="#pfx3b63c7be-fe86-62fd-8cb5-16ab6273efaa"><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>DCnPTQYBb1hKspbe6fg1U3q8xn4=</ds:DigestValue></ds:Reference></ds:SignedInfo><ds:SignatureValue>e0+aFomA0+JAY0f9tKqzIuqIVSSw7LiFUsneEDKPBWdiTz1sMdgr/2y1e9+rjaS2mRmCi/vSQLY3zTYz0hp6nJNU19+TWoXo9kHQyWT4KkeQL4Xs/gZ/AoKC20iHVKtpPps0IQ0Ml/qRoouSitt6Sf/WDz2LV/pWcH2hx5tv3xSw36hK2NQc7qw7r1mEXnvcjXReYo8rrVf7XHGGxNoRIEICUIi110uvsWemSXf0Z0dyb0FVYOWuSsQMDlzNpheADBifFO4UTfSEhFZvn8kVCGZUIwrbOhZ2d/+YEtgyuTg+qtslgfy4dwd4TvEcfuRzQTazeefprSFyiQckAXOjcw==</ds:SignatureValue><ds:KeyInfo><ds:X509Data><ds:X509Certificate>'+TEST_CERT+'</ds:X509Certificate></ds:X509Data></ds:KeyInfo></ds:Signature><saml:Subject><saml:NameID Format="urn:oasis:names:tc:SAML:2.0:nameid-format:transient">aaaaaaaa@bbbbb.local</saml:NameID><saml:SubjectConfirmation Method="urn:oasis:names:tc:SAML:2.0:cm:bearer"><saml:SubjectConfirmationData NotOnOrAfter="2014-05-28T00:19:08Z" Recipient="{recipient}" InResponseTo="_a6fc46be84e1e3cf3c50"/></saml:SubjectConfirmation></saml:Subject><saml:Conditions NotBefore="2014-05-28T00:13:08Z" NotOnOrAfter="2014-05-28T00:19:08Z"><saml:AudienceRestriction><saml:Audience>{audience}</saml:Audience></saml:AudienceRestriction></saml:Conditions><saml:AuthnStatement AuthnInstant="2014-05-28T00:16:07Z" SessionNotOnOrAfter="2014-05-29T00:16:08Z" SessionIndex="_30a4af50-c82b-0131-f8b5-782bcb56fcaa"><saml:AuthnContext><saml:AuthnContextClassRef>urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport</saml:AuthnContextClassRef></saml:AuthnContext></saml:AuthnStatement></saml:Assertion>' +
             '</samlp:Response>';
           var base64xml = Buffer.from( xml ).toString('base64');
           var container = { SAMLResponse: base64xml };
@@ -1634,7 +1634,7 @@ describe( 'passport-saml /', function() {
           samlObj.validatePostResponse( container, function( err, profile, logout ) {
             try {
               should.not.exist( err );
-              profile.nameID.should.startWith( 'ploer' );
+              profile.nameID.should.startWith( 'aaaaaaaa@bbbbb.local' );
               done();
             } catch (err2) {
               done(err2);

passport-saml$ npm run tsc && node_modules/mocha/bin/mocha -g 'valid onelogin xml document should validate'

  passport-saml /
    saml.js / 
      validatePostResponse checks /
        validatePostResponse xml signature checks /
          ✓ valid onelogin xml document should validate

  1 passing (31ms)

This should have failed due same reasons as "missing cert configuration option case".

---

It might be possible to spot incorrectly configured passport-saml stacks by sending SAML login responses with altered content and observing how servers respond to those login responses if services return enought error information to spot such a configuration error. Or in worst case one could manage to get authenticated session with first try.

passport-saml instance without login response signature validation is as good / bad as no authentication at all.

So instead of this:

passport-saml/src/passport-saml/saml.ts

Lines 125 to 127 in 932da9d

	if (Object.prototype.hasOwnProperty.call(options, 'cert') && !options.cert) {
	throw new Error('Invalid property: cert must not be empty');
	}

should it be e.g.:

    if (!options.cert) {
      throw new Error('Invalid property: cert must not be empty');
    }

or

    if (!Object.prototype.hasOwnProperty.call(options, 'cert')) {
      throw new Error('Certificate for IdP response validation missing');
    }

    if (!options.cert) {
      throw new Error('Invalid property: cert must not be empty');
    }

btw. passport-saml's documentation https://github.com/node-saml/passport-saml/blob/932da9d09a018fed4cb830e67090bb994f8539c1/README.md#security-and-signatures

It is a good idea to validate the signatures of the incoming SAML Responses. For this, you can provide the Identity Provider's public PEM-encoded X.509 signing certificate using the cert configuration key.

along with how cert is not a must have configuration option could give someone an idea that cert is not anything special.

[markstos]

Thanks for the detailed report, @srd90.

[srd90]

This comment is reply @markstos 's question at #536 (comment)

passport-saml is at the heart of quite many applications' authentication implementation.

At the moment https://github.com/node-saml/passport-saml/network/dependents shows > 1200 public dependents and npmjs shows > 137 public dependents and weekly downloads >110k

With such a large usage base and even larger installation base (and quite many of those applications might be something that provides some "authentication setup screen" for those applications' end users) even smallest chance to set things up incorrectly (meaning configuring passport-saml instance without cert option) means that there is probably quite a few applications/instances of applications out there without login response and assertion signature checking in place. Especially dangerous for incorrect configuration would be those applications which embed passport-saml and provide some "SAML authentication setup" dialog/wizard if those dialogs do not require IdP certificate as mandatory value (note: passport-saml does not require cert option thus it is not mandatory from passport-saml's client applications' point of view) because end users of such wizards/dialogs would assume that mandatory configuration values provide safe setup and everything else is just nice to have configuration.

As communicated by @pitbulk and @alexstuart in #180 and as shown in this issue missing cert option means accepting any login response (because response and assertion signature checkings are skipped). Here is what e.g. Shibboleth IdP's SAML2 SSO configuration documentation says about relying parties that behave as passport-saml when cert option is not provided:

If you encounter a relying party that accepts an unsigned response and assertion that is transmitted via POST (and not artifact), you have identified an insecure implementation and should report the issue immediately while following your local security incident response process.

source: https://wiki.shibboleth.net/confluence/display/IDP4/SAML2SSOConfiguration
exact versio of that wiki page: https://wiki.shibboleth.net/confluence/pages/viewpage.action?pageId=55804373 )

So IMHO passport-saml's login response signature checking can be considered broken because it allows configuration which disables signature validation (and because such situation could be quite easily spotted remotely and it is remotely exploitable).

btw. I tried if I could find with search engine some codebase with insecure passport-saml usage. It took less than 10 minutes to find this issue backstage/backstage#3326 by @lowjoel . Backstage seems to be actively maintained and pull requests seems to be actively reviewed but even with such active community they ended up embedding passport-saml insecurely.

After browsing Backstage's codebase I found this (link points to their current head of master branch):
https://github.com/backstage/backstage/blob/f7b951d9f5cfaf90eb5d228888a41ab100e52fdb/plugins/auth-backend/src/providers/saml/provider.ts#L128-L144
line 128 shows that they consider cert optional:

cert: config.getOptionalString('cert'),

and lines 140-144 show that they remove cert option from configuration object which is provided to passport-saml if end user has not filled in IdP certificate:

// passport-saml will return an error if the `cert` key is set, and the value is empty.
// Since we read from config (such as environment variables) an empty string should be equal to being unset.
if (!opts.cert) {
  delete opts.cert;
}
return new SamlAuthProvider(opts);

If backstage with such an active developer base ends up using passport-saml this way it is safe to assume that there are quite a few other projects with similar insecure configuration or which allow such insecure configuration.

Disclaimer: I just assumed that Backstage developers introduced a bug. I'm tagging @lowjoel and @freben (because they have contributed / managed pull requests related to Backstage's passport-saml configuration) if they would like to provide some view/opinion to this case.

[markstos]

@srd90 Thanks for more context.

I'm in favor of a "safe by default" approach and am comfortable with making the cert property required. This can be done with a major version release as a breaking change. Immediately, we could put out a release that starts to log warnings if cert is missing.

Are you employed as a white hat by chance?

@gugu @cjbarth Any objections?

[cjbarth]

I have no objections, but the code is currently in the process of being reworked such that even if we release now, we should probably do a semver major. I'm working on a PR to rework things to require cert. I also hope that there will be a PR removing all the deprecated callback-style functions.

[srd90]

Are you employed as a white hat by chance?

@markstos No. Checking passport-saml every now and then has become more like a hobby. Especially these COVID-19 pandemic times (restrictions, social distancing, etc.) makes one do all sort of things to prevent boredom.

[cjbarth]

Are we saying that we are requiring cert only or also requiring privateKey as well? I want to make sure I enforce the correct things in the work I'm doing.

[markstos]

I read the conversation above as applying only to `cert`.

…

On Mon, Feb 22, 2021 at 4:23 PM Chris Barth ***@***.***> wrote: Are we saying that we are requiring cert only or also requiring privateKey as well? I want to make sure I enforce the correct things in the work I'm doing. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#523 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAGJZOWMLZOJWDBD63WUF3TALDMJANCNFSM4WSIOARA> .

-- *Mark Stosberg* (he/him) Director of Systems & Security mark@rideamigos.com | 765.277.1916 https://www.rideamigos.com <https://rideamigos.com/> Changing the way the world commutes. <https://www.linkedin.com/company/rideamigos> <https://www.twitter.com/rideamigos> <https://www.facebook.com/rideamigos> <https://www.instagram.com/rideamigos> <https://rideamigos.com/newsletter-sign-up/>