Add request_url to TrustedHeaderClient::OnBeforeSendHeaders [chromium/src : main]
Anatoli Hancharou (Gerrit)
Anatoli Hancharou added 1 comment![]( "Open in Gerrit")
blundell@ for:
- service/network/*
- chrome/browser/net/ websocket_browsertest.cc
andreaorru@ for:
- extensions/browser/api/web_request/*
- components/guest_view/browser/slim_web_view/slim_web_view_url_loader_factory_interceptor.cc
nasko@ for:
- content/browser/devtools/ devtools_url_loader_interceptor.cc
Related details
- Chromium IPC Reviews
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: mk...@chromium.org, na...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): mk...@chromium.org, na...@chromium.org
Reviewer source(s):
mk...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Mike West
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nasko Oskov (Gerrit)
Nasko Oskov added 2 comments![]( "Open in Gerrit")
Should we check here that the `request_.url` is the same as the incoming `request_url` parameter, similarly to the check in the web sockets case?
What happens if the URLs mismatch?
DCHECK_EQ(request_url, info_.url);What happens if these two disagree? Would the code behave as expected?
Related details
- Anatoli Hancharou
- Mike West
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. |
Mike West (Gerrit)
Mike West added 1 comment![]( "Open in Gerrit")
Given my understanding of how this URL will be used (matching against a policy as described in the DD), there's not much additional checking I'd ask for beyond the type itself.
Assuming the relevant owners are happy with the approach generally, the mojo interface changes seem reasonable. Happy to stamp them once owners weigh in.
Related details
- Anatoli Hancharou
- Andrea Orru
- Colin Blundell
Anatoli Hancharou (Gerrit)
Anatoli Hancharou added 2 comments![]( "Open in Gerrit")
Should we check here that the `request_.url` is the same as the incoming `request_url` parameter, similarly to the check in the web sockets case?
What happens if the URLs mismatch?
Done.
"What happens if the URLs mismatch?" - replied in the second comment's thread
DCHECK_EQ(request_url, info_.url);What happens if these two disagree? Would the code behave as expected?
My understanding: if the Network Service's actual URL (`request_url`) and the Browser's expected URL (`request_.url` / `info_.url`) disagree, it creates a silent security issue:
1. **Incorrect rules applied:** The browser tells extensions we are loading URL A, so extensions inspect/modify headers thinking the request is going to URL A.
2. **Data leak:** The Network Service actually requests URL B, meaning sensitive headers could be sent to URL B.
3. **Blocking bypass:** A blocked URL B might load because the browser thinks it is the allowed URL A.
This problem exists regardless of this CL I guess. By passing the actual URL over Mojo here, we can now add `DCHECK_EQ(request_url, request_.url);` to at least not letting it go silently.
Potential problem related to my feature would be that headers that were supposed to be obtained for URL A, might be provided to URL B
Related details
- Andrea Orru
- Colin Blundell
- Nasko Oskov
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. |
Nasko Oskov (Gerrit)
Nasko Oskov voted and added 2 comments![]( "Open in Gerrit")
Votes added by Nasko Oskov
| Code-Review | +1 |
2 comments
DCHECK_EQ(request_url, info_.url);Anatoli HancharouWhat happens if these two disagree? Would the code behave as expected?
My understanding: if the Network Service's actual URL (`request_url`) and the Browser's expected URL (`request_.url` / `info_.url`) disagree, it creates a silent security issue:
1. **Incorrect rules applied:** The browser tells extensions we are loading URL A, so extensions inspect/modify headers thinking the request is going to URL A.
2. **Data leak:** The Network Service actually requests URL B, meaning sensitive headers could be sent to URL B.
3. **Blocking bypass:** A blocked URL B might load because the browser thinks it is the allowed URL A.This problem exists regardless of this CL I guess. By passing the actual URL over Mojo here, we can now add `DCHECK_EQ(request_url, request_.url);` to at least not letting it go silently.
Potential problem related to my feature would be that headers that were supposed to be obtained for URL A, might be provided to URL B
- Anatoli Hancharou
- Andrea Orru
- Colin Blundell
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. |
Colin Blundell (Gerrit)
Colin Blundell added 1 comment![]( "Open in Gerrit")
Thanks! //chrome and //components LGTM. +ricea@ for //services/network as closer OWNER
Related details
- Adam Rice
- Anatoli Hancharou
- Andrea Orru
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. |
Adam Rice (Gerrit)
Adam Rice added 1 comment![]( "Open in Gerrit")
DCHECK_EQ(request_url, request_.url);The browser process shouldn't DCHECK on data from a lower-privileged process. See https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/checks.md#failures-beyond-chromium_s-control
Also, I don't understand why we are plumbing the URL back from the network service through mojo when we already have it in the browser process. I read the design doc but it didn't explain the reason for doing it.
Related details
- Anatoli Hancharou
- Andrea Orru
Anatoli Hancharou (Gerrit)
Anatoli Hancharou added 1 comment![]( "Open in Gerrit")
DCHECK_EQ(request_url, request_.url);The browser process shouldn't DCHECK on data from a lower-privileged process. See https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/checks.md#failures-beyond-chromium_s-control
Also, I don't understand why we are plumbing the URL back from the network service through mojo when we already have it in the browser process. I read the design doc but it didn't explain the reason for doing it.
This plumbing is actually being added to support a new feature that will be landing in a follow-up CL.
You are completely right that the existing consumer of `TrustedHeaderClient` (the WebRequest API) already knows the URL because it proxies the entire `URLLoader` and tracks the URL through `CreateLoaderAndStart` and `FollowRedirect`.
However, to minimize performance overhead, the upcoming Enterprise Header Injection feature is designed to be much more lightweight. It avoids proxying the `URLLoader` pipe entirely, and instead only proxies the `URLLoaderFactory` to attach the `TrustedHeaderClient`.
Because the `URLLoader` remains unproxied, the browser process does not intercept `OnReceiveRedirect` or `FollowRedirect` for those requests. When a redirect occurs, the Network Service handles it internally and fires another `OnBeforeSendHeaders` to our `TrustedHeaderClient`. Without plumbing `request_url` through this mojo call, our client would have no idea that the URL has changed, and we would end up evaluating enterprise policies against the old pre-redirect URL
Related details
- Adam Rice
- Andrea Orru
Anatoli Hancharou (Gerrit)
Anatoli Hancharou added 1 comment![]( "Open in Gerrit")
DCHECK_EQ(request_url, request_.url);Anatoli HancharouThe browser process shouldn't DCHECK on data from a lower-privileged process. See https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/checks.md#failures-beyond-chromium_s-control
Also, I don't understand why we are plumbing the URL back from the network service through mojo when we already have it in the browser process. I read the design doc but it didn't explain the reason for doing it.
This plumbing is actually being added to support a new feature that will be landing in a follow-up CL.
You are completely right that the existing consumer of `TrustedHeaderClient` (the WebRequest API) already knows the URL because it proxies the entire `URLLoader` and tracks the URL through `CreateLoaderAndStart` and `FollowRedirect`.
However, to minimize performance overhead, the upcoming Enterprise Header Injection feature is designed to be much more lightweight. It avoids proxying the `URLLoader` pipe entirely, and instead only proxies the `URLLoaderFactory` to attach the `TrustedHeaderClient`.
Because the `URLLoader` remains unproxied, the browser process does not intercept `OnReceiveRedirect` or `FollowRedirect` for those requests. When a redirect occurs, the Network Service handles it internally and fires another `OnBeforeSendHeaders` to our `TrustedHeaderClient`. Without plumbing `request_url` through this mojo call, our client would have no idea that the URL has changed, and we would end up evaluating enterprise policies against the old pre-redirect URL
See [InProgressRequest](https://source.chromium.org/chromium/chromium/src/+/main:extensions/browser/api/web_request/web_request_proxying_url_loader_factory.h;bpv=1;bpt=1;l=75?gsn=InProgressRequest&gs=KYTHE%3A%2F%2Fkythe%3A%2F%2Fchromium.googlesource.com%2Fcodesearch%2Fchromium%2Fsrc%2F%2Fmain%3Flang%3Dc%252B%252B%3Fpath%3Dextensions%2Fbrowser%2Fapi%2Fweb_request%2Fweb_request_proxying_url_loader_factory.h%23NVFF1VzoSJn4J3b894HXJko6TkuMJACrkbDAcnJov4E) implementing `network::mojom::URLLoader`.
I won't introduce any custom UrlLoader wrappers as the goal is much simpler
Anatoli Hancharou (Gerrit)
Anatoli Hancharou added 1 comment![]( "Open in Gerrit")
DCHECK_EQ(request_url, info_.url);Anatoli HancharouWhat happens if these two disagree? Would the code behave as expected?
Nasko OskovMy understanding: if the Network Service's actual URL (`request_url`) and the Browser's expected URL (`request_.url` / `info_.url`) disagree, it creates a silent security issue:
1. **Incorrect rules applied:** The browser tells extensions we are loading URL A, so extensions inspect/modify headers thinking the request is going to URL A.
2. **Data leak:** The Network Service actually requests URL B, meaning sensitive headers could be sent to URL B.
3. **Blocking bypass:** A blocked URL B might load because the browser thinks it is the allowed URL A.This problem exists regardless of this CL I guess. By passing the actual URL over Mojo here, we can now add `DCHECK_EQ(request_url, request_.url);` to at least not letting it go silently.
Potential problem related to my feature would be that headers that were supposed to be obtained for URL A, might be provided to URL B
As per offline chat, let's proceed.
Done
Andrea Orru (Gerrit)
Andrea Orru added 1 comment![]( "Open in Gerrit")
DCHECK_EQ(request_url, request_.url);Anatoli HancharouThe browser process shouldn't DCHECK on data from a lower-privileged process. See https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/checks.md#failures-beyond-chromium_s-control
Also, I don't understand why we are plumbing the URL back from the network service through mojo when we already have it in the browser process. I read the design doc but it didn't explain the reason for doing it.
Anatoli HancharouThis plumbing is actually being added to support a new feature that will be landing in a follow-up CL.
You are completely right that the existing consumer of `TrustedHeaderClient` (the WebRequest API) already knows the URL because it proxies the entire `URLLoader` and tracks the URL through `CreateLoaderAndStart` and `FollowRedirect`.
However, to minimize performance overhead, the upcoming Enterprise Header Injection feature is designed to be much more lightweight. It avoids proxying the `URLLoader` pipe entirely, and instead only proxies the `URLLoaderFactory` to attach the `TrustedHeaderClient`.
Because the `URLLoader` remains unproxied, the browser process does not intercept `OnReceiveRedirect` or `FollowRedirect` for those requests. When a redirect occurs, the Network Service handles it internally and fires another `OnBeforeSendHeaders` to our `TrustedHeaderClient`. Without plumbing `request_url` through this mojo call, our client would have no idea that the URL has changed, and we would end up evaluating enterprise policies against the old pre-redirect URL
See [InProgressRequest](https://source.chromium.org/chromium/chromium/src/+/main:extensions/browser/api/web_request/web_request_proxying_url_loader_factory.h;bpv=1;bpt=1;l=75?gsn=InProgressRequest&gs=KYTHE%3A%2F%2Fkythe%3A%2F%2Fchromium.googlesource.com%2Fcodesearch%2Fchromium%2Fsrc%2F%2Fmain%3Flang%3Dc%252B%252B%3Fpath%3Dextensions%2Fbrowser%2Fapi%2Fweb_request%2Fweb_request_proxying_url_loader_factory.h%23NVFF1VzoSJn4J3b894HXJko6TkuMJACrkbDAcnJov4E) implementing `network::mojom::URLLoader`.
I won't introduce any custom UrlLoader wrappers as the goal is much simpler
As Adam said you shouldn't DCHECK here.
If `request_.url` == `request_url`, why don't you just use `request_.url`?
Related details
- Adam Rice
- Anatoli Hancharou
Adam Rice (Gerrit)
Adam Rice added 1 comment![]( "Open in Gerrit")
DCHECK_EQ(request_url, request_.url);Anatoli HancharouThe browser process shouldn't DCHECK on data from a lower-privileged process. See https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/checks.md#failures-beyond-chromium_s-control
Also, I don't understand why we are plumbing the URL back from the network service through mojo when we already have it in the browser process. I read the design doc but it didn't explain the reason for doing it.
Anatoli HancharouThis plumbing is actually being added to support a new feature that will be landing in a follow-up CL.
You are completely right that the existing consumer of `TrustedHeaderClient` (the WebRequest API) already knows the URL because it proxies the entire `URLLoader` and tracks the URL through `CreateLoaderAndStart` and `FollowRedirect`.
However, to minimize performance overhead, the upcoming Enterprise Header Injection feature is designed to be much more lightweight. It avoids proxying the `URLLoader` pipe entirely, and instead only proxies the `URLLoaderFactory` to attach the `TrustedHeaderClient`.
Because the `URLLoader` remains unproxied, the browser process does not intercept `OnReceiveRedirect` or `FollowRedirect` for those requests. When a redirect occurs, the Network Service handles it internally and fires another `OnBeforeSendHeaders` to our `TrustedHeaderClient`. Without plumbing `request_url` through this mojo call, our client would have no idea that the URL has changed, and we would end up evaluating enterprise policies against the old pre-redirect URL
Andrea OrruSee [InProgressRequest](https://source.chromium.org/chromium/chromium/src/+/main:extensions/browser/api/web_request/web_request_proxying_url_loader_factory.h;bpv=1;bpt=1;l=75?gsn=InProgressRequest&gs=KYTHE%3A%2F%2Fkythe%3A%2F%2Fchromium.googlesource.com%2Fcodesearch%2Fchromium%2Fsrc%2F%2Fmain%3Flang%3Dc%252B%252B%3Fpath%3Dextensions%2Fbrowser%2Fapi%2Fweb_request%2Fweb_request_proxying_url_loader_factory.h%23NVFF1VzoSJn4J3b894HXJko6TkuMJACrkbDAcnJov4E) implementing `network::mojom::URLLoader`.
I won't introduce any custom UrlLoader wrappers as the goal is much simpler
As Adam said you shouldn't DCHECK here.
If `request_.url` == `request_url`, why don't you just use `request_.url`?
Ah, okay. Please consider making that clearer in the design doc. If this needs to work on Android in future it makes sense not to reuse the existing proxying machinery.
Also, remove the DCHECK. If you really want a check here, you could do something like
```c++
if constexpr (DCHECK_IS_ON()) {
if (request_url != request_.url) {
mojo::ReportBadMessage("URL supplied to OnBeforeSendHeaders() did not match");
return;
}
}
```
Related details
- Anatoli Hancharou
Anatoli Hancharou (Gerrit)
Anatoli Hancharou voted and added 1 comment![]( "Open in Gerrit")
Votes added by Anatoli Hancharou
| Commit-Queue | +0 |
1 comment
Anatoli HancharouThe browser process shouldn't DCHECK on data from a lower-privileged process. See https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/checks.md#failures-beyond-chromium_s-control
Also, I don't understand why we are plumbing the URL back from the network service through mojo when we already have it in the browser process. I read the design doc but it didn't explain the reason for doing it.
Anatoli HancharouThis plumbing is actually being added to support a new feature that will be landing in a follow-up CL.
You are completely right that the existing consumer of `TrustedHeaderClient` (the WebRequest API) already knows the URL because it proxies the entire `URLLoader` and tracks the URL through `CreateLoaderAndStart` and `FollowRedirect`.
However, to minimize performance overhead, the upcoming Enterprise Header Injection feature is designed to be much more lightweight. It avoids proxying the `URLLoader` pipe entirely, and instead only proxies the `URLLoaderFactory` to attach the `TrustedHeaderClient`.
Because the `URLLoader` remains unproxied, the browser process does not intercept `OnReceiveRedirect` or `FollowRedirect` for those requests. When a redirect occurs, the Network Service handles it internally and fires another `OnBeforeSendHeaders` to our `TrustedHeaderClient`. Without plumbing `request_url` through this mojo call, our client would have no idea that the URL has changed, and we would end up evaluating enterprise policies against the old pre-redirect URL
Andrea OrruSee [InProgressRequest](https://source.chromium.org/chromium/chromium/src/+/main:extensions/browser/api/web_request/web_request_proxying_url_loader_factory.h;bpv=1;bpt=1;l=75?gsn=InProgressRequest&gs=KYTHE%3A%2F%2Fkythe%3A%2F%2Fchromium.googlesource.com%2Fcodesearch%2Fchromium%2Fsrc%2F%2Fmain%3Flang%3Dc%252B%252B%3Fpath%3Dextensions%2Fbrowser%2Fapi%2Fweb_request%2Fweb_request_proxying_url_loader_factory.h%23NVFF1VzoSJn4J3b894HXJko6TkuMJACrkbDAcnJov4E) implementing `network::mojom::URLLoader`.
I won't introduce any custom UrlLoader wrappers as the goal is much simpler
Adam RiceAs Adam said you shouldn't DCHECK here.
If `request_.url` == `request_url`, why don't you just use `request_.url`?
Ah, okay. Please consider making that clearer in the design doc. If this needs to work on Android in future it makes sense not to reuse the existing proxying machinery.
Also, remove the DCHECK. If you really want a check here, you could do something like
```c++
if constexpr (DCHECK_IS_ON()) {
if (request_url != request_.url) {
mojo::ReportBadMessage("URL supplied to OnBeforeSendHeaders() did not match");
return;
}
}
```
DCHECK removed, thank you!
I've added a task to expand on mojo modification reasons in the design doc and will address it later this day.
Related details
- Adam Rice
- Andrea Orru
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice voted and added 1 comment![]( "Open in Gerrit")
- Anatoli Hancharou
- Andrea Orru
Colin Blundell (Gerrit)
Colin Blundell voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Andrea Orru (Gerrit)
Andrea Orru voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Anatoli Hancharou
Code-Coverage
Code-Owners