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

Fix downloads in Chrome with xsrf token #6686

Merged
merged 2 commits into from Aug 9, 2019

Conversation

@tslaton
Copy link
Collaborator

@tslaton tslaton commented Jun 21, 2019

References

#6658

Code changes

I removed target=_blank from the href created for downloading files because it:

  • has no effect in Firefox or Safari, given the download attribute is present
  • is insufficient to allow downloads in Chrome, which still get a 403: Forbidden error

I added ?_xsrf=<token> and a specified the file name in the download attribute to fix the download issues in Chrome. Chrome does not automatically determine the file name when the attribute is blank.

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Jun 21, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 21, 2019

@minrk - are there any security ramifications of including the xsrf token in a link we are putting on the page, or making the xsrf token available from a function any extension can call?

Copy link
Member

@ian-r-rose ian-r-rose left a comment

Works well for me! We can revisit the xsrf approach later, but this fixes the behavior for the RC.
Edit: wrong PR.

@ian-r-rose ian-r-rose marked this pull request as ready for review Jun 21, 2019
@afshin
Copy link
Member

@afshin afshin commented Jun 21, 2019

@jasongrout:

Because we already put the token in the page configuration blob (<script id="jupyter-config-data" type="application/json">) which any hostile script could access ... and in PageConfig#getToken() which any extension could use I don't think this PR creates any additional attack surface area that we don't already tolerate.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 25, 2019

@afshin - I just checked, and the xsrf cookie has a different value than the pageconfig token.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 2, 2019

@minrk - are there any security ramifications of including the xsrf token in a link we are putting on the page, or making the xsrf token available from a function any extension can call?

ping @minrk again with the above question about how secure we should keep the xsrf token (see the changes in this PR for more context if you like)

@minrk
Copy link
Contributor

@minrk minrk commented Aug 2, 2019

Everything on the page should/needs to have access to the xsrf token. Leaking xsrf tokens in authenticated requests is generally not an issue. The xsrf token serves to verify that information the page can control (the URL, extra headers) matches information that the page does not control (cookies). A single xsrf token value isn't useful. What would be useful is if a malicious page could retrieve the value of the xsrf cookie for the origin domain. Placing it in the page content for authenticated requests should not be a problem because the cross-site request fetching the page directly would get it with a different token value than the one sent to your browser with a forged request.

A real cross-site vulnerability would be if the page contents with the xsrf token set by the current cookie could be retrieved and inspected by a cross-site request. Default cross-origin settings should prevent this.

Still, FWIW, we should ensure that the notebook server doesn't include the xsrf token in logs if given as a url param (it probably does). This isn't a vulnerability per se, but it's always good to avoid logging tokens.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 3, 2019

Still, FWIW, we should ensure that the notebook server doesn't include the xsrf token in logs if given as a url param (it probably does).

I just checked - the notebook server does log the xsrf token given in a GET request.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 3, 2019

Some more investigations:

It seems that:

experiment Chrome working FF working
include _xsrf query Works Works
no _xsrf query No, 403 Forbidden Works
no _xsrf query, rel=noreferrer No, 403 Forbidden No, 403 Forbidden (_xsrf cookie is transmitted, but referer header is not transmitted)

In the cases above where I get a 403, I get this error in the server logs:

[W 10:09:15.736 LabApp] Blocking request with no referer
[W 10:09:15.736 LabApp] 403 GET /files/Untitled.ipynb (::1): Blocking request from unknown origin

Interestingly, if I delete the download attribute, Chrome does send the referer header, and things work just fine even without the _xsrf query parameter, but of course the file replaces the lab page, which also doesn't work.

My guess is that the real problem here has to do with https://bugs.chromium.org/p/chromium/issues/detail?id=455987, but Chrome only seems to be omitting the referer header with a link that has the download attribute set.

From the above experiments, it seems that transmitting the _xsrf cookie but no referer header is not good enough. Only the _xsrf query parameter overrides a missing referer header.

@minrk
Copy link
Contributor

@minrk minrk commented Aug 7, 2019

Interestingly, if I delete the download attribute, Chrome does send the referer header, and things work just fine even without the _xsrf query parameter

That's why #6139 removed the download attribute, which was a fix for this very issue. #6546 reverted that and reintroduced the problem.

The comment in #6546 doesn't seem accurate, since the server sets content-disposition attachment, which triggers download instead of opening a new tab, at least in my testing. There's an explanation for this in #6139.

@minrk
Copy link
Contributor

@minrk minrk commented Aug 7, 2019

To be clear, it's still fine to use the download attribute and xsrf query. This may be a more robust fix anyway.

it seems that transmitting the _xsrf cookie but no referer header is not good enough

The xsrf functionality requires the xsrf token to be sent twice with each request: in the cookie (browser controls) and via query parameter or extra X-XSRFToken header. For ajax, we use the xsrf header, but for links the query parameter must be used. This double-send is a verification that the crafter of the request had access to the token value set by the server in the cookie (or placed elsewhere in page data). The cookie alone doesn't work because cross-origin requests are still sent with cookies for the target site. But since the cross-origin page doesn't have access to the cookies for the other site, it can't craft a request with the xsrf token stored in the cookies duplicated to another field. And if the site made its own request for the page containing the xsrf token, it would get a different value from the one sent by your browser with your cookies.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Aug 9, 2019

Thanks @minrk for the insights!

@blink1073 blink1073 merged commit 91eb753 into jupyterlab:master Aug 9, 2019
7 of 9 checks passed
@blink1073
Copy link
Member

@blink1073 blink1073 commented Aug 9, 2019

@meeseeksdev backport to 1.0.x

meeseeksmachine pushed a commit to meeseeksmachine/jupyterlab that referenced this issue Aug 9, 2019
blink1073 added a commit that referenced this issue Aug 9, 2019
…6-on-1.0.x

Backport PR #6686 on branch 1.0.x (Fix downloads in Chrome with xsrf token)
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Aug 9, 2019

To be clear, it's still fine to use the download attribute and xsrf query. This may be a more robust fix anyway.

Thanks!

The xsrf functionality requires the xsrf token to be sent twice with each request: in the cookie (browser controls) and via query parameter or extra X-XSRFToken header. For ajax, we use the xsrf header, but for links the query parameter must be used. This double-send is a verification that the crafter of the request had access to the token value set by the server in the cookie (or placed elsewhere in page data). The cookie alone doesn't work because cross-origin requests are still sent with cookies for the target site. But since the cross-origin page doesn't have access to the cookies for the other site, it can't craft a request with the xsrf token stored in the cookies duplicated to another field. And if the site made its own request for the page containing the xsrf token, it would get a different value from the one sent by your browser with your cookies.

Makes sense! Thanks (again) for an explanation.

@lock lock bot locked as resolved and limited conversation to collaborators Sep 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants