Remove InResponseTo value if response validation fails

[Archinowsk]

InResponseTo value is used to verify that the response is for a valid request. If InResponseTo doesn't match a request the response is invalid. This is a cheap verification before more expensive cryptographical validation.

If cryptographical validation fails the response is most likely invalid and the corresponding InResponseTo value should be removed so no further calls can be made using it.

[markstos]

@josecolella Would you mind peer-reviewing this?

[cjbarth]

Could these commits be squashed?

[cjbarth]

nit: The rest of the code uses } else { instead of putting that on two lines.

[markstos]

@Archinowsk or @cjbarth Could you link the part of the SAML spec that deals with InResponseTo validation, for reference?

[Archinowsk]

Squashed commits and fixed formatting as @cjbarth suggested.

I tried running tests against master and the same tests fail. Any ideas?

Is this ok for specs?

http://docs.oasis-open.org/security/saml/v2.0/saml-sec-consider-2.0-os.pdf

"6.1.3 Message Insertion

Threat: A fabricated request or response is inserted into the message stream. A false response such as a spurious “yes” reply to an authorization decision query or the return of false attribute information in response to an attribute query may result in inappropriate receiver action.

Countermeasures: The ability to insert a request is not a threat at the SOAP binding level. The threat of inserting a false response can be a denial of service attack, for example returning SOAP Faults for responses, but this attack would become quickly obvious. The more subtle attack of returning fabricated responses is addressed in the SAML protocol, appropriate since according to the SOAP Binding definition each SOAP response must contain a single SAML protocol response unless it contains a fault. The SAML Protocol addresses this with two mechanisms, correlation of responses to requests using the required InResponseTo attribute, making an attack harder since requests must be intercepted to generate responses, and through the support origin authentication, either via signed SAML responses or through a secured transport connection such as SSL/TLS."

[markstos]

I want to make sure I understand this by explaining this in my own words:

InResponseTo should only be used for one request/response cycle.

This patch is addressing a case where a response fails cryptographically. When this happens, the updated code will remove the related InResponseTo from the internal cache.

The benefit is that future attempts to respond using the same InReponseTo value will fail faster because InResponseTo is checked before the cryptographic check.

Are we on the same page about that?

However, it looks like the last release accidentally reverted an improvement to InResponseTo validation. See my note at: 7674e18e#r32746468

I think the other issue needs to be addressed as part of merging and releasing this.

And yes, there are some unrelated tests failing on master that need to be addressed. A separate PR is welcome to address those. It looks like there was a bad merge recently.

CC: @josecolella

[Archinowsk]

Are we on the same page about that?

Yes, you described the logic correctly.

[cjbarth]

@Archinowsk I find that there is a clean merge of master into this branch and all the tests pass. If @markstos would restart the Travis CI build again, I think the merge would succeed.

Otherwise, I can't write to your branch, so I merged bergie:master into Archinowsk:remove-inresponseto-if-response-validation-fails and pushed that to cjbarth:remove-inresponseto-if-response-validation-fails, so you can feel free to add my source as an origin and then reset --hard to cjbarth:remove-inresponseto-if-response-validation-fails and push that up to Archinowsk:remove-inresponseto-if-response-validation-fails which should kick off this CI job again and get everything working again.

Also, I see that your clone of this project has lots of little changes. Is there any interest on your part in getting those pushed upstream for us all to benefit from? If you need help, please let me know.

[markstos]

@cjbarth Thanks for helping move things along!

[Archinowsk]

Thanks, @cjbarth! Pushed changes and the build is passing now. Ended up cleaning some whitespaces in the process.

Also, I see that your clone of this project has lots of little changes. Is there any interest on your part in getting those pushed upstream for us all to benefit from? If you need help, please let me know.

I'm working with a project that has its own fork: https://github.com/vrk-kpa/suomifi-passport-saml. We have been adding suitable fixes to the upstream but there are lots of stuff that would not make sense in a standard implementation :).