[BUG] Invalid SAML response gets validated

[HendrikJan]

I am not sure, but I seem to have found a bug in the validation.

See #546

I edited a valid response (changed "https://evil-corp.com" to "https://hacker-corp.com" in two places) and the validation does not result in "Invalid response".

To Reproduce

Run the tests in #546

Expected behavior

Because the response is messed with, the validation should result in "Invalid response" but it does result in "SAML assertion expired".

Environment

• Node.js version: v15.8.0
• passport-saml version: 2.0.5

The problem seems to originate here:

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

Lines 783 to 786 in 8a1a377

	let validSignature = false;
	if (this.options.cert && this.validateSignature(xml, doc.documentElement, certs!)) {
	validSignature = true;
	}

The method this.validateSignature() returns a false when the signature is missing but also when the signature is wrong.
I think that in the case of an invalid signature, at this point in the code an error "Invalid signature" should be thrown.

[srd90]

Comments after looking at content of pull request #546 This comment was moved (and slightly modfied) from that pull request to this associated issue report because I felt that this is better place for this (and I noticed this issue report after I had already posted this comment to pull request).

---

First of all lets inspect how test case which uses valid/response.root-signed.assertion-signed.xml behaves because it is quite odd that your test receives "Assertion expired" (one could have expected for example issuer mismatch if/when issuer element at samlp namespace mismatch with issuer element at saml namespace).
So when R1A - both signed => valid test case is executed (link points to version at passport-saml's master branch):

passport-saml/test/test-signatures.spec.ts

Lines 50 to 53 in 8a1a377

	it(
	"R1A - both signed => valid",
	testOneResponse("/valid/response.root-signed.assertion-signed.xml", false, 1)
	);

I would not expect to see assertion expired because I would expect that test case controls current time behind the scenes so that response is processed without any exceptions.

Lets put break point to both of these lines and see what happens when aforementioned test case is executed:

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

Lines 1262 to 1263 in 8a1a377

	if (nowMs - this.options.acceptedClockSkewMs >= notOnOrAfterMs)
	return new Error("SAML assertion expired");

First time when we hit break points value of notOnOrAfter is 2020-09-25T16=7:00:00+00:00 (note = character) because of this

passport-saml/test/static/signatures/valid/response.root-signed.assertion-signed.xml

Line 13 in 8a1a377

<saml:SubjectConfirmationData InResponseTo="_e8df3fe5f04237d25670" NotOnOrAfter="2020-09-25T16=7:00:00+00:00" Recipient="https://evil-corp.madness.com/sso/callback"/>

and notOnOrAfterMs becomes NaN and test is some value >= NaN ... why author of response.root-signed.assertion-signed.xml and this testase wanted to have such value...dunno. Should there be failure due this issue ... don't have time to investigate...

Second time break points are hit when this

passport-saml/test/static/signatures/valid/response.root-signed.assertion-signed.xml

Line 16 in 8a1a377

<saml:Conditions NotBefore="2020-09-25T16:00:00+00:00" NotOnOrAfter="2020-09-25T17:00:00+00:00"/>

is processed. This time code ends up executing this:

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

Line 1263 in 8a1a377

return new Error("SAML assertion expired");

So R1A - both signed => valid is having same "Assertion expired" issue under the hood. Why is test case passing? Dunno. Maybe someone who really knows what happens at these lines can explain why this test case is reporting success:

passport-saml/test/test-signatures.spec.ts

Lines 32 to 36 in 8a1a377

	await assert.rejects(samlObj.validatePostResponseAsync(samlResponseBody), {
	message: shouldErrorWith || "SAML assertion expired",
	});
	//== Assert times `validateSignature` was called
	validateSignatureSpy.callCount.should.eql(amountOfSignatureChecks);

because message processing is definitely throwing exception while at the same time it is invoking signature check only once. Its as if passing validateSignatureSpy.callCount.should.eql(amountOfSignatureChecks); would be considered sufficient even if message processing ended up having exception.

Eitherway reference testcase seems to have issues/problems of its own. From your pull request's/question's point of view "Assertion expired" is not a "surprise".

---

Now lets move to case you have in this pull request.

passport-saml considers missing or invalid top level signature as "soft error" (assuming that cert option with IdP certificate was provided).

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

Lines 783 to 786 in 8a1a377

	let validSignature = false;
	if (this.options.cert && this.validateSignature(xml, doc.documentElement, certs!)) {
	validSignature = true;
	}

i.e. it leaves value of validSignature to false and proceeds to check whether assertion's signature is valid:

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

Lines 804 to 811 in 8a1a377

	if (
	this.options.cert &&
	!validSignature &&
	!this.validateSignature(xml, assertions[0], certs!)
	) {
	throw new Error("Invalid signature");
	}
	return this.processValidlySignedAssertionAsync(

and because you modified only content of surrounding samlp stuff instead of assertion's stuff signature of assertion's content is valid and passport-saml proceeds to consume it. See also #536 (comment)

Quote from https://wiki.shibboleth.net/confluence/pages/viewpage.action?pageId=55804373

As long as one of the response or assertion are signed, use of the profile is "safe" in terms of authentication integrity, but there are vulnerabilities in XML Encryption that make signing responses advisable when the most common encryption algorithms are used.

It would be a huge security problem if passport-saml would return any values from samlp namespace to application because those values MAY be modified by who ever is passing saml login response which - login response - has only/also validly signed assertion to passport-saml (because missing or invalid top level signature is considered "soft error" and if at least assertion signature is valid login is accepted). Quick and definitely not too careful code browsing gives an impression that values returned to application are taken from assertion and none of the values from samlp namespace are returned to application (NOTE: somone should check passport-saml implementation really carefully to make sure that values which are not "covered by" valid signature shall never be passed to application).

You wrote at issue report that:

I think that in the case of an invalid signature, at this point in the code an error "Invalid signature" should be thrown.

Someone can remove top level signature from login response before passing it to SP (passport-saml). You would have to introduce another flag to passport saml so that it would consider missing or invalid top level signature as "hard error". Of course you would have to make sure that your IdP is signing samlp (response). For example Shibboleth IdP signs response by default and assertion signing is optional (if e.g. SP requests it "WantAssertionsSigned = true").

Your pull request's testcase

passport-saml/test/test-signatures.spec.ts

Lines 76 to 83 in d6557d1

	it(
	"R1A - both signed with invalid root signature => error",
	testOneResponse(
	"/invalid/response.root-invalidly-signed.assertion-signed.xml",
	INVALID_SIGNATURE,
	2
	)
	);

is failing with the "Assertion expired" under the hoods due same reason as reference case examined at the start of this comment. Why your testcase is failing with such hard error while reference case is not ... dunno ...

But at the end of the date and based on how passport-saml is currently implemented testcase you defined is actually "success / valid" case i.e. similar to

passport-saml/test/test-signatures.spec.ts

Lines 50 to 53 in 8a1a377

	it(
	"R1A - both signed => valid",
	testOneResponse("/valid/response.root-signed.assertion-signed.xml", false, 1)
	);

with the exception that processed response file has invalid top level signature and amount of signature checks would be 2 (and passport-saml would still throw "SAML assertion expired" but test case would be success due same strange reason as reference case is passing).

---

So passport-saml seems validate that if response level signature is missing or invalid that at least assertion's signature is valid (and according to Shibboleth IdP wiki As long as one of the response or assertion are signed, use of the profile is "safe" in terms of authentication integrity...) and passport-saml seems to return values only from assertion (see note from above regarding code reviewing passport-saml implementation to validate this). Of course any signature validation happens if and only if cert option is present which is another story (see #523 )

[HendrikJan]

Thank you for looking into this.
I'll close this issue and the PR as it looks like fixing #523 will fix this issue.

I would not expect to see assertion expired because I would expect that test case controls current time behind the scenes so that response is processed without any exceptions.

I get the impression from reading the tests that they do not control current time behind the scenes, but instead accept the error "Assertion expired" as signifying a valid response.

I hope #523 is going to be fixed soon.