Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2364633004:
Lock down the registration of blob:chrome-extension:// URLs (Closed)
|
Created:
4 years, 3 months ago by ncarter (slow) Modified:
4 years, 2 months ago CC:
chromium-apps-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, extensions-reviews_chromium.org, loading-reviews_chromium.org, mmenke, Randy Smith (Not in Mondays), site-isolation-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate ChildProcessSecurityPolicy so that the chrome-extension:// scheme
is considered "web safe" to be requestable from any process, but only
"web safe" to commit in extension processes.
In ChildProcessSecurityPolicy::CanRequestURL and CanCommitURL, when
seeing blob and filesystem urls, make a security decision based
on the inner origin rather than the scheme.
When the extensions ProcessManager (via ExtensionWebContentsObserver) notices a
RenderFrame being created in an extension SiteInstance, grant that process
permission to commit chrome-extension:// URLs.
In BlobDispatcherHost, only allow creation of blob URLs from processes that
would be able to commit them.
Add a security exploit browsertest that verifies the above mechanisms working
together.
BUG=644966
Committed: https://crrev.com/a411fd062bc68fc2b5fc3aca7e4cbb8e4a3e074e
Committed: https://crrev.com/2a8ba8c4c186e5ea0a2ed938cc5d41441af64228
Cr-Original-Commit-Position: refs/heads/master@{#421964}
Cr-Commit-Position: refs/heads/master@{#422474}
Patch Set 1 #Patch Set 2 : storage DataElement unknown. #Patch Set 3 : Register kExtensionScheme as WebSafeIsolated #Patch Set 4 : Stuff. #Patch Set 5 : Format. #Patch Set 6 : ' #Patch Set 7 : Fix extension shell compile. #Patch Set 8 : Pare down CL #
Total comments: 13
Patch Set 9 : Acquire lock in CanSetAsOriginHeader #Patch Set 10 : Reviewer fixes. #Patch Set 11 : ENABLE_EXTENSIONS #Patch Set 12 : Phrasing. #
Total comments: 18
Patch Set 13 : Reviewer fixes. #Patch Set 14 : Fix layout test. #Patch Set 15 : Fix layout test. #
Total comments: 4
Dependent Patchsets: Messages
Total messages: 87 (57 generated)
The CQ bit was checked by nick@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_asan_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_...) 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 nick@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by nick@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by nick@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by nick@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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nick@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...
Description was changed from ========== Update ChildProcessSecurityPolicy so that the chrome-extension:// scheme is considered "web safe" to be requestable from any process, but only "web safe" to commit in extension processes. In ChildProcessSecurityPolicy::CanRequestURL and CanCommitURL, when seeing blob and filesystem urls, make a security decision based on the inner origin rather than the scheme. In BlobDispatcherHost, only allow creation of blob URLs from processes that would be able to commit them. TODO(nick) Enforce open filesystem too BUG= ========== to ========== Update ChildProcessSecurityPolicy so that the chrome-extension:// scheme is considered "web safe" to be requestable from any process, but only "web safe" to commit in extension processes. In ChildProcessSecurityPolicy::CanRequestURL and CanCommitURL, when seeing blob and filesystem urls, make a security decision based on the inner origin rather than the scheme. In BlobDispatcherHost, only allow creation of blob URLs from processes that would be able to commit them. TODO(nick) Enforce open filesystem too BUG= ==========
Description was changed from ========== Update ChildProcessSecurityPolicy so that the chrome-extension:// scheme is considered "web safe" to be requestable from any process, but only "web safe" to commit in extension processes. In ChildProcessSecurityPolicy::CanRequestURL and CanCommitURL, when seeing blob and filesystem urls, make a security decision based on the inner origin rather than the scheme. In BlobDispatcherHost, only allow creation of blob URLs from processes that would be able to commit them. TODO(nick) Enforce open filesystem too BUG= ========== to ========== Update ChildProcessSecurityPolicy so that the chrome-extension:// scheme is considered "web safe" to be requestable from any process, but only "web safe" to commit in extension processes. In ChildProcessSecurityPolicy::CanRequestURL and CanCommitURL, when seeing blob and filesystem urls, make a security decision based on the inner origin rather than the scheme. When the extensions ProcessManager (via ExtensionWebContentsObserver) notices a RenderFrame being created in an extension SiteInstance, grant that process permission to commit chrome-extension:// URLs. In BlobDispatcherHost, only allow creation of blob URLs from processes that would be able to commit them. Add a security exploit browsertest that verifies the above mechanisms working together. BUG=644966 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by nick@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...
nick@chromium.org changed reviewers: + creis@chromium.org, mek@chromium.org, thestig@chromium.org
creis@chromium.org: Please review everything mek@chromium.org: Please review changes in extensions/ thestig@chromium.org: Please review changes in chrome/
On 2016/09/28 20:35:28, ncarter wrote: > mailto:mek@chromium.org: Please review changes in extensions/ OOO
nick@chromium.org changed reviewers: + reillyg@chromium.org - mek@chromium.org
-mek +reillyg reillyg: please review extensions/ changes
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Looks good, apart from the DevTools extension question and the failing test. https://codereview.chromium.org/2364633004/diff/140001/chrome/browser/browser... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/2364633004/diff/140001/chrome/browser/browser... chrome/browser/browser_process_impl.cc:224: // commit (including in iframes) in extension processes. Sanity check: Will this affect DevTools extensions? https://codereview.chromium.org/2364633004/diff/140001/content/browser/child_... File content/browser/child_process_security_policy_impl.cc (left): https://codereview.chromium.org/2364633004/diff/140001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:636: // https://crbug.com/515309. Can we keep this TODO, or is there a reason that it wouldn't work? https://codereview.chromium.org/2364633004/diff/140001/content/public/browser... File content/public/browser/child_process_security_policy.h (right): https://codereview.chromium.org/2364633004/diff/140001/content/public/browser... content/public/browser/child_process_security_policy.h:57: // to accept an URL instead. Good call! Can we list a bug number here so we don't lose track of it? https://codereview.chromium.org/2364633004/diff/140001/content/public/browser... content/public/browser/child_process_security_policy.h:80: virtual bool CanRequestURL(int child_id, const GURL& url) = 0; Are these only public so tests can call them? I suppose that's not a problem-- we've got similar CanReadFile (etc) ones below, and there's no risk to having embedders call these methods once they're exposed.
reillyg@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Adding Devlin because I'm not 100% sure but I think that until we have extension pages always loading in out-of-process iframes then we need to be able to commit a chrome-extension:// URL from a regular renderer.
On 2016/09/29 05:00:07, Reilly Grant wrote: > Adding Devlin because I'm not 100% sure but I think that until we have extension > pages always loading in out-of-process iframes then we need to be able to commit > a chrome-extension:// URL from a regular renderer. That's actually how things work now, as of M55: https://cs.chromium.org/chromium/src/chrome/common/extensions/extension_proce... However, if we want to have this fix mergeable to M54, it probably makes sense to gate the scheme registration behavior on IsIsolateExtensionsEnabled(). This would definitely benefit from Devlin's review, though.
https://codereview.chromium.org/2364633004/diff/140001/extensions/shell/brows... File extensions/shell/browser/shell_browser_main_parts.cc (right): https://codereview.chromium.org/2364633004/diff/140001/extensions/shell/brows... extensions/shell/browser/shell_browser_main_parts.cc:137: ->RegisterWebSafeIsolatedScheme(extensions::kExtensionResourceScheme); reillyg's comment prompted me to rethink this. If --isolate-extensions is not fully functional in extensions shell (and it's not, afaik), then this is not probably correct. However, the tests pass -- probably indicates that we don't have extensions_browsertests that exercise oopif cases. So two options here: (1) leave this as-is, for the sake of being consistent with chrome/; treating this as a step towards having --isolate-extensions work in the extensions shell. (2) Revert this part of the change, and leave these schemes as web-safe until we push an implementation of --isolate-extensions down into the //extensions layer. This situation makes me think it's fairly urgent to arrive at a consistent process policy for extensions shell and chrome. Thoughts?
On 2016/09/29 16:13:02, ncarter wrote: > On 2016/09/29 05:00:07, Reilly Grant wrote: > > Adding Devlin because I'm not 100% sure but I think that until we have > extension > > pages always loading in out-of-process iframes then we need to be able to > commit > > a chrome-extension:// URL from a regular renderer. > > That's actually how things work now, as of M55: > https://cs.chromium.org/chromium/src/chrome/common/extensions/extension_proce... > However, if we want to have this fix mergeable to M54, it probably makes sense > to gate the scheme registration behavior on IsIsolateExtensionsEnabled(). > > This would definitely benefit from Devlin's review, though. I'd gate it to be safe. Since we have a bunch of other checks, we'll need to do a sweep of them anyway. Alternatively, we could add that to the merged patch, but it's typically easier to merge an identical version. https://codereview.chromium.org/2364633004/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/2364633004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/process_manager_browsertest.cc:684: GURL("blob:chrome-extension://some-extension-id/some-guid"))); nit: I'd recommend using some kind of valid id, because I could definitely see us adding logic to disallow invalid ids (or even foreign ids). https://codereview.chromium.org/2364633004/diff/140001/extensions/shell/brows... File extensions/shell/browser/shell_browser_main_parts.cc (right): https://codereview.chromium.org/2364633004/diff/140001/extensions/shell/brows... extensions/shell/browser/shell_browser_main_parts.cc:137: ->RegisterWebSafeIsolatedScheme(extensions::kExtensionResourceScheme); On 2016/09/29 16:27:43, ncarter wrote: > reillyg's comment prompted me to rethink this. If --isolate-extensions is not > fully functional in extensions shell (and it's not, afaik), then this is not > probably correct. However, the tests pass -- probably indicates that we don't > have extensions_browsertests that exercise oopif cases. > > So two options here: > (1) leave this as-is, for the sake of being consistent with chrome/; treating > this as a step towards having --isolate-extensions work in the extensions shell. > (2) Revert this part of the change, and leave these schemes as web-safe until > we push an implementation of --isolate-extensions down into the //extensions > layer. > > This situation makes me think it's fairly urgent to arrive at a consistent > process policy for extensions shell and chrome. > > Thoughts? My $0.02: If this moves us toward parity, I'd say keep it. We'll need it later, and it might help us avoid regressing.
The CQ bit was checked by nick@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 checked by nick@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by nick@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 checked by nick@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...
Charlie: PTAL. pretty sure the current patch set should be all green. Updated to include fixes for the content-script issue, and the devtools-extension issue. https://codereview.chromium.org/2364633004/diff/140001/chrome/browser/browser... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/2364633004/diff/140001/chrome/browser/browser... chrome/browser/browser_process_impl.cc:224: // commit (including in iframes) in extension processes. On 2016/09/28 22:07:16, Charlie Reis wrote: > Sanity check: Will this affect DevTools extensions? Excellent catch. This was a real bug; I've fixed it and added a test. https://codereview.chromium.org/2364633004/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/2364633004/diff/140001/chrome/browser/extensi... chrome/browser/extensions/process_manager_browsertest.cc:684: GURL("blob:chrome-extension://some-extension-id/some-guid"))); On 2016/09/29 17:31:09, Devlin (catching up) wrote: > nit: I'd recommend using some kind of valid id, because I could definitely see > us adding logic to disallow invalid ids (or even foreign ids). Your point is good. It would actually be pretty straightforward to convert the GrantScheme calls in this CL to be GrantOrigin() based: we already have a particular extension in hand when we're doing the granting. The only reason I haven't done this, is because I'm paranoid about whether we have any races between the Grant() operations on the UI thread, and the CanAccessWhatever checks on the IO thread, and making the grant be scheme-based seemed harder to screw up (the risk being, I guess, if we ever have a case where one extension can do a renderer-initiated, in-process navigation to another extension? Like I said, I'm paranoid here.) For now, though, I prefer this test to use invalid ids only; it makes it clearer that the enforcement today is based simply on the scheme. And since this test includes both EXPECT_TRUE and EXPECT_FALSE cases, I believe this test will start failing if we add any kind of logic to disallow invalid ids. At that point, I'd hope that we would expand this to include both valid and invalid ids. https://codereview.chromium.org/2364633004/diff/140001/content/browser/child_... File content/browser/child_process_security_policy_impl.cc (left): https://codereview.chromium.org/2364633004/diff/140001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:636: // https://crbug.com/515309. On 2016/09/28 22:07:16, Charlie Reis wrote: > Can we keep this TODO, or is there a reason that it wouldn't work? I updated the TODO. https://codereview.chromium.org/2364633004/diff/140001/content/public/browser... File content/public/browser/child_process_security_policy.h (right): https://codereview.chromium.org/2364633004/diff/140001/content/public/browser... content/public/browser/child_process_security_policy.h:57: // to accept an URL instead. On 2016/09/28 22:07:16, Charlie Reis wrote: > Good call! Can we list a bug number here so we don't lose track of it? Done. https://codereview.chromium.org/2364633004/diff/140001/content/public/browser... content/public/browser/child_process_security_policy.h:80: virtual bool CanRequestURL(int child_id, const GURL& url) = 0; On 2016/09/28 22:07:16, Charlie Reis wrote: > Are these only public so tests can call them? I suppose that's not a problem-- > we've got similar CanReadFile (etc) ones below, and there's no risk to having > embedders call these methods once they're exposed. Yes, they're exposed for test. Embedders already control the knobs that determine the return value here, so I don't think there's an information hiding concern. https://codereview.chromium.org/2364633004/diff/140001/extensions/shell/brows... File extensions/shell/browser/shell_browser_main_parts.cc (right): https://codereview.chromium.org/2364633004/diff/140001/extensions/shell/brows... extensions/shell/browser/shell_browser_main_parts.cc:137: ->RegisterWebSafeIsolatedScheme(extensions::kExtensionResourceScheme); On 2016/09/29 17:31:09, Devlin (catching up) wrote: > On 2016/09/29 16:27:43, ncarter wrote: > > reillyg's comment prompted me to rethink this. If --isolate-extensions is not > > fully functional in extensions shell (and it's not, afaik), then this is not > > probably correct. However, the tests pass -- probably indicates that we don't > > have extensions_browsertests that exercise oopif cases. > > > > So two options here: > > (1) leave this as-is, for the sake of being consistent with chrome/; treating > > this as a step towards having --isolate-extensions work in the extensions > shell. > > (2) Revert this part of the change, and leave these schemes as web-safe until > > we push an implementation of --isolate-extensions down into the //extensions > > layer. > > > > This situation makes me think it's fairly urgent to arrive at a consistent > > process policy for extensions shell and chrome. > > > > Thoughts? > > My $0.02: If this moves us toward parity, I'd say keep it. We'll need it later, > and it might help us avoid regressing. I decided to revert this hunk, with the logic that when we do push --isolate-extensions down into src/extensions, we probably will want to move this code to a shared location anyway.
LGTM! Minor nits below. https://codereview.chromium.org/2364633004/diff/210001/chrome/browser/browser... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/2364633004/diff/210001/chrome/browser/browser... chrome/browser/browser_process_impl.cc:226: // commit (including in iframes) in extension processes. nit: Move comment inside the conditional, since it doesn't apply to the else block. https://codereview.chromium.org/2364633004/diff/210001/chrome/browser/browser... chrome/browser/browser_process_impl.cc:227: if (extensions::IsIsolateExtensionsEnabled()) { Nice. Agreed that we need this if we want to merge this to M54, where --isolate-extensions is still part of a Finch trial. https://codereview.chromium.org/2364633004/diff/210001/chrome/browser/devtool... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/2364633004/diff/210001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1004: // Creating blobs from chrome-extension:// origins is only permtted if the nit: permitted https://codereview.chromium.org/2364633004/diff/210001/chrome/browser/extensi... File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/2364633004/diff/210001/chrome/browser/extensi... chrome/browser/extensions/process_manager_browsertest.cc:700: EXPECT_TRUE(policy->CanCommitURL( nit: Blank line before (to improve legibility)? https://codereview.chromium.org/2364633004/diff/210001/content/browser/child_... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/2364633004/diff/210001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:334: // IsWebSafeScheme(), and then eliminate the next two lines. Let's list https://crbug.com/651534 here, too. https://codereview.chromium.org/2364633004/diff/210001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:689: // enforce that http pages cannot commit in an extension process. Good point. https://codereview.chromium.org/2364633004/diff/210001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:725: if (base::ContainsKey(schemes_okay_to_appear_as_origin_headers_, Now I see why Joel's CanSetAsOriginHeader refactor proved useful here. :) This set's name wouldn't have made as much sense if we consulted it from CanCommitURL. https://codereview.chromium.org/2364633004/diff/210001/content/browser/child_... File content/browser/child_process_security_policy_impl.h (right): https://codereview.chromium.org/2364633004/diff/210001/content/browser/child_... content/browser/child_process_security_policy_impl.h:232: // These schemes are white-listed for all child processes in various contexts. Is it worth keeping the comment about being protected by |lock_|?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2364633004/diff/210001/chrome/browser/chrome_... File chrome/browser/chrome_security_exploit_browsertest.cc (right): https://codereview.chromium.org/2364633004/diff/210001/chrome/browser/chrome_... chrome/browser/chrome_security_exploit_browsertest.cc:87: std::string target_origin = // The bookmark manager extension. nit: just put the comment on the line above.
extensions lgtm
https://codereview.chromium.org/2364633004/diff/210001/chrome/browser/browser... File chrome/browser/browser_process_impl.cc (right): https://codereview.chromium.org/2364633004/diff/210001/chrome/browser/browser... chrome/browser/browser_process_impl.cc:226: // commit (including in iframes) in extension processes. On 2016/09/29 21:39:37, Charlie Reis wrote: > nit: Move comment inside the conditional, since it doesn't apply to the else > block. Done. https://codereview.chromium.org/2364633004/diff/210001/chrome/browser/browser... chrome/browser/browser_process_impl.cc:227: if (extensions::IsIsolateExtensionsEnabled()) { On 2016/09/29 21:39:37, Charlie Reis wrote: > Nice. Agreed that we need this if we want to merge this to M54, where > --isolate-extensions is still part of a Finch trial. Done. FWIW, I'm a little nervous about consulting the finch config here -- it's really early in startup -- but I figure if it doesn't work, it should be obvious. https://codereview.chromium.org/2364633004/diff/210001/chrome/browser/chrome_... File chrome/browser/chrome_security_exploit_browsertest.cc (right): https://codereview.chromium.org/2364633004/diff/210001/chrome/browser/chrome_... chrome/browser/chrome_security_exploit_browsertest.cc:87: std::string target_origin = // The bookmark manager extension. On 2016/09/29 21:47:07, Lei Zhang wrote: > nit: just put the comment on the line above. Done. https://codereview.chromium.org/2364633004/diff/210001/chrome/browser/devtool... File chrome/browser/devtools/devtools_sanity_browsertest.cc (right): https://codereview.chromium.org/2364633004/diff/210001/chrome/browser/devtool... chrome/browser/devtools/devtools_sanity_browsertest.cc:1004: // Creating blobs from chrome-extension:// origins is only permtted if the On 2016/09/29 21:39:37, Charlie Reis wrote: > nit: permitted Done. https://codereview.chromium.org/2364633004/diff/210001/chrome/browser/extensi... File chrome/browser/extensions/process_manager_browsertest.cc (right): https://codereview.chromium.org/2364633004/diff/210001/chrome/browser/extensi... chrome/browser/extensions/process_manager_browsertest.cc:700: EXPECT_TRUE(policy->CanCommitURL( On 2016/09/29 21:39:37, Charlie Reis wrote: > nit: Blank line before (to improve legibility)? Done. https://codereview.chromium.org/2364633004/diff/210001/content/browser/child_... File content/browser/child_process_security_policy_impl.cc (right): https://codereview.chromium.org/2364633004/diff/210001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:334: // IsWebSafeScheme(), and then eliminate the next two lines. On 2016/09/29 21:39:37, Charlie Reis wrote: > Let's list https://crbug.com/651534 here, too. Done. https://codereview.chromium.org/2364633004/diff/210001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:689: // enforce that http pages cannot commit in an extension process. On 2016/09/29 21:39:37, Charlie Reis wrote: > Good point. Done. https://codereview.chromium.org/2364633004/diff/210001/content/browser/child_... content/browser/child_process_security_policy_impl.cc:725: if (base::ContainsKey(schemes_okay_to_appear_as_origin_headers_, On 2016/09/29 21:39:37, Charlie Reis wrote: > Now I see why Joel's CanSetAsOriginHeader refactor proved useful here. :) This > set's name wouldn't have made as much sense if we consulted it from > CanCommitURL. Yeah, well-timed ... except that it'll complicate the merge. https://codereview.chromium.org/2364633004/diff/210001/content/browser/child_... File content/browser/child_process_security_policy_impl.h (right): https://codereview.chromium.org/2364633004/diff/210001/content/browser/child_... content/browser/child_process_security_policy_impl.h:232: // These schemes are white-listed for all child processes in various contexts. On 2016/09/29 21:39:37, Charlie Reis wrote: > Is it worth keeping the comment about being protected by |lock_|? Done.
The CQ bit was checked by nick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, creis@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2364633004/#ps230001 (title: "Reviewer fixes.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Update ChildProcessSecurityPolicy so that the chrome-extension:// scheme is considered "web safe" to be requestable from any process, but only "web safe" to commit in extension processes. In ChildProcessSecurityPolicy::CanRequestURL and CanCommitURL, when seeing blob and filesystem urls, make a security decision based on the inner origin rather than the scheme. When the extensions ProcessManager (via ExtensionWebContentsObserver) notices a RenderFrame being created in an extension SiteInstance, grant that process permission to commit chrome-extension:// URLs. In BlobDispatcherHost, only allow creation of blob URLs from processes that would be able to commit them. Add a security exploit browsertest that verifies the above mechanisms working together. BUG=644966 ========== to ========== Update ChildProcessSecurityPolicy so that the chrome-extension:// scheme is considered "web safe" to be requestable from any process, but only "web safe" to commit in extension processes. In ChildProcessSecurityPolicy::CanRequestURL and CanCommitURL, when seeing blob and filesystem urls, make a security decision based on the inner origin rather than the scheme. When the extensions ProcessManager (via ExtensionWebContentsObserver) notices a RenderFrame being created in an extension SiteInstance, grant that process permission to commit chrome-extension:// URLs. In BlobDispatcherHost, only allow creation of blob URLs from processes that would be able to commit them. Add a security exploit browsertest that verifies the above mechanisms working together. BUG=644966 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:230001)
Message was sent while issue was closed.
Description was changed from ========== Update ChildProcessSecurityPolicy so that the chrome-extension:// scheme is considered "web safe" to be requestable from any process, but only "web safe" to commit in extension processes. In ChildProcessSecurityPolicy::CanRequestURL and CanCommitURL, when seeing blob and filesystem urls, make a security decision based on the inner origin rather than the scheme. When the extensions ProcessManager (via ExtensionWebContentsObserver) notices a RenderFrame being created in an extension SiteInstance, grant that process permission to commit chrome-extension:// URLs. In BlobDispatcherHost, only allow creation of blob URLs from processes that would be able to commit them. Add a security exploit browsertest that verifies the above mechanisms working together. BUG=644966 ========== to ========== Update ChildProcessSecurityPolicy so that the chrome-extension:// scheme is considered "web safe" to be requestable from any process, but only "web safe" to commit in extension processes. In ChildProcessSecurityPolicy::CanRequestURL and CanCommitURL, when seeing blob and filesystem urls, make a security decision based on the inner origin rather than the scheme. When the extensions ProcessManager (via ExtensionWebContentsObserver) notices a RenderFrame being created in an extension SiteInstance, grant that process permission to commit chrome-extension:// URLs. In BlobDispatcherHost, only allow creation of blob URLs from processes that would be able to commit them. Add a security exploit browsertest that verifies the above mechanisms working together. BUG=644966 Committed: https://crrev.com/a411fd062bc68fc2b5fc3aca7e4cbb8e4a3e074e Cr-Commit-Position: refs/heads/master@{#421964} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/a411fd062bc68fc2b5fc3aca7e4cbb8e4a3e074e Cr-Commit-Position: refs/heads/master@{#421964}
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:230001) has been created in https://codereview.chromium.org/2385553002/ by tsergeant@chromium.org. The reason for reverting is: Speculative revert to fix failing test http/tests/xmlhttprequest/xhr-to-blob-in-isolated-world.html in webkit_tests. See failures in: https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7/builds/46369 https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.10/build....
Message was sent while issue was closed.
Description was changed from ========== Update ChildProcessSecurityPolicy so that the chrome-extension:// scheme is considered "web safe" to be requestable from any process, but only "web safe" to commit in extension processes. In ChildProcessSecurityPolicy::CanRequestURL and CanCommitURL, when seeing blob and filesystem urls, make a security decision based on the inner origin rather than the scheme. When the extensions ProcessManager (via ExtensionWebContentsObserver) notices a RenderFrame being created in an extension SiteInstance, grant that process permission to commit chrome-extension:// URLs. In BlobDispatcherHost, only allow creation of blob URLs from processes that would be able to commit them. Add a security exploit browsertest that verifies the above mechanisms working together. BUG=644966 Committed: https://crrev.com/a411fd062bc68fc2b5fc3aca7e4cbb8e4a3e074e Cr-Commit-Position: refs/heads/master@{#421964} ========== to ========== Update ChildProcessSecurityPolicy so that the chrome-extension:// scheme is considered "web safe" to be requestable from any process, but only "web safe" to commit in extension processes. In ChildProcessSecurityPolicy::CanRequestURL and CanCommitURL, when seeing blob and filesystem urls, make a security decision based on the inner origin rather than the scheme. When the extensions ProcessManager (via ExtensionWebContentsObserver) notices a RenderFrame being created in an extension SiteInstance, grant that process permission to commit chrome-extension:// URLs. In BlobDispatcherHost, only allow creation of blob URLs from processes that would be able to commit them. Add a security exploit browsertest that verifies the above mechanisms working together. BUG=644966 Committed: https://crrev.com/a411fd062bc68fc2b5fc3aca7e4cbb8e4a3e074e Cr-Commit-Position: refs/heads/master@{#421964} ==========
charlie: I'd like to re-land this. PTAL @ ps 13 vs 15, which should consist of only the layout test fix.
The updated layout test seems reasonable to me. LGTM.
The CQ bit was checked by nick@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2364633004/#ps270001 (title: "Fix layout test.")
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
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nick@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Update ChildProcessSecurityPolicy so that the chrome-extension:// scheme is considered "web safe" to be requestable from any process, but only "web safe" to commit in extension processes. In ChildProcessSecurityPolicy::CanRequestURL and CanCommitURL, when seeing blob and filesystem urls, make a security decision based on the inner origin rather than the scheme. When the extensions ProcessManager (via ExtensionWebContentsObserver) notices a RenderFrame being created in an extension SiteInstance, grant that process permission to commit chrome-extension:// URLs. In BlobDispatcherHost, only allow creation of blob URLs from processes that would be able to commit them. Add a security exploit browsertest that verifies the above mechanisms working together. BUG=644966 Committed: https://crrev.com/a411fd062bc68fc2b5fc3aca7e4cbb8e4a3e074e Cr-Commit-Position: refs/heads/master@{#421964} ========== to ========== Update ChildProcessSecurityPolicy so that the chrome-extension:// scheme is considered "web safe" to be requestable from any process, but only "web safe" to commit in extension processes. In ChildProcessSecurityPolicy::CanRequestURL and CanCommitURL, when seeing blob and filesystem urls, make a security decision based on the inner origin rather than the scheme. When the extensions ProcessManager (via ExtensionWebContentsObserver) notices a RenderFrame being created in an extension SiteInstance, grant that process permission to commit chrome-extension:// URLs. In BlobDispatcherHost, only allow creation of blob URLs from processes that would be able to commit them. Add a security exploit browsertest that verifies the above mechanisms working together. BUG=644966 Committed: https://crrev.com/a411fd062bc68fc2b5fc3aca7e4cbb8e4a3e074e Cr-Commit-Position: refs/heads/master@{#421964} ==========
Message was sent while issue was closed.
Committed patchset #15 (id:270001)
Message was sent while issue was closed.
Description was changed from ========== Update ChildProcessSecurityPolicy so that the chrome-extension:// scheme is considered "web safe" to be requestable from any process, but only "web safe" to commit in extension processes. In ChildProcessSecurityPolicy::CanRequestURL and CanCommitURL, when seeing blob and filesystem urls, make a security decision based on the inner origin rather than the scheme. When the extensions ProcessManager (via ExtensionWebContentsObserver) notices a RenderFrame being created in an extension SiteInstance, grant that process permission to commit chrome-extension:// URLs. In BlobDispatcherHost, only allow creation of blob URLs from processes that would be able to commit them. Add a security exploit browsertest that verifies the above mechanisms working together. BUG=644966 Committed: https://crrev.com/a411fd062bc68fc2b5fc3aca7e4cbb8e4a3e074e Cr-Commit-Position: refs/heads/master@{#421964} ========== to ========== Update ChildProcessSecurityPolicy so that the chrome-extension:// scheme is considered "web safe" to be requestable from any process, but only "web safe" to commit in extension processes. In ChildProcessSecurityPolicy::CanRequestURL and CanCommitURL, when seeing blob and filesystem urls, make a security decision based on the inner origin rather than the scheme. When the extensions ProcessManager (via ExtensionWebContentsObserver) notices a RenderFrame being created in an extension SiteInstance, grant that process permission to commit chrome-extension:// URLs. In BlobDispatcherHost, only allow creation of blob URLs from processes that would be able to commit them. Add a security exploit browsertest that verifies the above mechanisms working together. BUG=644966 Committed: https://crrev.com/a411fd062bc68fc2b5fc3aca7e4cbb8e4a3e074e Committed: https://crrev.com/2a8ba8c4c186e5ea0a2ed938cc5d41441af64228 Cr-Original-Commit-Position: refs/heads/master@{#421964} Cr-Commit-Position: refs/heads/master@{#422474} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/2a8ba8c4c186e5ea0a2ed938cc5d41441af64228 Cr-Commit-Position: refs/heads/master@{#422474}
Message was sent while issue was closed.
jam@chromium.org changed reviewers: + jam@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2364633004/diff/270001/chrome/browser/DEPS File chrome/browser/DEPS (right): https://codereview.chromium.org/2364633004/diff/270001/chrome/browser/DEPS#ne... chrome/browser/DEPS:112: '+content/common', we never allow includes from within content from outside, even for tests. Can you wrap the code you need with a helper method in content/public/test and then call that from the browsertest?
Message was sent while issue was closed.
https://codereview.chromium.org/2364633004/diff/270001/chrome/browser/DEPS File chrome/browser/DEPS (right): https://codereview.chromium.org/2364633004/diff/270001/chrome/browser/DEPS#ne... chrome/browser/DEPS:112: '+content/common', On 2016/10/04 23:28:54, jam wrote: > we never allow includes from within content from outside, even for tests. > > Can you wrap the code you need with a helper method in content/public/test and > then call that from the browsertest? Hi John -- can you respond to the argument made on line 109? Exploits are usually layering violations by definition; the bug under test here is a bug in //content, that's can only be reproed in //chrome (because extensions) and the fix lives in //extensions. Charlie, nasko and I had discussed this before carving out this exception, and it all seemed reasonable to us. Test methods like you suggest are possible, but I'd need several of them per test, and they have no chance of reusability, and they just seem like the restriction ties the hands of testwriters here. We need to make these sorts of cases easier to test, not harder; curious if you have any ideas for ways to do that that also fit inside our layering goals for the architecture.
Message was sent while issue was closed.
https://codereview.chromium.org/2364633004/diff/270001/chrome/browser/DEPS File chrome/browser/DEPS (right): https://codereview.chromium.org/2364633004/diff/270001/chrome/browser/DEPS#ne... chrome/browser/DEPS:112: '+content/common', On 2016/10/05 18:22:09, ncarter wrote: > On 2016/10/04 23:28:54, jam wrote: > > we never allow includes from within content from outside, even for tests. > > > > Can you wrap the code you need with a helper method in content/public/test and > > then call that from the browsertest? > > Hi John -- can you respond to the argument made on line 109? Exploits are > usually layering violations by definition; I don't really grep this sentence; it all depends on how you define layering violations. Regardless of the semantics though, there are way of testing this without having chrome reach into the internals of content by using public methods. This is the pattern we've used for several years, so I don't see a reason to change that now. > the bug under test here is a bug in > //content, that's can only be reproed in //chrome (because extensions) and the > fix lives in //extensions. Sure, that's fine. > > Charlie, nasko and I had discussed this before carving out this exception, and > it all seemed reasonable to us. > > Test methods like you suggest are possible, but I'd need several of them per > test, and they have no chance of reusability, I'm not sure how the fact that we have non-reusable test methods affects things? It's a small effort to create public methods for this, so it's not a large overhead. > and they just seem like the > restriction ties the hands of testwriters here. Why does have having a public test method restrict test authors? > We need to make these sorts of > cases easier to test, not harder; curious if you have any ideas for ways to do > that that also fit inside our layering goals for the architecture. Agreed we need more tests, however no need to break the separation of chrome vs content internals that we've spent a few years creating, and then have maintained for a few years since.
Message was sent while issue was closed.
On 2016/10/05 19:11:08, jam wrote: > https://codereview.chromium.org/2364633004/diff/270001/chrome/browser/DEPS > File chrome/browser/DEPS (right): > > https://codereview.chromium.org/2364633004/diff/270001/chrome/browser/DEPS#ne... > chrome/browser/DEPS:112: '+content/common', > On 2016/10/05 18:22:09, ncarter wrote: > > On 2016/10/04 23:28:54, jam wrote: > > > we never allow includes from within content from outside, even for tests. > > > > > > Can you wrap the code you need with a helper method in content/public/test > and > > > then call that from the browsertest? > > > > Hi John -- can you respond to the argument made on line 109? Exploits are > > usually layering violations by definition; > > I don't really grep this sentence; it all depends on how you define layering > violations. > > Regardless of the semantics though, there are way of testing this without having > chrome reach into the internals of content by using public methods. This is the > pattern we've used for several years, so I don't see a reason to change that > now. > > > the bug under test here is a bug in > > //content, that's can only be reproed in //chrome (because extensions) and the > > fix lives in //extensions. > > Sure, that's fine. > > > > > Charlie, nasko and I had discussed this before carving out this exception, and > > it all seemed reasonable to us. > > > > Test methods like you suggest are possible, but I'd need several of them per > > test, and they have no chance of reusability, > > I'm not sure how the fact that we have non-reusable test methods affects things? > It's a small effort to create public methods for this, so it's not a large > overhead. > > > and they just seem like the > > restriction ties the hands of testwriters here. > > Why does have having a public test method restrict test authors? > > > We need to make these sorts of > > cases easier to test, not harder; curious if you have any ideas for ways to do > > that that also fit inside our layering goals for the architecture. > > Agreed we need more tests, however no need to break the separation of chrome vs > content internals that we've spent a few years creating, and then have > maintained for a few years since. FWIW, the two methods that can service the tests added here would be reused by the followup https://codereview.chromium.org/2387173005/ :)
Message was sent while issue was closed.
https://codereview.chromium.org/2364633004/diff/270001/chrome/browser/DEPS File chrome/browser/DEPS (right): https://codereview.chromium.org/2364633004/diff/270001/chrome/browser/DEPS#ne... chrome/browser/DEPS:112: '+content/common', On 2016/10/05 19:11:08, jam wrote: > On 2016/10/05 18:22:09, ncarter wrote: > > On 2016/10/04 23:28:54, jam wrote: > > > we never allow includes from within content from outside, even for tests. > > > > > > Can you wrap the code you need with a helper method in content/public/test > and > > > then call that from the browsertest? > > > > Hi John -- can you respond to the argument made on line 109? Exploits are > > usually layering violations by definition; > > I don't really grep this sentence; it all depends on how you define layering > violations. I'm saying that an attacker doesn't have to ask OWNERS for permission to use an IPC in an exploit chain. > Regardless of the semantics though, there are way of testing this without having > chrome reach into the internals of content by using public methods. This is the > pattern we've used for several years, so I don't see a reason to change that > now. I see a sharp distinction between security exploit tests and other type of tests. I agree with your statement strongly when applied to the latter. But for the former, this restriction makes about as much sense as requiring a fuzzer's output to comply with the js style guide. Maybe these types of tests belong in their own target? > > the bug under test here is a bug in > > //content, that's can only be reproed in //chrome (because extensions) and the > > fix lives in //extensions. > > Sure, that's fine. > > > > > Charlie, nasko and I had discussed this before carving out this exception, and > > it all seemed reasonable to us. > > > > Test methods like you suggest are possible, but I'd need several of them per > > test, and they have no chance of reusability, > > I'm not sure how the fact that we have non-reusable test methods affects things? > It's a small effort to create public methods for this, so it's not a large > overhead. > > > and they just seem like the > > restriction ties the hands of testwriters here. > > Why does have having a public test method restrict test authors? Mostly I'm making a productivity argument. It's more work to do the same thing, so it makes experimentation harder. If we allow the #includes, we basically create an environment where it's straightforward to spoof any message, without adding a helper method to content/public/test for each method. In my view, such an environment encourages testwriting. Also, if the messages aren't in content/, but somewhere else, using the IPC message in this way might require creating a test_util target. I don't know if we have a PwnMessageReceived story figured out for mojo yet, but in a mojo world, I would hope that chrome_security_exploit_browsertest should have access to an internal Mojo interface that normally we would want to hide from a chrome browsertest. > > We need to make these sorts of > > cases easier to test, not harder; curious if you have any ideas for ways to do > > that that also fit inside our layering goals for the architecture. > > Agreed we need more tests, however no need to break the separation of chrome vs > content internals that we've spent a few years creating, and then have > maintained for a few years since. If you are an attacker, there is no separation of chrome vs. content internals.
Message was sent while issue was closed.
On 2016/10/05 20:45:34, ncarter wrote: > https://codereview.chromium.org/2364633004/diff/270001/chrome/browser/DEPS > File chrome/browser/DEPS (right): > > https://codereview.chromium.org/2364633004/diff/270001/chrome/browser/DEPS#ne... > chrome/browser/DEPS:112: '+content/common', > On 2016/10/05 19:11:08, jam wrote: > > On 2016/10/05 18:22:09, ncarter wrote: > > > On 2016/10/04 23:28:54, jam wrote: > > > > we never allow includes from within content from outside, even for tests. > > > > > > > > Can you wrap the code you need with a helper method in content/public/test > > and > > > > then call that from the browsertest? > > > > > > Hi John -- can you respond to the argument made on line 109? Exploits are > > > usually layering violations by definition; > > > > I don't really grep this sentence; it all depends on how you define layering > > violations. > > I'm saying that an attacker doesn't have to ask OWNERS for permission > to use an IPC in an exploit chain. > > > Regardless of the semantics though, there are way of testing this without > having > > chrome reach into the internals of content by using public methods. This is > the > > pattern we've used for several years, so I don't see a reason to change that > > now. > > I see a sharp distinction between security exploit tests and other type of > tests. I agree with your statement strongly when applied to the latter. But for > the former, this restriction makes about as much sense as requiring a fuzzer's > output to comply with the js style guide. Can you explain more why we need a distinction? Stepping back, I understand and agree we need to be able to send various IPCs to verify our security checks are valid. I just don't get why security exploit tests don't have to follow the same code layering that we have for all the other test & non-test code? > > Maybe these types of tests belong in their own target? > > > > the bug under test here is a bug in > > > //content, that's can only be reproed in //chrome (because extensions) and > the > > > fix lives in //extensions. > > > > Sure, that's fine. > > > > > > > > Charlie, nasko and I had discussed this before carving out this exception, > and > > > it all seemed reasonable to us. > > > > > > Test methods like you suggest are possible, but I'd need several of them per > > > test, and they have no chance of reusability, > > > > I'm not sure how the fact that we have non-reusable test methods affects > things? > > It's a small effort to create public methods for this, so it's not a large > > overhead. > > > > > and they just seem like the > > > restriction ties the hands of testwriters here. > > > > Why does have having a public test method restrict test authors? > > Mostly I'm making a productivity argument. It's more work to do the same thing, > so it makes experimentation harder. We're talking about just adding a public method to send an IPC right? That doesn't seem much more work? This can be thought of almost like the fact that we do security reviews for IPC file changes, and that adds more latency, but is accepted because it's beneficial. Here we also have code outside content/ can't include internals, just like content/ can't include internals of blink, but we accept the extra work for this because of the other benefits. > If we allow the #includes, we basically > create an environment where it's straightforward to spoof any message, without > adding a helper method to content/public/test for each method. In my view, such > an environment encourages testwriting. Also, if the messages aren't in content/, > but somewhere else, using the IPC message in this way might require creating a > test_util target. It just sounds like we have different opinions on how much effort it is to add a wrapper function, so I don't want to go in circles. With respect to this change, it'll be two methods in content/public/test. No rule is set in stone, but if we want to allow code outside content/ to include internal code, this is a large change from how we've done things for the last 5 years or so, so this should be something we discuss with all the top level content owners to reach consensus. Until that happens, please route this through content/public/test. > > I don't know if we have a PwnMessageReceived story figured out for mojo yet, but > in a mojo world, I would hope that chrome_security_exploit_browsertest should > have access to an internal Mojo interface that normally we would want to hide > from a chrome browsertest. Agreed we will have to make sure this is possible (if it's not already), as our goal is not to make it any harder to do what we can with chrome ipc. > > > > We need to make these sorts of > > > cases easier to test, not harder; curious if you have any ideas for ways to > do > > > that that also fit inside our layering goals for the architecture. > > > > Agreed we need more tests, however no need to break the separation of chrome > vs > > content internals that we've spent a few years creating, and then have > > maintained for a few years since. > > If you are an attacker, there is no separation of chrome vs. content internals. Sure, there's also no separation between content and webkit/source and v8/src but that's orthogonal from writing tests. |
