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

Issue 771709 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug-Security



Sign in to add a comment

PWA app installation can be requested from sandboxed page

Reported by s.h.h.n....@gmail.com, Oct 4 2017

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36

Steps to reproduce the problem:
1. Go to https://vuln.shhnjk.com/pwa.html
2. Now Go to https://vuln.shhnjk.com/pwa.php
3. Open Dev Tool > Application > Manifest > Add to homescreen 
4. PWA is successfully installed

What is the expected behavior?
PWA app installation could not be initialized from sandboxed page.

What went wrong?
Chrome allows PWA app installation from sandboxed page. If victim visited main website at least once, legit service worker are registered and would run inside scope.

Attack who has ability to inject content to any page inside SW's scope could initialize arbitrary app installation with only using link tag and manifest. This is also possible in CSP sandboxed page with maximum restriction (CSP: sandbox;).

Since this is patched in AppCache (https://bugs.chromium.org/p/chromium/issues/detail?id=696806), this should be patched too.

Did this work before? N/A 

Chrome version: 61.0.3163.100  Channel: stable
OS Version: OS X 10.13.0
Flash Version:
 
Cc: mlamouri@chromium.org
Components: Blink>AppManifest
Labels: Security_Severity-Medium Security_Impact-Stable
Owner: mgiuca@chromium.org
Status: Assigned (was: Unconfirmed)
mgiuca, mlamouri could you please take a look at this one too, or help with getting it assigned to the right person?


Cc: mgiuca@chromium.org
Components: -Blink>AppManifest Blink>ServiceWorker
Owner: ----
Status: Available (was: Assigned)
I don't really understand this and it seems more closely related to Service Workers than Manifest.

See also  Issue 771416  which is closely related (I think).
Project Member

Comment 3 by sheriffbot@chromium.org, Oct 5 2017

Labels: M-62
Project Member

Comment 4 by sheriffbot@chromium.org, Oct 5 2017

Labels: -Pri-2 Pri-1
As far as I understand things, this is related to https://w3c.github.io/manifest/, as implemented by /content/public/common/manifest.h

I think the proposal in this issue is that ManifestManagerHost::GetManifest should return an empty manifest if document->IsSandboxed returns true for the document in the Web Contents, akin to what we do here: https://codereview.chromium.org/2813123002



Owner: mkwst@chromium.org
Status: Assigned (was: Available)
mkwst, could you please take a look at this one too?
Project Member

Comment 7 by sheriffbot@chromium.org, Oct 19 2017

mkwst: 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 8 by mkwst@chromium.org, Oct 19 2017

Cc: -mlamouri@chromium.org mkwst@chromium.org
Owner: mlamouri@chromium.org
Apologies. I guess I conflated this with the discussion in  Issue 771416 , and didn't realize it was a separate issue.

I agree that we should not be fetching manifests for sandboxed documents. Moreover, we should not be allowing sandboxed documents to register service workers.

elawrence@'s suggestion to deal with this concrete issue makes sense to me in general, though I don't know the manifest code in particular (in particular, I don't know how ManifestManagerHost deals with subframes; there's a check in ManifestManagerHost::GetManifestManager that implies it only works for top level?)

mlamouri@: As that directory's OWNER, can you triage this to someone who knows that code and can adjust it accordingly? :)
Cc: mlamouri@chromium.org
Owner: mkwst@chromium.org
Indeed, Manifest should only be usable for top level. Though, it's unclear to me how this is related to the Manifest. The initial message is about using SW, not Manifest.

Comment 10 by mkwst@chromium.org, Oct 19 2017

Cc: -mlamouri@chromium.org
Owner: mlamouri@chromium.org
Ok. Reading through this again, I think I misunderstood on first read. Looking at this again, here's how I understand things:

1.  Navigating to https://vuln.shhnjk.com/pwa.html at the top-level installs a service worker. This is fine and expected.

2.  With the service worker in place, navigating to https://vuln.shhnjk.com/pwa.php at the top level (sandboxed via `Content-Security-Policy: sandbox`), which contains `<link rel="manifest" href="/manifest.txt">`, causes `manifest.txt` to be requested.

It does seem like we should avoid doing that.

The checks in ManifestManagerHost don't seem to be sufficient, because they don't seem to account for a sandboxed top-level frame. It might be the case that extending the check in `::GetManifestManager()` to hop through the WebContents to the FrameTreeNode to the FramePolicy to read the sandbox flags. It might make sense to do the check lower down the stack in Blink? I don't know this code well enough to make that decision, but it does seem like we should both follow the example of AppCache in neutering the markup-based loading mechanism for sandboxed documents, and add some language to the spec to cover this case.

Ponging this back to Mounir in the hopes that this explanation both makes sense and is reasonable. :)
Cc: mlamouri@chromium.org
Owner: dominickn@chromium.org
Is downloading the manifest the real issue or a symptom here? As far as I understand, it should be fine to download the manifest for reasons such as getting favicons. Arguably, installing the app could be an issue but the website would run with the same restrictions. If that's the concern, we could simply by-pass installation when CSP: sandbox is set.

However, I'm not sure I understand the treat here. Can someone elaborate?

Comment 12 by mkwst@chromium.org, Oct 20 2017

Labels: -Pri-1 -Security_Severity-Medium Security_Severity-Low Pri-2
Downloading the manifest isn't a problem in itself, but a user shouldn't be able to install an app from a sandboxed context. Sandboxing a page basically means you don't trust it and don't want it to be part of your origin: giving it the ability to persist on a user's device seems like a bad idea. I'd prefer to simply not download the manifest, as that means we don't have to look through all of the properties Chrome supports to determine which ones could be used in a sandboxed context, and which ones are safe.

I agree with your implication, though, that this isn't a medium-severity bug, as it requires a SW to be pre-installed and then requires user interaction to cause the downloaded manifest to do anything even potentially malicious. Dropping severity accordingly.
I'm not fully aware of CSP: sandbox but would it block the manifest download otherwise?

Regarding downloading the manifest, we download it on demand. because the page has a SW and a manifest, the Add to Homescreen code triggers the download. I'm not certain that pretending that there are no manifests would be the right solution here as, again, the manifest could be used for cosmetic reasons and it shouldn't be any different that a bunch of <meta> values in the HTML code.

Comment 14 by mkwst@chromium.org, Oct 20 2017

`Content-Security-Policy: sandbox` is basically the same as `<iframe sandbox>`, but opted-into by the page itself as opposed to its parent. This means, for example, that it can be applied to a top-level document, as we're doing here. It has the effect of flipping various sandboxing flags[1], which push the page into an opaque origin, turn off script, etc.

Regarding "on demand", how do you determine demand? I saw the file being downloaded without interacting with the page at all, but I did have devtools open. Is that enough?

Regarding cosmetic reasons, I'm reluctant to accept even those. If the page has been pushed into an opaque origin, it shouldn't be able to have an effect on the origin it was pushed out of. If we can guarantee that no state-altering side-effects will happen based on downloading/parsing/whatever, and that the user can't install the app if the manifest load was triggered from an opaque origin, great. It seems simpler to just reject the whole thing. :)

[1]: https://html.spec.whatwg.org/multipage/origin.html#sandboxing
>>> Downloading the manifest isn't a problem in itself, but a user shouldn't be able to install an app from a sandboxed context. Sandboxing a page basically means you don't trust it and don't want it to be part of your origin: giving it the ability to persist on a user's device seems like a bad idea. 

Could you please elaborate on why installing an app from an sandboxed context is a bad idea with regard to not wanting it to be part of your origin? I'm not clear on the exact threat here. :)

Re downloading the manifest: we will automatically download the manifest to determine whether or not a site is a PWA if the user has spent sufficient time interacting with a site. Triggering "add to home screen" from the menu will explicitly download the manifest if it hasn't already been fetched.

More broadly, if you truly want to be isolated from the rest of your origin, doesn't the CSP manifest-src directive allow a site to explicitly control where its manifest comes from?
Re #15: The typical use of a "sandbox" directive is to disavow a piece of content that is hosted by your domain (e.g. you have some untrusted user-generated content that you need to serve from your origin, but you want to prevent it from doing anything to abuse your origin's permissions). While it sounds like every site using a CSP: sandbox directive could further bolster that with a manifest-src: none directive, the general philosophy of the "sandbox" token is to lock down as much as possible by default, which improves security and deployment simplicity.
c#16: yes, but using an installed app would still retain the sandbox, and hence the isolation from the rest of the origin?

Again, I'm still not seeing how preventing sandboxed sites from being installed runs counter to the general sandbox philosophy - what am I missing? :)
I suppose it's persistence? You don't want to allow untrusted origin to control your site's experience. If attacker managed to let user install an app, then attacker controls user entry point to victim's site permanently. Whether attacker wants to spoof a login page, or just show an add. But now user needs to visit sandboxed pages every time they use an app.

I don't mind wont-fixing it, but it just seems bad idea to provide such possibility from browser point-of-view.
There are other persistence features in Chrome like bookmarks, or the often-visited pages shown on the NTP, or even omnibox suggestions which also don't take the sandbox attribute into account and provide a controlled entry point to a site.

My thinking is that if we want this sort of restriction in place, it should happen at the CSP layer (e.g. the existing manifest-src directive can explicitly forbid any manifest from being applied to an untrusted site). Restricting add to home screen feels arbitrary.

mkwst: do you have more thoughts on this?
ping mkwst: I'm planning on WontFixing this next week, please chime in if you have more thoughts.

Comment 21 by mkwst@chromium.org, Oct 27 2017

> There are other persistence features in Chrome like bookmarks, or the often-
> visited pages shown on the NTP, or even omnibox suggestions which also don't
> take the sandbox attribute into account and provide a controlled entry point
> to a site.

None of these provide an entry point to a site that's under the page's control. That is, if a user bookmarks a page, they're bookmarking _that page_. Likewise, the NTP gives you suggestions based on pages _you've visited_. There seems to be a relevant distinction when the site has control over the destination.

In general, it seems prudent to verify that the page making assertions about a given origin is actually associated with that origin. In the case of a sandbox moving a page into an opaque origin, that association is explicitly broken. Why would we accept the page's assertions?

> My thinking is that if we want this sort of restriction in place, it should
> happen at the CSP layer (e.g. the existing manifest-src directive can
> explicitly forbid any manifest from being applied to an untrusted site).

Sandboxing a page in some way obviates the need to control it in detail via CSP. That is, if you put a terrible monster in a box, and close the box and tape it shut, why would you also feel the need to put up a sign that says "Don't feed the monster?" You shut the box. Feeding the monster should be impossible.

(I have no idea how I came up with this _astounding_ metaphor. Sorry. ;) )

> Restricting add to home screen feels arbitrary.

We restrict more than add to homescreen. We take the page's origin away, we prevent script execution, etc. Restricting add to homescreen is a consequence of those restrictions, not an arbitrary new restriction. :)
mkwst and I synced offline, and determined that the core issue is that for PWA installation, we take the start URL of the (untrusted) manifest and use that in the homescreen shortcut.

An acceptable solution would be to simply ignore the manifest directives, and add a homescreen shortcut to the current URL. This reduces add to homescreen to be the equivalent of a bookmark shortcut, and also avoids the arbitrary-ness of disallowing add to homescreen completely for a site that you can still bookmark or get to from your history.
Nice! It'd be great if this behavior actually gets into the spec.

Also, I would love to see the restriction of manifest file to be same-origin. Sandboxed content would never get manifest if there is a same-origin restriction. And this I think is the main difference between SW/AppCache VS Web App Manifest.
Project Member

Comment 24 by sheriffbot@chromium.org, Dec 7 2017

Labels: -M-62 M-63
Proposed fix is to block manifest fetching from opaque (i.e. sandboxed) origins. I'll follow up with a PR for the web manifest spec.

CL is at https://crrev.com/c/866529
c#23: fetching the web manifest is already subject to a same-origin restriction, unless the manifest is served with the appropriate CORS headers. :)
#27: Responding with Access-Control-Allow-Origin header is a piece of cake. And you should be able to use data URL, which you don't need to have actual manifest file anywhere.

It's wise to only allow same-origin manifest file. This is what Worker, Service Worker, and even AppCache do.
c#28: the web app manifest spec explicitly permits manifests to be fetched cross-origin with the appropriate CORS headers / content security policy. mlamouri, do you have any comments on this?
Project Member

Comment 30 by bugdroid1@chromium.org, Jan 23 2018

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

commit ffac0ee4b8b00944e2ddf23f7f4f55daff3c117e
Author: Dominick Ng <dominickn@chromium.org>
Date: Tue Jan 23 01:47:58 2018

Fail the web app manifest fetch if the document is sandboxed.

This ensures that sandboxed pages are regarded as non-PWAs, and that
other features in the browser process which trust the web manifest do
not receive the manifest at all if the document itself cannot access the
manifest.

BUG= 771709 

Change-Id: Ifd4d00c2fccff8cc0e5e8d2457bd55b992b0a8f4
Reviewed-on: https://chromium-review.googlesource.com/866529
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531121}
[modify] https://crrev.com/ffac0ee4b8b00944e2ddf23f7f4f55daff3c117e/content/browser/manifest/manifest_browsertest.cc
[modify] https://crrev.com/ffac0ee4b8b00944e2ddf23f7f4f55daff3c117e/content/renderer/manifest/manifest_change_notifier.cc
[modify] https://crrev.com/ffac0ee4b8b00944e2ddf23f7f4f55daff3c117e/content/renderer/manifest/manifest_manager.cc
[modify] https://crrev.com/ffac0ee4b8b00944e2ddf23f7f4f55daff3c117e/content/renderer/manifest/manifest_manager.h
[modify] https://crrev.com/ffac0ee4b8b00944e2ddf23f7f4f55daff3c117e/content/renderer/manifest/manifest_uma_util.cc
[modify] https://crrev.com/ffac0ee4b8b00944e2ddf23f7f4f55daff3c117e/content/renderer/manifest/manifest_uma_util.h
[modify] https://crrev.com/ffac0ee4b8b00944e2ddf23f7f4f55daff3c117e/content/test/data/manifest/dynamic-manifest.html
[add] https://crrev.com/ffac0ee4b8b00944e2ddf23f7f4f55daff3c117e/content/test/data/manifest/sandboxed.html
[add] https://crrev.com/ffac0ee4b8b00944e2ddf23f7f4f55daff3c117e/content/test/data/manifest/sandboxed.html.mock-http-headers
[add] https://crrev.com/ffac0ee4b8b00944e2ddf23f7f4f55daff3c117e/content/test/data/manifest/script.js
[modify] https://crrev.com/ffac0ee4b8b00944e2ddf23f7f4f55daff3c117e/tools/metrics/histograms/enums.xml

