search

SAML-Toolkits / python3-saml

MIT License
683 stars 304 forks source link

Resolved issue 226 with testing: Destination URL Comparison is now case-insensitive for netloc #231

Closed tbloomer4 closed 3 years ago

tbloomer4 commented 3 years ago

PR to resolve: https://github.com/onelogin/python3-saml/issues/226. This PR resolves ignoring netloc case when comparing the Destination URL to the actual URL when validating SAML responses (per RFC 4343/7617)

Although a fix like this was suggested: https://github.com/onelogin/ruby-saml/blob/17fce0a6caa6282ebd51c6276d3a7b91138b8d24/lib/onelogin/ruby-saml/utils.rb#L284

The preferred change appears to be attempting to parse the both the expected and the actual url, converting the hostname to lowercase, and returning it as a string. This solution results in the fewest code changes. If the URL parsing fails, the original url is returned, so the backend behavior regresses to the current implementation.

I do wonder if we should be using:

if not self.__standardize_url(destination).equals(self.__standardize_url(current_url))

instead of 

if not self.__standardize_url(destination).startswith(self.__standardize_url(current_url))

The SAML Docs https://docs.oasis-open.org/security/saml/v2.0/saml-core-2.0-os.pdf define destination as:

"A URI reference indicating the address to which this request has been sent. This is useful to prevent malicious forwarding of requests to unintended recipients, a protection that is required by some protocol bindings. If it is present, the actual recipient MUST check that the URI reference identifies the location at which the message was received"

This seems to imply that they must be an exact match, as such .equals may be more appropriate here.

pitbulk commented 3 years ago

The same solution should be applied to:

Maybe add the url check to the utils to reuse it?

The problem with the use of equals instead of startswith is when the URL has query parameters.

tbloomer4 commented 3 years ago

Comments resolved, with further testing

pitbulk commented 3 years ago

This is only compatible with python 3.X,

2.X fails by:

  File "/home/pitbulk/proyectos/python3-saml/src/onelogin/saml2/utils.py", line 1081
    scheme, netloc, *rest = urlsplit(url)

https://travis-ci.org/github/onelogin/python3-saml/jobs/752894706

tbloomer4 commented 3 years ago

@pitbulk is this all set? I've just seen your comment regarding compatibility, but also that this PR has been merged.

Going to assume the issue is resolved, unless you say otherwise.

pitbulk commented 3 years ago

Yes, I solved it at: https://github.com/onelogin/python3-saml/commit/949425fd81250fff588075aaa704ff2f92168dfd