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.

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

Validate acs url #102

Merged
merged 8 commits into from Nov 9, 2018

Conversation

Projects
None yet
3 participants
@jphenow
Copy link
Member

jphenow commented Oct 22, 2018

Matching standard (line 2063) so that IDP performs validation on the AssertionConsumerServiceURL field to match Metadata Host or some other configured Host to prevent MitM attacks

@jphenow

This comment has been minimized.

Copy link
Member Author

jphenow commented Oct 23, 2018

@ikkisoft @itstehkman does this make sense? I have to update docs before merging, but basically this would make it so ACS URLs are whitelisted based on the Host of the metadata URL or the list from config.assertion_consumer_service_hosts - based on what you know does it make more sense to configure it on a per-service-provider basis? Should we not even assume based on metadata URL? I'm also considering make this an enabled feature that's disabled by default for now to prevent breaking changes for this version.

@ikkisoft

This comment has been minimized.

Copy link

ikkisoft commented Oct 23, 2018

My understanding of the Destination (saml 2 core lines 1477-1482) vs AssertionConsumerServiceURL (saml 2 core lines 1483-1488) is that they're the same value, but just used for different purposes:

  • Destination is the URL of the endpoint where the Service Provider is expecting the response
  • AssertionConsumerServiceURL is the same URL within the IDP request, where peer IDP must send the response to

So, I think you can simply use the value you already have in the config for the service_provide and not the metadata URL.

some-issuer-url.com/saml --> some-issuer-url.com

service_providers = {
    "some-issuer-url.com/saml" => {
      fingerprint: "9E:65:2E:03:06:8D:80:F2:86:C7:6C:77:A1:D9:14:97:0A:4D:F4:4D",
      metadata_url: "http://some-issuer-url.com/saml/metadata"
    },
  }

If you allow override via config file (as implemented), I think is safe enough to enforce it since people can change the config after the new deployment. Especially, if it's clear in the README.

Btw, reading the standard (http://docs.oasis-open.org/security/saml/v2.0/saml-core-2.0-os.pdf line 2063) they are actually saying that the IDP is required to perform validation on the AssertionConsumerServiceURL field:

The responder MUST ensure by some means that the value specified is in fact associated
with the requester.
@ikkisoft

This comment has been minimized.

Copy link

ikkisoft commented Oct 23, 2018

ruby-saml (https://github.com/onelogin/ruby-saml) seems to have three separate fields for those items:

So, this is probably the safest / most flexible option.

@ikkisoft

This comment has been minimized.

Copy link

ikkisoft commented Oct 23, 2018

For this issue, you can use CVE-2018-18604 (in case you will send out security update notices, etc.)

@itstehkman

This comment has been minimized.

Copy link

itstehkman commented Oct 24, 2018

I agree with @ikkisoft. I think this PR is mostly good. The only issue I see is if you've configured the IdP with N acceptable_response_hosts, where one of these could maliciously accept a response meant for another host:

EG:

acceptable_response_hosts = [A, B]

I make a SAMLRequest for host A, and it gets maliciously modified such that its Destination / ACS URL is sent to B instead. In the unlikely case that B would accept the assertion, this is an issue.

I think it is simpler and more accurate to just have this on a per-service-provider configuration level without checking a set of acceptable hosts.

@@ -136,6 +141,20 @@ def session_index
@_session_index ||= xpath("//samlp:SessionIndex", samlp: samlp).first.try(:content)
end

def acceptable_reponse_hosts

This comment has been minimized.

@itstehkman

itstehkman Oct 24, 2018

also small nit: typo -> acceptable_response_hosts

@itstehkman

This comment has been minimized.

Copy link

itstehkman commented Oct 30, 2018

@jphenow is this ready? I think this implementation should suffice.

@jphenow

This comment has been minimized.

Copy link
Member Author

jphenow commented Nov 6, 2018

Updated to set on per-service-provider basis with a list just for flexibility. If that's a security hole I deem that a hole on their configuration, not the lib. Let me know what you think and I can make a minor version release later today or tomorrow

@itstehkman

This comment has been minimized.

Copy link

itstehkman commented Nov 8, 2018

@jphenow this looks great! Thanks for the updates. LGTM.

@jphenow jphenow merged commit f579b15 into master Nov 9, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jphenow jphenow deleted the validate-acs-url branch Nov 9, 2018

@jphenow

This comment has been minimized.

Copy link
Member Author

jphenow commented Nov 9, 2018

Released as 0.8.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment