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

Check 'Host' header for local connections #3714

Merged
merged 5 commits into from Jul 3, 2018

Conversation

Projects
None yet
2 participants
@takluyver
Copy link
Member

takluyver commented Jun 25, 2018

cc @minrk @rgbkrk

I was reading another article about DNS rebinding, and it reminded me that I meant to add another layer of security to protect against such attacks. The default options in this PR will reject any request where the Host header isn't localhost or a loopback IP address.

To be clear, I believe that our on-by-default token authentication already protects against DNS rebinding attacks. I want to add an extra layer of protection in case there are flaws in our existing security, or in case people disable token authentication without understanding the implications.

This is probably too stringent to drop on people directly - it would break all servers where remote access is meant to be possible, and require people to update their settings. Possible ways to soften it:

  • Disable the check if NotebookApp.ip is configured to anything other than localhost. Could make it easy to switch off security accidentally, e.g. if you configure localhost6 to experiment with IPv6.
  • Disable the check if we're telling the server to listen on any non-loopback interface - assume people who configure this are using some external security measures. Can we reliably know whether the server is listening on a public IP?
  • Disable the check by default and let people enable it if they want. I'd rather not do this, because approximately no-one will enable it. But it could be used as a transition period.
@@ -831,6 +833,29 @@ def _token_changed(self, change):
"""
)

allow_remote_access = Bool(False, config=True,

This comment has been minimized.

@minrk

minrk Jun 25, 2018

Member

allow_remote_access should be set to True when binding to anything other than localhost, otherwise we're over-complicating hosted notebook servers (e.g. this would break mybinder.org).

@minrk

This comment has been minimized.

Copy link
Member

minrk commented Jun 25, 2018

I think option 2 is probably the right one. A simple check for localhost or .is_loopback on the bind ip, and only enable by default if this is the case.

Can we reliably know whether the server is listening on a public IP?

Not strictly (e.g. in docker, it always looks like a public server), but I think it's probably the right thing to do to disable this check if the bind IP is anything other than a very simple localhost or 127.* address.

@takluyver

This comment has been minimized.

Copy link
Member Author

takluyver commented Jun 25, 2018

OK, I've added a dynamic default, using socket.getaddrinfo() if self.ip is not an IP address. Is that OK to run on startup, or should we hardcode a check against 'localhost' (and maybe localhost6 - I don't know if this is the same on all platforms).

allow = addr.is_loopback

if not allow:
self.log.warning("Blocking request with non-local 'Host' %s (%s)",

This comment has been minimized.

@minrk

minrk Jun 29, 2018

Member

Maybe add a reference to NotebookApp.allow_remote_access so people who see this message can take the appropriate action if they want to change it (e.g. localhost is proxied through another service)?

@minrk

This comment has been minimized.

Copy link
Member

minrk commented Jun 29, 2018

I realized this will require changes to the default jupyterhub configuration (I disable this behavior entirely in jupyterhub/jupyterhub#2015), which uses localhost by default for single-user servers, and it got me thinking that maybe the Host header isn't the right thing to use here. Maybe we should be checking the direct ip of the connection? That way, if it's being proxied the notebook is relinquishing responsibility for this kind of check, and only checks that the connection comes directly from localhost, not what the connecting browser thinks is localhost.

I believe this is available in tornado as self.address[0]. What do you think?

@takluyver

This comment has been minimized.

Copy link
Member Author

takluyver commented Jun 29, 2018

As I understand it, checking the client IP doesn't give you any protection against DNS rebinding, because the connection is being made from localhost in that case. I believe the only way to check that is to look at the domain name that the browser thinks it's talking to.

Do we need any coordination to avoid problems for Jupyterhub? E.g. we could have this feature present but off by default for a while to give people a chance to get on a newer Jupyterhub before the check is used.

@minrk

This comment has been minimized.

Copy link
Member

minrk commented Jul 2, 2018

checking the client IP doesn't give you any protection against DNS rebinding

Good point! I forgot that was the point of this whole thing.

Do we need any coordination to avoid problems for Jupyterhub?

It's not a disaster. I think we'll get 0.9.1 out this week, which will have this disabled. Worst case is users on earlier Hub versions with latest notebook can set this flag in their Spawner config.

@takluyver

This comment has been minimized.

Copy link
Member Author

takluyver commented Jul 2, 2018

OK, great. I think I'm happy with this, except that I don't know if it's reasonable to call socket.getaddrinfo() at startup if we're asked to listen on a hostname rather than an IP address.

@minrk

This comment has been minimized.

Copy link
Member

minrk commented Jul 3, 2018

The getaddrinfo works for me with socket.gethostname(), so I think it should be fine.

@minrk minrk merged commit b670fae into jupyter:master Jul 3, 2018

4 checks passed

codecov/patch 79.41% of diff hit (target 0%)
Details
codecov/project Absolute coverage decreased by -0.27% but relative coverage increased by +5.34% compared to b8b6633
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@minrk minrk added this to the 5.6 milestone Jul 3, 2018

@takluyver takluyver deleted the takluyver:check-host branch Jul 3, 2018

@takluyver takluyver referenced this pull request Jul 12, 2018

Merged

Upgrade mathjax #3751

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.