[ui/gfx] Fix NativePixmapPlane offset corruption [chromium/src : main]
Sergio Solano (Gerrit)
Sergio Solano added 1 comment![]( "Open in Gerrit")
Hi guys, can you help me with a review of this qick fix about project Fortify? (Internal only) go/code-terracotta-review-explainer
Related details
- Andres Calderon Jaramillo
- Brandon Jones
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: tse...@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): tse...@chromium.org
Reviewer source(s):
tse...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Andres Calderon Jaramillo
- Brandon Jones
- Colin Blundell
- Tom Sepez
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Sepez (Gerrit)
Tom Sepez voted and added 1 comment![]( "Open in Gerrit")
Votes added by Tom Sepez
| Code-Review | +1 |
1 comment
base::checked_cast<uint32_t>(stride), 0, height_in_pixels * stride,can this overflow as well? Then truncated from size_t to 32 bits?
Related details
- Andres Calderon Jaramillo
- Brandon Jones
- Colin Blundell
- Sergio Solano
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. |
Andres Calderon Jaramillo (Gerrit)
Andres Calderon Jaramillo voted and added 5 comments![]( "Open in Gerrit")
Votes added by Andres Calderon Jaramillo
| Code-Review | +1 |
5 comments
plane.offset, plane.size,Should these maybe be `base::strict_cast<uint64_t>()` and `base::strict_cast<uint64_t>()`?
plane.offset, plane.size,Should these maybe be `base::strict_cast<uint64_t>()` and `base::strict_cast<uint64_t>()`?
base::checked_cast<uint32_t>(plane.stride), plane.offset,
plane.size, std::move(duped_fds[i]));Should these maybe be `base::strict_cast<uint64_t>()` and `base::strict_cast<uint64_t>()`?
UNSAFE_TODO(import_data.strides[plane]) =
base::checked_cast<uint32_t>(handle.planes[plane].stride);
UNSAFE_TODO(import_data.offsets[plane]) =
base::checked_cast<uint32_t>(handle.planes[plane].offset);These ones should remain `base::checked_cast<int>` because `int` is used in [`gbm_import_fd_modifier_data`](https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/minigbm/gbm.h;l=375-376;drc=698c5e9e8d94bc4ed988bd07b246cf37380851d0).
native_pixmap_handle, output);nit: Add `ASSERT_FALSE(output.planes.empty());`
Related details
- Brandon Jones
- Colin Blundell
- Sergio Solano
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. |
Sergio Solano (Gerrit)
Sergio Solano voted and added 7 comments![]( "Open in Gerrit")
Votes added by Sergio Solano
| Commit-Queue | +1 |
7 comments
Hi Tom and Andres,
Thanks for the reviews. To land this safety fix as quickly as possible and avoid cross-component approval delays, I've decided to split the original CL into two parts:
1. This CL (Mojo Security): Focused strictly on the Mojo boundary. I've added range validation to the Traits to prevent the sign-extension exploit from a compromised renderer. This is the core fix for b:497542537.
2. Follow-up CL (Media Safety): I've moved the internal hygiene and safety improvements for media/gpu and chromeos/ash call sites here: https://crrev.com/c/7724047
In this patchset, I have also:
- Restored the ui/gfx headers and .mojom files to their base state to keep the public API unchanged for this quick fix.
- Included the requested Mojo regression test with Andres' nits (ASSERT_FALSE).
- Addressed the security concerns while avoiding unnecessary structural changes.
Do you agree to leave the full type migration (to uint32_t/uint64_t) and the overflow checks in test_shared_image_interface.cc in the subsequent permanent fix?
base::checked_cast<uint32_t>(stride), 0, height_in_pixels * stride,can this overflow as well? Then truncated from size_t to 32 bits?
Agree, however, to land the core security fix as quickly as possible and avoid cross-component approval delays, I have focused this CL strictly on the Mojo boundary and reverted changes to this test-only file.
For the internal Media/ChromeOS safety improvements, I've made follow-up CL: https://crrev.com/c/7724047
Should these maybe be `base::strict_cast<uint64_t>()` and `base::strict_cast<uint64_t>()`?
I've kept the constructor parameters as int (matching base state) to land the safety fix quickly. Since strict_cast to uint64_t would cause a compile error, I've used base::checked_cast<int> instead. This provides the same security guarantee by crashing on overflow.
You can see these updated call sites in the follow-up CL: https://crrev.com/c/7724047. The full header migration to uint64_t will follow in the permanent fix.
Should these maybe be `base::strict_cast<uint64_t>()` and `base::strict_cast<uint64_t>()`?
Same as above. Updated in the follow-up CL: https://crrev.com/c/7724047
base::checked_cast<uint32_t>(plane.stride), plane.offset,
plane.size, std::move(duped_fds[i]));Should these maybe be `base::strict_cast<uint64_t>()` and `base::strict_cast<uint64_t>()`?
Same as my previous comment. I've used checked_cast<int> where the destination is the NativePixmapPlane constructor (to match the int parameters in the header), and safe casts where the destination allows uint64_t.
Updated in: https://crrev.com/c/7724047
UNSAFE_TODO(import_data.strides[plane]) =
base::checked_cast<uint32_t>(handle.planes[plane].stride);
UNSAFE_TODO(import_data.offsets[plane]) =
base::checked_cast<uint32_t>(handle.planes[plane].offset);These ones should remain `base::checked_cast<int>` because `int` is used in [`gbm_import_fd_modifier_data`](https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/platform/minigbm/gbm.h;l=375-376;drc=698c5e9e8d94bc4ed988bd07b246cf37380851d0).
Done
native_pixmap_handle, output);Sergio Solanonit: Add `ASSERT_FALSE(output.planes.empty());`
Done
Related details
- Andres Calderon Jaramillo
- Stephen Nusko
- Tom Sepez
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Stephen Nusko (Gerrit)
Stephen Nusko voted and added 1 comment![]( "Open in Gerrit")
Votes added by Stephen Nusko
| Code-Review | +1 |
1 comment
out->stride = base::checked_cast<int>(data.stride());can't these just be static_cast? Because we've already checked that the data will fit inside the requested `int`? We don't need the runtime check of base::checked_cast right?
Related details
- Andres Calderon Jaramillo
- Sergio Solano
- Tom Sepez
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. |
Stephen Nusko (Gerrit)
Stephen Nusko added 1 comment![]( "Open in Gerrit")
base::checked_cast<uint32_t>(stride), 0, height_in_pixels * stride,Sergio Solanocan this overflow as well? Then truncated from size_t to 32 bits?
Agree, however, to land the core security fix as quickly as possible and avoid cross-component approval delays, I have focused this CL strictly on the Mojo boundary and reverted changes to this test-only file.
For the internal Media/ChromeOS safety improvements, I've made follow-up CL: https://crrev.com/c/7724047
I don't see the this file being changed in your follow up CL? Looks like we might have needed CheckMul?
Tom Sepez (Gerrit)
Tom Sepez voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Andres Calderon Jaramillo
- Sergio Solano
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. |