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

Consider blocking downloads from a sandboxed `<iframe>`. #3236

Open
mikewest opened this Issue Nov 17, 2017 · 17 comments

Comments

8 participants
@mikewest
Copy link
Member

mikewest commented Nov 17, 2017

Blast from the past: https://lists.w3.org/Archives/Public/public-whatwg-archive/2013Feb/0010.html that came to mind because someone filed another feature request for it against Chrome (duped against crbug #539938).

It's currently possible to force a download by serving a file with a
"Content-Disposition: attachment; filename=..." header. Notably, this
mechanism can be used to download a file with minimal user interaction by
including the resource to be downloaded in an IFrame. This holds even for
sandboxed IFrames, as demonstrated by
http://lcamtuf.coredump.cx/sandboxed.html (clicking that link will download
a file, fair warning).

It seems consistent with the general thought behind the sandbox attribute
that it should control downloads as well as the bits it already locks down.
I'd propose adjusting the spec to include a sandboxed downloads flag,
which, when present, would block all downloads from inside the frame (or,
perhaps only require user confirmation?). This restriction could be lifted
via an 'allow-downloads' keyword, if present in the sandbox attribute's
token list.

Would folks in @whatwg/security be on board with this kind of change?

@mikewest

This comment has been minimized.

Copy link
Member Author

mikewest commented Nov 17, 2017

(And also folks who should probably be in that group, like @dveditz, @johnwilander, @patrickkettner, @bzbarsky, @ckerschb, @arturjanc, @dbates-wk)

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Nov 17, 2017

(I've invited those in your list that were not already invited to join the team. For anyone curious, the way we use teams is documented here: https://github.com/whatwg/meta/blob/master/GITHUB-TEAMS.md.)

@bzbarsky

This comment has been minimized.

Copy link
Collaborator

bzbarsky commented Nov 17, 2017

Seems plausible to me, especially if we can get some telemetry on how often this is used in practice right now. I expect not too much.

@devd

This comment has been minimized.

Copy link

devd commented Nov 17, 2017

I am worried about breaking existing clients during the transition. But maybe the telemetry can say how often its being used. I also think we could mitigate this risk by saying "allow downloads only if allow-top-navigation or allow-popups is set". If someone's setting those two flags, not allowing downloads seems weird. I dislike how the list of keywords for sandbox keeps growing.

@lubin2010

This comment has been minimized.

Copy link

lubin2010 commented Nov 17, 2017

Yes, I agree that we need to collect some telemetry first.

"allow downloads only if allow-top-navigation or allow-popups is set"

@devd One concern is that malicious ads (in sandbox usually with 'allow-popups' to allow new pages to be opened) are automatically downloading say, APKs on user device, and it would not be able to block such behaviors.

@devd

This comment has been minimized.

Copy link

devd commented Nov 17, 2017

I forget the spec ... does the popup inherit the sandbox? If not, can't the popup allow the download?

@bzbarsky

This comment has been minimized.

Copy link
Collaborator

bzbarsky commented Nov 17, 2017

I forget the spec ... does the popup inherit the sandbox?

Yes, unless you set allow-popups-to-escape-sandbox.

@mikewest

This comment has been minimized.

Copy link
Member Author

mikewest commented Nov 17, 2017

I also think we could mitigate this risk by saying "allow downloads only if allow-top-navigation or allow-popups is set".

  1. I'd prefer this be controlled by a clear and distinct mechanism. allow-downloads is simpler to explain and to use.
  2. It's not clear to me why "can download" should imply "can navigate". I just want to construct a blob from some data I have lying around and hand it to the user for safekeeping, it seems better to do so without also giving that context the ability to navigate (e.g. just sandbox="allow-download")
@lubin2010

This comment has been minimized.

Copy link

lubin2010 commented Nov 17, 2017

I forget the spec ... does the popup inherit the sandbox? If not, can't the popup allow the download?

As @bzbarsky said, the popup inherits the sandbox unless you set allow-popups-to-escape-sandbox. But auto popup would be blocked by popup blockers that most browsers have, while auto download won't. So popups are guarded by user gestures (interactions), but not downloads.

@xyaoinum

This comment has been minimized.

Copy link

xyaoinum commented Dec 19, 2018

Hi folks, I’m looking to spec out the details and add that to Chrome. Here’s my proposal:

We are targeting to prevent only downloads that doesn’t involve a user gesture, which include synthetic <a download> link clicks, as well as navigations to non-web-renderable content without user gesture. And the click or navigation occurs in a sandboxed browsing context with download flag. “allow-downloads” can be used to turn off this restriction.

This implies: Alt-Click, Drag-And-Drop, Context Menu triggered download won’t be blocked. Top-level-navigation to a download won’t be blocked.

The reason to allow top level navigation to a download is: 1) it’s a clear mechanism to spec out, as this is implied by the fact that top navigation doesn't inherit the sandbox; on the other hand, popup to a download will be blocked as it inherit the sandbox. 2) sandbox already disallow top-level-navigation, and if the client opt in, it makes sense to trust it regardless whether it results in a download or not.

