Skip to content
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

Insecure allowedOrigins validation #691

Closed
vetsin opened this issue Jun 14, 2016 · 11 comments

Comments

Projects
None yet
5 participants
@vetsin
Copy link

commented Jun 14, 2016

Autobahn|Python incorrectly checks the Origin header when the 'allowedOrigins' value is set.

The following will set

class OriginCheckServerFactory(WebSocketServerFactory):
    protocol = ...arbitrary entry here...

    def __init__(self, url):
        WebSocketServerFactory.__init__(self, url)
        self.setProtocolOptions(allowedOrigins=[u"127.0.0.1",u"*.example.com"])

Then the following connection request will result in a valid 101 Protocol Switch Response:

GET /ws HTTP/1.1
Host: www.example.com
Sec-WebSocket-Version: 13
Origin: http://www.example.com.malicious.com
Sec-WebSocket-Extensions: permessage-deflate
Sec-WebSocket-Key: tXAxWFUqnhi86Ajj7dRY5g==
Connection: keep-alive, Upgrade
Upgrade: websocket

This is due to the wildcard2patterns functions, which turns u"*.example.com" into r".*\.example\.com". This regex pattern is then matched against the complete incoming origin value. The websocket_origin value should be first parsed as a URI object and the authority section extracted, and the regex matched against authority ensuring that ^ and $ are used. E.g. r"^.*\.example\.com$"

@vetsin vetsin changed the title Insecure allowedOrigin validation Insecure allowedOrigins validation Jun 14, 2016

@eastein

This comment has been minimized.

Copy link

commented Jun 14, 2016

The scheme must be validated as well. Merely validating the host would be insufficient as MITM + failure to validate scheme = vuln, if the end user intends to use HTTPS only.

@meejah

This comment has been minimized.

Copy link
Member

commented Jun 15, 2016

I have written a unit-test simulating vestin's example above, and can confirm that the matching does indeed fail (i.e. the above will not call failHandshake() as it should).

@meejah

This comment has been minimized.

Copy link
Member

commented Jun 16, 2016

Thanks to Toba, vetsin and jchampion for discussion in #autobahn

@meejah

This comment has been minimized.

Copy link
Member

commented Jun 16, 2016

Okay, I think I incorporated the feedback in the PR.

Testing with Chrome on file:///home/blah/src/autobahn-python/examples/... etc shows that Firefox sends a null Origin in that case while Chrome sends a file:// URL .. so instead of complete file:// handling, I just remap any file:// scheme to "null". Presumably the only use-case for allowing connections from "null" Origins is debugging, testing etc when loading directly from disk.

I also introduced an allowNullOrigin option (whose default is True :/) so that you can make the examples work easily, "out of the box". That has the downside of having less-secure defaults :(

To partially mitigate this I made the default value in setProtocolOptions be False (instead of None), so that if you ever set any options (e.g. non-default allowOrigins list, which is ['*'] by default anyway) then you get a default-secure setup (i.e. unless you explicitly pass allowNullOrigin=True). So, if you've tried to set the allowOrigins list to something more-secure than ["*"] you can't "accidentally" accept null origins.

@meejah

This comment has been minimized.

Copy link
Member

commented Jun 16, 2016

There's also an issue with port-matching to consider. As it sits, the only configuration is the list of allowedOrigins which is turned into regular expressions. But per RFC6454 we should be matching on a 3-tuple of (scheme, host, port). I could see a couple possibilities:

  1. just change the allowedOrigins API to something more expressive
  2. continue just running the reg-ex's over the whole Origin header (if it exists), thus the only change is really to wrap the reg-exes in ^ and $.
  3. match schemes, but turn (host, port) into a string and run the existing reg-exs on that (which is more-likely to work with existing code). Only append the :port part if it's non-standard.

I guess a 4th way would be to run the (fixed/wrapped) reg-exs on any incoming complete Origin headers (as now) if allowedOrigins is specified, but introduce a new API that lets you match on the 3-tuple (scheme, host, port) somehow.

@vetsin

This comment has been minimized.

Copy link
Author

commented Jun 18, 2016

All sound like they can be made secure, in my opinion 3 seems the safest to avoid shooting yourself in the foot.

@oberstet

This comment has been minimized.

Copy link
Member

commented Jun 23, 2016

Will look into this soonish (we are currently exhibiting at a trade fair) ..

@oberstet

This comment has been minimized.

Copy link
Member

commented Jul 17, 2016

I believe this was fixed and merged in #693

@oberstet oberstet closed this Jul 17, 2016

@jeffreyfroman-temboo

This comment has been minimized.

Copy link

commented Jul 20, 2016

Is there documentation for this change anywhere? It's broken my origin validation, and I'm having trouble figuring out how to fix it. In particular, what should both the allowedOrigin list and the Origin header look like now, in order to match?

I'm seeing errors such as
No host part in Origin 'www-foo.bar.com'

After adding the scheme to my allowedOrigins list, I see:
origin 'https://www-foo.bar.com' not allowed

It does work when adding both the scheme and port to allowedOrigins, which I assume is because the origin port differs from the server port. A pointer to some explicit documentation about that necessity would still be appreciated. Thanks!

@oberstet

This comment has been minimized.

Copy link
Member

commented Jul 20, 2016

@jeffreyfroman-temboo please see the description of the allowedOrigin option here (the test cases here might also be helpful). The change is also mentioned in the changelog https://autobahn-python.readthedocs.io/en/latest/changelog.html

@jeffreyfroman-temboo

This comment has been minimized.

Copy link

commented Jul 21, 2016

Excellent, thanks very much.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.