New issue
Advanced search Search tips

Issue 737648 link

Starred by 2 users

Issue metadata

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



Sign in to add a comment

Security: bypassing CORS of multipart images by ServiceWorker

Project Member Reported by hirosh...@chromium.org, Jun 28 2017

Issue description

VULNERABILITY DETAILS
Because MultipartImageResourceParser::ParseHeaders() doesn't copy
|was_fetched_via_service_worker_| and other SW-related members of
ResourceResponse from the actual response, the CORS checks that depends
on WasFetchedViaServiceWorker() is bypassed.
Particularly, CORS check is bypassed and thus the content of multipart
images can be obtained by canvas/getImageData() from the scripts on
any origin.

VERSION
Chrome Version: ToT (M-61) and probably all previous versions?
Operating System: All

REPRODUCTION CASE
https://codereview.chromium.org/2956343002

 
We can fix this issue by copying the SW-related flags in ParseHeaders().
I feel it's safer to copy the whole ResourceResponse except for the headers (See Patch Set 2 of https://codereview.chromium.org/2956343002) rather than copying URL/headers to avoid similar issues. (I expect we have to clear more fields though, e.g. multipart boundary)
However, this solution might also have risks to copy some fields unintentionally. (Probably "leaking" some fields from the original multipart response to the copied response doesn't have security impacts?)

yhirano@, WDTY?
Owner: hirosh...@chromium.org
Status: Started (was: Unconfirmed)
hiroshige@, could you add labels to reflect Security_Impact and Security_Severity? Thanks!
Labels: Security_Severity-Medium Security_Impact-Stable
Project Member

Comment 5 by sheriffbot@chromium.org, Jul 7 2017

Labels: M-60

Comment 6 by falken@chromium.org, Aug 10 2017

hiroshige: do you have any update on this? Is Pri=1, Status=Started still accurate?
Project Member

Comment 7 by sheriffbot@chromium.org, Sep 6 2017

Labels: -M-60 M-61

Comment 8 by raymes@chromium.org, Sep 20 2017

hiroshige: friendly security sheriff ping! This bug hasn't had an update in over 2 months. Could you please help provide an update or find another owner if needed? Thanks
S13n service worker/CORS/URLLoader are all moving parts now... if there is no quick fix for this bug and it's not super critical it might make sense to just wait for the new architecture to emerge.
Project Member

Comment 10 by sheriffbot@chromium.org, Oct 18 2017

Labels: -M-61 M-62
Labels: Hotlist-EnamelAndFriendsFixIt
Cc: mek@chromium.org kinuko@chromium.org
Labels: -OS-All OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows
#9: Is there an ETA for that S13n work?

+additional Service Worker friends, FYI.
It's our main project. ETA for S13nServiceWorker is 2018Q1-Q2. But I'm not sure things shaping up to change CORS and ResourceResponse in a way that will make this bug go away automatically.
Project Member

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

Labels: -M-62 M-63
Cc: horo@chromium.org
Project Member

Comment 16 by sheriffbot@chromium.org, Jan 25 2018

Labels: -M-63 M-64
I'm now realizing this looks similar to  issue 780435 .

Maybe we need to look at ResourceResponse::fetch_response_type similar to there, and check if the response is opaque?

As S13nServiceWorker is taking shape, it looks like it likely won't fix this alone. Sorry for leading us down that path.
hiroshige: do you think you'd be able to resume this work, maybe using fetch_response_type which is available now?
Owner: bashi@chromium.org
Passing to bashi@ who might be able to look at this. I think we can base a solution on hiroshige@'s patch/test. Most likely the only info we need to keep between "multiparts" is the FetchResponseType. Once the FetchResponseType is opaque in one part, the whole response should remain opaque. This is similar to  issue 780435  (CL: https://chromium-review.googlesource.com/#/c/828564/)  
Uploaded a WIP CL based on hiroshige@'s repro.
https://chromium-review.googlesource.com/c/chromium/src/+/907012

(I'm not sure how to set reviewers for private CLs on gerrit. I couldn't set reviewers)
Labels: -Hotlist-EnamelAndFriendsFixIt
Project Member

Comment 22 by bugdroid1@chromium.org, Feb 19 2018

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

commit d356217626d029ab76d4355e0fdfe5f744266a35
Author: Kenichi Ishibashi <bashi@chromium.org>
Date: Mon Feb 19 05:21:32 2018

Set WasFetchedViaServiceWorker for multipart image response

When creating a ResourceResponse from another ResourceResponse,
we need to copy WasFetchedViaServiceWorker and
ResponseTypeViaServiceWorker.

Bug:  737648 
Change-Id: I9405738221ef5232e235f9fc400f204b7b558596
Reviewed-on: https://chromium-review.googlesource.com/907012
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537579}
[add] https://crrev.com/d356217626d029ab76d4355e0fdfe5f744266a35/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/multipart-image.https.html
[add] https://crrev.com/d356217626d029ab76d4355e0fdfe5f744266a35/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/multipart-image-iframe.html
[add] https://crrev.com/d356217626d029ab76d4355e0fdfe5f744266a35/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/multipart-image-worker.js
[add] https://crrev.com/d356217626d029ab76d4355e0fdfe5f744266a35/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/resources/multipart-image.py
[modify] https://crrev.com/d356217626d029ab76d4355e0fdfe5f744266a35/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
[modify] https://crrev.com/d356217626d029ab76d4355e0fdfe5f744266a35/third_party/WebKit/Source/core/loader/resource/MultipartImageResourceParser.cpp

Comment 23 by bashi@chromium.org, Feb 19 2018

Status: Fixed (was: Started)
Project Member

Comment 24 by sheriffbot@chromium.org, Feb 20 2018

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: -M-64 M-66
Labels: Release-0-M66
Project Member

Comment 27 by sheriffbot@chromium.org, May 29 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

Sign in to add a comment