New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug-Security

Blocking:
issue 848123


Participants' hotlists:
x..


Sign in to add a comment
link

Issue 848535: Security: history.back() can be used to bypass multiple downloads restriction.

Reported by mea...@chromium.org, Jun 1 2018 Project Member

Issue description

Split from  bug 848123 . POC is download.html and back.html in that bug.

dtrainor: Can you please take a look?
 

Comment 1 by sheriffbot@chromium.org, Jun 1 2018

Project Member
Labels: Pri-2

Comment 2 by palmer@chromium.org, Jun 5 2018

Cc: luan.her...@hotmail.com
Components: UI>Browser>History
Labels: -Pri-2 M-69 OS-Fuchsia Pri-1
Is there any reason this would not also affect Fuchsia?

Changing to Pr-1 to avoid priority inversion: This bug blocks a Pri-1 Medium bug.

Comment 3 by palmer@chromium.org, Jun 5 2018

Labels: -M-69 M-68

Comment 4 by dtrainor@chromium.org, Jun 6 2018

Cc: dtrainor@chromium.org
Owner: qin...@chromium.org
Min can you take a look?  I thought we only reset the limiter for non-renderer initiated navigations.  I'm wondering if history.back() isn't included in that guard though.

Comment 5 by qin...@chromium.org, Jun 6 2018

can I get access to 848123?

Comment 6 by qin...@chromium.org, Jun 14 2018

Tested and seems that history.back() no longer be used to bypass multiple download restrictions, unless user give the permission to the destination address.

also using download attribute to copy and rename a local file seems also not working on trunk.  Downloading a local file will result in the same file name (unless there are conflicts) in the downloads dir, file name passed to download attribute will not be applied.

So looks like the attack chain in 848123 has broken.

Comment 7 by luan.her...@hotmail.com, Jun 14 2018

#6 - That's strange, I just tested on 69.0.3461.0 and the history.back() bypass is working. About copying and renaming local files, a change indeed was made in which broke the attack chain in m69 onwards.

Comment 8 by dtrainor@chromium.org, Jun 14 2018

I haven't had time today, but I'm going to try to regression test and find the offending CL.  @7 also mentioned that alt clicking (not simulating) still worked to allow a download which I need to look into as well.

Comment 9 by dtrainor@chromium.org, Jun 18 2018

I think we're technically supposed to catch this here: https://cs.chromium.org/chromium/src/chrome/browser/download/download_request_limiter.cc?sq=package:chromium&dr&g=0&l=143

I haven't looked, but my assumption is that we set the renderer-initiated flag when we create the navigation handle and history.back() restores that handle but doesn't update the flag or reflect that the new navigation was initiated from the renderer.

I don't think it makes sense to update the renderer-initiated flag but maybe we need something else to determine whether or not the navigation stack traversal was renderer initiated or not.

Comment 10 by qin...@chromium.org, Jun 20 2018

Ok, I think i misunderstood the issue. Content setting is still applied on history.back(), however, the TabDownloadState is reset every time on history.back(), which defaults the value to allow one download.

So history.back() can allow one download on the current page, but not multiple downloads due to the content settings unless user changes that. But user can use javascript to trigger multiple history.back() to download multiple files.

Comment 11 by qin...@chromium.org, Jul 3 2018

Cc: elawrence@chromium.org dcheng@chromium.org
adding some security folks.

dcheng@, elawrence@, i have a CL
https://chromium-review.googlesource.com/c/chromium/src/+/1108959 to fix this. Basically that CL will make TabDownloadState unchanged if this is a forward/backward navigation.
So if download is already requiring prompt on the current tab, then clicking on history back will not change the state.

Does that sounds like a reasonable solution? 

One drawback of the approach is that if user didn't download anything on domain A, then goes to domain B and download 1 item (which makes prompting needed for follow up downloads), then going back to A will require prompting for new download.  

A workaround is that once tab started requiring prompting, we can record all the domains we encountered (either through history forward/backward, or renderer initiated navigation) in a list, and once a domain outside the list is navigated, we reset the TabDownloadState.

Comment 12 by qin...@chromium.org, Jul 6 2018

Cc: xingliu@chromium.org

Comment 13 by bugdroid1@chromium.org, Jul 12 2018

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/01a6365651e0b1e9896b251411a18a4ab5a4306d

commit 01a6365651e0b1e9896b251411a18a4ab5a4306d
Author: Min Qin <qinmin@chromium.org>
Date: Thu Jul 12 00:57:49 2018

Don't reset TabDownloadState on history back/forward

Currently performing forward/backward on a tab will reset the TabDownloadState.
Which allows javascript code to do trigger multiple downloads.
This CL disables that behavior by not resetting the TabDownloadState on
forward/back.
It is still possible to reset the TabDownloadState through user gesture
or using browser initiated download.

BUG= 848535 

Change-Id: I7f9bf6e8fb759b4dcddf5ac0c214e8c6c9f48863
Reviewed-on: https://chromium-review.googlesource.com/1108959
Commit-Queue: Min Qin <qinmin@chromium.org>
Reviewed-by: Xing Liu <xingliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574437}
[modify] https://crrev.com/01a6365651e0b1e9896b251411a18a4ab5a4306d/chrome/browser/download/download_request_limiter.cc
[modify] https://crrev.com/01a6365651e0b1e9896b251411a18a4ab5a4306d/chrome/browser/download/download_request_limiter.h
[modify] https://crrev.com/01a6365651e0b1e9896b251411a18a4ab5a4306d/chrome/browser/download/download_request_limiter_unittest.cc

Comment 14 by mea...@chromium.org, Jul 27 2018

qinmin: Can we mark this as fixed?

Comment 15 by xingliu@chromium.org, Jul 27 2018

Status: Fixed (was: Assigned)

Comment 16 by sheriffbot@chromium.org, Jul 28 2018

Project Member
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify

Comment 17 by awhalley@google.com, Aug 16

Labels: -M-68 M-69 Release-0-M69

Comment 18 by awhalley@chromium.org, Sep 5

Labels: CVE-2018-16087 CVE_description-missing

Comment 19 by sheriffbot@chromium.org, Nov 3

Project Member
Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 20 by awhalley@chromium.org, Jan 4

Labels: -CVE_description-missing CVE_description-submitted

Sign in to add a comment