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 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security



Sign in to add a comment
link

Issue 616429: Security: Saving WebPage with file: resources access SMB resources

Reported by gregory....@gmail.com, Jun 1 2016

Issue description

VULNERABILITY DETAILS
Saving WebPage with file: resources access/saves SMB resources. This can potentially expose the user to SMB Relay Attack (stealing credentials). Loading of file: resource is blocked when page is viewed over http/https, however, when saving the page file: URLs are allowed.

VERSION
Chrome Version: 51.0.2704.63 m (Stable)
Operating System: Windows (All)

REPRODUCTION CASE
1. Visit http://grpdmp.tk:27275/gchrome1/_SMB_Access.html
2. Save Page (Complete)
3. Saved SMB resource is typically saved to C:\Users\<Username>\Downloads\_SMB_Access_files ( In this case file://localhost/C$/windows/system32/calc.exe is saved to C:\Users\<Username>\Downloads\_SMB_Access_files\calc.exe )
 

Comment 1 by f...@chromium.org, Jun 2 2016

Components: UI>Browser>Offline
Labels: Security_Severity-Medium M-51 Security_Impact-Stable Pri-1
Owner: petewil@chromium.org
Status: Assigned (was: Unconfirmed)
Thank you for the report.

petewil@, can you please look into this?

Comment 2 by gregory....@gmail.com, Jun 13 2016

FYI - Related Issue 617215 [Bypass Download Protection on using file:// WebDav URL]

Comment 3 by sheriffbot@chromium.org, Jun 15 2016

Project Member
petewil: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

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

Comment 4 by petewil@chromium.org, Jun 15 2016

Status: Started (was: Assigned)
I am probably not the best owner for this, but since it is assigned to me, I'll take a look.

Comment 5 by petewil@chromium.org, Jun 16 2016

In render_frame_impl.cc, OnGetSavebleResourceLinks() uses a list of savable schemes to save resources by, and url::kFileScheme is in the list.  It seems that the list is not used elsewhere, so we could easily remove kFileScheme from the list.  However, I wonder if any scenarios might break, or if there is any reason we want to download files.

Comment 6 by gregory....@gmail.com, Jun 17 2016

(pasting the salient email contents I sent to Pete -- for future reference)

HTTP[S] resources when rendered in browser, do not allow loading of local resource schemes, however, when the user saves the complete page, these file:// resources get accessed. If the fetch is successful, it is written to disk.

This poses a few risks to the user:
#1 - Access to non-local resource through SMB is permitted through file: scheme. It exposes user to Potential SMB Relay Attack under specific scenarios.
 Ref http://www.darkreading.com/vulnerabilities---threats/new-smb-relay-attack-steals-user-credentials-over-internet/d/d-id/1321633
       https://www.blackhat.com/docs/us-15/materials/us-15-Brossard-SMBv2-Sharing-More-Than-Just-Your-Files.pdf
#2 - Exposing a User's Real IP-Address when chrome specific proxy is configured in the browser [Eg: using --proxy-server cmdline param].
       ( In case Chrome is configured to use system-wide proxy; accessing WebDav resources won't expose real IP; but accessing SMB resources will )
#3 - Landing a blacklisted binary on disk that bypasses the SafeBrowsing layer [ its a combination of  Issue 616424  and Issue 617215 ]
[a discussion in Issue 617215 indicated that a fix in SMB access issue *might* address it]

Access to SMB [local] resource can be demonstrated by saving the following URL
1. Visit http://grpdmp.tk/gchrome1/_SMB_Access.html
2. Save Page (Complete)
3. Saved SMB resource is typically saved to C:\Users\<Username>\Downloads\_SMB_Access_files ( In this case file://localhost/C$/windows/system32/calc.exe is saved to C:\Users\<Username>\Downloads\_SMB_Access_files\calc.exe )
[Please note that localhost can be replaced with any SMB network resource]

Access to WebDav [internet] resource can be demonstrated by saving the following URL.
1. Start "WebClient" windows services manually [See Issue 617215 for reference on why this is required]
2. Visit http://grpdmp.tk/gchrome1/1.html
3. Save Page (Complete)
4. Saved SMB resource is typically saved to C:\Users\<Username>\Downloads\1_files ( In this case file://grpdmp.tk/webdav/content.exe is saved to C:\Users\<Username>\Downloads\1_files\content.exe ). content.exe is the blacklisted malware test binary.
[ If Chrome was started with custom proxy (for all traffic), instead of using system proxy -- the above steps will expose User's real IP to the WebDav host ]

So, I have the following suggestions for Chromium:
1. Do not allow local schemes to be accessed when saving a page from http/https etc. ( following same policy as the renderer )
2. When directly access of file: URL with smb/webdav resource occurs [using OmniBar; or malicious extension (with no permissions) opens a new tab] .. To Address: Issue 617215 , then Either SafeBrowsing should subscribe and scan the downloaded content Or show some warning to the user that he/she is accessing a possible untrusted network resource.

Comment 7 by dcheng@chromium.org, Jun 17 2016

Cc: lukasza@chromium.org
+lukasza, who is the save page expert.

I think we should do this generically: if a resource is blocked, for whatever reason, we need to make sure the serialized page reflects that.

Comment 8 by lukasza@chromium.org, Jun 17 2016

Cc: -lukasza@chromium.org asanka@chromium.org yosin@chromium.org petewil@chromium.org rdsmith@chromium.org mkwst@chromium.org
Owner: lukasza@chromium.org
Status: Assigned (was: Started)
1. I should probably be the owner of this bug.  OTOH, I am heading on a really long vacation soon (coming back on July 15th).  Let ma also add to CC people who might have some ideas on how to fix this - yosin@ (for Blink HTML serialization code) and rdsmith@ + asanka@ (for c/browser/downloads).

2. dcheng@ is correct in observing that the impact is broader than just access to SMB resources.  For example once internet->intranet restrictions (https://crbug.com/590714, +mkwst@ as an FYI) are in place, we would also want to preserve them during Save-Page-As-Complete-Html operation.

3. I am not sure what is the best approach for solving this.

3.a. Only save resources from within the renderer (tracked by issue 158957).  One way to implement this might be to merge MHTML and CompleteHtml serializers (tracked by issue 328354).  OTOH, this seems tricky wrt preserving all data - I am guessing that some resources are only parsed by Blink and don't yet have corresponding serializers (css?);  additionally, some resources are handled outside of Blink and would need to be serialized by the corresponding plugins (pdf?).

3.b. The alternative approach would be to make sure that all resource requests made during Save-Page-As-Complete-HTML are performed with effective permissions of the renderer where the link came from.  This can be slightly tricky in case where the same link is used from 2 frames (one of which can have access, while the other doesn't).

3.c. We can also continue to process resource requests with user's authority, but verify in SavePackage::OnSavableResourceLinksResponse that savable resources reported by a frame are actually allowed for that frame.  This avoids the issue of deduplicating resources by link from 3b.  OTOH, it seems wrong to be duplicating the security checks between 1) SavePackage::OnSavableResourceLinksResponse and 2) the code that actually processes the resource requests (e.g. I am sure we would miss some checks [like SafeBrowsing?]).

4. It is not clear to me what is the urgency of doing "something" about this bug.  If we need to do something before M53 branch point, then maybe filtering URIs received in SavePackage::OnSavableResourceLinksResponse is one way to go (as proposed in #c6 and #c5).

Comment 9 by lukasza@chromium.org, Jun 17 2016

It seems that SaveFileManager::SaveLocalFile handles file: URIs itself (using base::CopyFile rather than handing off the URI to ResourceDispatcherHostImpl::Get()->BeginSaveFile).  I wonder if anything will break if I just yank out SaveFileManager::SaveLocalFile (i.e. unify code that handles SAVE_FILE_FROM_FILE and SAVE_FILE_FROM_NET).  And I wonder if this will also fix the security issue reported here.

Comment 10 by lukasza@chromium.org, Jun 17 2016

Yanking out SaveFileManager::SaveLocalFile still seems desirable, but is not sufficient to address the security issue here.  In particular, I see that ResourceDispatcherHostImpl::BeginSaveFile directly calls BeginRequestInternal and doesn't go through ShouldServiceRequest and/or ChildProcessSecurityPolicyImpl::CanRequestURL for authorization checks.

It is not clear to me whether ChildProcessSecurityPolicyImpl::CanRequestURL is sufficient here.  FWIW, calling it from ResourceDispatcherHostImpl::BeginSaveFile makes my browser test pass, so I'll go forward with this approach for now.

Comment 11 by lukasza@chromium.org, Jun 17 2016

BTW: the ChildProcessSecurityPolicyImpl::CanRequestURL is based on |child_id|.  The problem is that SavePackage always passes main frame's process id to SaveFileManager::SaveURL (in addition to passing main frame's routing_id and its render view routing id...).  Still - this will only be a concern once --site-per-process ships (AFAIK save-page feature is not considered a blocker for shipping --isolate-extensions).

Comment 12 by lukasza@chromium.org, Jun 18 2016

I published a draft of a fix at https://codereview.chromium.org/2075273002/

Comment 13 by lukasza@chromium.org, Jun 18 2016

In the draft of a fix I try to prevent access to an unauthorized resource in 2 scenarios: 1) when saving the webpage and 2) when opening the saved webpage.  AFAICT the fix works for all cases of scenario #1, but only for cases of scenario #2 where we rewrite resource links (so - only in case of links from html).  If the hostile link (i.e. to http://bob-s-intranet/give-all-money-to-eve.php) is embedded in css or other resource that we do not reserialize, then the resource request will be performed when the saved page is opened.

I don't see a good way to address this.  We do generate a mark-of-the-web in the saved html (see FrameSerializer::markOfTheWebDeclaration), but it is not clear how this could be used here (i.e. the saved page has to have access to file: resources that got saved into foo_files/, but at the same time [unlike other html files from local filesystem] it shouldn't have access to http://bob-s-intranet/give-all-money-to-eve.php [because mark-of-the-web] says that it came from eve-s-website.com.  To be honest, I think that right now we only write mark-of-the-web to saved pages, but never recognized one ourselves (when reading html files from the local filesystem).

Comment 14 by lukasza@chromium.org, Jun 20 2016

Another observation is that (in the proposed fix) protecting against requesting a restricted resource 1) works reasonably well while a website is being saved, 2) works only for non-compromised renderer when opening an already saved file.  In particular, a compromised renderer can forge a mark-of-the-web and/or include any links it pleases.  Right now the browser doesn't validate presence of the mark-of-the-web (in case of complete-html, in case of mhtml it could validate Content-Location header) and furthermore doesn't use the information from mark-of-the-web when opening the already saved file.

Given above, I am less sure if we should cripple legitimate scenarios, given that the protection gained only works assuming a non-compromised renderer.  (this comment obviously only applies to the open-already-saved-file attach vector;  we should still fix the no-unauthorized-resource-requests-while-saving scenario).

Comment 15 by lukasza@chromium.org, Jun 20 2016

Components: -UI>Browser>Offline UI>Browser>Downloads

Comment 16 by sheriffbot@chromium.org, Jul 5 2016

Project Member
lukasza: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

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

Comment 17 by rdsmith@chromium.org, Jul 7 2016

Status: Started (was: Assigned)
Setting status to started based on c#12.  However, note that Lukasz is on vacation at the moment, so this may not get fixed in the short-term time frame :-J.

Comment 18 by ta...@google.com, Jul 13 2016

Labels: OS-Windows

Comment 19 by ta...@google.com, Jul 13 2016

Labels: -OS-Windows OS-All

Comment 20 by sheriffbot@chromium.org, Jul 21 2016

Project Member
Labels: -M-51 M-52

Comment 21 by bugdroid1@chromium.org, Jul 27 2016

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

commit eff8e457298d01b437e4fd78194ad6de3c8d7ad6
Author: lukasza <lukasza@chromium.org>
Date: Wed Jul 27 21:03:26 2016

Resource requests from Save-Page-As should go through CanRequestURL checks.

This CL:

- Added checks to ResourceDispatcherHostImpl::BeginSaveFile to verify if
  the renderer process is authorized to access a given resource.

- Removed separate code path for file: URIs that used to be implemented
  in SaveFileManager::SaveLocalFile.  Avoiding a separate code path
  helps consolidate all authorization checks in one place.

BUG= 616429 

Review-Url: https://codereview.chromium.org/2075273002
Cr-Commit-Position: refs/heads/master@{#408235}

[modify] https://crrev.com/eff8e457298d01b437e4fd78194ad6de3c8d7ad6/chrome/browser/download/save_page_browsertest.cc
[modify] https://crrev.com/eff8e457298d01b437e4fd78194ad6de3c8d7ad6/chrome/test/data/save_page/frames-objects.htm
[add] https://crrev.com/eff8e457298d01b437e4fd78194ad6de3c8d7ad6/chrome/test/data/save_page/text.txt
[add] https://crrev.com/eff8e457298d01b437e4fd78194ad6de3c8d7ad6/chrome/test/data/save_page/unauthorized-access.htm
[modify] https://crrev.com/eff8e457298d01b437e4fd78194ad6de3c8d7ad6/content/browser/download/docs/save-page-as.md
[modify] https://crrev.com/eff8e457298d01b437e4fd78194ad6de3c8d7ad6/content/browser/download/save_file_manager.cc
[modify] https://crrev.com/eff8e457298d01b437e4fd78194ad6de3c8d7ad6/content/browser/download/save_file_resource_handler.cc
[modify] https://crrev.com/eff8e457298d01b437e4fd78194ad6de3c8d7ad6/content/browser/download/save_file_resource_handler.h
[modify] https://crrev.com/eff8e457298d01b437e4fd78194ad6de3c8d7ad6/content/browser/download/save_item.cc
[modify] https://crrev.com/eff8e457298d01b437e4fd78194ad6de3c8d7ad6/content/browser/download/save_item.h
[modify] https://crrev.com/eff8e457298d01b437e4fd78194ad6de3c8d7ad6/content/browser/download/save_package.cc
[modify] https://crrev.com/eff8e457298d01b437e4fd78194ad6de3c8d7ad6/content/browser/download/save_types.h
[modify] https://crrev.com/eff8e457298d01b437e4fd78194ad6de3c8d7ad6/content/browser/loader/resource_dispatcher_host_impl.cc

Comment 22 by lukasza@chromium.org, Jul 28 2016

Status: Fixed (was: Started)

Comment 23 by lukasza@chromium.org, Jul 28 2016

commit eff8e457298d01b437e4fd78194ad6de3c8d7ad6 was initially in 54.0.2810.0

Comment 24 by lukasza@chromium.org, Jul 28 2016

Labels: Merge-Request-53

Comment 25 by dimu@chromium.org, Jul 28 2016

Labels: -Merge-Request-53 Merge-Approved-53 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M53 (branch: 2785)

Comment 26 by bugdroid1@chromium.org, Jul 28 2016

Project Member
Labels: -merge-approved-53 merge-merged-2785
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0dbf3f74229c711ad91b21bd1ddc722580a7499e

commit 0dbf3f74229c711ad91b21bd1ddc722580a7499e
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Thu Jul 28 20:22:41 2016

Resource requests from Save-Page-As should go through CanRequestURL checks.

This CL:

- Added checks to ResourceDispatcherHostImpl::BeginSaveFile to verify if
  the renderer process is authorized to access a given resource.

- Removed separate code path for file: URIs that used to be implemented
  in SaveFileManager::SaveLocalFile.  Avoiding a separate code path
  helps consolidate all authorization checks in one place.

BUG= 616429 

Review-Url: https://codereview.chromium.org/2075273002
Cr-Commit-Position: refs/heads/master@{#408235}
(cherry picked from commit eff8e457298d01b437e4fd78194ad6de3c8d7ad6)

Review URL: https://codereview.chromium.org/2187353004 .

Cr-Commit-Position: refs/branch-heads/2785@{#394}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/0dbf3f74229c711ad91b21bd1ddc722580a7499e/chrome/browser/download/save_page_browsertest.cc
[modify] https://crrev.com/0dbf3f74229c711ad91b21bd1ddc722580a7499e/chrome/test/data/save_page/frames-objects.htm
[add] https://crrev.com/0dbf3f74229c711ad91b21bd1ddc722580a7499e/chrome/test/data/save_page/text.txt
[add] https://crrev.com/0dbf3f74229c711ad91b21bd1ddc722580a7499e/chrome/test/data/save_page/unauthorized-access.htm
[modify] https://crrev.com/0dbf3f74229c711ad91b21bd1ddc722580a7499e/content/browser/download/docs/save-page-as.md
[modify] https://crrev.com/0dbf3f74229c711ad91b21bd1ddc722580a7499e/content/browser/download/save_file_manager.cc
[modify] https://crrev.com/0dbf3f74229c711ad91b21bd1ddc722580a7499e/content/browser/download/save_file_resource_handler.cc
[modify] https://crrev.com/0dbf3f74229c711ad91b21bd1ddc722580a7499e/content/browser/download/save_file_resource_handler.h
[modify] https://crrev.com/0dbf3f74229c711ad91b21bd1ddc722580a7499e/content/browser/download/save_item.cc
[modify] https://crrev.com/0dbf3f74229c711ad91b21bd1ddc722580a7499e/content/browser/download/save_item.h
[modify] https://crrev.com/0dbf3f74229c711ad91b21bd1ddc722580a7499e/content/browser/download/save_package.cc
[modify] https://crrev.com/0dbf3f74229c711ad91b21bd1ddc722580a7499e/content/browser/download/save_types.h
[modify] https://crrev.com/0dbf3f74229c711ad91b21bd1ddc722580a7499e/content/browser/loader/resource_dispatcher_host_impl.cc

Comment 27 by sheriffbot@chromium.org, Jul 29 2016

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

Comment 28 by awhalley@chromium.org, Aug 10 2016

Labels: -Hotlist-Merge-Approved

Comment 29 by gregory....@gmail.com, Aug 18 2016

Does this bug report qualify for the VRP ?

Comment 30 by lukasza@chromium.org, Aug 18 2016

Labels: reward-topanel
Thanks for following up. I think it's eligible for a bounty but it isn't my decision. Adding reward-topanel which should put this in the queue.

Comment 31 by awhalley@chromium.org, Aug 30 2016

Labels: Release-0-M53

Comment 32 by awhalley@chromium.org, Sep 8 2016

Labels: -reward-topanel reward-unpaid reward-undefined

Comment 33 by awhalley@chromium.org, Sep 8 2016

Labels: -reward-undefined reward-1000

Comment 34 by awhalley@chromium.org, Sep 8 2016

yep, eligible for consideration and after consideration the panel awarded $1,000!

Comment 35 by gregory....@gmail.com, Sep 9 2016

Thank you for the reward :-)

Comment 36 by awhalley@chromium.org, Sep 14 2016

Labels: CVE-2016-5166

Comment 37 by awhalley@chromium.org, Sep 23 2016

Labels: -reward-unpaid reward-inprocess

Comment 38 by sheriffbot@chromium.org, Nov 4 2016

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 39 by awhalley@chromium.org, Apr 25 2018

Labels: CVE_description-submitted

Sign in to add a comment