Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2958343002:
[Extensions] Change renderer-side web accessible resource determination (Closed)
Description[Extensions] Change renderer-side web accessible resource determination
The web accessible resource determination in the renderer has an
early-out for non-existent extensions. Unfortunately, this makes it
possible to determine whether a user has a given extension installed
through timing attacks - it takes longer to make a decision about
whether to allow a resource to load when the extension is installed than
when it isn't. This can be surprisingly accurate.
Rejigger the web accessible resource determination in a few ways. Most
importantly, keep a set of loaded extensions that have any accessible
resources, and check this (rather than the full extension set) for
whether to potentially allow the load. This way, an extension with no
accessible resources and an uninstalled extension should take the same
amount of time to reach a decision, which is the desired outcome.
BUG=709464
BUG=611420
Review-Url: https://codereview.chromium.org/2958343002
Cr-Commit-Position: refs/heads/master@{#485494}
Committed: https://chromium.googlesource.com/chromium/src/+/0e41fe8632d60a94200666e5eae0660bfdcd33e9
Patch Set 1 #Patch Set 2 : fix #Patch Set 3 : . #
Total comments: 28
Patch Set 4 : Alex's/Nasko's #Patch Set 5 : Revert ancestor check #Patch Set 6 : Expand comments #
Total comments: 6
Patch Set 7 : lazyboy's #
Total comments: 2
Patch Set 8 : s/web_accessible_ids/web_accessible_ids_ #
Messages
Total messages: 51 (38 generated)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== printing BUG= ========== to ========== [Extensions] Change renderer-side web accessible resource determination The web accessible resource determination in the renderer has an early-out for non-existent extensions. Unfortunately, this makes it possible to determine whether a user has a given extension installed through timing attacks - it takes longer to make a decision about whether to allow a resource to load when the extension is installed than when it isn't. This can be surprisingly accurate. Rejigger the web accessible resource determination in a few ways. Most importantly, keep a set of loaded extensions that have any accessible resources, and check this (rather than the full extension set) for whether to potentially allow the load. This way, an extension with no accessible resources and an uninstalled extension should take the same amount of time to reach a decision, which is the desired outcome. BUG=709464 BUG=611420 ==========
rdevlin.cronin@chromium.org changed reviewers: + alexmos@chromium.org, lazyboy@chromium.org, nasko@chromium.org
Hey all, mind taking a look? lazyboy@ - extensions nasko/alexmos - web accessible resources gurus There's a repro for this attached to crbug.com/709464, but since it's entirely based on timing attacks, I get the feeling it wouldn't fare too well on any bots, and otherwise I don't have a good way of testing this. But, the repro no longer repros. :)
https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... File chrome/renderer/extensions/resource_request_policy.cc (right): https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/resource_request_policy.cc:46: (extension.is_hosted_app() && !IconsInfo::GetIcons(&extension).empty())) { I see this call scattered a few places already. Should we abstract this behind HasWebAccessibleResources which can implement the hosted apps check and return true if icons are present? It feels fragile to always have this check all over the place. https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/resource_request_policy.cc:72: GURL page_origin = url::Origin(frame->Top()->GetSecurityOrigin()).GetURL(); Ok, I realize this is move only operation here. However, why are we dependent on the top level page origin?! I think we fixed browser side to check for any cross-origin ancestor and deny the navigation. Shouldn't we be doing the same here? https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/resource_request_policy.cc:79: // Empty origin (needed for some edge cases when we have empty origins). Empty origins or empty URLs? https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/resource_request_policy.cc:81: return true; In general, if you want timing neutral code, early returns are bad. Adding a boolean variable that holds the return value and updating it on each check, then returning it at the end will ensure all if branches are executed. It is also the case that if branches can shortcut based on conditions and each method you call can have different timing, but at least early returns aren't leaking timing data. https://codereview.chromium.org/2958343002/diff/40001/extensions/renderer/ext... File extensions/renderer/extensions_renderer_client.h (right): https://codereview.chromium.org/2958343002/diff/40001/extensions/renderer/ext... extensions/renderer/extensions_renderer_client.h:35: // Notifies the client when an extension is added or removed. Loaded/Unloaded? I can have an extension added to my profile but uncheck the enabled box, which will unload it, right? Also, those are invoked from OnLoaded/OnUnloaded, which will keep them consistent in terminology.
https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... File chrome/renderer/extensions/resource_request_policy.cc (right): https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/resource_request_policy.cc:85: if (frame_url.GetOrigin() == extension_origin || I know you're not changing this logic... but why do we not want frame->GetSecurityContext()->GetSecurityOrigin() here? What if an extension opens an about:blank popup and then scripts it to request a resource? Its frame_url will be "about:blank" but SecurityOrigin will be the extension's origin -- shouldn't this need the latter? OTOH, the document's URL might be needed to be able to handle sandboxed frames, so maybe we need to check both? https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/resource_request_policy.cc:99: if (!is_dev_tools && !web_accessible_ids_.count(extension_origin.host())) { nit: no { needed https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/resource_request_policy.cc:99: if (!is_dev_tools && !web_accessible_ids_.count(extension_origin.host())) { Maybe add a comment about why you needed to add this? https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/resource_request_policy.cc:100: return false; Just to clarify: does this help for an extension that does have some web-accessible resources? If it's not installed, we'll return here, and if it is, we'll exit after the extension lookup and the WAR check, which seems like it can still be timed. You said the repro no longer works, is that for all extensions or the non-WAR ones? There seem to be plenty of extensions with WARs on https://poc.tom.vg/extension-fingerprint/ https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/resource_request_policy.cc:105: DCHECK(extension); I'm curious if it's possible to hit this when |is_dev_tools| is true. Is the expectation that devtools should never make a chrome-extension:// request to a non-existing (devtools) extension?
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... File chrome/renderer/extensions/resource_request_policy.cc (right): https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/resource_request_policy.cc:46: (extension.is_hosted_app() && !IconsInfo::GetIcons(&extension).empty())) { On 2017/06/30 18:08:10, nasko (out until Jul 17th) wrote: > I see this call scattered a few places already. Should we abstract this behind > HasWebAccessibleResources which can implement the hosted apps check and return > true if icons are present? It feels fragile to always have this check all over > the place. Most places check a specific path to see if it's referencing an icon, so that we would be able to coalesce many calls (as far as I can see - maybe I'm missing some?). Additionally, I'm not sure we'd always want to include the icons in the web accessible resources check. I'd like to save that for a separate CL to keep this one targeted, but I've added a TODO. https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/resource_request_policy.cc:72: GURL page_origin = url::Origin(frame->Top()->GetSecurityOrigin()).GetURL(); On 2017/06/30 18:08:09, nasko (out until Jul 17th) wrote: > Ok, I realize this is move only operation here. However, why are we dependent on > the top level page origin?! I think we fixed browser side to check for any > cross-origin ancestor and deny the navigation. Shouldn't we be doing the same > here? Yes, we probably should. I tried this (see PS4), but the implementation was incomplete - specifically, we're missing the allowance for subresources to be included by an extension frame regardless of web accessible resources (so that e.g. frame.html can include frame.js even if only frame.html is specified). On the browser side, we can check the resource type - is there an analogous check here? Regardless, I'd have a slight preference for tackling this separately (mostly for revert reasons), so I've added a TODO here. But if it's a super simple addition, I could be persuaded to do it in this patch. :) https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/resource_request_policy.cc:79: // Empty origin (needed for some edge cases when we have empty origins). On 2017/06/30 18:08:09, nasko (out until Jul 17th) wrote: > Empty origins or empty URLs? This was copy-pasted, but code implies url, so changed it. https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/resource_request_policy.cc:81: return true; On 2017/06/30 18:08:09, nasko (out until Jul 17th) wrote: > In general, if you want timing neutral code, early returns are bad. Adding a > boolean variable that holds the return value and updating it on each check, then > returning it at the end will ensure all if branches are executed. It is also the > case that if branches can shortcut based on conditions and each method you call > can have different timing, but at least early returns aren't leaking timing > data. Except if we return true, it's because the navigation is allowed, so it doesn't matter (see also response to Alex's comment below). Because of that, I think the early returns here are fine. https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/resource_request_policy.cc:85: if (frame_url.GetOrigin() == extension_origin || On 2017/06/30 21:10:17, alexmos (OOO until Jul 5) wrote: > I know you're not changing this logic... but why do we not want > frame->GetSecurityContext()->GetSecurityOrigin() here? What if an extension > opens an about:blank popup and then scripts it to request a resource? Its > frame_url will be "about:blank" but SecurityOrigin will be the extension's > origin -- shouldn't this need the latter? > > OTOH, the document's URL might be needed to be able to handle sandboxed frames, > so maybe we need to check both? Sorry, you lost me at the sandboxed frame part - could you elaborate? https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/resource_request_policy.cc:99: if (!is_dev_tools && !web_accessible_ids_.count(extension_origin.host())) { On 2017/06/30 21:10:17, alexmos (OOO until Jul 5) wrote: > nit: no { needed Done. https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/resource_request_policy.cc:99: if (!is_dev_tools && !web_accessible_ids_.count(extension_origin.host())) { On 2017/06/30 21:10:18, alexmos (OOO until Jul 5) wrote: > Maybe add a comment about why you needed to add this? Done. https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/resource_request_policy.cc:100: return false; On 2017/06/30 21:10:18, alexmos (OOO until Jul 5) wrote: > Just to clarify: does this help for an extension that does have some > web-accessible resources? If it's not installed, we'll return here, and if it > is, we'll exit after the extension lookup and the WAR check, which seems like it > can still be timed. You said the repro no longer works, is that for all > extensions or the non-WAR ones? There seem to be plenty of extensions with WARs > on https://poc.tom.vg/extension-fingerprint/ If an extension has a web accessible resource, it is inherently detectable. There would be no reason to test an arbitrary, non-accessible file and compare timing results if you could instead test an accessible file and determine the result by checking if the frame is allowed. We don't really care about protecting the "allowed" case, because if the load's allowed, we already leaked whether or not the extension is installed. https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/resource_request_policy.cc:105: DCHECK(extension); On 2017/06/30 21:10:17, alexmos (OOO until Jul 5) wrote: > I'm curious if it's possible to hit this when |is_dev_tools| is true. Is the > expectation that devtools should never make a chrome-extension:// request to a > non-existing (devtools) extension? Umm... I'd hope not, but I'm not 100% sure. I've added a TODO and a graceful exit. In a followup, I'll add UMA or just remove the check (in which case we can monitor for crashes and revert that small change rather than this one). https://codereview.chromium.org/2958343002/diff/40001/extensions/renderer/ext... File extensions/renderer/extensions_renderer_client.h (right): https://codereview.chromium.org/2958343002/diff/40001/extensions/renderer/ext... extensions/renderer/extensions_renderer_client.h:35: // Notifies the client when an extension is added or removed. On 2017/06/30 18:08:10, nasko (out until Jul 17th) wrote: > Loaded/Unloaded? I can have an extension added to my profile but uncheck the > enabled box, which will unload it, right? Also, those are invoked from > OnLoaded/OnUnloaded, which will keep them consistent in terminology. I went back and forth on this, because this is in the renderer process, which only has knowledge of loaded extensions. But, sure.
LGTM https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... File chrome/renderer/extensions/resource_request_policy.cc (right): https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/resource_request_policy.cc:72: GURL page_origin = url::Origin(frame->Top()->GetSecurityOrigin()).GetURL(); On 2017/07/06 00:40:38, Devlin wrote: > On 2017/06/30 18:08:09, nasko (out until Jul 17th) wrote: > > Ok, I realize this is move only operation here. However, why are we dependent > on > > the top level page origin?! I think we fixed browser side to check for any > > cross-origin ancestor and deny the navigation. Shouldn't we be doing the same > > here? > > Yes, we probably should. I tried this (see PS4), but the implementation was > incomplete - specifically, we're missing the allowance for subresources to be > included by an extension frame regardless of web accessible resources (so that > e.g. frame.html can include frame.js even if only frame.html is specified). On > the browser side, we can check the resource type - is there an analogous check > here? > > Regardless, I'd have a slight preference for tackling this separately (mostly > for revert reasons), so I've added a TODO here. But if it's a super simple > addition, I could be persuaded to do it in this patch. :) (Pitching in here since Nasko's OOO for a while) I think we can check that from WebURLRequest (something similar to how is_navigational_request is set in RenderFrameImpl::WillSendRequest), but that will require additional plumbing into here. So a TODO is probably ok for this CL. https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/resource_request_policy.cc:85: if (frame_url.GetOrigin() == extension_origin || On 2017/07/06 00:40:38, Devlin wrote: > On 2017/06/30 21:10:17, alexmos (OOO until Jul 5) wrote: > > I know you're not changing this logic... but why do we not want > > frame->GetSecurityContext()->GetSecurityOrigin() here? What if an extension > > opens an about:blank popup and then scripts it to request a resource? Its > > frame_url will be "about:blank" but SecurityOrigin will be the extension's > > origin -- shouldn't this need the latter? > > > > OTOH, the document's URL might be needed to be able to handle sandboxed > frames, > > so maybe we need to check both? > > Sorry, you lost me at the sandboxed frame part - could you elaborate? If a frame is sandboxed via <iframe sandbox>, the frame's origin will be unique/"null" (unless it includes the allow-same-origin sandbox attribute). So, comparing frame->GetSecurityContext()->GetSecurityOrigin() to extension_origin won't work in that case. Blink code deals with this in other places (such as [1]) by comparing to the origin created from the document's URL instead, which stays valid for sandboxed frames, except that remote frames don't have that URL replicated, so it's always been a pain for us. :( So I guess I'm saying that: 1. To handle sandboxed frames correctly, the current approach of looking at frame_url.GetOrigin() is actually correct. 2. But to handle inherited origins in about:blank cases, you'd need to also look at frame->GetSecurityContext()->GetSecurityOrigin(). I realized my original about:blank popup example would probably be handled by "page_origin == extension_origin", since page_origin comes from SecurityOrigin, so to break the current check, we could try something like a page embedding an extension subframe, which embeds an about:blank grandchild frame and scripts it to load something. This isn't something for this CL but might be something to consider as part of your TODO to fix this check. [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum... https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/resource_request_policy.cc:99: if (!is_dev_tools && !web_accessible_ids_.count(extension_origin.host())) { On 2017/07/06 00:40:39, Devlin wrote: > On 2017/06/30 21:10:18, alexmos (OOO until Jul 5) wrote: > > Maybe add a comment about why you needed to add this? > > Done. Was it added somewhere where I missed it? :) I was thinking about something to explain why web_accessible_ids_ is maintained/checked so that someone doesn't reintroduce the timing issue later. https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/resource_request_policy.cc:100: return false; On 2017/07/06 00:40:39, Devlin wrote: > On 2017/06/30 21:10:18, alexmos (OOO until Jul 5) wrote: > > Just to clarify: does this help for an extension that does have some > > web-accessible resources? If it's not installed, we'll return here, and if it > > is, we'll exit after the extension lookup and the WAR check, which seems like > it > > can still be timed. You said the repro no longer works, is that for all > > extensions or the non-WAR ones? There seem to be plenty of extensions with > WARs > > on https://poc.tom.vg/extension-fingerprint/ > > If an extension has a web accessible resource, it is inherently detectable. > There would be no reason to test an arbitrary, non-accessible file and compare > timing results if you could instead test an accessible file and determine the > result by checking if the frame is allowed. We don't really care about > protecting the "allowed" case, because if the load's allowed, we already leaked > whether or not the extension is installed. Acknowledged. https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/resource_request_policy.cc:105: DCHECK(extension); On 2017/07/06 00:40:38, Devlin wrote: > On 2017/06/30 21:10:17, alexmos (OOO until Jul 5) wrote: > > I'm curious if it's possible to hit this when |is_dev_tools| is true. Is the > > expectation that devtools should never make a chrome-extension:// request to a > > non-existing (devtools) extension? > > Umm... I'd hope not, but I'm not 100% sure. I've added a TODO and a graceful > exit. In a followup, I'll add UMA or just remove the check (in which case we > can monitor for crashes and revert that small change rather than this one). Acknowledged.
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... File chrome/renderer/extensions/resource_request_policy.cc (right): https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/resource_request_policy.cc:72: GURL page_origin = url::Origin(frame->Top()->GetSecurityOrigin()).GetURL(); On 2017/07/06 18:20:06, alexmos wrote: > On 2017/07/06 00:40:38, Devlin wrote: > > On 2017/06/30 18:08:09, nasko (out until Jul 17th) wrote: > > > Ok, I realize this is move only operation here. However, why are we > dependent > > on > > > the top level page origin?! I think we fixed browser side to check for any > > > cross-origin ancestor and deny the navigation. Shouldn't we be doing the > same > > > here? > > > > Yes, we probably should. I tried this (see PS4), but the implementation was > > incomplete - specifically, we're missing the allowance for subresources to be > > included by an extension frame regardless of web accessible resources (so that > > e.g. frame.html can include frame.js even if only frame.html is specified). > On > > the browser side, we can check the resource type - is there an analogous check > > here? > > > > Regardless, I'd have a slight preference for tackling this separately (mostly > > for revert reasons), so I've added a TODO here. But if it's a super simple > > addition, I could be persuaded to do it in this patch. :) > > (Pitching in here since Nasko's OOO for a while) > > I think we can check that from WebURLRequest (something similar to how > is_navigational_request is set in RenderFrameImpl::WillSendRequest), but that > will require additional plumbing into here. So a TODO is probably ok for this > CL. Acknowledged; will leave the TODO and submit as-is for now. https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/resource_request_policy.cc:85: if (frame_url.GetOrigin() == extension_origin || On 2017/07/06 18:20:06, alexmos wrote: > On 2017/07/06 00:40:38, Devlin wrote: > > On 2017/06/30 21:10:17, alexmos (OOO until Jul 5) wrote: > > > I know you're not changing this logic... but why do we not want > > > frame->GetSecurityContext()->GetSecurityOrigin() here? What if an extension > > > opens an about:blank popup and then scripts it to request a resource? Its > > > frame_url will be "about:blank" but SecurityOrigin will be the extension's > > > origin -- shouldn't this need the latter? > > > > > > OTOH, the document's URL might be needed to be able to handle sandboxed > > frames, > > > so maybe we need to check both? > > > > Sorry, you lost me at the sandboxed frame part - could you elaborate? > > If a frame is sandboxed via <iframe sandbox>, the frame's origin will be > unique/"null" (unless it includes the allow-same-origin sandbox attribute). So, > comparing frame->GetSecurityContext()->GetSecurityOrigin() to extension_origin > won't work in that case. Blink code deals with this in other places (such as > [1]) by comparing to the origin created from the document's URL instead, which > stays valid for sandboxed frames, except that remote frames don't have that URL > replicated, so it's always been a pain for us. :( > > So I guess I'm saying that: > 1. To handle sandboxed frames correctly, the current approach of looking at > frame_url.GetOrigin() is actually correct. > 2. But to handle inherited origins in about:blank cases, you'd need to also look > at frame->GetSecurityContext()->GetSecurityOrigin(). > > I realized my original about:blank popup example would probably be handled by > "page_origin == extension_origin", since page_origin comes from SecurityOrigin, > so to break the current check, we could try something like a page embedding an > extension subframe, which embeds an about:blank grandchild frame and scripts it > to load something. > > This isn't something for this CL but might be something to consider as part of > your TODO to fix this check. > > [1] > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum... Thanks for the explanation! I've expanded the TODO, and will try to follow up in another CL. https://codereview.chromium.org/2958343002/diff/40001/chrome/renderer/extensi... chrome/renderer/extensions/resource_request_policy.cc:99: if (!is_dev_tools && !web_accessible_ids_.count(extension_origin.host())) { On 2017/07/06 18:20:06, alexmos wrote: > On 2017/07/06 00:40:39, Devlin wrote: > > On 2017/06/30 21:10:18, alexmos (OOO until Jul 5) wrote: > > > Maybe add a comment about why you needed to add this? > > > > Done. > > Was it added somewhere where I missed it? :) I was thinking about something to > explain why web_accessible_ids_ is maintained/checked so that someone doesn't > reintroduce the timing issue later. Whoops - it *was* added (https://codereview.chromium.org/2958343002/diff2/60001:80001/chrome/renderer/...). It got reverted when I removed the ancestor check that was failing in patch set 4. Reinstated.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lazyboy@, friendly ping when you get a chance. :)
https://codereview.chromium.org/2958343002/diff/100001/chrome/renderer/extens... File chrome/renderer/extensions/resource_request_policy.cc (right): https://codereview.chromium.org/2958343002/diff/100001/chrome/renderer/extens... chrome/renderer/extensions/resource_request_policy.cc:106: // Note: we check web_accessible_ids (rather than first looking up the nit: |web_accessible_ids_| I'd also expand this comment a bit. This "return false" is the code that's protecting us from exposing installed-ness info of extensions without WAR entries, right? Also note the fact that we are not as concerned about extensions with WAR entries. https://codereview.chromium.org/2958343002/diff/100001/chrome/renderer/extens... chrome/renderer/extensions/resource_request_policy.cc:110: return false; This used to show "Denying load of ... " message before (line 151) if extension was installed. Not sure if that's a good thing or bad thing. https://codereview.chromium.org/2958343002/diff/100001/chrome/renderer/extens... chrome/renderer/extensions/resource_request_policy.cc:114: if (is_dev_tools && !extension) { Can you fold this and if in line 124: if (is_dev_tools) { if (!extension) ... return true; else ... return true; } Then add DCHECK(extension); This devtools logic is hard to read.
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2958343002/diff/100001/chrome/renderer/extens... File chrome/renderer/extensions/resource_request_policy.cc (right): https://codereview.chromium.org/2958343002/diff/100001/chrome/renderer/extens... chrome/renderer/extensions/resource_request_policy.cc:106: // Note: we check web_accessible_ids (rather than first looking up the On 2017/07/10 22:36:39, lazyboy wrote: > nit: |web_accessible_ids_| > > I'd also expand this comment a bit. > This "return false" is the code that's protecting us from exposing > installed-ness info of extensions without WAR entries, right? > Also note the fact that we are not as concerned about extensions with WAR > entries. Done. https://codereview.chromium.org/2958343002/diff/100001/chrome/renderer/extens... chrome/renderer/extensions/resource_request_policy.cc:110: return false; On 2017/07/10 22:36:39, lazyboy wrote: > This used to show "Denying load of ... " message before (line 151) if extension > was installed. Not sure if that's a good thing or bad thing. It may have in some situations been useful, but adding it requires a difference in timing again, so I don't think it's worth keeping (since web accessible resources are documented elsewhere). https://codereview.chromium.org/2958343002/diff/100001/chrome/renderer/extens... chrome/renderer/extensions/resource_request_policy.cc:114: if (is_dev_tools && !extension) { On 2017/07/10 22:36:39, lazyboy wrote: > Can you fold this and if in line 124: > > if (is_dev_tools) { > if (!extension) ... return true; > else ... return true; > } > > Then add > > DCHECK(extension); > > This devtools logic is hard to read. Done. That is a bit cleaner; thanks!
lgtm https://codereview.chromium.org/2958343002/diff/120001/chrome/renderer/extens... File chrome/renderer/extensions/resource_request_policy.cc (right): https://codereview.chromium.org/2958343002/diff/120001/chrome/renderer/extens... chrome/renderer/extensions/resource_request_policy.cc:106: // Note: we check |web_accessible_ids| (rather than first looking up the |web_accessible_ids_|
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2958343002/diff/120001/chrome/renderer/extens... File chrome/renderer/extensions/resource_request_policy.cc (right): https://codereview.chromium.org/2958343002/diff/120001/chrome/renderer/extens... chrome/renderer/extensions/resource_request_policy.cc:106: // Note: we check |web_accessible_ids| (rather than first looking up the On 2017/07/10 23:13:36, lazyboy wrote: > |web_accessible_ids_| Whoops, done.
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alexmos@chromium.org, lazyboy@chromium.org Link to the patchset: https://codereview.chromium.org/2958343002/#ps140001 (title: "s/web_accessible_ids/web_accessible_ids_")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1499738772453770,
"parent_rev": "be6b241a17964378fb18c62efbb56de7b781a725", "commit_rev":
"0e41fe8632d60a94200666e5eae0660bfdcd33e9"}
Message was sent while issue was closed.
Description was changed from ========== [Extensions] Change renderer-side web accessible resource determination The web accessible resource determination in the renderer has an early-out for non-existent extensions. Unfortunately, this makes it possible to determine whether a user has a given extension installed through timing attacks - it takes longer to make a decision about whether to allow a resource to load when the extension is installed than when it isn't. This can be surprisingly accurate. Rejigger the web accessible resource determination in a few ways. Most importantly, keep a set of loaded extensions that have any accessible resources, and check this (rather than the full extension set) for whether to potentially allow the load. This way, an extension with no accessible resources and an uninstalled extension should take the same amount of time to reach a decision, which is the desired outcome. BUG=709464 BUG=611420 ========== to ========== [Extensions] Change renderer-side web accessible resource determination The web accessible resource determination in the renderer has an early-out for non-existent extensions. Unfortunately, this makes it possible to determine whether a user has a given extension installed through timing attacks - it takes longer to make a decision about whether to allow a resource to load when the extension is installed than when it isn't. This can be surprisingly accurate. Rejigger the web accessible resource determination in a few ways. Most importantly, keep a set of loaded extensions that have any accessible resources, and check this (rather than the full extension set) for whether to potentially allow the load. This way, an extension with no accessible resources and an uninstalled extension should take the same amount of time to reach a decision, which is the desired outcome. BUG=709464 BUG=611420 Review-Url: https://codereview.chromium.org/2958343002 Cr-Commit-Position: refs/heads/master@{#485494} Committed: https://chromium.googlesource.com/chromium/src/+/0e41fe8632d60a94200666e5eae0... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/0e41fe8632d60a94200666e5eae0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
