Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1775423002:
Fix potential XSS on the NTP (Closed)
|
Created:
4 years, 9 months ago by Marc Treib Modified:
4 years, 9 months ago CC:
arv+watch_chromium.org, chromium-reviews, David Black, dhollowa+watch_chromium.org, donnd+watch_chromium.org, dougw+watch_chromium.org, Jered, jfweitz+watch_chromium.org, kmadhusu+watch_chromium.org, melevin+watch_chromium.org, samarth+watch_chromium.org, skanuj+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix potential XSS on the NTP
BUG=592956
Committed: https://crrev.com/120894e0c50e42babadb6314cc997b1f3d7ddd00
Cr-Commit-Position: refs/heads/master@{#380640}
Patch Set 1 #
Total comments: 3
Patch Set 2 : whitelist #Messages
Total messages: 16 (3 generated)
treib@chromium.org changed reviewers: + fserb@chromium.org, jochen@chromium.org
PTAL! https://codereview.chromium.org/1775423002/diff/1/chrome/browser/resources/lo... File chrome/browser/resources/local_ntp/most_visited_single.js (left): https://codereview.chromium.org/1775423002/diff/1/chrome/browser/resources/lo... chrome/browser/resources/local_ntp/most_visited_single.js:274: if (navigator.sendBeacon) { navigator.sendBeacon has existed since Chrome 39, I don't think we need the fallback anymore.
https://codereview.chromium.org/1775423002/diff/1/chrome/browser/resources/lo... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://codereview.chromium.org/1775423002/diff/1/chrome/browser/resources/lo... chrome/browser/resources/local_ntp/most_visited_single.js:347: if (!data.url.startsWith('javascript:')) { what about blob URLs etc? Would prefer a whitelist approach over a blacklist
https://codereview.chromium.org/1775423002/diff/1/chrome/browser/resources/lo... File chrome/browser/resources/local_ntp/most_visited_single.js (right): https://codereview.chromium.org/1775423002/diff/1/chrome/browser/resources/lo... chrome/browser/resources/local_ntp/most_visited_single.js:347: if (!data.url.startsWith('javascript:')) { On 2016/03/09 15:26:41, jochen wrote: > what about blob URLs etc? Would prefer a whitelist approach over a blacklist I agree that a whitelist is generally nicer, but I don't know where we could get an appropriate whitelist from - right now, TopSites supports "any" scheme, though I hope to change that soon (context on crbug.com/584461). Are you okay with a blacklist for now, and revisiting once we can actually produce a meaningful whitelist?
On 2016/03/09 at 16:28:34, treib wrote: > https://codereview.chromium.org/1775423002/diff/1/chrome/browser/resources/lo... > File chrome/browser/resources/local_ntp/most_visited_single.js (right): > > https://codereview.chromium.org/1775423002/diff/1/chrome/browser/resources/lo... > chrome/browser/resources/local_ntp/most_visited_single.js:347: if (!data.url.startsWith('javascript:')) { > On 2016/03/09 15:26:41, jochen wrote: > > what about blob URLs etc? Would prefer a whitelist approach over a blacklist > > I agree that a whitelist is generally nicer, but I don't know where we could get an appropriate whitelist from - right now, TopSites supports "any" scheme, though I hope to change that soon (context on crbug.com/584461). Are you okay with a blacklist for now, and revisiting once we can actually produce a meaningful whitelist? you can easily inject javascript via blob: javascript: or data: URLs. allowing file: without additional restrictions on the path is also scary. Can't we just filter out non-http(s) from the most visited URLs?
On 2016/03/09 16:36:03, jochen wrote: > On 2016/03/09 at 16:28:34, treib wrote: > > > https://codereview.chromium.org/1775423002/diff/1/chrome/browser/resources/lo... > > File chrome/browser/resources/local_ntp/most_visited_single.js (right): > > > > > https://codereview.chromium.org/1775423002/diff/1/chrome/browser/resources/lo... > > chrome/browser/resources/local_ntp/most_visited_single.js:347: if > (!data.url.startsWith('javascript:')) { > > On 2016/03/09 15:26:41, jochen wrote: > > > what about blob URLs etc? Would prefer a whitelist approach over a blacklist > > > > I agree that a whitelist is generally nicer, but I don't know where we could > get an appropriate whitelist from - right now, TopSites supports "any" scheme, > though I hope to change that soon (context on crbug.com/584461). Are you okay > with a blacklist for now, and revisiting once we can actually produce a > meaningful whitelist? > > you can easily inject javascript via blob: javascript: or data: URLs. allowing > file: without additional restrictions on the path is also scary. > > Can't we just filter out non-http(s) from the most visited URLs? Nope, not quite yet. Currently, at least file:// and chrome-extension:// are explicitly supported. I am planning to change that, but that's still a while out, let's say M53ish. In the meantime, a blacklist is still better than nothing. WDYT?
On 2016/03/09 at 17:03:35, treib wrote: > On 2016/03/09 16:36:03, jochen wrote: > > On 2016/03/09 at 16:28:34, treib wrote: > > > > > https://codereview.chromium.org/1775423002/diff/1/chrome/browser/resources/lo... > > > File chrome/browser/resources/local_ntp/most_visited_single.js (right): > > > > > > > > https://codereview.chromium.org/1775423002/diff/1/chrome/browser/resources/lo... > > > chrome/browser/resources/local_ntp/most_visited_single.js:347: if > > (!data.url.startsWith('javascript:')) { > > > On 2016/03/09 15:26:41, jochen wrote: > > > > what about blob URLs etc? Would prefer a whitelist approach over a blacklist > > > > > > I agree that a whitelist is generally nicer, but I don't know where we could > > get an appropriate whitelist from - right now, TopSites supports "any" scheme, > > though I hope to change that soon (context on crbug.com/584461). Are you okay > > with a blacklist for now, and revisiting once we can actually produce a > > meaningful whitelist? > > > > you can easily inject javascript via blob: javascript: or data: URLs. allowing > > file: without additional restrictions on the path is also scary. > > > > Can't we just filter out non-http(s) from the most visited URLs? > > Nope, not quite yet. Currently, at least file:// and chrome-extension:// are explicitly supported. I am planning to change that, but that's still a while out, let's say M53ish. > In the meantime, a blacklist is still better than nothing. WDYT? Let's do a whitelist of web safe + file & extensions then?
On 2016/03/09 17:09:35, jochen wrote: > On 2016/03/09 at 17:03:35, treib wrote: > > On 2016/03/09 16:36:03, jochen wrote: > > > On 2016/03/09 at 16:28:34, treib wrote: > > > > > > > > https://codereview.chromium.org/1775423002/diff/1/chrome/browser/resources/lo... > > > > File chrome/browser/resources/local_ntp/most_visited_single.js (right): > > > > > > > > > > > > https://codereview.chromium.org/1775423002/diff/1/chrome/browser/resources/lo... > > > > chrome/browser/resources/local_ntp/most_visited_single.js:347: if > > > (!data.url.startsWith('javascript:')) { > > > > On 2016/03/09 15:26:41, jochen wrote: > > > > > what about blob URLs etc? Would prefer a whitelist approach over a > blacklist > > > > > > > > I agree that a whitelist is generally nicer, but I don't know where we > could > > > get an appropriate whitelist from - right now, TopSites supports "any" > scheme, > > > though I hope to change that soon (context on crbug.com/584461). Are you > okay > > > with a blacklist for now, and revisiting once we can actually produce a > > > meaningful whitelist? > > > > > > you can easily inject javascript via blob: javascript: or data: URLs. > allowing > > > file: without additional restrictions on the path is also scary. > > > > > > Can't we just filter out non-http(s) from the most visited URLs? > > > > Nope, not quite yet. Currently, at least file:// and chrome-extension:// are > explicitly supported. I am planning to change that, but that's still a while > out, let's say M53ish. > > In the meantime, a blacklist is still better than nothing. WDYT? > > Let's do a whitelist of web safe + file & extensions then? Huh, turns out blob:// and data:// are in fact considered web safe, see: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ch... As are chrome-extension:// and chrome-search://, see: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro... Which means I have no idea what "web safe" actually means. Can you clarify, or point me towards something? Thanks!
On 2016/03/10 at 14:21:30, treib wrote: > On 2016/03/09 17:09:35, jochen wrote: > > On 2016/03/09 at 17:03:35, treib wrote: > > > On 2016/03/09 16:36:03, jochen wrote: > > > > On 2016/03/09 at 16:28:34, treib wrote: > > > > > > > > > > > https://codereview.chromium.org/1775423002/diff/1/chrome/browser/resources/lo... > > > > > File chrome/browser/resources/local_ntp/most_visited_single.js (right): > > > > > > > > > > > > > > > > https://codereview.chromium.org/1775423002/diff/1/chrome/browser/resources/lo... > > > > > chrome/browser/resources/local_ntp/most_visited_single.js:347: if > > > > (!data.url.startsWith('javascript:')) { > > > > > On 2016/03/09 15:26:41, jochen wrote: > > > > > > what about blob URLs etc? Would prefer a whitelist approach over a > > blacklist > > > > > > > > > > I agree that a whitelist is generally nicer, but I don't know where we > > could > > > > get an appropriate whitelist from - right now, TopSites supports "any" > > scheme, > > > > though I hope to change that soon (context on crbug.com/584461). Are you > > okay > > > > with a blacklist for now, and revisiting once we can actually produce a > > > > meaningful whitelist? > > > > > > > > you can easily inject javascript via blob: javascript: or data: URLs. > > allowing > > > > file: without additional restrictions on the path is also scary. > > > > > > > > Can't we just filter out non-http(s) from the most visited URLs? > > > > > > Nope, not quite yet. Currently, at least file:// and chrome-extension:// are > > explicitly supported. I am planning to change that, but that's still a while > > out, let's say M53ish. > > > In the meantime, a blacklist is still better than nothing. WDYT? > > > > Let's do a whitelist of web safe + file & extensions then? > > Huh, turns out blob:// and data:// are in fact considered web safe, see: https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ch... > As are chrome-extension:// and chrome-search://, see: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro... > > Which means I have no idea what "web safe" actually means. Can you clarify, or point me towards something? Thanks! ok, I'd propose to restrict this to http: https: file: and chrome-extension:
On 2016/03/10 14:25:43, jochen wrote: > On 2016/03/10 at 14:21:30, treib wrote: > > On 2016/03/09 17:09:35, jochen wrote: > > > On 2016/03/09 at 17:03:35, treib wrote: > > > > On 2016/03/09 16:36:03, jochen wrote: > > > > > On 2016/03/09 at 16:28:34, treib wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/1775423002/diff/1/chrome/browser/resources/lo... > > > > > > File chrome/browser/resources/local_ntp/most_visited_single.js > (right): > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/1775423002/diff/1/chrome/browser/resources/lo... > > > > > > chrome/browser/resources/local_ntp/most_visited_single.js:347: if > > > > > (!data.url.startsWith('javascript:')) { > > > > > > On 2016/03/09 15:26:41, jochen wrote: > > > > > > > what about blob URLs etc? Would prefer a whitelist approach over a > > > blacklist > > > > > > > > > > > > I agree that a whitelist is generally nicer, but I don't know where we > > > could > > > > > get an appropriate whitelist from - right now, TopSites supports "any" > > > scheme, > > > > > though I hope to change that soon (context on crbug.com/584461). Are you > > > okay > > > > > with a blacklist for now, and revisiting once we can actually produce a > > > > > meaningful whitelist? > > > > > > > > > > you can easily inject javascript via blob: javascript: or data: URLs. > > > allowing > > > > > file: without additional restrictions on the path is also scary. > > > > > > > > > > Can't we just filter out non-http(s) from the most visited URLs? > > > > > > > > Nope, not quite yet. Currently, at least file:// and chrome-extension:// > are > > > explicitly supported. I am planning to change that, but that's still a while > > > out, let's say M53ish. > > > > In the meantime, a blacklist is still better than nothing. WDYT? > > > > > > Let's do a whitelist of web safe + file & extensions then? > > > > Huh, turns out blob:// and data:// are in fact considered web safe, see: > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ch... > > As are chrome-extension:// and chrome-search://, see: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/bro... > > > > Which means I have no idea what "web safe" actually means. Can you clarify, or > point me towards something? Thanks! > > ok, I'd propose to restrict this to http: https: file: and chrome-extension: Alright, fair enough. I also added ftp:// to the list.
lgtm
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1775423002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775423002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix potential XSS on the NTP BUG=592956 ========== to ========== Fix potential XSS on the NTP BUG=592956 Committed: https://crrev.com/120894e0c50e42babadb6314cc997b1f3d7ddd00 Cr-Commit-Position: refs/heads/master@{#380640} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/120894e0c50e42babadb6314cc997b1f3d7ddd00 Cr-Commit-Position: refs/heads/master@{#380640} |
