Potential security vulnerability

[shahpawan]

Hi,
I tried to use passport-saml with okta as an identity provider.
I faced an issue with parsing the response from okta.
In the saml.js, there is this function called SAML.prototype.getElement.
This function gets the saml element from response.
In this the keys used to parse saml tags are 'saml' and 'samlp'.
Okta sends back 'saml2' and 'saml2p'.

So I had to add these keys in the code.
Here's how the method looks now.
Can you add it in the main library?

SAML.prototype.getElement = function (parentElement, elementName) {
if (parentElement['saml:' + elementName]) {
return parentElement['saml:' + elementName];
} else if (parentElement['samlp:'+elementName]) {
return parentElement['samlp:'+elementName];
} else if (parentElement['saml2p:'+elementName]) {
return parentElement['saml2p:'+elementName];
} else if (parentElement['saml2:'+elementName]) {
return parentElement['saml2:'+elementName];
}
return parentElement[elementName];
};

[ploer]

This should be addressed by #31.

[shahpawan]

Thanks a lot.
I had forked the repo and added some fixes there.
We have aded a fix for a major saml security vulnerability.
Its on this repo --> https://github.com/pawanvshah/passport-saml
If you see anything new in it and worth adding, do add it to the original repo.

On Thursday, 29 May 2014 11:06 PM, Peter Loer [email protected] wrote:

Closed #19.
—
Reply to this email directly or view it on GitHub.

[shahpawan]

This is the fix i was mentioning--> https://github.com/pawanvshah/passport-saml/commit/8b7aa68571e34cbeb270021e862b62661ff3ebf9 .

The details of the security issue in this link --> https://www.usenix.org/system/files/conference/usenixsecurity12/sec12-final91.pdf

[ploer]

Looks like an important fix.

Any chance you have a document that demonstrates the vulnerability?

[shahpawan]

Hi,
Here's the document --> https://www.usenix.org/system/files/conference/usenixsecurity12/sec12-final91.pdf

Here's the associated talk on this --> https://media.ccc.de/browse/congress/2012/29c3-5210-en-on_breaking_saml_h264.html

The problem is with the way passport-saml extracts user identity info.
Due to this (as explained in the doc and video) a user that has access to a system through single sign on can elevate their privileges.

So we had added a fix for this in the forked repo of passport saml library.

On Friday, 30 May 2014 1:35 AM, Peter Loer [email protected] wrote:

Looks like an important fix.
Any chance you have a document that demonstrates the vulnerability?
—
Reply to this email directly or view it on GitHub.

[ploer]

Sorry, I didn't ask my question very well. Do you have a saml response XML document that is crafted in such a way that it exposes this vulnerability?

If so, and if it is something you could share, I would like to use it as a test case.

Thanks!

[ploer]

Never mind, I was able to create a test case that demonstrates this.

[shahpawan]

Cool.
Just an FYI (again), we had added a fixed for this issue in the forked repo at 
https://github.com/pawanvshah/passport-saml.

Here is the checkin.
at https://github.com/pawanvshah/passport-saml/commit/8b7aa68571e34cbeb270021e862b62661ff3ebf9

This was reviewed by our info security team and they were happy.
Please do review it and see if it helps you to get a proper fix in the original passport saml.

On Friday, 30 May 2014 4:11 AM, Peter Loer [email protected] wrote:

Never mind, I was able to create a test case that demonstrates this.
—
Reply to this email directly or view it on GitHub.

[ploer]

@pawanvshah , I've checked in the following change that I believe should address this issue; I'd be interested in any feedback you might have: 9ce5992

It should be similar to your change in that it checks the ID of the object verified, but it also:

• Tolerates multiple signatures on a document (for instance, I have an example Okta doc where both the Response and the Assertion are signed)
• Rejects additional suspicious cases such as nested assertions with the same id.
• Checks the ID of the object verified against the ID actually referenced in the signature, instead of just against the ID of an xml block that contains a valid signature.

On the last point, I am not positive but I think your implementation might allow a document that looks something like the following:

<Response><Assertion ID=1><Signature block, signing ID=2><Assertion ID=2 /></Sig></Assertion></Response>

[shahpawan]

Thanks a lot. Good to see the library becoming an active module again.
Node.js usage has increased a lot amongst the people I know and passport-saml is a critical library for most apps after express.