[SAA] Remove mojo navigation params that convey renderer's SAA status [chromium/src : main]
Chris Fredrickson (Gerrit)
Chris Fredrickson voted and added 1 comment![]( "Open in Gerrit")
Votes added by Chris Fredrickson
| Commit-Queue | +1 |
1 comment
Chris FredricksonIdeally: this would be a CHECK here, and a mojo::ReceivedBadMessage on the IPC that caused the browser process to enter this bad state.
Could you please investigate what could be done in this direction and report back if you found it was possible or not?
To clarify: happy to merge this today with a TODO for later if that's not possible, but I would like to keep open the path for doing the right thing here;
Chris FredricksonDone. Still not sure how easy it is to test a compromised renderer, but we at least have good test coverage for the various happy paths [here](https://crsrc.org/c/chrome/browser/storage_access_api/api_browsertest.cc;drc=2d8f83c4b3354e64cf0a74ff6631a24752d6e4b4;l=1622-1914).
Actually, I'll do one better: since the browser already knows the "correct state", we shouldn't ask the renderer to supply the state at all. We can remove some fields from mojo structs in that case.
Related details
- Arthur Sonzogni
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Arthur Sonzogni (Gerrit)
Arthur Sonzogni voted and added 2 comments![]( "Open in Gerrit")
Votes added by Arthur Sonzogni
| Code-Review | +1 |
2 comments
return previous_document_rfh->document_associated_data()
.cookie_setting_overrides()
.Has(net::CookieSettingOverride::
kStorageAccessGrantEligible) &&
common_params.initiator_origin &&
common_params.initiator_origin->IsSameOriginWith(
response_url) &&
begin_params.initiator_frame_token ==
previous_document_rfh->GetFrameToken() &&
!did_encounter_cross_origin_redirect
? net::StorageAccessApiStatus::kAccessViaAPI
: net::StorageAccessApiStatus::kNone;That's a complex condition. Maybe it can be split?
Something along the lines of:
```
// Ensure the frame has the grant eligible override.
if (!previous_document_rfh->document_associated_data()
.cookie_setting_overrides()
.Has(net::CookieSettingOverride::kStorageAccessGrantEligible)) {
return net::StorageAccessApiStatus::kNone;
}
// Validate initiator origin and routing.
if (!common_params.initiator_origin ||
!common_params.initiator_origin->IsSameOriginWith(response_url) ||
begin_params.initiator_frame_token != previous_document_rfh->GetFrameToken() ||
did_encounter_cross_origin_redirect) {
return net::StorageAccessApiStatus::kNone;
}
return net::StorageAccessApiStatus::kAccessViaAPI;
```
(Feel free to tweak)
Related details
- Chris Fredrickson
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. |
Chris Fredrickson (Gerrit)
Chris Fredrickson voted and added 2 comments![]( "Open in Gerrit")
Votes added by Chris Fredrickson
| Auto-Submit | +1 |
2 comments
+dtapuska for //t_p/blink/renderer and web_navigation_params.h
return previous_document_rfh->document_associated_data()
.cookie_setting_overrides()
.Has(net::CookieSettingOverride::
kStorageAccessGrantEligible) &&
common_params.initiator_origin &&
common_params.initiator_origin->IsSameOriginWith(
response_url) &&
begin_params.initiator_frame_token ==
previous_document_rfh->GetFrameToken() &&
!did_encounter_cross_origin_redirect
? net::StorageAccessApiStatus::kAccessViaAPI
: net::StorageAccessApiStatus::kNone;That's a complex condition. Maybe it can be split?
Something along the lines of:
```
// Ensure the frame has the grant eligible override.
if (!previous_document_rfh->document_associated_data()
.cookie_setting_overrides()
.Has(net::CookieSettingOverride::kStorageAccessGrantEligible)) {
return net::StorageAccessApiStatus::kNone;
}// Validate initiator origin and routing.
if (!common_params.initiator_origin ||
!common_params.initiator_origin->IsSameOriginWith(response_url) ||
begin_params.initiator_frame_token != previous_document_rfh->GetFrameToken() ||
did_encounter_cross_origin_redirect) {
return net::StorageAccessApiStatus::kNone;
}return net::StorageAccessApiStatus::kAccessViaAPI;
```(Feel free to tweak)
- Dave Tapuska
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Dave Tapuska voted Code-Review+1![]( "Open in Gerrit")
- Chris Fredrickson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Fredrickson (Gerrit)
Chris Fredrickson voted Commit-Queue+2![]( "Open in Gerrit")
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![]( "Open in Gerrit")
Change information
[SAA] Remove mojo navigation params that convey renderer's SAA status
These params are now unnecessary (after https://crrev.com/c/6494679),
and were never trustworthy anyway. Instead of asking the renderer to
provide this state, we should just use the browser's copy of it.
This removal cleans up some complexity w.r.t. local/remote frames.
- M content/browser/fenced_frame/fenced_frame.cc
- M content/browser/loader/navigation_url_loader_impl.cc
- M content/browser/loader/navigation_url_loader_impl_unittest.cc
- M content/browser/loader/navigation_url_loader_unittest.cc
- M content/browser/renderer_host/navigation_controller_impl.cc
- M content/browser/renderer_host/navigation_controller_impl.h
- M content/browser/renderer_host/navigation_request.cc
- M content/browser/renderer_host/navigation_request.h
- M content/browser/renderer_host/navigator.cc
- M content/browser/renderer_host/navigator.h
- M content/browser/renderer_host/render_frame_host_impl.cc
- M content/browser/renderer_host/render_frame_proxy_host.cc
- M content/browser/security_exploit_browsertest.cc
- M content/renderer/render_frame_impl.cc
- M content/test/navigation_simulator_impl.cc
- M content/test/test_render_frame_host.cc
- M third_party/blink/public/mojom/frame/remote_frame.mojom
- M third_party/blink/public/mojom/navigation/navigation_params.mojom
- M third_party/blink/public/web/web_navigation_params.h
- M third_party/blink/renderer/core/frame/local_frame_client_impl.cc
- M third_party/blink/renderer/core/frame/remote_frame.cc
Code-Review: +1 by Dave Tapuska, +1 by Arthur Sonzogni
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Victor Obando (Gerrit)
Victor Obando added 1 comment![]( "Open in Gerrit")
Suggestions for CL 7733321: Refining SAA Mojo Parameter Removal
The transition to a browser-side source of truth for Storage Access API (SAA) status is a significant security improvement. To further harden this patch, I suggest considering the following refinements:
1. Strict Browser-Side Validation: Ensure that `NavigationRequest` performs a synchronous check against the `StorageAccessGrant` at the exact moment of the navigation commit. This prevents potential discrepancies between the browser's permission state and the session state, especially during rapid navigation sequences.
2. IPC Hardening: With the removal of these parameters from `CommonNavigationParams`, consider adding a `CHECK` or `DCHECK` within the Mojo deserialization path on the browser side to explicitly verify that no legacy or malformed SAA status signals are being processed from a potentially compromised renderer.
3. Race Condition Mitigation: In cross-document navigations, ensure the SAA token clearance in the `RenderFrameHost` is strictly ordered before the new document's initial script execution. A potential improvement would be to encapsulate the SAA state lifecycle within the `NavigationRequest` to guarantee immutability once the navigation has started.
4. Test Coverage: It would be beneficial to add a Mojo JS Bindings test that attempts to manually inject SAA status parameters during a navigation. This would confirm that the browser process correctly ignores and drops any renderer-provided SAA claims, validating the "source of truth" architectural shift.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Victor Obando (Gerrit)
Victor Obando added 1 comment![]( "Open in Gerrit")
Automated Verification Race Condition Mitigation IPC Hardening & Sanitization Enforce Synchronous Validation
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Victor Obando (Gerrit)
Victor Obando added 1 comment![]( "Open in Gerrit")
```suggestion
Suggestions for CL 7733321: Refining SAA Mojo Parameter Removal
The transition to a browser-side source of truth for Storage Access API (SAA) status is a significant security improvement. To further harden this patch, I suggest considering the following refinements:
1. Strict Browser-Side Validation: Ensure that `NavigationRequest` performs a synchronous check against the `StorageAccessGrant` at the exact moment of the navigation commit. This prevents potential discrepancies between the browser's permission state and the session state, especially during rapid navigation sequences.
2. IPC Hardening: With the removal of these parameters from `CommonNavigationParams`, consider adding a `CHECK` or `DCHECK` within the Mojo deserialization path on the browser side to explicitly verify that no legacy or malformed SAA status signals are being processed from a potentially compromised renderer.
3. Race Condition Mitigation: In cross-document navigations, ensure the SAA token clearance in the `RenderFrameHost` is strictly ordered before the new document's initial script execution. A potential improvement would be to encapsulate the SAA state lifecycle within the `NavigationRequest` to guarantee immutability once the navigation has started.
4. Test Coverage: It would be beneficial to add a Mojo JS Bindings test that attempts to manually inject SAA status parameters during a navigation. This would confirm that the browser process correctly ignores and drops any renderer-provided SAA claims, validating the "source of truth" architectural shift.
```
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |