Disallow clipboard reading for certain renderers [chromium/src : main]
Evan Stade (Gerrit)
Attention is currently required from: Adrian Taylor, Devlin Cronin, Evan Stade.
Patch set 1:Commit-Queue +1
1 comment:
File chrome/browser/chrome_content_browser_client.cc:
Patch Set #1, Line 6292:
return true;I verified this change manually by changing the first two checks to return false. Then, pasting into [1] with the web API fails (even when the permission is granted).
I then installed the "Clipboard Helper" extension [2] which injects a content script and uses the extension clipboardRead permission. Pasting succeeded.
I'm not sure how to write a good integration test for this as I had to modify the code so that renderer and browser disagreed about the permission state.
[1] https://rawcdn.githack.com/sitepoint-editors/clipboardapi/a8dfad6a1355bbb79381e61a2ae68394af144cc2/demotext.html
[2] https://chrome.google.com/webstore/detail/clipboard-helper/meljmedplehjlnnaempfdoecookjenph/related?hl=en
To view, visit change 3803699. To unsubscribe, or for help writing mail filters, visit settings.
Devlin Cronin (Gerrit)
Attention is currently required from: Adrian Taylor, Evan Stade.
4 comments:
Patchset:
Thanks, Evan! This is great to see!
/cc lukasza@ FYI
File chrome/browser/chrome_content_browser_client.cc:
Patch Set #1, Line 6292:
return true;
I verified this change manually by changing the first two checks to return false. […]
Two possibilities for a test:
- (easier) directly test the value of ChromeContentBrowserClient::IsClipboardPasteAllowed() in a test. Essentially:
```
LoadExtensionWithClipboardPermission();
EXPECT_FALSE(IsClipboardPasteAllowed());
RunContentScript();
EXPECT_TRUE(IsClipboardPasteAllowed());
```- (harder) fake an IPC call from the renderer requesting a paste and verify that it fails. You can see some examples of this in ContentScriptTrackerBrowserTest.
WDYT?
Patch Set #1, Line 6323:
return false;Poking through some callsites here, it didn't _seem_ like we ever killed the renderer if the result was false. Should we? (We typically kill any renderers that we can identify as sending bogus messages.)
Obviously, not for this CL; it's out of scope of this change.
File extensions/browser/content_script_tracker.h:
Patch Set #1, Line 59:
GetContentScriptsRunInProcessnit: s/GetContentScriptsRun/GetExtensionsThatRanScripts or similar. "GetContentScripts" makes it sound like we're returning the collection of the scripts themselves.
To view, visit change 3803699. To unsubscribe, or for help writing mail filters, visit settings.
Evan Stade (Gerrit)
Attention is currently required from: Adrian Taylor, Devlin Cronin.
2 comments:
File chrome/browser/chrome_content_browser_client.cc:
Patch Set #1, Line 6292:
return true;
Two possibilities for a test: […]
Thanks. Went the easier route as I couldn't find the fake IPC (mojo?) calls you were referring to in ContentScriptTrackerBrowserTest.
Patch Set #1, Line 6323:
return false;
Poking through some callsites here, it didn't _seem_ like we ever killed the renderer if the result […]
I believe your analysis is correct. We should probably do that.
To view, visit change 3803699. To unsubscribe, or for help writing mail filters, visit settings.
Devlin Cronin (Gerrit)
Attention is currently required from: Adrian Taylor, Evan Stade.
Patch set 3:Code-Review +1
4 comments:
Patchset:
Thanks, Evan! LGTM % a comment about the test (hopefully the code pasted mostly works - I haven't compiled or ran it, but it should be a good jumping off point).
File chrome/browser/chrome_content_browser_client.cc:
an
// extension content script.nit: "// ... a content script from an extension with clipboard permission."
File chrome/browser/extensions/extension_dom_clipboard_apitest.cc:
Patch Set #3, Line 131:
LoadExtension(test_data_dir_.AppendASCII("clipboard/extension"));Hmm... I think this is probably racy. Assuming the new code in the client is correct, this test would only pass if the extension finishes injecting all of the content scripts by the time LoadExtension() returns, which isn't really guaranteed. The extension also does a bunch of other work (it's its own API test).
I think we can simplify this a bit to have a more concentrated extension. Let's do something like:
```
ASSERT_TRUE(StartEmbeddedTestServer());
content::RenderFrameHost* rfh = ui_test_utils::NavigateToURL(
browser(), embedded_test_server()->GetURL("/english_page.html"));
// No extensions are installed. Clipboard access should be disallowed.
EXPECT_FALSE(
content::GetContentClientForTesting()->browser()
->IsClipboardPasteAllowed(rfh));static constexpr char kManifest[] =
R"({
"name": "Ext",
"manifest_version": 3,
"version": "1",
"background": {"service_worker": "background.js"},
"permissions": ["scripting"],
"host_permissions": ["<all_urls>"]
}");
TestExtensionDir test_dir;
test_dir.WriteManifest(kManifest);
test_dir.WriteFile(FILE_PATH_LITERAL("background.js"), "// Blank");const Extension* extension = LoadExtension(test_dir.UnpackedPath());
ASSERT_TRUE(extension);// Even with an extension installed, clipboard access is disallowed for
// the page.
EXPECT_FALSE(
content::GetContentClientForTesting()->browser()
->IsClipboardPasteAllowed(rfh));// Inject a script on the page through the extension.
static constexpr char kScript[] =
R"((async () => {
let tabs = await chrome.tabs.query({active: true});
await chrome.scripting.executeScript(
{target: {tabId: tabs[0].id}},
func: function() {});
chrome.test.sendScriptResult('done');
})();
// This will execute the script and wait for it to complete, ensuring
// the browser is aware of the executing content script.
BackgroundScriptExecutor::ExecuteScript(
profile(), extension->id(), kScript,
BackgroundScriptExecutor::ResultCapture::kSendScriptResult);
// Now the page should have access to the clipboard.
EXPECT_TRUE(
content::GetContentClientForTesting()->browser()
->IsClipboardPasteAllowed(rfh));
```I know this is a lot more code than what's here, but it should be significantly less flaky and also gives us a little bit more coverage.
File extensions/browser/content_script_tracker.h:
Patch Set #1, Line 59:
GetContentScriptsRunInProcess
nit: s/GetContentScriptsRun/GetExtensionsThatRanScripts or similar. […]
Done
To view, visit change 3803699. To unsubscribe, or for help writing mail filters, visit settings.
Evan Stade (Gerrit)
Attention is currently required from: Adrian Taylor.
3 comments:
File chrome/browser/chrome_content_browser_client.cc:
an
// extension content script.
nit: "// ... a content script from an extension with clipboard permission. […]
Done
File chrome/browser/extensions/extension_dom_clipboard_apitest.cc:
Patch Set #3, Line 131:
LoadExtension(test_data_dir_.AppendASCII("clipboard/extension"));
Hmm... I think this is probably racy. […]
thanks for the code. Updated.
(pretty much perfect save one pesky misplaced curly brace that failed to spur any helpful error logs)
File extensions/browser/content_script_tracker.h:
Patch Set #1, Line 59:
GetContentScriptsRunInProcessnit: s/GetContentScriptsRun/GetExtensionsThatRanScripts or similar. […]
oops, missed this comment. Done.
To view, visit change 3803699. To unsubscribe, or for help writing mail filters, visit settings.
Evan Stade (Gerrit)
Attention is currently required from: Adrian Taylor, Devlin Cronin.
Patch set 4:Auto-Submit +1Commit-Queue +1
1 comment:
Patchset:
gerrit seems to be having issues --- it can't see your +1, Devlin. Can you try pressing the button again?
To view, visit change 3803699. To unsubscribe, or for help writing mail filters, visit settings.
Devlin Cronin (Gerrit)
Attention is currently required from: Adrian Taylor, Evan Stade.
Patch set 4:Code-Review +1
5 comments:
Patchset:
gerrit seems to be having issues --- it can't see your +1, Devlin. […]
I think this might have been because the file list changed - IIRC, we're doing that now to make sure no one makes _too_ drastic of changes after stamps.
s lgtm % nits. Thanks, Evan!
File chrome/browser/chrome_content_browser_client.cc:
Patch Set #1, Line 6323:
return false;
I believe your analysis is correct. We should probably do that.
Is there a bug filed? If not, mind doing that? (not sure who the right owner would be, but if it's not you or someone on your team, lukasza@ would be able to help triage)
File chrome/browser/extensions/extension_dom_clipboard_apitest.cc:
Patch Set #4, Line 149:
test_dir.WriteFile(FILE_PATH_LITERAL("background.js"), "console.log('loaded');");nit: git cl format (line over 80 chars)
File extensions/browser/background_script_executor.cc:
Patch Set #4, Line 94:
LOG(ERROR) << " EXECUTING!!";Remove debugging logs
To view, visit change 3803699. To unsubscribe, or for help writing mail filters, visit settings.
Evan Stade (Gerrit)
Attention is currently required from: Adrian Taylor.
Patch set 4:Commit-Queue +2
4 comments:
File chrome/browser/chrome_content_browser_client.cc:
Patch Set #1, Line 6292:
return true;Thanks. […]
Ack
Patch Set #1, Line 6323:
return false;
Is there a bug filed? If not, mind doing that? (not sure who the right owner would be, but if it's […]
filed crbug.com/1351753
File chrome/browser/extensions/extension_dom_clipboard_apitest.cc:
Patch Set #4, Line 149:
test_dir.WriteFile(FILE_PATH_LITERAL("background.js"), "console.log('loaded');");nit: git cl format (line over 80 chars)
wonder how I managed to upload this... reverted to " // blank".
File extensions/browser/background_script_executor.cc:
Patch Set #4, Line 94:
LOG(ERROR) << " EXECUTING!!";Remove debugging logs
Done
To view, visit change 3803699. To unsubscribe, or for help writing mail filters, visit settings.
Devlin Cronin (Gerrit)
Attention is currently required from: Adrian Taylor, Evan Stade.
Patch set 5:Code-Review +1Commit-Queue +2
1 comment:
Patchset:
S LGTM
To view, visit change 3803699. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Disallow clipboard reading for certain renderers
In order to thwart compromised renderers gaining access to user
clipboard contents, limit clipboard reading to renderers where the read
request may have come from an extension content script (or a couple
other cases). This has no effect on Android, where the browser process
already knew that the request couldn't have come from a content script
since Android has no extensions.
There are a couple other exceptions that allow the clipboard to be
accessed, which were already in place before this patch:
* user activation
* web API permission
Bug: 1051198
Change-Id: I39c30603ae8883d84f552f3ba6d86c5dd587f59b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3803699
Auto-Submit: Evan Stade <est...@chromium.org>
Commit-Queue: Devlin Cronin <rdevlin...@chromium.org>
Reviewed-by: Devlin Cronin <rdevlin...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1033826}
---
M chrome/browser/chrome_content_browser_client.cc
M chrome/browser/extensions/extension_dom_clipboard_apitest.cc
M extensions/browser/content_script_tracker.cc
M extensions/browser/content_script_tracker.h
4 files changed, 143 insertions(+), 28 deletions(-)
Łukasz Anforowicz (Gerrit)
1 comment:
File chrome/browser/chrome_content_browser_client.cc:
Patch Set #1, Line 6292:
return true;
I couldn't find the fake IPC (mojo?) calls you were referring to in ContentScriptTrackerBrowserTest.
You can take a look at
//chrome/browser/extensions/extension_security_exploit_browsertest.cc - look how tests use RenderProcessHostBadIpcMessageWaiter and RenderProcessHostBadIpcMessageWaiter. I am not sure how easy-or-difficult it is to intercept the clipboard-related messages (the tests in extension_security_exploit_browsertest.cc have examples both for mojo and legacy IPC, but I am not sure if this approach can be easily copied to other messages).
To view, visit change 3803699. To unsubscribe, or for help writing mail filters, visit settings.