| # IPC Reviews |
| |
| [TOC] |
| |
| ## tl;dr |
| |
| 📎 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/). |
| |
| ## What is IPC review and why is it needed? |
| |
| A critical part of Chrome’s security is built on process isolation. |
| Untrustworthy content is isolated into unprivileged and sandboxed child |
| processes (e.g. renderer process, GPU process). In contrast, the browser |
| process is fully privileged and is not sandboxed, as it is considered |
| trustworthy. |
| |
| In this model, the browser process acts as an operating system kernel for the |
| untrustworthy child processes. The interfaces defined by IPC are system calls |
| exposed to these (potentially compromised and) untrustworthy child processes, |
| allowing them to use system capabilities (such as the network) as appropriate. |
| Because IPCs cross trust and privilege boundaries between processes, it is |
| critical that IPCs themselves are robust against malicious input. |
| |
| Consider the example interface below, assuming it is exposed to renderer |
| processes: |
| |
| ``` |
| interface CookieJar { |
| GetCookies(url.mojom.Url) => (string); |
| }; |
| ``` |
| |
| In normal circumstances, this isn’t problematic: a well-behaved renderer won’t |
| pass arbitrary URLs to `GetCookies()`; it will only pass URLs of Documents it’s |
| rendering. If the renderer is displaying email, it won’t need (or request) the |
| bank login cookies. However, an attacker with full control over the renderer |
| process can pass any URL to `GetCookies()`. An interface like this that |
| incorrectly trusts the renderer process would allow an attacker to exfiltrate |
| much more data than they should be able to. |
| |
| IPC review is intended to catch these types of bugs. An adversary with full |
| control over an untrustworthy process must not be able to exploit IPC to escape |
| the sandboxed process. The goal is to ensure: |
| |
| - IPC invariants are easy to maintain |
| - IPC interfaces are documented and understandable |
| - IPC handlers are resistant to malicious input |
| |
| Note that IPC review is distinct from [security review for launches or major |
| changes to Chrome][chrome-security-review]; the latter is generally focused on |
| evaluating feature security at a high level, while IPC review is focused on |
| specific implementation details. |
| |
| ## What does an IPC reviewer look for? |
| |
| - **What are the endpoints of the changed IPCs?** Almost all IPCs cross trust |
| and privilege boundaries, so it is important to understand what processes are |
| communicating, which way data flows, and if any of the endpoints are |
| untrustworthy. This is often documented with interface-level comments, e.g. |
| “the Widget interface is used by the browser process to inform the renderer |
| process of UI state changes”. |
| - **What are the changes in capabilities being exposed over IPC?** Many changes |
| provide new capabilities to an untrustworthy process. An IPC reviewer will |
| evaluate if the new capabilities can be abused by an attacker (e.g. retrieve |
| cookies of an unrelated page, write to arbitrary files, et cetera). |
| - **What breaks if an attacker provides malicious input?** If an array argument |
| represents a point in 3D space, one assumption might be that it should contain |
| exactly three elements. An IPC handler processing input from an untrustworthy |
| process must not assume this though; it must validate that the sender actually |
| provided exactly three elements. Note: in this case, an even better |
| alternative would be define a `struct Point` with an `x`, `y`, and `z` fields: |
| then it would be impossible for even a malicious sender to pass a malformed |
| point! |
| |
| To answer these questions, it’s often necessary to evaluate both the code |
| sending and the code reviewing the IPC. Avoid sending out changes where the IPC |
| handler is simply marked `NOTIMPLEMENTED()`: without an actual implementation, |
| it is often impossible to evaluate for potential security issues. |
| |
| Please also keep in mind that an IPC reviewer often will not have all the |
| domain-specific knowledge in a given area (whether that be accessibility, GPU, |
| XR, or something else). Ensure that a change has appropriate context (in the CL |
| description and associated bugs), link to design documents, and thoroughly |
| document interfaces/methods/structs. Good documentation also helps future |
| readers of the code understand the system more easily. |
| |
| ## Guidelines and Best Practices for IPC |
| |
| IPC reviewers will primarily evaluate a CL using the [IPC Review |
| Reference][ipc-review-reference], which contains guidelines and recommendations |
| for how to structure Mojo IPC definitions as well as the corresponding C++/JS code. |
| |
| Additional non-canonical references that may be interesting reading: |
| |
| - The previous [Mojo best practices guide][mojo-best-practices]: this is |
| somewhat out-of-date and may occasionally conflict with the aforementioned |
| reference. The reference should take priority. |
| - [Security Tips for Legacy IPC][legacy-ipc-security] |
| |
| ## When should IPC review happen? |
| |
| In general, include an IPC reviewer when sending a change out for review. Even |
| if a change is under active development, it’s still OK to add an IPC reviewer. |
| While the IPC reviewer might not be actively involved if the design is still in |
| flux with other reviewers, simply being in the loop often provides useful |
| context for the change (and can sometimes save significant future pain). |
| |
| It’s important to note that IPC review isn’t just a rubberstamp; as mentioned |
| above, an IPC reviewer’s focus is on reviewing cross-process interactions, from |
| the perspective of a hostile attacker trying to hijack a user’s machine. Adding |
| an IPC reviewer only after receiving all other LGTMs can sometimes be |
| frustrating for everyone involved, especially if significant revisions are |
| requested. |
| |
| ## Is it OK to TBR a simple change in IPC? |
| |
| Avoid TBRing CLs with IPC changes. If the change is simple, ping an IPC reviewer |
| directly and ask them for a quick LGTM. Erring on the side of safety is |
| preferred for security-critical changes. |
| |
| ## IPC review is slow! |
| |
| Please reach out to <ipc-security-reviewers@chromium.org> (for public issues) |
| or <chrome-security-ipc@google.com> (for internal issues). Large and complex |
| features can be difficult to evaluate on a change by change basis; reaching out |
| can help provide IPC reviewers with better context on the security properties |
| of the overall system, making it much easier to evaluate individual changes. |
| |
| [chrome-security-review]: https://www.chromium.org/Home/chromium-security/security-reviews |
| [ipc-review-reference]: https://docs.google.com/document/d/1Kw4aTuISF7csHnjOpDJGc7JYIjlvOAKRprCTBVWw_E4/edit# |
| [mojo-best-practices]: https://chromium.googlesource.com/chromium/src/+/main/docs/security/mojo.md |
| [legacy-ipc-security]: https://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc/ |