Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1669723002:
NTP: don't allow navigateContentWindow to navigate where it pleases. (Closed)
|
Created:
4 years, 10 months ago by dcheng Modified:
4 years, 10 months ago Reviewers:
jochen (gone - plz use gerrit), Mathieu, samarth, Marc Treib, Nico, nasko, Robert Sesek, Charlie Reis, Jered CC:
chromium-apps-reviews_chromium.org, chromium-reviews, David Black, dhollowa+watch_chromium.org, donnd+watch_chromium.org, dougw+watch_chromium.org, extensions-reviews_chromium.org, jfweitz+watch_chromium.org, kmadhusu, melevin+watch_chromium.org, nasko, samarth, skanuj+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNTP: don't allow navigateContentWindow to navigate where it pleases.
BUG=509313
Committed: https://crrev.com/d523a41aed4e321d4c8197b5cccb73be23c8dcc2
Cr-Commit-Position: refs/heads/master@{#373598}
Patch Set 1 #Patch Set 2 : Check in browser ^_^ #Patch Set 3 : Move all checks into InstantService. #Patch Set 4 : Some tests #Patch Set 5 : Add a null check #Patch Set 6 : Add missing _ #
Total comments: 2
Patch Set 7 : Use constants for schemes. #Patch Set 8 : Let it navigate more places it pleases. #Patch Set 9 : -IWYU #
Total comments: 8
Patch Set 10 : Comments #
Total comments: 2
Patch Set 11 : Better comments #
Messages
Total messages: 54 (17 generated)
dcheng@chromium.org changed reviewers: + jochen@chromium.org
+jochen for primary review. The owners of this code appear to be inactive in Chromium, but I've CCed them as well. Note that I would have preferred to implement this check in SearchIPCRouter::OnSearchBoxNavigate, but there's a lot of delegate and policy interfaces. So unfortunately, implementing the check in the concrete implementation of SearchIPCRouter::Delegate used by the real browser is the easiest way. I'm thinking about ways to add a unit test, but it's a bit tricky given how this is currently written. I'm guessing I'll end up friending a lot of random things.
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1669723002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1669723002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1669723002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1669723002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
treib@chromium.org changed reviewers: + treib@chromium.org
LGTM (but I don't own anything here :-/) https://codereview.chromium.org/1669723002/diff/100001/chrome/browser/search/... File chrome/browser/search/instant_service.cc (right): https://codereview.chromium.org/1669723002/diff/100001/chrome/browser/search/... chrome/browser/search/instant_service.cc:307: if (url.SchemeIs("chrome") || url.SchemeIs("chrome-extension")) nit: content::kChromeUIScheme and extensions::kExtensionScheme, respectively
On 2016/02/04 14:00:14, Marc Treib wrote: > LGTM (but I don't own anything here :-/) > > https://codereview.chromium.org/1669723002/diff/100001/chrome/browser/search/... > File chrome/browser/search/instant_service.cc (right): > > https://codereview.chromium.org/1669723002/diff/100001/chrome/browser/search/... > chrome/browser/search/instant_service.cc:307: if (url.SchemeIs("chrome") || > url.SchemeIs("chrome-extension")) > nit: content::kChromeUIScheme and extensions::kExtensionScheme, respectively Jochen has been OOO today (Thursday CET), so feel free to bring in another owner during the PST day.
dcheng@chromium.org changed reviewers: + creis@chromium.org, nasko@chromium.org, thakis@chromium.org
+thakis for OWNERS review +creis, +nasko as navigation experts I was looking at the scheme blacklist, and I'm wondering if there's a better way: there are other 'weird' schemes that I wouldn't expect to be valid navigation targets for 1993 (such as chrome-devtools://). Do we need to worry about other schemes here as well? Is there a more generic way to determine a dangerous scheme that will grant a renderer additional privileges? rsesek and I initially considered a whitelist approach, but were a little worried that this might not capture everything a user might have in their most visited items (e.g. I'm not sure if an ftp URL could end up in there). https://codereview.chromium.org/1669723002/diff/100001/chrome/browser/search/... File chrome/browser/search/instant_service.cc (right): https://codereview.chromium.org/1669723002/diff/100001/chrome/browser/search/... chrome/browser/search/instant_service.cc:307: if (url.SchemeIs("chrome") || url.SchemeIs("chrome-extension")) On 2016/02/04 at 14:00:14, Marc Treib wrote: > nit: content::kChromeUIScheme and extensions::kExtensionScheme, respectively Ah, thanks. Done.
The CQ bit was checked by dcheng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1669723002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1669723002/120001
On 2016/02/04 16:21:01, dcheng wrote: > I was looking at the scheme blacklist, and I'm wondering if there's a better > way: there are other 'weird' schemes that I wouldn't expect to be valid > navigation targets for 1993 (such as chrome-devtools://). Do we need to worry > about other schemes here as well? Is there a more generic way to determine a > dangerous scheme that will grant a renderer additional privileges? > > rsesek and I initially considered a whitelist approach, but were a little > worried that this might not capture everything a user might have in their most > visited items (e.g. I'm not sure if an ftp URL could end up in there). I was thinking about this more last night, and I think a whiltelist may be better than a blacklist. I don't think schemes other than http, https, file, and ftp should be allowed.
jered@chromium.org changed reviewers: + jered@chromium.org, mathp@chromium.org
+mathp will this break most likely? Given the immediate concerns this lgtm.
On 2016/02/04 17:05:55, Jered wrote: > +mathp will this break most likely? > > Given the immediate concerns this lgtm. Depends if the SuggestionsService is used. Since it's becoming the default soon for the server NTP (finkm@ and team can confirm), I'd say that it won't break it. If the current server side code ran on this then yes it would break (because |suggestions_items_| wouldn't know about the most likely items).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/02/04 16:35:15, Robert Sesek wrote: > On 2016/02/04 16:21:01, dcheng wrote: > > I was looking at the scheme blacklist, and I'm wondering if there's a better > > way: there are other 'weird' schemes that I wouldn't expect to be valid > > navigation targets for 1993 (such as chrome-devtools://). Do we need to worry > > about other schemes here as well? Is there a more generic way to determine a > > dangerous scheme that will grant a renderer additional privileges? > > > > rsesek and I initially considered a whitelist approach, but were a little > > worried that this might not capture everything a user might have in their most > > visited items (e.g. I'm not sure if an ftp URL could end up in there). > > I was thinking about this more last night, and I think a whiltelist may be > better than a blacklist. I don't think schemes other than http, https, file, and > ftp should be allowed. On one hand, I'd love to lock it down to a whitelist. ChildProcessSecurityPolicy::IsWebSafeScheme might be a good candidate. On the other hand, what restrictions do we have on the most visited and suggestion URLs? Can chrome-extension:// URLs show up there (e.g., if they're web accessible)? Can chrome:// URLs? (Hopefully not.) I don't want to introduce a regression where users can't click on those tiles, assuming that the tiles use this API.
On 2016/02/04 17:13:32, Charlie Reis wrote: > On 2016/02/04 16:35:15, Robert Sesek wrote: > > On 2016/02/04 16:21:01, dcheng wrote: > > > I was looking at the scheme blacklist, and I'm wondering if there's a better > > > way: there are other 'weird' schemes that I wouldn't expect to be valid > > > navigation targets for 1993 (such as chrome-devtools://). Do we need to > worry > > > about other schemes here as well? Is there a more generic way to determine a > > > dangerous scheme that will grant a renderer additional privileges? > > > > > > rsesek and I initially considered a whitelist approach, but were a little > > > worried that this might not capture everything a user might have in their > most > > > visited items (e.g. I'm not sure if an ftp URL could end up in there). > > > > I was thinking about this more last night, and I think a whiltelist may be > > better than a blacklist. I don't think schemes other than http, https, file, > and > > ftp should be allowed. > > On one hand, I'd love to lock it down to a whitelist. > ChildProcessSecurityPolicy::IsWebSafeScheme might be a good candidate. > > On the other hand, what restrictions do we have on the most visited and > suggestion URLs? Can chrome-extension:// URLs show up there (e.g., if they're > web accessible)? Can chrome:// URLs? (Hopefully not.) I don't want to > introduce a regression where users can't click on those tiles, assuming that the > tiles use this API. I've been trying to dig into that, but it's kind of a complicated subsystem. TopSites gets its data from the HistoryService, and so I'm not sure at what layer a scheme filter would be applied at. I agree that whatever whitelist we come up with should apply to both the frontend and backend.
On 2016/02/04 17:13:32, Charlie Reis wrote:
> On 2016/02/04 16:35:15, Robert Sesek wrote:
> > On 2016/02/04 16:21:01, dcheng wrote:
> > > I was looking at the scheme blacklist, and I'm wondering if there's a
better
> > > way: there are other 'weird' schemes that I wouldn't expect to be valid
> > > navigation targets for 1993 (such as chrome-devtools://). Do we need to
> worry
> > > about other schemes here as well? Is there a more generic way to determine
a
> > > dangerous scheme that will grant a renderer additional privileges?
> > >
> > > rsesek and I initially considered a whitelist approach, but were a little
> > > worried that this might not capture everything a user might have in their
> most
> > > visited items (e.g. I'm not sure if an ftp URL could end up in there).
> >
> > I was thinking about this more last night, and I think a whiltelist may be
> > better than a blacklist. I don't think schemes other than http, https, file,
> and
> > ftp should be allowed.
>
> On one hand, I'd love to lock it down to a whitelist.
> ChildProcessSecurityPolicy::IsWebSafeScheme might be a good candidate.
>
> On the other hand, what restrictions do we have on the most visited and
> suggestion URLs? Can chrome-extension:// URLs show up there (e.g., if they're
> web accessible)? Can chrome:// URLs? (Hopefully not.) I don't want to
> introduce a regression where users can't click on those tiles, assuming that
the
> tiles use this API.
For example, see this comment in search_ipc_router.h:
// Called when the page wants to navigate to |url|. Usually used by the
// page to navigate to privileged destinations (e.g. chrome:// URLs) or to
// navigate to URLs that are hidden from the page using Restricted IDs (rid
// in the API).
virtual void NavigateToURL(const GURL& url,
WindowOpenDisposition disposition,
bool is_most_visited_item_url) = 0;
It makes me sad that there are apparently no tests covering this behavior, if it
was actually introduced to support navigations to privileged URLs.
samarth@chromium.org changed reviewers: + samarth@chromium.org - rsesek@chromium.org
We could also make this check in the renderer process in searchbox_extension here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/se... Note that it has access to the most visited items: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/se... On one hand, that's where the check for javascript: scheme resides and it'd be better for these checks to be centralized. But I don't know if it's preferable to have these be in the browser process instead. Thanks, Samarth
On 2016/02/04 17:20:44, samarth wrote: > We could also make this check in the renderer process in searchbox_extension > here: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/se... > > Note that it has access to the most visited items: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/se... > > On one hand, that's where the check for javascript: scheme resides and it'd be > better for these checks to be centralized. But I don't know if it's preferable > to have these be in the browser process instead. > > Thanks, > Samarth Thanks for looking. We definitely want to enforce this in the browser process, though I'm not opposed to also having a check in the renderer process. Samarth, it looks like you added the comment about using this API for privileged URLs in https://codereview.chromium.org/11824050 (in instant_page.h at the time). Can you let us know why that was needed and what will break if we prevent it now?
On 2016/02/04 17:26:25, Charlie Reis wrote: > On 2016/02/04 17:20:44, samarth wrote: > > We could also make this check in the renderer process in searchbox_extension > > here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/se... > > > > Note that it has access to the most visited items: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/se... > > > > On one hand, that's where the check for javascript: scheme resides and it'd be > > better for these checks to be centralized. But I don't know if it's > preferable > > to have these be in the browser process instead. > > > > Thanks, > > Samarth > > Thanks for looking. We definitely want to enforce this in the browser process, > though I'm not opposed to also having a check in the renderer process. > > Samarth, it looks like you added the comment about using this API for privileged > URLs in https://codereview.chromium.org/11824050 (in instant_page.h at the > time). Can you let us know why that was needed and what will break if we > prevent it now? There's a long history behind NavigateContentWindow. In the beginning, this API was meant to be used both for most visited items as well as for suggestions in the HTML-based omnibox popup. The latter never launched. In the suggest case, you could definitely end up with chrome:// URLs in the suggestions and this API provided a way for the HTML omnibox popup to navigate to those URLs when the user selected those suggestions. The comment here probably dates to that use case. At this point, we only need NCW for most visited/most likely so it can much more limited. Even better, each most visited item is rendered as regular links inside chrome-search:// iframes, so you might wonder why we need this API at all. I think there are three issues: 1) For a11y, we need to support keyboard navigation. That logic is still in the JS in the NTP so it needs a way to trigger navigations. 2) You can't arbitrarily link to privileged URLs like chrome://. Though, it might be better to allow chrome-search:// to link to these URLs rather than make NCW to support it. 3) With MostLikely, the browser didn't actually have knowledge of what the URLs were so you couldn't just have a RID-based NCW. Sounds like this is changing from Mathieu's reply earlier. So the summary of all of this I think is that if change that Mathieu alluded to earlier is done, we should at least get rid of the URL-based NCW and only have a RID version. It's also potentially feasible to kill this altogether and just have them be regular links but that will require more changes to make sure we don't break a11y. In the short term, unfortunately, I don't know what URLs can and can not enter Most Visited/Likely. But I'm pretty sure that comment was not added to specifically deal with that case. Thanks, Samarth
On 2016/02/04 at 17:11:26, mathp wrote: > On 2016/02/04 17:05:55, Jered wrote: > > +mathp will this break most likely? > > > > Given the immediate concerns this lgtm. > > Depends if the SuggestionsService is used. Since it's becoming the default soon for the server NTP (finkm@ and team can confirm), I'd say that it won't break it. If the current server side code ran on this then yes it would break (because |suggestions_items_| wouldn't know about the most likely items). Can you elaborate on this? The current check uses both |most_visited_items_| and |suggestions_items_| (which is actually a lot less strict than I'd prefer). On 2016/02/04 at 17:20:44, samarth wrote: > We could also make this check in the renderer process in searchbox_extension here: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/se... > > Note that it has access to the most visited items: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/se... > > On one hand, that's where the check for javascript: scheme resides and it'd be better for these checks to be centralized. But I don't know if it's preferable to have these be in the browser process instead. > > Thanks, > Samarth That's where I added it initially, but the renderer is untrusted and therefore checks in the renderer are fundamentally meaningless. There have been exploits in the past that sent IPCs directly, which would bypass this check completely (I know that we now verify that the IPC is from an instant process, but it is still better to verify browser side). On 2016/02/04 at 17:18:15, creis wrote: > On 2016/02/04 17:13:32, Charlie Reis wrote: > > On 2016/02/04 16:35:15, Robert Sesek wrote: > > > On 2016/02/04 16:21:01, dcheng wrote: > > > > I was looking at the scheme blacklist, and I'm wondering if there's a better > > > > way: there are other 'weird' schemes that I wouldn't expect to be valid > > > > navigation targets for 1993 (such as chrome-devtools://). Do we need to > > worry > > > > about other schemes here as well? Is there a more generic way to determine a > > > > dangerous scheme that will grant a renderer additional privileges? > > > > > > > > rsesek and I initially considered a whitelist approach, but were a little > > > > worried that this might not capture everything a user might have in their > > most > > > > visited items (e.g. I'm not sure if an ftp URL could end up in there). > > > > > > I was thinking about this more last night, and I think a whiltelist may be > > > better than a blacklist. I don't think schemes other than http, https, file, > > and > > > ftp should be allowed. > > > > On one hand, I'd love to lock it down to a whitelist. > > ChildProcessSecurityPolicy::IsWebSafeScheme might be a good candidate. > > > > On the other hand, what restrictions do we have on the most visited and > > suggestion URLs? Can chrome-extension:// URLs show up there (e.g., if they're > > web accessible)? Can chrome:// URLs? (Hopefully not.) I don't want to > > introduce a regression where users can't click on those tiles, assuming that the > > tiles use this API. > > For example, see this comment in search_ipc_router.h: > > // Called when the page wants to navigate to |url|. Usually used by the > // page to navigate to privileged destinations (e.g. chrome:// URLs) or to > // navigate to URLs that are hidden from the page using Restricted IDs (rid > // in the API). > virtual void NavigateToURL(const GURL& url, > WindowOpenDisposition disposition, > bool is_most_visited_item_url) = 0; > > It makes me sad that there are apparently no tests covering this behavior, if it was actually introduced to support navigations to privileged URLs. Breaking Chrome in a security merge is definitely something that was worrying me. One "easy" fix is to filter dangerous URLs from |most_visited_items_| and |suggestions_items_| when they're set. On the other hand, this starts getting into "uncomfortably exciting to merge into stable branch" territory. Maybe we can limit the scheme filter to just chrome:// URLs for now? I'd hate to check this in without it being that effective because there's actually some way of chrome:// URLs getting into most visited sites pretty easily...
On 2016/02/04 17:43:46, samarth wrote: > On 2016/02/04 17:26:25, Charlie Reis wrote: > > On 2016/02/04 17:20:44, samarth wrote: > > > We could also make this check in the renderer process in > searchbox_extension > > > here: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/se... > > > > > > Note that it has access to the most visited items: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/se... > > > > > > On one hand, that's where the check for javascript: scheme resides and it'd > be > > > better for these checks to be centralized. But I don't know if it's > > preferable > > > to have these be in the browser process instead. > > > > > > Thanks, > > > Samarth > > > > Thanks for looking. We definitely want to enforce this in the browser > process, > > though I'm not opposed to also having a check in the renderer process. > > > > Samarth, it looks like you added the comment about using this API for > privileged > > URLs in https://codereview.chromium.org/11824050 (in instant_page.h at the > > time). Can you let us know why that was needed and what will break if we > > prevent it now? > > There's a long history behind NavigateContentWindow. > > In the beginning, this API was meant to be used both for most visited items as > well as for suggestions in the HTML-based omnibox popup. The latter never > launched. In the suggest case, you could definitely end up with chrome:// URLs > in the suggestions and this API provided a way for the HTML omnibox popup to > navigate to those URLs when the user selected those suggestions. The comment > here probably dates to that use case. > > At this point, we only need NCW for most visited/most likely so it can much more > limited. Even better, each most visited item is rendered as regular links > inside chrome-search:// iframes, so you might wonder why we need this API at > all. I think there are three issues: > 1) For a11y, we need to support keyboard navigation. That logic is still in the > JS in the NTP so it needs a way to trigger navigations. > 2) You can't arbitrarily link to privileged URLs like chrome://. Though, it > might be better to allow chrome-search:// to link to these URLs rather than make > NCW to support it. > 3) With MostLikely, the browser didn't actually have knowledge of what the URLs > were so you couldn't just have a RID-based NCW. Sounds like this is changing > from Mathieu's reply earlier. > > So the summary of all of this I think is that if change that Mathieu alluded to > earlier is done, we should at least get rid of the URL-based NCW and only have a > RID version. It's also potentially feasible to kill this altogether and just > have them be regular links but that will require more changes to make sure we > don't break a11y. > > In the short term, unfortunately, I don't know what URLs can and can not enter > Most Visited/Likely. But I'm pretty sure that comment was not added to > specifically deal with that case. > > Thanks, > Samarth Fantastic. Thank for all the context. That makes me happier with the approach taken here, though there's still some risk of regression since we don't know which chrome:// or chrome-extension:// URLs can show up in the tiles. I'll copy your comment over to the bug and we can discuss followup CLs from there. For now, I think the decision to make is whether to limit the URL-based API to a whitelist (like IsWebSafeScheme) or just trust what's in the Most Visited and Suggestions lists. I want to land something that's unlikely to get reverted and is easy to merge.
On 2016/02/04 17:43:46, samarth wrote: > On 2016/02/04 17:26:25, Charlie Reis wrote: > > On 2016/02/04 17:20:44, samarth wrote: > > > We could also make this check in the renderer process in > searchbox_extension > > > here: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/se... > > > > > > Note that it has access to the most visited items: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/se... > > > > > > On one hand, that's where the check for javascript: scheme resides and it'd > be > > > better for these checks to be centralized. But I don't know if it's > > preferable > > > to have these be in the browser process instead. > > > > > > Thanks, > > > Samarth > > > > Thanks for looking. We definitely want to enforce this in the browser > process, > > though I'm not opposed to also having a check in the renderer process. > > > > Samarth, it looks like you added the comment about using this API for > privileged > > URLs in https://codereview.chromium.org/11824050 (in instant_page.h at the > > time). Can you let us know why that was needed and what will break if we > > prevent it now? > > There's a long history behind NavigateContentWindow. > > In the beginning, this API was meant to be used both for most visited items as > well as for suggestions in the HTML-based omnibox popup. The latter never > launched. In the suggest case, you could definitely end up with chrome:// URLs > in the suggestions and this API provided a way for the HTML omnibox popup to > navigate to those URLs when the user selected those suggestions. The comment > here probably dates to that use case. > > At this point, we only need NCW for most visited/most likely so it can much more > limited. Even better, each most visited item is rendered as regular links > inside chrome-search:// iframes, so you might wonder why we need this API at > all. I think there are three issues: > 1) For a11y, we need to support keyboard navigation. That logic is still in the > JS in the NTP so it needs a way to trigger navigations. > 2) You can't arbitrarily link to privileged URLs like chrome://. Though, it > might be better to allow chrome-search:// to link to these URLs rather than make > NCW to support it. > 3) With MostLikely, the browser didn't actually have knowledge of what the URLs > were so you couldn't just have a RID-based NCW. Sounds like this is changing > from Mathieu's reply earlier. > > So the summary of all of this I think is that if change that Mathieu alluded to > earlier is done, we should at least get rid of the URL-based NCW and only have a > RID version. It's also potentially feasible to kill this altogether and just > have them be regular links but that will require more changes to make sure we > don't break a11y. iirc bing are using the url api so coordinating that change may be complex. > > In the short term, unfortunately, I don't know what URLs can and can not enter > Most Visited/Likely. But I'm pretty sure that comment was not added to > specifically deal with that case. > > Thanks, > Samarth
On 2016/02/04 17:51:07, Charlie Reis wrote: > On 2016/02/04 17:43:46, samarth wrote: > > On 2016/02/04 17:26:25, Charlie Reis wrote: > > > On 2016/02/04 17:20:44, samarth wrote: > > > > We could also make this check in the renderer process in > > searchbox_extension > > > > here: > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/se... > > > > > > > > Note that it has access to the most visited items: > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/se... > > > > > > > > On one hand, that's where the check for javascript: scheme resides and > it'd > > be > > > > better for these checks to be centralized. But I don't know if it's > > > preferable > > > > to have these be in the browser process instead. > > > > > > > > Thanks, > > > > Samarth > > > > > > Thanks for looking. We definitely want to enforce this in the browser > > process, > > > though I'm not opposed to also having a check in the renderer process. > > > > > > Samarth, it looks like you added the comment about using this API for > > privileged > > > URLs in https://codereview.chromium.org/11824050 (in instant_page.h at the > > > time). Can you let us know why that was needed and what will break if we > > > prevent it now? > > > > There's a long history behind NavigateContentWindow. > > > > In the beginning, this API was meant to be used both for most visited items as > > well as for suggestions in the HTML-based omnibox popup. The latter never > > launched. In the suggest case, you could definitely end up with chrome:// > URLs > > in the suggestions and this API provided a way for the HTML omnibox popup to > > navigate to those URLs when the user selected those suggestions. The comment > > here probably dates to that use case. > > > > At this point, we only need NCW for most visited/most likely so it can much > more > > limited. Even better, each most visited item is rendered as regular links > > inside chrome-search:// iframes, so you might wonder why we need this API at > > all. I think there are three issues: > > 1) For a11y, we need to support keyboard navigation. That logic is still in > the > > JS in the NTP so it needs a way to trigger navigations. > > 2) You can't arbitrarily link to privileged URLs like chrome://. Though, it > > might be better to allow chrome-search:// to link to these URLs rather than > make > > NCW to support it. > > 3) With MostLikely, the browser didn't actually have knowledge of what the > URLs > > were so you couldn't just have a RID-based NCW. Sounds like this is changing > > from Mathieu's reply earlier. > > > > So the summary of all of this I think is that if change that Mathieu alluded > to > > earlier is done, we should at least get rid of the URL-based NCW and only have > a > > RID version. It's also potentially feasible to kill this altogether and just > > have them be regular links but that will require more changes to make sure we > > don't break a11y. > > > > In the short term, unfortunately, I don't know what URLs can and can not enter > > Most Visited/Likely. But I'm pretty sure that comment was not added to > > specifically deal with that case. > > > > Thanks, > > Samarth > > Fantastic. Thank for all the context. That makes me happier with the approach > taken here, though there's still some risk of regression since we don't know > which chrome:// or chrome-extension:// URLs can show up in the tiles. > > I'll copy your comment over to the bug and we can discuss followup CLs from > there. > > For now, I think the decision to make is whether to limit the URL-based API to a > whitelist (like IsWebSafeScheme) or just trust what's in the Most Visited and > Suggestions lists. I want to land something that's unlikely to get reverted and > is easy to merge. Based on mathp's comments, I think IsWebSafeScheme() would be a safer merge. But I think this is a better fix assuming that the suggestions list is actually populated.
On 2016/02/04 17:48:58, dcheng wrote: > Breaking Chrome in a security merge is definitely something that was worrying > me. > > One "easy" fix is to filter dangerous URLs from |most_visited_items_| and > |suggestions_items_| when they're set. On the other hand, this starts getting > into "uncomfortably exciting to merge into stable branch" territory. Maybe we > can limit the scheme filter to just chrome:// URLs for now? I'd hate to check > this in without it being that effective because there's actually some way of > chrome:// URLs getting into most visited sites pretty easily... Do you know of a way for an attacker to get chrome:// URLs into Most Visited, or are you just worried that it might be possible? I would hope that would be just as difficult as navigating to a chrome:// URL in the first place, which is what the attack was using this API for. I could be ok with limiting the check to chrome:// URLs for now. That might be a good balance between security and avoiding possible regressions.
On 2016/02/04 at 17:53:40, creis wrote: > On 2016/02/04 17:48:58, dcheng wrote: > > Breaking Chrome in a security merge is definitely something that was worrying > > me. > > > > One "easy" fix is to filter dangerous URLs from |most_visited_items_| and > > |suggestions_items_| when they're set. On the other hand, this starts getting > > into "uncomfortably exciting to merge into stable branch" territory. Maybe we > > can limit the scheme filter to just chrome:// URLs for now? I'd hate to check > > this in without it being that effective because there's actually some way of > > chrome:// URLs getting into most visited sites pretty easily... > > Do you know of a way for an attacker to get chrome:// URLs into Most Visited, or are you just worried that it might be possible? I would hope that would be just as difficult as navigating to a chrome:// URL in the first place, which is what the attack was using this API for. > > I could be ok with limiting the check to chrome:// URLs for now. That might be a good balance between security and avoiding possible regressions. I don't but I'm not familiar with the code. I'll update the CL to just filter on chrome://. Re: IsWebSafeScheme: that's definitely a much more aggressive and restrictive change, so I think that should be done in followups by whoever owns the code now.
On 2016/02/04 17:53:20, Jered wrote: > > Fantastic. Thank for all the context. That makes me happier with the > approach > > taken here, though there's still some risk of regression since we don't know > > which chrome:// or chrome-extension:// URLs can show up in the tiles. > > > > I'll copy your comment over to the bug and we can discuss followup CLs from > > there. > > > > For now, I think the decision to make is whether to limit the URL-based API to > a > > whitelist (like IsWebSafeScheme) or just trust what's in the Most Visited and > > Suggestions lists. I want to land something that's unlikely to get reverted > and > > is easy to merge. > > Based on mathp's comments, I think IsWebSafeScheme() would be a safer merge. > But I think this is a better fix assuming that the suggestions list is actually > populated. As Daniel mentions, IsWebSafeScheme seems like the least safe merge, at least from a regression perspective. It will almost certainly break attempts to visit chrome-extension:// URLs that are in Most Visited, assuming that's possible.
On 2016/02/04 17:11:26, Mathieu Perreault wrote: > On 2016/02/04 17:05:55, Jered wrote: > > +mathp will this break most likely? > > > > Given the immediate concerns this lgtm. > > Depends if the SuggestionsService is used. Since it's becoming the default soon > for the server NTP (finkm@ and team can confirm) We haven't started working on that. I'll discuss with the team if/when we can squeeze that in. On 2016/02/04 17:53:20, Jered wrote: > I don't but I'm not familiar with the code. I'll update the CL to just filter on > chrome://. SGTM
PTAL, I've relaxed the check to just forbid chrome:// URLs. I've also added TODOs for followup work (trieb, I've put your name in them for now, hopefully that's ok ;-)
LGTM with nits. https://codereview.chromium.org/1669723002/diff/160001/chrome/browser/ui/sear... File chrome/browser/ui/search/search_tab_helper.cc (right): https://codereview.chromium.org/1669723002/diff/160001/chrome/browser/ui/sear... chrome/browser/ui/search/search_tab_helper.cc:454: // items list. Note that the |is_most_visited_item_url| is apparently Maybe this second sentence should be a TODO to remove it? Right now it's just going to confuse people reading the code, as if it's part of the intended design. https://codereview.chromium.org/1669723002/diff/160001/chrome/renderer/search... File chrome/renderer/searchbox/searchbox_extension.cc (right): https://codereview.chromium.org/1669723002/diff/160001/chrome/renderer/search... chrome/renderer/searchbox/searchbox_extension.cc:1168: // TODO(treib): Remove this check, since it's redundant with the browser-side I disagree with removing this. It's fine to have the renderer skip the IPC if it knows the URL won't be allowed, and just have the browser-side check as a backup.
https://codereview.chromium.org/1669723002/diff/160001/chrome/browser/ui/sear... File chrome/browser/ui/search/search_tab_helper.cc (right): https://codereview.chromium.org/1669723002/diff/160001/chrome/browser/ui/sear... chrome/browser/ui/search/search_tab_helper.cc:454: // items list. Note that the |is_most_visited_item_url| is apparently On 2016/02/04 at 18:37:53, Charlie Reis wrote: > Maybe this second sentence should be a TODO to remove it? Right now it's just going to confuse people reading the code, as if it's part of the intended design. Hm... I think it's actually kind of important to document that this variable doesn't mean what a reader thinks it does: my initial impression was that this would allow me to check |suggestions_items_| or |most_visited_items_| based on the value of this flag, but that's not true. Do you have suggestions on how to improve this? https://codereview.chromium.org/1669723002/diff/160001/chrome/renderer/search... File chrome/renderer/searchbox/searchbox_extension.cc (right): https://codereview.chromium.org/1669723002/diff/160001/chrome/renderer/search... chrome/renderer/searchbox/searchbox_extension.cc:1168: // TODO(treib): Remove this check, since it's redundant with the browser-side On 2016/02/04 at 18:37:53, Charlie Reis wrote: > I disagree with removing this. It's fine to have the renderer skip the IPC if it knows the URL won't be allowed, and just have the browser-side check as a backup. On the flip side, having it here makes it tempting to add further checks here (I fell into this trap initially yesterday).
Some suggestions below. https://codereview.chromium.org/1669723002/diff/160001/chrome/browser/ui/sear... File chrome/browser/ui/search/search_tab_helper.cc (right): https://codereview.chromium.org/1669723002/diff/160001/chrome/browser/ui/sear... chrome/browser/ui/search/search_tab_helper.cc:454: // items list. Note that the |is_most_visited_item_url| is apparently On 2016/02/04 18:41:32, dcheng wrote: > On 2016/02/04 at 18:37:53, Charlie Reis wrote: > > Maybe this second sentence should be a TODO to remove it? Right now it's just > going to confuse people reading the code, as if it's part of the intended > design. > > Hm... I think it's actually kind of important to document that this variable > doesn't mean what a reader thinks it does: my initial impression was that this > would allow me to check |suggestions_items_| or |most_visited_items_| based on > the value of this flag, but that's not true. Do you have suggestions on how to > improve this? TODO(...): The |is_most_visited_item_url| is apparently meaningless. Can it be removed? https://codereview.chromium.org/1669723002/diff/160001/chrome/renderer/search... File chrome/renderer/searchbox/searchbox_extension.cc (right): https://codereview.chromium.org/1669723002/diff/160001/chrome/renderer/search... chrome/renderer/searchbox/searchbox_extension.cc:1168: // TODO(treib): Remove this check, since it's redundant with the browser-side On 2016/02/04 18:41:32, dcheng wrote: > On 2016/02/04 at 18:37:53, Charlie Reis wrote: > > I disagree with removing this. It's fine to have the renderer skip the IPC if > it knows the URL won't be allowed, and just have the browser-side check as a > backup. > > On the flip side, having it here makes it tempting to add further checks here (I > fell into this trap initially yesterday). // Navigate the main frame. Note that the safety checks are enforced by the browser process in InstantService but are useful here for avoiding unnecessary IPCs.
https://codereview.chromium.org/1669723002/diff/160001/chrome/browser/ui/sear... File chrome/browser/ui/search/search_tab_helper.cc (right): https://codereview.chromium.org/1669723002/diff/160001/chrome/browser/ui/sear... chrome/browser/ui/search/search_tab_helper.cc:454: // items list. Note that the |is_most_visited_item_url| is apparently On 2016/02/04 at 18:45:23, Charlie Reis wrote: > On 2016/02/04 18:41:32, dcheng wrote: > > On 2016/02/04 at 18:37:53, Charlie Reis wrote: > > > Maybe this second sentence should be a TODO to remove it? Right now it's just > > going to confuse people reading the code, as if it's part of the intended > > design. > > > > Hm... I think it's actually kind of important to document that this variable > > doesn't mean what a reader thinks it does: my initial impression was that this > > would allow me to check |suggestions_items_| or |most_visited_items_| based on > > the value of this flag, but that's not true. Do you have suggestions on how to > > improve this? > > TODO(...): The |is_most_visited_item_url| is apparently meaningless. Can it be removed? Done. https://codereview.chromium.org/1669723002/diff/160001/chrome/renderer/search... File chrome/renderer/searchbox/searchbox_extension.cc (right): https://codereview.chromium.org/1669723002/diff/160001/chrome/renderer/search... chrome/renderer/searchbox/searchbox_extension.cc:1168: // TODO(treib): Remove this check, since it's redundant with the browser-side On 2016/02/04 at 18:45:23, Charlie Reis wrote: > On 2016/02/04 18:41:32, dcheng wrote: > > On 2016/02/04 at 18:37:53, Charlie Reis wrote: > > > I disagree with removing this. It's fine to have the renderer skip the IPC if > > it knows the URL won't be allowed, and just have the browser-side check as a > > backup. > > > > On the flip side, having it here makes it tempting to add further checks here (I > > fell into this trap initially yesterday). > > // Navigate the main frame. Note that the safety checks are enforced by the browser process in InstantService but are useful here for avoiding unnecessary IPCs. Done.
Thanks. LGTM. https://codereview.chromium.org/1669723002/diff/180001/chrome/browser/ui/sear... File chrome/browser/ui/search/search_tab_helper.cc (right): https://codereview.chromium.org/1669723002/diff/180001/chrome/browser/ui/sear... chrome/browser/ui/search/search_tab_helper.cc:455: // TODO(trieb) The |is_most_visited_item_url| is meaningless: the way it's nit: treib (I'm sensitive to ie/ei as well.) :) nit: Add colon after close paren.
The CQ bit was checked by dcheng@chromium.org
https://codereview.chromium.org/1669723002/diff/180001/chrome/browser/ui/sear... File chrome/browser/ui/search/search_tab_helper.cc (right): https://codereview.chromium.org/1669723002/diff/180001/chrome/browser/ui/sear... chrome/browser/ui/search/search_tab_helper.cc:455: // TODO(trieb) The |is_most_visited_item_url| is meaningless: the way it's On 2016/02/04 at 19:10:04, Charlie Reis wrote: > nit: treib (I'm sensitive to ie/ei as well.) :) > nit: Add colon after close paren. Done.
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, jered@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/1669723002/#ps200001 (title: "Better comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1669723002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1669723002/200001
Description was changed from ========== NTP: don't allow navigateContentWindow to navigate where it pleases. BUG=509313 ========== to ========== NTP: don't allow navigateContentWindow to navigate where it pleases. BUG=509313 ==========
rsesek@chromium.org changed reviewers: + rsesek@chromium.org
Message was sent while issue was closed.
Description was changed from ========== NTP: don't allow navigateContentWindow to navigate where it pleases. BUG=509313 ========== to ========== NTP: don't allow navigateContentWindow to navigate where it pleases. BUG=509313 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== NTP: don't allow navigateContentWindow to navigate where it pleases. BUG=509313 ========== to ========== NTP: don't allow navigateContentWindow to navigate where it pleases. BUG=509313 Committed: https://crrev.com/d523a41aed4e321d4c8197b5cccb73be23c8dcc2 Cr-Commit-Position: refs/heads/master@{#373598} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/d523a41aed4e321d4c8197b5cccb73be23c8dcc2 Cr-Commit-Position: refs/heads/master@{#373598} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
