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

[XavierTalpe]

I came across an issue when the SAML login response has the following structure:

<Response ...>
    <Issuer ...>
        ...
    </Issuer>
    <Signature>
        ...
    </Signature>
    <Status>
        <:StatusCode Value="urn:oasis:names:tc:SAML:2.0:status:Success" />
    </Status>
    <EncryptedAssertion>
        ...
    </EncryptedAssertion>
</Response>

Note that both the EncryptedAssertion and the Signature block are part of the root node. The EncryptedAssertion also does not contain any Signature elements after decryption.

The current version of the code fails with the error Signed data did not contain a SAML Assertion!.

This bug is caused by the combination of

signed_data = check_saml_signature(result, cert) or check_saml_signature saml_response_str, cert

and

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
return cb_wf new Error("Signed data did not contain a SAML Assertion!")

In case the EncryptedAssertion block does not contain any Signatures, check_saml_signature(result, cert) will fail and check_saml_signature saml_response_str, cert is executed to retrieve the Signatures from the original SAML response. Because this SAML response still contains the EncryptedAssertion, getElementsByTagNameNS(XMLNS.SAML, 'Assertion') will fail.

I modified the code so it can handle each use case. I looked into adding a test but this seems a bit trickier than expected because I can't use the original SAML response (company privacy/security).

My apologies if I overlooked something, I'm not a SAML expert :).

[XavierTalpe]

Any update on this?

[sebakerckhof]

+1

[cozmo]

@XavierTalpe @sebakerckhof Sorry about the delay here - We'll take a look at this today.

[jefff]

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.

[sebakerckhof]

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

[bensimian]

+1

[mohit]

closing in favor of first reviewing #74 Please comment if you think #74 is not a solution for the same issue