Will wait until this hits Canary, and then request a merge to M64.
Labels: -M-63 M-65
Err, M65 that is. M64 is just going to stable channel now. :)
Components: -Blink>ServiceWorker Blink>AppManifest
Labels: Merge-Request-65
Requesting merge of #30 to M65. This addresses a security issue, and has been on Canary for a day without issue.
Cc: awhalley@chromium.org
+ awhalley@ (Security TPM) for M65 merge review
Project Member

Comment 35 by sheriffbot@chromium.org, Jan 24 2018

Labels: -Merge-Request-65 Hotlist-Merge-Approved Merge-Approved-65
Your change meets the bar and is auto-approved for M65. Please go ahead and merge the CL to branch 3325 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Pls merge your change to M65 branch ASAP so we can pick it up for next dev release. Thank you.
Project Member

Comment 37 by bugdroid1@chromium.org, Jan 24 2018

Labels: -merge-approved-65 merge-merged-3325
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c5c499969d298274bb2ffec2f1871a999aa60be0

commit c5c499969d298274bb2ffec2f1871a999aa60be0
Author: Dominick Ng <dominickn@chromium.org>
Date: Wed Jan 24 23:35:57 2018

Fail the web app manifest fetch if the document is sandboxed.

This ensures that sandboxed pages are regarded as non-PWAs, and that
other features in the browser process which trust the web manifest do
not receive the manifest at all if the document itself cannot access the
manifest.

BUG= 771709 

Change-Id: Ifd4d00c2fccff8cc0e5e8d2457bd55b992b0a8f4
Reviewed-on: https://chromium-review.googlesource.com/866529
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#531121}(cherry picked from commit ffac0ee4b8b00944e2ddf23f7f4f55daff3c117e)
Reviewed-on: https://chromium-review.googlesource.com/884921
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/branch-heads/3325@{#77}
Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369}
[modify] https://crrev.com/c5c499969d298274bb2ffec2f1871a999aa60be0/content/browser/manifest/manifest_browsertest.cc
[modify] https://crrev.com/c5c499969d298274bb2ffec2f1871a999aa60be0/content/renderer/manifest/manifest_change_notifier.cc
[modify] https://crrev.com/c5c499969d298274bb2ffec2f1871a999aa60be0/content/renderer/manifest/manifest_manager.cc
[modify] https://crrev.com/c5c499969d298274bb2ffec2f1871a999aa60be0/content/renderer/manifest/manifest_manager.h
[modify] https://crrev.com/c5c499969d298274bb2ffec2f1871a999aa60be0/content/renderer/manifest/manifest_uma_util.cc
[modify] https://crrev.com/c5c499969d298274bb2ffec2f1871a999aa60be0/content/renderer/manifest/manifest_uma_util.h
[modify] https://crrev.com/c5c499969d298274bb2ffec2f1871a999aa60be0/content/test/data/manifest/dynamic-manifest.html
[add] https://crrev.com/c5c499969d298274bb2ffec2f1871a999aa60be0/content/test/data/manifest/sandboxed.html
[add] https://crrev.com/c5c499969d298274bb2ffec2f1871a999aa60be0/content/test/data/manifest/sandboxed.html.mock-http-headers
[add] https://crrev.com/c5c499969d298274bb2ffec2f1871a999aa60be0/content/test/data/manifest/script.js
[modify] https://crrev.com/c5c499969d298274bb2ffec2f1871a999aa60be0/tools/metrics/histograms/enums.xml

Status: Fixed (was: Assigned)
Project Member

Comment 39 by sheriffbot@chromium.org, Feb 8 2018

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: reward-topanel
Labels: -reward-topanel reward-0
Hello! Thanks for the report. I'm afraid the VRP panel declined to reward for this bug.
Labels: Release-0-M65
Labels: CVE-2018-6083
Labels: CVE_description-missing
Project Member

Comment 45 by sheriffbot@chromium.org, May 8 2018

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
Labels: -CVE_description-missing CVE_description-submitted

Sign in to add a comment