Skip to content

Prevent error when an EncryptedAssertion is next to the Signature in the document root. #69

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
XavierTalpe wants to merge 2 commits into from
Closed

Prevent error when an EncryptedAssertion is next to the Signature in the document root. #69

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
049ca2e
Prevent "SAML Assertion not found error" when an EncryptedAssertion i…
Jun 27, 2016
e90a933
Unused variable.
Jun 27, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 37 additions & 20 deletions lib/saml2.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,36 +413,54 @@ parse_authn_response = (saml_response, sp_private_keys, idp_certificates, allow_

async.waterfall [
(cb_wf) ->
decrypt_assertion saml_response, sp_private_keys, (err, result) ->
return cb_wf null, result unless err?
return cb_wf err, result unless allow_unencrypted
assertion = saml_response.getElementsByTagNameNS(XMLNS.SAML, 'Assertion')
unless assertion.length is 1
return cb_wf new Error("Expected 1 Assertion or 1 EncryptedAssertion; found #{assertion.length}")
cb_wf null, assertion[0].toString()
(result, cb_wf) ->
debug result
unencrypted_assertions = saml_response.getElementsByTagNameNS( XMLNS.SAML, 'Assertion' )
encrypted_assertions = saml_response.getElementsByTagNameNS( XMLNS.SAML, 'EncryptedAssertion' )

nb_assertions = unencrypted_assertions.length + encrypted_assertions.length
unless nb_assertions is 1
return cb_wf new Error("Expected one Assertion or EncryptedAssertion. Found #{unencrypted_assertions.length} Assertions and #{encrypted_assertions.length} EncryptedAssertions")

if encrypted_assertions.length == 1
decrypt_assertion saml_response, sp_private_keys, (err, result) ->
return cb_wf err, result
else if unencrypted_assertions.length == 1 and allow_unencrypted
return cb_wf null, unencrypted_assertions[0].toString()
else
return cb_wf new Error("Found one unencrypted Assertion but flag 'allow_unencrypted' is set to false!")
(decrypted_assertion, cb_wf) ->
debug decrypted_assertion
if ignore_signature
return cb_wf null, (new xmldom.DOMParser()).parseFromString(result)
return cb_wf null, (new xmldom.DOMParser()).parseFromString(decrypted_assertion)

saml_response_str = saml_response.toString()
for cert in idp_certificates or []
signed_data = check_saml_signature(result, cert) or check_saml_signature saml_response_str, cert
# Signature is either part of the assertion node or the root node.
signatures_from_assertion = check_saml_signature decrypted_assertion, cert
signatures_from_response = check_saml_signature saml_response.toString(), cert

signed_data = signatures_from_assertion or signatures_from_response
unless signed_data
continue # Cert was not valid, try the next one

for sd in signed_data
signed_dom = (new xmldom.DOMParser()).parseFromString(sd)
assertion = signed_dom.getElementsByTagNameNS(XMLNS.SAML, 'Assertion')
if assertion.length is 1
return cb_wf null, signed_dom

if signatures_from_assertion
assertion = signed_dom.getElementsByTagNameNS( XMLNS.SAML, 'Assertion' )
if assertion.length is 1
return cb_wf null, signed_dom
else
return cb_wf new Error( "Signed data does not contain a SAML Assertion!" )
else if signatures_from_response
decrypted_assertion_dom = (new xmldom.DOMParser()).parseFromString(decrypted_assertion)
Copy link
Contributor

@jefff jefff Jul 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about the delay of this review; I was on vacation and didn't have a chance to look it over.

I think there's a potential security pitfall here. Specifically, the fact that signatures_from_response is set only means that the response contains some signature, but this line is parsing the decrypted assertion and passing it on as validated. The signature may not have covered the encrypted assertion (or unencrypted assertion) of the response and this means it would accept invalid data.

I'm not sure what the best fix is for this. It seems like either it would have to be decrypted again from the signed data or some reference would have to be maintained and checked that the assertion was part of the signed data. It's also possible I'm misunderstanding this logic so please let me know if that is the case.

Copy link

@sebakerckhof sebakerckhof Jul 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take a look at this in one of the coming days.

return cb_wf null, decrypted_assertion_dom

return cb_wf new Error("Signed data did not contain a SAML Assertion!")
return cb_wf new Error("SAML Assertion signature check failed! (checked #{idp_certificates.length} certificate(s))")
(decrypted_assertion, cb_wf) ->
(decrypted_assertion_dom, cb_wf) ->
try
user.name_id = get_name_id decrypted_assertion
user.session_index = get_session_index decrypted_assertion
assertion_attributes = parse_assertion_attributes decrypted_assertion
user.name_id = get_name_id decrypted_assertion_dom
user.session_index = get_session_index decrypted_assertion_dom
assertion_attributes = parse_assertion_attributes decrypted_assertion_dom
user = _.extend user, pretty_assertion_attributes(assertion_attributes)
user = _.extend user, attributes: assertion_attributes
cb_wf null, { user }
Expand Down Expand Up @@ -538,7 +556,6 @@ module.exports.ServiceProvider =
return setImmediate cb, new Error("Request body does not contain SAMLResponse or SAMLRequest.")

saml_response = null
decrypted_assertion = null
response = {}

async.waterfall [
Expand Down