I'm happy to see what people are thinking.

@johnwilander

This comment has been minimized.

Copy link

johnwilander commented Dec 19, 2018

We use “-by-user-activation” when that’s a requirement for the capability. Should we say “allow-download-without-user-activation” here?

@xyaoinum

This comment has been minimized.

Copy link

xyaoinum commented Dec 20, 2018

We use “-by-user-activation” when that’s a requirement for the capability. Should we say “allow-download-without-user-activation” here?

sounds reasonable to me

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Dec 20, 2018

Hmm, aren't all sanbbox allow-X flags implicitly "without user activation"? Is there a reason to be more explicit with this one?

@xyaoinum

This comment has been minimized.

Copy link

xyaoinum commented Dec 20, 2018

Hmm, aren't all sandbox allow-X flags implicitly "without user activation"? Is there a reason to be more explicit with this one?

I looked at the spec, the only flags involving the notion of user activation is:

  • "top-level navigation without user activation flag", suppressed by "allow-top-navigation" keyword.
  • "top-level navigation with user activation flag", suppressed by either "allow-top-navigation" or "allow-top-navigation-by-user-activation" keyword.

If we single out the 1st one, the "allow" keyword is implicitly "without user activation", but looking them together it's not necessarily so. But it does suggest another option which is to use 2 flags and keywords in the same way as "top-level navigation" does.

Here are some options and my thoughts:

  1. "downloads without user activation flag", suppressed by "allow-downloads" keyword.
    -- Simple. Conform to existing practice if we can assume "allow" is implicitly "without user activation", which is questionable.
  2. "downloads without user activation flag", suppressed by "allow-downloads-without-user-activation" keyword.
    -- More clear on what it controls. But by just looking at the keyword without seeing the restriction, it tends to imply it still won't allow download with user activation, although restriction is only on "without activation".
  3. "downloads without user activation flag", suppressed by "allow-downloads" keyword.
    "downloads with user activation flag", suppressed by either "allow-downloads" or allow-downloads-by-user-activation"" keyword.
    -- Most flexible. Conform to existing practice. At the cost of growing flags / keywords by 2, and more breakage to the web compared with 1) and 2).

I personally is in favor of 1) for simplicity, but all options make sense to me. Happy to know what other people think.

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Dec 20, 2018

I meant all the others. I.e. we say allow-scripts, not allow-scripts-without-user-activation. Etc. We don't involve the concept of user activation (even in the flag name) except in the one case of top navigation, where we have both and need to contrast the two.

@xyaoinum

This comment has been minimized.

Copy link

xyaoinum commented Dec 20, 2018

I meant all the others. I.e. we say allow-scripts, not allow-scripts-without-user-activation. Etc. We don't involve the concept of user activation (even in the flag name) except in the one case of top navigation, where we have both and need to contrast the two.

My proposal of the restriction was targeting at just to prevent download without user activation (the definition of that is either the navigation-to-download is without user activation, or <a download> click is synthetic), and this restriction is different from all the others. Of course this is also debatable - maybe we can prevent download irrespective of user activation. In that case, "allow-downloads" makes total sense.

@domenic

This comment has been minimized.

Copy link
Member

domenic commented Dec 20, 2018

Oh, I see. Sorry, I completely missed that key point from your original post... somehow. In that case I think including -without-user-activation makes perfect sense, and I apologize for wasting time on the back and forth.

aarongable pushed a commit to chromium/chromium that referenced this issue Jan 25, 2019

Only block download in sandbox without user activation
Follow the proposal discussed in whatwg/html#3236.

Bug: 539938
Change-Id: I1841eb1f3c1f121586eed5a08e7e0e9f94ec4430
Reviewed-on: https://chromium-review.googlesource.com/c/1413377
Commit-Queue: Yao Xiao <yaoxia@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#625878}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment