Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
136393004:
PPB_Flash.Navigate(): Disallow certain HTTP request headers. (Closed)
|
Created:
6 years, 11 months ago by yzshen1 Modified:
6 years, 10 months ago CC:
chromium-reviews, Nate Chapin, Xing Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionPPB_Flash.Navigate(): Disallow certain HTTP request headers.
With this CL, PPB_Flash.Navigate() fails the operation with
PP_ERROR_NOACCESS if the request headers contain non-simple headers.
BUG=332023
TEST=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249114
Patch Set 1 #
Total comments: 6
Patch Set 2 : only allow simple headers #
Total comments: 2
Patch Set 3 : added changes to histograms.xml #
Total comments: 4
Patch Set 4 : #
Messages
Total messages: 27 (0 generated)
Hi, Adam and Justin. Would you please take a look? On the bug report 332023, we mentioned that we could consider rejecting all non-simple requests. However, I looked at the Flash code and realized that it might use a broader set of request headers. Therefore, I allowed more headers than simple headers. Please take a look at pepper_flash_renderer_host.cc and let me know if they don't seem safe to you. Please note that this only affects those requests to navigate frames. It doesn't affect URL fetching. (Those requests go through AssociatedURLLoader and thus have CORS checking enabled.) If this change is proved to break too many sites, maybe we can consider enabling CORS checking when blink handle redirection for frame navigation. Thanks!
https://codereview.chromium.org/136393004/diff/1/chrome/renderer/pepper/peppe... File chrome/renderer/pepper/pepper_flash_renderer_host.cc (right): https://codereview.chromium.org/136393004/diff/1/chrome/renderer/pepper/peppe... chrome/renderer/pepper/pepper_flash_renderer_host.cc:68: }; Presumably we have this list of headers elsewhere. Shouldn't we re-use the existing list instead of replicating it here?
https://codereview.chromium.org/136393004/diff/1/chrome/renderer/pepper/peppe... File chrome/renderer/pepper/pepper_flash_renderer_host.cc (right): https://codereview.chromium.org/136393004/diff/1/chrome/renderer/pepper/peppe... chrome/renderer/pepper/pepper_flash_renderer_host.cc:50: // - "Referer" may be set by the Flash player itself. Don't we want to use the list of CORS simple headers? Why is it safe to let Flash set the Pragma header but not safe to let XMLHttpRequest set it?
Thank you Adam! https://codereview.chromium.org/136393004/diff/1/chrome/renderer/pepper/peppe... File chrome/renderer/pepper/pepper_flash_renderer_host.cc (right): https://codereview.chromium.org/136393004/diff/1/chrome/renderer/pepper/peppe... chrome/renderer/pepper/pepper_flash_renderer_host.cc:50: // - "Referer" may be set by the Flash player itself. On 2014/01/26 02:10:38, abarth wrote: > Don't we want to use the list of CORS simple headers? Why is it safe to let > Flash set the Pragma header but not safe to let XMLHttpRequest set it? I agree that the CORS simple headers list is preferable. However, Flash allows these headers to be set by Flash contents via ActionScript. If we block them, we might break some sites. Besides, here the request is for frame navigation, we don't give the response to Flash. Maybe that is somewhat acceptable? (I admit I don't know much about the security implications here.) What do you think? Thanks! https://codereview.chromium.org/136393004/diff/1/chrome/renderer/pepper/peppe... chrome/renderer/pepper/pepper_flash_renderer_host.cc:68: }; On 2014/01/26 02:09:31, abarth wrote: > Presumably we have this list of headers elsewhere. Shouldn't we re-use the > existing list instead of replicating it here? There is a similar list is in the Flash source code, so we cannot re-use it. (Moreover, the list there is a blacklist instead of a whitelist.
https://codereview.chromium.org/136393004/diff/1/chrome/renderer/pepper/peppe... File chrome/renderer/pepper/pepper_flash_renderer_host.cc (right): https://codereview.chromium.org/136393004/diff/1/chrome/renderer/pepper/peppe... chrome/renderer/pepper/pepper_flash_renderer_host.cc:50: // - "Referer" may be set by the Flash player itself. On 2014/01/26 02:39:03, yzshen1 wrote: > On 2014/01/26 02:10:38, abarth wrote: > > Don't we want to use the list of CORS simple headers? Why is it safe to let > > Flash set the Pragma header but not safe to let XMLHttpRequest set it? > > I agree that the CORS simple headers list is preferable. However, Flash allows > these headers to be set by Flash contents via ActionScript. If we block them, we > might break some sites. > > Besides, here the request is for frame navigation, we don't give the response to > Flash. Maybe that is somewhat acceptable? (I admit I don't know much about the > security implications here.) That's doesn't help with the security problem. You can't read back a cross-origin XMLHttpRequest unless the server opts in either. > What do you think? Thanks! I don't see any reason why this would be secure. Without data on how many Flash sites would break, it's hard to know what the right trade-off is here. I'd probably start by using the list of simple headers from CORS and letting this change flow through the release channels so we can learn about compatibility problems. A more disciplined approach would be to combine that with an UMA histogram to see how we would have allowed a header with this list that is blocked because we're using the CORS list of simple headers.
On 2014/01/26 02:44:36, abarth wrote: > https://codereview.chromium.org/136393004/diff/1/chrome/renderer/pepper/peppe... > File chrome/renderer/pepper/pepper_flash_renderer_host.cc (right): > > https://codereview.chromium.org/136393004/diff/1/chrome/renderer/pepper/peppe... > chrome/renderer/pepper/pepper_flash_renderer_host.cc:50: // - "Referer" may be > set by the Flash player itself. > On 2014/01/26 02:39:03, yzshen1 wrote: > > On 2014/01/26 02:10:38, abarth wrote: > > > Don't we want to use the list of CORS simple headers? Why is it safe to let > > > Flash set the Pragma header but not safe to let XMLHttpRequest set it? > > > > I agree that the CORS simple headers list is preferable. However, Flash allows > > these headers to be set by Flash contents via ActionScript. If we block them, > we > > might break some sites. My inclination would be to use the simple CORS headers and expand from there only if we see breakage in the wild.
Thanks Adam and Justin. Please take another look. https://codereview.chromium.org/136393004/diff/1/chrome/renderer/pepper/peppe... File chrome/renderer/pepper/pepper_flash_renderer_host.cc (right): https://codereview.chromium.org/136393004/diff/1/chrome/renderer/pepper/peppe... chrome/renderer/pepper/pepper_flash_renderer_host.cc:50: // - "Referer" may be set by the Flash player itself. Done. Changed to only allow simple headers && record UMA stats.
On 2014/01/27 23:41:12, yzshen1 wrote: > Thanks Adam and Justin. > > Please take another look. > > https://codereview.chromium.org/136393004/diff/1/chrome/renderer/pepper/peppe... > File chrome/renderer/pepper/pepper_flash_renderer_host.cc (right): > > https://codereview.chromium.org/136393004/diff/1/chrome/renderer/pepper/peppe... > chrome/renderer/pepper/pepper_flash_renderer_host.cc:50: // - "Referer" may be > set by the Flash player itself. > Done. Changed to only allow simple headers && record UMA stats. High level lgtm, but I'd rely on Adam for final sign-off.
LGTM, but I'm not an owner of this code. https://codereview.chromium.org/136393004/diff/110001/chrome/renderer/pepper/... File chrome/renderer/pepper/pepper_flash_renderer_host.cc (right): https://codereview.chromium.org/136393004/diff/110001/chrome/renderer/pepper/... chrome/renderer/pepper/pepper_flash_renderer_host.cc:102: bool IsSimpleHeader(const std::string& lower_case_header_name, Can you add a comment about how we should share this function with CORS?
Thanks, Adam! https://codereview.chromium.org/136393004/diff/110001/chrome/renderer/pepper/... File chrome/renderer/pepper/pepper_flash_renderer_host.cc (right): https://codereview.chromium.org/136393004/diff/110001/chrome/renderer/pepper/... chrome/renderer/pepper/pepper_flash_renderer_host.cc:102: bool IsSimpleHeader(const std::string& lower_case_header_name, On 2014/01/28 21:52:08, abarth wrote: > Can you add a comment about how we should share this function with CORS? Happy to make changes, just try to make sure I understand: Do you mean I need to add a comment (or TODO) that we should share the same check between here and third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.cpp? I noticed that isOnAccessControlSimpleRequestHeaderWhitelist() there also allows "referer" and "origin". So they are not exactly the same check.
isherman@chromium.org: Please review changes in histograms.xml. Thanks!
On 2014/01/30 19:00:48, yzshen1 wrote: > isherman@chromium.org: Please review changes in histograms.xml. > > Thanks! Realized that Ilya is out of office. Adding Alexei for reviewing histograms.xml. Thanks, Alexei!
https://codereview.chromium.org/136393004/diff/130001/chrome/renderer/pepper/... File chrome/renderer/pepper/pepper_flash_renderer_host.cc (right): https://codereview.chromium.org/136393004/diff/130001/chrome/renderer/pepper/... chrome/renderer/pepper/pepper_flash_renderer_host.cc:49: // of rejecting PPB_Flash.Navigate() requests with non-simple headers. Can you expand the comment to mention that new entries should be added to the end and existing entries shouldn't be re-ordered or removed, since this ordering is used in a histogram? https://codereview.chromium.org/136393004/diff/130001/chrome/renderer/pepper/... chrome/renderer/pepper/pepper_flash_renderer_host.cc:324: UMA_HISTOGRAM_ENUMERATION("Plugin.FlashNavigateUsage", usage, Can you make a helper function in the anon namespace that logs this histogram? It can take |usage| as a param. (The macro expands to a lot of code, so this will avoid code bloat as well as possible typos in the name of the histogram since it won't be repeated.)
Thanks, Alexei! PTAL https://codereview.chromium.org/136393004/diff/130001/chrome/renderer/pepper/... File chrome/renderer/pepper/pepper_flash_renderer_host.cc (right): https://codereview.chromium.org/136393004/diff/130001/chrome/renderer/pepper/... chrome/renderer/pepper/pepper_flash_renderer_host.cc:49: // of rejecting PPB_Flash.Navigate() requests with non-simple headers. On 2014/01/30 20:23:31, Alexei Svitkine wrote: > Can you expand the comment to mention that new entries should be added to the > end and existing entries shouldn't be re-ordered or removed, since this ordering > is used in a histogram? Done. https://codereview.chromium.org/136393004/diff/130001/chrome/renderer/pepper/... chrome/renderer/pepper/pepper_flash_renderer_host.cc:324: UMA_HISTOGRAM_ENUMERATION("Plugin.FlashNavigateUsage", usage, Good point! Done.
On 2014/01/31 18:14:11, yzshen1 wrote: > Thanks, Alexei! PTAL > > https://codereview.chromium.org/136393004/diff/130001/chrome/renderer/pepper/... > File chrome/renderer/pepper/pepper_flash_renderer_host.cc (right): > > https://codereview.chromium.org/136393004/diff/130001/chrome/renderer/pepper/... > chrome/renderer/pepper/pepper_flash_renderer_host.cc:49: // of rejecting > PPB_Flash.Navigate() requests with non-simple headers. > On 2014/01/30 20:23:31, Alexei Svitkine wrote: > > Can you expand the comment to mention that new entries should be added to the > > end and existing entries shouldn't be re-ordered or removed, since this > ordering > > is used in a histogram? > > Done. > > https://codereview.chromium.org/136393004/diff/130001/chrome/renderer/pepper/... > chrome/renderer/pepper/pepper_flash_renderer_host.cc:324: > UMA_HISTOGRAM_ENUMERATION("Plugin.FlashNavigateUsage", usage, > Good point! Done. Friendly ping, Alexei.
LGTM
The CQ bit was checked by yzshen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yzshen@chromium.org/136393004/150001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) sync_integration_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by yzshen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yzshen@chromium.org/136393004/150001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, sync_integration_tests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by yzshen@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yzshen@chromium.org/136393004/150001
Message was sent while issue was closed.
Change committed as 249114 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
