Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
889323002:
Allow SW registration only if it's secure AND it's HTTP or HTTPS (Closed)
|
Created:
5 years, 10 months ago by kinuko Modified:
5 years, 10 months ago CC:
chromium-reviews, michaeln, jsbell+serviceworker_chromium.org, tzik, serviceworker-reviews, jam, nhiroki, darin-cc_chromium.org, horo+watch_chromium.org, kinuko+serviceworker, kinuko+watch, Kunihiko Sakamoto Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow SW registration only if it's secure AND it's HTTP or HTTPS
BUG=453982
TEST=tested manually
TEST=content_unittests:ServiceWorkerDispatcherHostTest.Register_FileSystem*
Committed: https://crrev.com/22394d843a6c36eb2e6d7bdf4fb8e7c4b7ae8d68
Cr-Commit-Position: refs/heads/master@{#314143}
Patch Set 1 #
Total comments: 8
Patch Set 2 : revised #Patch Set 3 : check unregister too #
Total comments: 6
Patch Set 4 : #Patch Set 5 : added simple tests #Patch Set 6 : remove LOG(ERROR) #
Messages
Total messages: 24 (6 generated)
kinuko@chromium.org changed reviewers: + falken@chromium.org, nhiroki@chromium.org
Does this look to be the right fix? Can someone review?
This looks like the right fix, the spec says SW should be HTTPS only with some exceptions like HTTP localhost possible for development. https://codereview.chromium.org/889323002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/889323002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_dispatcher_host.cc:298: BadMessageReceived(); With the new restriction, it's probably over-aggressive to kill the renderer now. The original idea was that Blink-side does the sanity checking first, so if it fails in Chrome we know the renderer has been compromised. I think we should change this to sending a RegistrationError. https://codereview.chromium.org/889323002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_dispatcher_host.cc:383: BadMessageReceived(); Same here https://codereview.chromium.org/889323002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_dispatcher_host.cc:453: BadMessageReceived(); Same here.
https://codereview.chromium.org/889323002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/889323002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_dispatcher_host.cc:55: bool OriginCanAccessServiceWorkers(const GURL& url) { This function is only used for document_url. Probably we should also check scope_url and script_url.
Fixed the patch and did manual testing. Currently the error message is terrible, suggestions are welcome (but as I wrote in the patch I think we can return better message in blink) PTAL? https://codereview.chromium.org/889323002/diff/1/content/browser/service_work... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/889323002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_dispatcher_host.cc:55: bool OriginCanAccessServiceWorkers(const GURL& url) { On 2015/02/02 06:14:27, nhiroki (slow) wrote: > This function is only used for document_url. Probably we should also check > scope_url and script_url. Done. https://codereview.chromium.org/889323002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_dispatcher_host.cc:298: BadMessageReceived(); On 2015/02/02 06:01:23, falken wrote: > With the new restriction, it's probably over-aggressive to kill the renderer > now. The original idea was that Blink-side does the sanity checking first, so if > it fails in Chrome we know the renderer has been compromised. I think we should > change this to sending a RegistrationError. I looked into this deeper, actually we do check if protocol's http or https for document url in the renderer too but we don't do so for script or pattern URL. For a band-aid I've made fixes in the browser side only and changed these RegistrationError (for easier merge). https://codereview.chromium.org/889323002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_dispatcher_host.cc:383: BadMessageReceived(); On 2015/02/02 06:01:23, falken wrote: > Same here Done. https://codereview.chromium.org/889323002/diff/1/content/browser/service_work... content/browser/service_worker/service_worker_dispatcher_host.cc:453: BadMessageReceived(); On 2015/02/02 06:01:23, falken wrote: > Same here. I assume this should be fine, as we check the scheme for document URL in the renderer.
LGTM after tests get fixed https://codereview.chromium.org/889323002/diff/40001/content/browser/service_... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/889323002/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_dispatcher_host.cc:38: "The URL is not supported."; Ideally we should show more descriptive message specialized for each case as falken@ has recently worked (https://codereview.chromium.org/845313004/), but I think it's ok to keep as is in order to merge easier. https://codereview.chromium.org/889323002/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_dispatcher_host.cc:88: OriginCanAccessServiceWorkers(document_url); Can we also check |given_document_url| so that we can reject an invalid URL sent from a compromised renderer? "return ... && OriginCanAccessServiceWorkers(given_document_url);"
I'm a bit confused, if we check Blink-side for document_url is HTTP or HTTPS, wouldn't AllOriginsMatch fail if script_url or scope_url is not? https://codereview.chromium.org/889323002/diff/40001/content/browser/service_... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/889323002/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_dispatcher_host.cc:70: OriginCanAccessServiceWorkers(pattern) && Actually, doesn't AllOriginsMatch mean you only need to call OriginCanAccessServiceWorkers on one of the args? https://codereview.chromium.org/889323002/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_dispatcher_host.cc:397: base::ASCIIToUTF16(kServiceWorkerRegisterErrorPrefix) + kServiceWorkerUnregisterErrorPrefix
On 2015/02/02 08:44:04, falken wrote: > I'm a bit confused, if we check Blink-side for document_url is HTTP or HTTPS, > wouldn't AllOriginsMatch fail if script_url or scope_url is not? I think we were assuming that should fail, and that's where this subtle bug is introduced. GURL 'understands' how to interpret FileSystem URL, and uses its inner URL for most operations, so, say, calling GURL::GetOrigin() for filesystem:https://foo/ returns https://foo/, and AllOriginsMatch() for https://foo/ and filesystem:https://foo/ returns true.
Thanks! https://codereview.chromium.org/889323002/diff/40001/content/browser/service_... File content/browser/service_worker/service_worker_dispatcher_host.cc (right): https://codereview.chromium.org/889323002/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_dispatcher_host.cc:88: OriginCanAccessServiceWorkers(document_url); On 2015/02/02 08:37:43, nhiroki (slow) wrote: > Can we also check |given_document_url| so that we can reject an invalid URL sent > from a compromised renderer? > > "return ... && OriginCanAccessServiceWorkers(given_document_url);" Done. https://codereview.chromium.org/889323002/diff/40001/content/browser/service_... content/browser/service_worker/service_worker_dispatcher_host.cc:397: base::ASCIIToUTF16(kServiceWorkerRegisterErrorPrefix) + On 2015/02/02 08:44:04, falken wrote: > kServiceWorkerUnregisterErrorPrefix Done. Also fixed the error msg type (RegistrationError -> UnregistrationError)
Added a simple unittest too.
The CQ bit was checked by kinuko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/889323002/80001
The CQ bit was unchecked by horo@chromium.org
On 2015/02/02 11:22:20, horo wrote: > The CQ bit was unchecked by mailto:horo@chromium.org Oops, the patch in the CQ included debugging LOG(ERROR) output... I'll copy the patch and upload a new one unless kinuko@ comes back online and wants to do it.
On 2015/02/02 11:25:53, falken wrote: > On 2015/02/02 11:22:20, horo wrote: > > The CQ bit was unchecked by mailto:horo@chromium.org > > Oops, the patch in the CQ included debugging LOG(ERROR) output... > > I'll copy the patch and upload a new one unless kinuko@ comes back online and > wants to do it. :( Thanks for noticing, I can reupload if you haven't done so yet.
LGTM
The CQ bit was checked by kinuko@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/889323002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_gn_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gn_...)
The CQ bit was checked by nhiroki@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/889323002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/22394d843a6c36eb2e6d7bdf4fb8e7c4b7ae8d68 Cr-Commit-Position: refs/heads/master@{#314143} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
