Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(289)

Issue 1645313002: Don't apply the SandboxPlugins flag until we know a plugin will be used (Closed)

Created:
4 years, 10 months ago by fs
Modified:
4 years, 10 months ago
Reviewers:
pdr., Mike West
CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Don't apply the SandboxPlugins flag until we know a plugin will be used Move the check of the SandboxPlugins flag out of the pluginIsLoadable function and to just before the actual load/instantiation of the plugin is initiated. This means the URL and MIME-type is still subject to SecurityOrigin, (some) CSP and Mixed-Content checks, but the flag will not block resources if they will not use a plugin. Split pluginIsLoadable into one part that checks if the URL/MIME-type is allowed, and one part that checks if the plugin itself can be loaded/instantiated. The former is allowedToLoadObject while the latter is allowedToLoadPlugin. Only call the latter if we determine that a plugin will be used to view the content (URL or not). Sink the allowedToLoadPlugin check into loadPlugin, which in turn means it will apply to the code-path through createPluginWithoutLayoutObject() as well, while adding a call to allowedToLoadObject there as well. Also make sure that shouldUsePlugin() sets the |useFallback| out- variable before all returns. (Could previously be used uninitialized. Found by code inspection.) BUG=578916 Committed: https://crrev.com/4acd739260230f59c08858a31e795c2dcfdc54d4 Cr-Commit-Position: refs/heads/master@{#377559}

Patch Set 1 #

Total comments: 6

Patch Set 2 : pluginIsLoadable returns #

Total comments: 6

Patch Set 3 : Refactor, update names and add basic positive test #

Patch Set 4 : ASSERTs #

Patch Set 5 : Allow empty URLs to be rejected #

Patch Set 6 : Hoisted that compiles #

Patch Set 7 : (╯°□°)╯︵ ┻━┻ #

Patch Set 8 : Side-effecting assert == bad #

Total comments: 4

Patch Set 9 : Testharness-based test #

Messages

Total messages: 29 (11 generated)
pdr.
https://codereview.chromium.org/1645313002/diff/1/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (left): https://codereview.chromium.org/1645313002/diff/1/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp#oldcode577 third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:577: if (document().isSandboxed(SandboxPlugins)) It seems odd to me that sandboxing ...
4 years, 10 months ago (2016-01-29 22:14:54 UTC) #2
fs
https://codereview.chromium.org/1645313002/diff/1/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (left): https://codereview.chromium.org/1645313002/diff/1/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp#oldcode577 third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:577: if (document().isSandboxed(SandboxPlugins)) On 2016/01/29 at 22:14:54, pdr wrote: > ...
4 years, 10 months ago (2016-01-30 00:03:09 UTC) #3
fs
https://codereview.chromium.org/1645313002/diff/1/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (left): https://codereview.chromium.org/1645313002/diff/1/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp#oldcode577 third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:577: if (document().isSandboxed(SandboxPlugins)) On 2016/01/30 at 00:03:09, fs wrote: > ...
4 years, 10 months ago (2016-02-01 09:21:06 UTC) #5
fs
https://codereview.chromium.org/1645313002/diff/1/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp File third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp (left): https://codereview.chromium.org/1645313002/diff/1/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp#oldcode577 third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:577: if (document().isSandboxed(SandboxPlugins)) On 2016/02/01 at 09:21:06, fs wrote: > ...
4 years, 10 months ago (2016-02-01 11:52:35 UTC) #6
pdr.
+cc mkwst, could you take a look at this too? The more securiteyes the merrier. ...
4 years, 10 months ago (2016-02-02 04:32:42 UTC) #8
Mike West
On 2016/02/02 at 04:32:42, pdr wrote: > +cc mkwst, could you take a look at ...
4 years, 10 months ago (2016-02-04 18:20:08 UTC) #9
Mike West
On 2016/02/04 at 18:20:08, Mike West (OOO until 2nd) wrote: > On 2016/02/02 at 04:32:42, ...
4 years, 10 months ago (2016-02-04 18:20:20 UTC) #10
fs
On 2016/02/04 at 18:20:20, mkwst wrote: > On 2016/02/04 at 18:20:08, Mike West (OOO until ...
4 years, 10 months ago (2016-02-04 18:52:04 UTC) #11
fs
Prodding this a bit again, but am a bit puzzled by the following error: https://storage.googleapis.com/chromium-layout-test-archives/mac_chromium_rel_ng/183607/layout-test-results/results.html ...
4 years, 10 months ago (2016-02-22 18:36:56 UTC) #17
pdr.
On 2016/02/22 at 18:36:56, fs wrote: > Prodding this a bit again, but am a ...
4 years, 10 months ago (2016-02-22 23:18:35 UTC) #18
fs
On 2016/02/22 at 23:18:35, pdr wrote: > On 2016/02/22 at 18:36:56, fs wrote: > > ...
4 years, 10 months ago (2016-02-23 16:05:13 UTC) #19
pdr.
On 2016/02/23 at 16:05:13, fs wrote: > On 2016/02/22 at 23:18:35, pdr wrote: > > ...
4 years, 10 months ago (2016-02-23 21:29:16 UTC) #20
fs
On 2016/02/23 at 21:29:16, pdr wrote: > On 2016/02/23 at 16:05:13, fs wrote: > > ...
4 years, 10 months ago (2016-02-24 12:11:30 UTC) #21
Mike West
LGTM with nits about the test. Sorry this completely fell off my radar! Thanks for ...
4 years, 10 months ago (2016-02-24 13:30:55 UTC) #22
fs
https://codereview.chromium.org/1645313002/diff/180001/third_party/WebKit/LayoutTests/http/tests/svg/svg-in-object-in-sandboxed-iframe.html File third_party/WebKit/LayoutTests/http/tests/svg/svg-in-object-in-sandboxed-iframe.html (right): https://codereview.chromium.org/1645313002/diff/180001/third_party/WebKit/LayoutTests/http/tests/svg/svg-in-object-in-sandboxed-iframe.html#newcode8 third_party/WebKit/LayoutTests/http/tests/svg/svg-in-object-in-sandboxed-iframe.html:8: } On 2016/02/24 at 13:30:55, Mike West wrote: > ...
4 years, 10 months ago (2016-02-24 17:57:16 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1645313002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1645313002/200001
4 years, 10 months ago (2016-02-25 12:28:09 UTC) #26
commit-bot: I haz the power
Committed patchset #9 (id:200001)
4 years, 10 months ago (2016-02-25 12:34:42 UTC) #27
commit-bot: I haz the power
4 years, 10 months ago (2016-02-25 12:36:01 UTC) #29
Message was sent while issue was closed.
Patchset 9 (id:??) landed as
https://crrev.com/4acd739260230f59c08858a31e795c2dcfdc54d4
Cr-Commit-Position: refs/heads/master@{#377559}

Powered by Google App Engine
This is Rietveld 408576698