Enforce security checks in DevTools child sessions and strengthen extension trust. [chromium/src : main]
Danil Somsikov (Gerrit)
Danil Somsikov voted Auto-Submit+1![]( "Open in Gerrit")
| Auto-Submit | +1 |
Related details
- Andrey Kosyakov
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin added 8 comments![]( "Open in Gerrit")
Thanks, Danil! One big question here (and some nits)
Manifest::IsComponentLocation(extension.location());this would be great, but is the perfetto extension loaded as a component extension?
I see it in the webstore [here](https://chromewebstore.google.com/detail/perfetto-ui/lfmkphfpdbjijhpomgecfikhfohaoine), and I don't see references to it being loaded as a component extension elsehwere in the code (e.g. in component_loader and friends).
constexpr char kPerfettoUIExtensionId[] = "lfmkphfpdbjijhpomgecfikhfohaoine";We have a shared constant for this in extension_constants.h:
new DebuggerAttachFunction();nit: prefer base::MakeRefCounted<>; here and below
std::string actual_error;
if (!api_test_utils::RunFunction(attach_function.get(),
"[{\"targetId\": \"browser\"}, \"1.1\"]",
profile())) {
actual_error = attach_function->GetError();
}nit: cleaner to use RunFunctionAndReturnError()
std::string actual_error;
if (!api_test_utils::RunFunction(attach_function.get(),
"[{\"targetId\": \"browser\"}, \"1.1\"]",
profile())) {
actual_error = attach_function->GetError();
}
EXPECT_EQ("", actual_error);and then here, just EXPECT_TRUE(RunFunction(...)) (which will fail if it throws an error)
"[{\"targetId\": \"browser\"}, \"Target.attachToTarget\", {\"targetId\": "
"\"%s\"}]",nit: prefer R"(a string literal)" so that you don't have to escape all these quotes.
base::WrapUnique(new Session(handler, agent_host, id, flatten_protocol));nit: git cl format
Related details
- Andrey Kosyakov
- Danil Somsikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 3 comments![]( "Open in Gerrit")
using child sessions to attach to privileged WebUI contexts (e.g.,What's the practical possibility for that? We only deny this sort of things to extension clients, which can't use flattened mode so far. As for the front-end, the permissions would still allow this?
Manifest::IsComponentLocation(extension.location());+esec...@chromium.org on this one to assure it would not interfere with thier development workflow.
I'm not quite sure what scenario we're fixing here. If an attacker is able to load an unpacked extension, that's the end of the game already, isn't it?
return "";Let's make the function return `std::optional<string>` (or `base::expected<string, protocol::Response>`) for a bit more robustness.
Related details
- Danil Somsikov
- Eric Seckler
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eric Seckler (Gerrit)
Eric Seckler added 2 comments![]( "Open in Gerrit")
Manifest::IsComponentLocation(extension.location());this would be great, but is the perfetto extension loaded as a component extension?
I see it in the webstore [here](https://chromewebstore.google.com/detail/perfetto-ui/lfmkphfpdbjijhpomgecfikhfohaoine), and I don't see references to it being loaded as a component extension elsehwere in the code (e.g. in component_loader and friends).
It's a webstore extension, yes, not a component.
Manifest::IsComponentLocation(extension.location());+esec...@chromium.org on this one to assure it would not interfere with thier development workflow.
I'm not quite sure what scenario we're fixing here. If an attacker is able to load an unpacked extension, that's the end of the game already, isn't it?
+1 😊 We also would need a way to develop and test the extension locally, so we'd at least need a commandline flag for Chrome or similar to support a non-webstore / locally-loaded extension.
Related details
- Danil Somsikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Somsikov (Gerrit)
Danil Somsikov voted and added 9 comments![]( "Open in Gerrit")
Votes added by Danil Somsikov
| Auto-Submit | +1 |
9 comments
Manifest::IsComponentLocation(extension.location());Eric Seckler+esec...@chromium.org on this one to assure it would not interfere with thier development workflow.
I'm not quite sure what scenario we're fixing here. If an attacker is able to load an unpacked extension, that's the end of the game already, isn't it?
+1 😊 We also would need a way to develop and test the extension locally, so we'd at least need a commandline flag for Chrome or similar to support a non-webstore / locally-loaded extension.
we could piggy back on --silent-debugger-extension-api or introduce a dedicated flag. What does everyone think?
Eric Secklerthis would be great, but is the perfetto extension loaded as a component extension?
I see it in the webstore [here](https://chromewebstore.google.com/detail/perfetto-ui/lfmkphfpdbjijhpomgecfikhfohaoine), and I don't see references to it being loaded as a component extension elsehwere in the code (e.g. in component_loader and friends).
It's a webstore extension, yes, not a component.
Done
constexpr char kPerfettoUIExtensionId[] = "lfmkphfpdbjijhpomgecfikhfohaoine";We have a shared constant for this in extension_constants.h:
Done
nit: prefer base::MakeRefCounted<>; here and below
Done
std::string actual_error;
if (!api_test_utils::RunFunction(attach_function.get(),
"[{\"targetId\": \"browser\"}, \"1.1\"]",
profile())) {
actual_error = attach_function->GetError();
}nit: cleaner to use RunFunctionAndReturnError()
Done
std::string actual_error;
if (!api_test_utils::RunFunction(attach_function.get(),
"[{\"targetId\": \"browser\"}, \"1.1\"]",
profile())) {
actual_error = attach_function->GetError();
}
EXPECT_EQ("", actual_error);and then here, just EXPECT_TRUE(RunFunction(...)) (which will fail if it throws an error)
Done
"[{\"targetId\": \"browser\"}, \"Target.attachToTarget\", {\"targetId\": "
"\"%s\"}]",nit: prefer R"(a string literal)" so that you don't have to escape all these quotes.
Done
base::WrapUnique(new Session(handler, agent_host, id, flatten_protocol));Danil Somsikovnit: git cl format
Done
Let's make the function return `std::optional<string>` (or `base::expected<string, protocol::Response>`) for a bit more robustness.
- Andrey Kosyakov
- Devlin Cronin
- Eric Seckler
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin added 2 comments![]( "Open in Gerrit")
Manifest::IsComponentLocation(extension.location());Eric Seckler+esec...@chromium.org on this one to assure it would not interfere with thier development workflow.
I'm not quite sure what scenario we're fixing here. If an attacker is able to load an unpacked extension, that's the end of the game already, isn't it?
Danil Somsikov+1 😊 We also would need a way to develop and test the extension locally, so we'd at least need a commandline flag for Chrome or similar to support a non-webstore / locally-loaded extension.
we could piggy back on --silent-debugger-extension-api or introduce a dedicated flag. What does everyone think?
I'm not quite sure what scenario we're fixing here. If an attacker is able to load an unpacked extension, that's the end of the game already, isn't it?
Strictly speaking, yes, we consider loading an unpacked extension to be the same as local access, which isn't in our threat model.
That said, we also know that social engineering attacks exist that trick people into loading unpacked extensions. We can't fully prevent these, but we can raise the bar a bit, and a commandline flag (or similar) helps a bit. I'd be supportive of that (and would lean towards just having a dedicated one; commandline flags are cheap : ))
Related details
- Andrey Kosyakov
- Danil Somsikov
- Eric Seckler
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eric Seckler (Gerrit)
Eric Seckler added 2 comments![]( "Open in Gerrit")
Manifest::IsComponentLocation(extension.location());Eric Seckler+esec...@chromium.org on this one to assure it would not interfere with thier development workflow.
I'm not quite sure what scenario we're fixing here. If an attacker is able to load an unpacked extension, that's the end of the game already, isn't it?
Danil Somsikov+1 😊 We also would need a way to develop and test the extension locally, so we'd at least need a commandline flag for Chrome or similar to support a non-webstore / locally-loaded extension.
Devlin Croninwe could piggy back on --silent-debugger-extension-api or introduce a dedicated flag. What does everyone think?
I'm not quite sure what scenario we're fixing here. If an attacker is able to load an unpacked extension, that's the end of the game already, isn't it?
Strictly speaking, yes, we consider loading an unpacked extension to be the same as local access, which isn't in our threat model.
That said, we also know that social engineering attacks exist that trick people into loading unpacked extensions. We can't fully prevent these, but we can raise the bar a bit, and a commandline flag (or similar) helps a bit. I'd be supportive of that (and would lean towards just having a dedicated one; commandline flags are cheap : ))
OK, a dedicated cmdline flag sgtm. +steve...@google.com FYI
BrowserTargetNotAllowedForUnpackedPerfettoUI) {Assuming we add a commandline flag, let's add a test that still allows this when flag is provided as well please 😊
Related details
- Andrey Kosyakov
- Danil Somsikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Somsikov (Gerrit)
Danil Somsikov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Danil Somsikov
| Auto-Submit | +1 |
2 comments
Manifest::IsComponentLocation(extension.location());Eric Seckler+esec...@chromium.org on this one to assure it would not interfere with thier development workflow.
I'm not quite sure what scenario we're fixing here. If an attacker is able to load an unpacked extension, that's the end of the game already, isn't it?
Danil Somsikov+1 😊 We also would need a way to develop and test the extension locally, so we'd at least need a commandline flag for Chrome or similar to support a non-webstore / locally-loaded extension.
Devlin Croninwe could piggy back on --silent-debugger-extension-api or introduce a dedicated flag. What does everyone think?
Eric SecklerI'm not quite sure what scenario we're fixing here. If an attacker is able to load an unpacked extension, that's the end of the game already, isn't it?
Strictly speaking, yes, we consider loading an unpacked extension to be the same as local access, which isn't in our threat model.
That said, we also know that social engineering attacks exist that trick people into loading unpacked extensions. We can't fully prevent these, but we can raise the bar a bit, and a commandline flag (or similar) helps a bit. I'd be supportive of that (and would lean towards just having a dedicated one; commandline flags are cheap : ))
OK, a dedicated cmdline flag sgtm. +steve...@google.com FYI
Done
Assuming we add a commandline flag, let's add a test that still allows this when flag is provided as well please 😊
- Andrey Kosyakov
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 3 comments![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | +1 |
3 comments
R"([{"targetId": "browser"}, "Target.attachToTarget", {"targetId": "%s"}])",nit: can we break this line up to wrap at 80 char?
const char kAllowUnpackedExtensionTrust[] = "allow-unpacked-extension-trust";nit: let's be more specific. Maybe "allow-unpacked-perfetto-extension"? (Similarly for the switch variable name)
Related details
- Andrey Kosyakov
- Danil Somsikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin voted and added 1 comment![]( "Open in Gerrit")
Votes added by Devlin Cronin
| Code-Review | +0 |
1 comment
2. Strengthen extension trust: ExtensionIsTrusted in debugger_api.cc is
updated to verify that the extension is not from an unpacked location. This prevents attackers from gaining "trusted" status by
loading an unpacked extension with the Perfetto UI extension ID.nit: maybe also update this
Related details
- Andrey Kosyakov
- Danil Somsikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Andrey Kosyakov
- Danil Somsikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |