Verify sessionId in incoming protocol messages from renderer [chromium/src : main]
Danil Somsikov (Gerrit)
Danil Somsikov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Danil Somsikov
| Auto-Submit | +1 |
2 comments
Danil SomsikovAndrey, wdyt? Is it worth it?
Resolved
Forcefully inject sessionId into child DevTools session messagesDanil SomsikovThe sessionId should already be put there by a child, so rather than injecting it, perhaps we can validate that a correct id is there and kill the child for bad IPC if it's not? Perhaps this would be both cheaper to implement and prevent a compromised renderer from trying other paths?
Done
Related details
- Andrey Kosyakov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Somsikov (Gerrit)
Danil Somsikov voted![]( "Open in Gerrit")
| Auto-Submit | +1 |
| Commit-Queue | +1 |
Related details
- Andrey Kosyakov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 4 comments![]( "Open in Gerrit")
crdtp::cbor::CBORTokenizer tokenizer(crdtp::SpanFrom(message));That's a bit scary logic to have in such a place! In general, something using tokenizer so extensively belongs to crdtp (see `AppendString8EntryToCBORMap()` for example), but I think here we can do without dedicated low-level code and go with existent form of lazy patching. One option would be using a (Dispatchable)[https://source.chromium.org/chromium/chromium/src/+/main:third_party/inspector_protocol/crdtp/dispatch.h;bpv=1;bpt=1], another would be utilizing protocol core support for deferred messages, please see an [example here](https://source.chromium.org/chromium/chromium/src/+/main:v8/third_party/inspector_protocol/crdtp/protocol_core_test.cc;l=421).
message->data = mojo_base::BigBuffer(cbor);Is there a reason we're not doing this on the test side? The less test-specific code we have in the prod files the better. Let's just make it a trivial wrapper around DispatchProtocolNotification, i.e. `DispatchProtocolNotificationForTesting(blink::mojom::DevToolsMessagePtr message)`, and handle json-to-cbor business on the test side.
for (DevToolsSession* root_session : host->sessions_) {Can we just extract `DevToolsAgentHostImpl::GetSessionForTesting()` and move the rest of the logic to the test side? This would also let us get rid of on_before_dispatch, as you would be able to straigthen the logic on the test side:
- find session
- get agent host and process host
- dispatch the message.
if (!session) {Is there a meaningful scneario where we would hit that? Perhaps CHECK() instead?
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 4 comments![]( "Open in Gerrit")
Votes added by Danil Somsikov
| Auto-Submit | +1 |
| Commit-Queue | +1 |
4 comments
crdtp::cbor::CBORTokenizer tokenizer(crdtp::SpanFrom(message));That's a bit scary logic to have in such a place! In general, something using tokenizer so extensively belongs to crdtp (see `AppendString8EntryToCBORMap()` for example), but I think here we can do without dedicated low-level code and go with existent form of lazy patching. One option would be using a (Dispatchable)[https://source.chromium.org/chromium/chromium/src/+/main:third_party/inspector_protocol/crdtp/dispatch.h;bpv=1;bpt=1], another would be utilizing protocol core support for deferred messages, please see an [example here](https://source.chromium.org/chromium/chromium/src/+/main:v8/third_party/inspector_protocol/crdtp/protocol_core_test.cc;l=421).
Done
Is there a reason we're not doing this on the test side? The less test-specific code we have in the prod files the better. Let's just make it a trivial wrapper around DispatchProtocolNotification, i.e. `DispatchProtocolNotificationForTesting(blink::mojom::DevToolsMessagePtr message)`, and handle json-to-cbor business on the test side.
Done
for (DevToolsSession* root_session : host->sessions_) {Can we just extract `DevToolsAgentHostImpl::GetSessionForTesting()` and move the rest of the logic to the test side? This would also let us get rid of on_before_dispatch, as you would be able to straigthen the logic on the test side:
- find session
- get agent host and process host
- dispatch the message.
Done
Is there a meaningful scneario where we would hit that? Perhaps CHECK() instead?
- Andrey Kosyakov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov added 4 comments![]( "Open in Gerrit")
DevToolsSession* DevToolsSession::GetSessionById(const std::string& session_id) {So all the class methods are just trivial wrappers now? Can we get rid of the class and expose ....ForTesting() methods directly as we do elsewhere please?
crdtp::json::ConvertJSONToCBOR(crdtp::SpanFrom(json), &cbor);This one can certainly be done directly in the test.
Please note this is a rolled dependency, this needs to be landed upstream in crdtp and rolled here.
span<uint8_t> GetString8ValueFromMap(span<uint8_t> message,Please also provide a unit test for this.
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 4 comments![]( "Open in Gerrit")
Votes added by Danil Somsikov
| Auto-Submit | +1 |
| Commit-Queue | +1 |
4 comments
DevToolsSession* DevToolsSession::GetSessionById(const std::string& session_id) {So all the class methods are just trivial wrappers now? Can we get rid of the class and expose ....ForTesting() methods directly as we do elsewhere please?
Done
crdtp::json::ConvertJSONToCBOR(crdtp::SpanFrom(json), &cbor);This one can certainly be done directly in the test.
Done
Please note this is a rolled dependency, this needs to be landed upstream in crdtp and rolled here.
span<uint8_t> GetString8ValueFromMap(span<uint8_t> message,Please also provide a unit test for this.
- Andrey Kosyakov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Somsikov (Gerrit)
Danil Somsikov voted![]( "Open in Gerrit")
| Auto-Submit | +1 |
| Commit-Queue | +1 |
Related details
- Andrey Kosyakov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrey Kosyakov (Gerrit)
Andrey Kosyakov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Andrey Kosyakov
| Code-Review | +1 |
1 comment
Hmm.. What happened to the tests though?
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 1 comment![]( "Open in Gerrit")
Votes added by Danil Somsikov
| Auto-Submit | +1 |
1 comment
Hmm.. What happened to the tests though?
Not sure, restored.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Somsikov (Gerrit)
Danil Somsikov voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Somsikov (Gerrit)
Danil Somsikov voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Somsikov (Gerrit)
Danil Somsikov voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
46 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: content/browser/devtools/protocol/devtools_protocol_browsertest.cc
Insertions: 182, Deletions: 0.
The diff is too large to show. Please review the diff.
```
```
The name of the file: content/browser/devtools/devtools_session.h
Insertions: 1, Deletions: 2.
The diff is too large to show. Please review the diff.
```
```
The name of the file: content/browser/devtools/devtools_session.cc
Insertions: 52, Deletions: 39.
The diff is too large to show. Please review the diff.
```
Change information
Verify sessionId in incoming protocol messages from renderer
In DevTools "flattened" protocol mode, the renderer process is responsible
for including the correct sessionId in every protocol response and
notification. Previously, the browser process forwarded these messages to
the client without verification. A compromised renderer could deliberately
omit or spoof the sessionId, potentially allowing it to inject events into
the root session or other child sessions.
This CL fixes the vulnerability by having the browser process verify the
sessionId of every protocol message originating from a renderer-side
session. If the sessionId is missing, incorrect, or unexpectedly present
(in the case of the root session), the browser now considers the renderer
compromised and terminates it using bad_message::ReceivedBadMessage.
- M content/browser/devtools/devtools_agent_host_impl.cc
- M content/browser/devtools/devtools_agent_host_impl.h
- M content/browser/devtools/devtools_session.cc
- M content/browser/devtools/devtools_session.h
- M content/browser/devtools/protocol/devtools_protocol_browsertest.cc
- M content/test/BUILD.gn
Code-Review: +1 by Andrey Kosyakov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |