blob: e23ae4f65fb3e0d908228def78fb9d17a186aa63 [file] [log] [blame] [view]
Daniel Chengd21b551f2017-10-30 08:43:231# Mojo "Style" Guide
2
3Mojo is Chrome's new IPC system and provides lots of useful abstractions. These
4abstractions can make it easier to write code that makes interprocess calls,
5but can also add significant complexity. Below are some recommendation from
6Mojo and IPC reviewers for best practices.
7
Michael Giuffrida68208212019-02-11 09:15:218For questions, concerns, or suggestions, reach out to
9[[email protected]](https://groups.google.com/a/chromium.org/forum/#!forum/chromium-mojo).
Daniel Chengd21b551f2017-10-30 08:43:2310
11> For legacy IPC, please see [security tips for IPC][security-tips-for-ipc].
12
13[TOC]
14
15
16## Simplicity
17
18Strive to write simple interfaces. Minimize the amount of cross-process state
19that needs to be maintained in sync.
20
21**_Good_**
22
23```c++
Daniel Chengaa8a2672017-11-02 08:05:0124interface TeleporterFactory {
Oksana Zhuravlova3fde5ed2020-04-11 00:40:5525 Create(Location start, Location end) => (pending_remote<Teleporter>);
Daniel Chengd21b551f2017-10-30 08:43:2326};
Daniel Chengaa8a2672017-11-02 08:05:0127
28interface Teleporter {
29 TeleportGoat(Goat) = ();
Daniel Chengd21b551f2017-10-30 08:43:2330};
31```
32
33**_Bad_**
34
35```c++
Daniel Chengaa8a2672017-11-02 08:05:0136interface Teleporter {
37 // Bad: comments will need to explicitly call out that both locations need to
38 // be bound before calling TeleportGoat()!
Daniel Chengd21b551f2017-10-30 08:43:2339 //
Daniel Chengaa8a2672017-11-02 08:05:0140 // In addition, if untrustworthy processes can talk to trustworthy processes,
41 // the Teleporter implementation will need to also handle the case where the
42 // Location objects are not yet bound.
43 SetStart(Location);
44 SetEnd(Location);
45 TeleportGoat(Goat) = ();
Daniel Chengd21b551f2017-10-30 08:43:2346};
47```
48
49Similarly, strive to make methods focused. Do not overuse optional types.
50
51**_Good_**
52
53```c++
Daniel Chengaa8a2672017-11-02 08:05:0154struct TeleporterStats {
55 AnimalStats animal_stats;
56 FungiStats fungi_stats;
57 GoatStats goat_stats;
58 PlantStats plant_stats;
59};
60
61interface Teleporter {
62 TeleportAnimal(Animal) => ();
63 TeleportFungi(Fungi) => ();
64 TeleportGoat(Goat) = ();
65 TeleportPlant(Plant) => ();
66
Roland Bock3afe8cc2020-11-06 23:14:5367 // TeleporterStats will be have a value if and only if the call was
68 // successful.
69 GetStats() => (TeleporterStats?);
Daniel Chengd21b551f2017-10-30 08:43:2370};
71```
72
73**_Bad_**
74
75```c++
Daniel Chengaa8a2672017-11-02 08:05:0176interface Teleporter {
77 // The intent of four optional arguments is unclear: can this call teleport
78 // multiple objects of different types at once, or is the caller only
79 // supposed to only pass one non-null argument per call?
80 Teleport(Animal?, Fungi?, Goat?, Plant?) => ();
81
Roland Bock3afe8cc2020-11-06 23:14:5382 // Does this return all stats if success is true? Or just the categories that
Daniel Chengaa8a2672017-11-02 08:05:0183 // the teleporter already has stats for? The intent is uncertain, so wrapping
84 // the disparate values into a result struct would be cleaner.
85 GetStats() =>
86 (bool success, AnimalStats?, FungiStats?, PlantStats?, FungiStats?);
Daniel Chengd21b551f2017-10-30 08:43:2387};
88```
89
90
91## Documentation
92
93Mojo structs, interfaces, and methods should all have comments. Make sure the
94comments cover the "how" and the "why" of using an interface and its methods,
95and not just the "what". Document preconditions, postconditions, and trust: if
96an interface is implemented in the browser process and handles requests from
97the renderer process, this should be mentioned in the comments. Complex features
98should also have an external `README.md` that covers the high-level flow of
99information through interfaces and how they interact to implement the feature.
100
101**_Good_**
102
103```c++
Daniel Chengaa8a2672017-11-02 08:05:01104// Interface for controlling a teleporter. Lives in the browser process, and
105// used to implement the Teleportation over Mojo IPC RFC.
106interface Teleporter {
107 // Teleportation helpers for different taxonomic kingdoms. Teleportation is
108 // not complete until the reply callback is invoked. The caller must NOT
109 // release the sender side resources until the reply callback runs; releasing
110 // the resources early will cause splinching.
111 TeleportAnimal(Animal) => ();
112 TeleportFungi(Fungi) => ();
113 // Goats require a specialized teleportation channel distinct from
114 // TeleportAnimal to ensure goatiness isolation.
115 TeleportGoat(Goat) => ();
116 TeleportPlant(Plant) => ();
Daniel Chengd21b551f2017-10-30 08:43:23117
Roland Bock3afe8cc2020-11-06 23:14:53118 // Returns current teleporter stats. On failure (e.g. a teleportation
119 // operation is currently in progress) a null stats object will be returned.
120 GetStats() => (TeleporterStats?);
Daniel Chengd21b551f2017-10-30 08:43:23121};
122```
123
124
125## Security
126
127Policy should be controlled solely by the browser process. "Policy" can mean
128any number of things, such as sizes, addresses, permissions, URLs, origins,
129etc. In an ideal world:
130
1311. Unprivileged process asks for a capability from the privileged process that
132 owns the resource.
1331. Privileged process applies policy to find an implementation for the
134 capability.
1351. Unprivileged process performs operations on the capability, constrained in
136 scope.
137
138The privileged process must own the capability lifecycle.
139
140
141### Do not trust less privileged processes
142
143This is the overriding principle for all guidelines in this section. When
144receiving data from a less trusted process, treat the data as if it were
145generated by a malicious adversary. Message handlers cannot assume that offsets
146are valid, calculations won't overflow, et cetera.
147
148In general:
149
150* the browser process is the most privileged process type and therefore, must
151 be maximally suspicious of its IPC inputs
152* the renderer and the ARC++ processes are the least privileged and least
153 trustworthy process types
154* other process types, such as GPU and plugin, fall in between
155
156When passing objects up a privilege gradient (from less → more privileged), the
157callee must validate the inputs before acting on them. When passing objects
158down a privilege gradient, such as from browser → renderer, it is OK for the
159callee to trust the caller.
160
161> See also: [Do not Handle Impossible Situations](#Do-not-handle-impossible-situations)
162
163
164### Do not send unnecessary or privilege-presuming data
165
Oksana Zhuravlovaee1afd12020-02-15 00:47:27166> Each `BrowserInterfaceBroker` for frames and workers is strongly associated with an
Darwin Huangb4bd2452019-10-08 22:56:04167> origin. Where possible, prefer to use this associated origin rather than
168> sending it over IPC. (See <https://crbug.com/734210> and
169> <https://crbug.com/775792/>).
Daniel Chengd21b551f2017-10-30 08:43:23170
171For example, the browser process must not (fully) trust the renderer's claims
172about origins. The browser process should already know what origin the renderer
173is evaluating, and thus should already have this data (for example, see
174`RenderFrameHost::GetLastCommittedOrigin()`). Thus, a method that requires
175passing an origin from the renderer to the browser process has a conceptual
176error, and quite possibly, a vulnerability.
177
178> Note: there are currently subtle races when using `GetLastCommittedOrigin()`
179> that will be resolved by fixing <https://crbug.com/729021>.
180
181Similarly, the browser process must not trust the renderer's claims about file
182pathnames. It would be unsafe for the browser process to save a downloaded file
183to `~/.bashrc` just because the renderer asked. Instead, it would be better for
184the browser process to:
185
1861. Kill the renderer if `basename(pathname) != pathname`, since the renderer is
187 obviously compromised if it makes this mistake.
1881. Defang the basename, by removing leading dots, et cetera. Note that the
189 definition of proper defanging varies per platform.
1901. Prepend its own parent directory to the basename, e.g. ~/Downloads.
191
192> TODO(https://crbug.com/779196): Even better would be to implement a C++ type
193> performs the appropriate sanitizations and recommend its usage directly here.
194
195
Lukasz Anforowiczbb99a8562019-01-25 19:19:38196### Validate privilege-presuming data received over IPC
197
198If it is not possible to avoid sending privilege-presuming data over IPC (see
199the previous section), then such data should be verified before being used.
200
201* Browser process:
202 - Use `ChildProcessSecurityPolicy`'s methods like
203 `CanAccessDataForOrigin` or `CanReadFile` to verify IPC messages
204 received from less privileged processes.
205 - When verification fails, ignore the IPC and terminate the renderer process
206 using `mojo::ReportBadMessage` (or using `mojo::GetBadMessageCallback` for
207 messages handled asynchronously). For legacy IPC, the renderer process
208 may be terminated by calling the `ReceivedBadMessage` function (separate
209 implementations exist for `//content`, `//chrome` and other layers).
210
Lukasz Anforowiczbb99a8562019-01-25 19:19:38211
Daniel Cheng0a7a4ec2018-02-13 23:30:27212### Do not define unused or unimplemented things
213
214Mojo interfaces often cross privilege boundaries. Having well-defined interfaces
215that don't contain stubbed out methods or unused parameters makes it easier to
Daniel Chengcda1df5b2018-03-30 21:30:16216understand and evaluate the implications of crossing these boundaries. Several
217common areas to watch out for:
Daniel Cheng0a7a4ec2018-02-13 23:30:27218
219
220#### Do use EnableIf to guard platform-specific constructs
Daniel Chengd21b551f2017-10-30 08:43:23221
222Platform-specific functionality should only be defined on the platforms where
Daniel Cheng0a7a4ec2018-02-13 23:30:27223it is implemented. Use the Mojo `EnableIf` annotation to guard definitions that
224should only be visible in certain build configurations.
Daniel Chengd21b551f2017-10-30 08:43:23225
Daniel Cheng0a7a4ec2018-02-13 23:30:27226**_Good_**
227```c++
228// GN file:
229mojom("view_bindings") {
230 // ...
Daniel Chengd21b551f2017-10-30 08:43:23231
Daniel Cheng0a7a4ec2018-02-13 23:30:27232 enabled_features = []
233 if (is_android) {
Bill Orrdc01ca32019-01-30 00:22:11234 enabled_features += [ "is_android" ]
Daniel Cheng0a7a4ec2018-02-13 23:30:27235 }
236}
237
238// mojom definition:
239interface View {
240 // ...
241
242 [EnableIf=is_android]
243 UpdateBrowserControlsState(bool enable_hiding, bool enable_showing,
244 bool animate);
245};
246
247// C++ implementation:
248class View : public mojom::View {
249 public:
250 // ...
251
Xiaohan Wang0727d882022-01-21 03:03:43252#if BUILDFLAG(IS_ANDROID)
Daniel Cheng0a7a4ec2018-02-13 23:30:27253 void UpdateBrowserControlsState(bool enable_hiding, bool enable_showing,
254 bool animate);
255#endif
256};
257```
258
259**_Bad_**
260```c++
261// mojom definition:
262interface View {
263 // ...
264
265 UpdateBrowserControlsState(bool enable_hiding, bool enable_showing,
266 bool animate);
267};
268
269// C++ implementation:
270class View : public mojom::View {
271 public:
272 // ...
273
Xiaohan Wang0727d882022-01-21 03:03:43274#if BUILDFLAG(IS_ANDROID)
Daniel Cheng0a7a4ec2018-02-13 23:30:27275 void UpdateBrowserControlsState(bool enable_hiding, bool enable_showing,
276 bool animate) override;
277#else
278 void UpdateBrowserControlsState(bool enable_hiding, bool enable_showing,
279 bool animate) override {
280 NOTREACHED();
281 }
282#endif
283};
284```
285
Xiaohan Wang688f49d2018-02-26 19:39:53286The `EnableIf` annotation can be applied to almost anything: imports,
287interfaces, methods, arguments, constants, structs, struct members, enums,
288enumerator values, et cetera.
Daniel Cheng0a7a4ec2018-02-13 23:30:27289
290
291#### Do not define unimplemented methods
292
293Reviewing IPC requires reviewing a concrete implementation of the Mojo
294interface, to evaluate how the (possibly untrustworthy) inputs are used, what
295outputs are produced, et cetera. If a method is not yet implemented, do not
296define it in the interface.
297
298**_Bad_**
299```c++
300// mojom definition:
301interface Spaceship {
302 EnterHyperspace();
303 ExitHyperspace();
304};
305
306// C++ implementation:
307class SpaceshipPrototype : public mojom::Spaceship {
308 void EnterHyperspace() { /* TODO(dcheng): Implement. */ }
309 void ExitHyperspace() { /* TODO(dcheng): Implement. */ }
310};
311```
312
313
Daniel Chengcda1df5b2018-03-30 21:30:16314#### Do not define placeholder enumerator values
Daniel Cheng0a7a4ec2018-02-13 23:30:27315
Daniel Chengcda1df5b2018-03-30 21:30:16316Do not define placeholder enumerator values like `kLast`, `kMax`, `kCount`, et
317cetera. Instead, rely on the autogenerated `kMaxValue` enumerator emitted for
318Mojo C++ bindings.
Daniel Chengd21b551f2017-10-30 08:43:23319
Daniel Chengcda1df5b2018-03-30 21:30:16320For UMA histograms, logging a Mojo enum is simple: simply use the two argument
321version of `UMA_HISTOGRAM_ENUMERATION`:
Daniel Chengd21b551f2017-10-30 08:43:23322
Daniel Chengcda1df5b2018-03-30 21:30:16323**_Good_**
324```c++
325// mojom definition:
326enum GoatStatus {
327 kHappy,
328 kSad,
329 kHungry,
330 kGoaty,
331};
332
333// C++:
334UMA_HISTOGRAM_ENUMERATION("Goat.Status", status);
335```
336
337Using a `kCount` sentinel complicates `switch` statements and makes it harder to
338enforce invariants: code needs to actively enforce that the otherwise invalid
339`kCount` sentinel value is not incorrectly passed around.
340
341**_Bad_**
342```c++
343// mojom definition:
344enum CatStatus {
345 kAloof,
346 kCount,
347};
348
349// C++
350switch (cat_status) {
351 case CatStatus::kAloof:
352 IgnoreHuman();
353 break;
354 case CatStatus::kCount:
355 // this should never happen
356}
357```
358
359Defining `kLast` manually results in ugly casts to perform arithmetic:
360
361**_Bad_**
362```c++
363// mojom definition:
364enum WhaleStatus {
365 kFail,
366 kNotFail,
367 kLast = kNotFail,
368};
369
370// C++:
371UMA_HISTOGRAM_ENUMERATION("Whale.Status", status,
372 static_cast<int>(WhaleStatus::kLast) + 1);
373```
374
375For interoperation with legacy IPC, also use `kMaxValue` rather than defining a
376custom `kLast`:
377
378**_Good_**
379```c++
380IPC_ENUM_TRAITS_MAX_VALUE(GoatStatus, GoatStatus::kMaxValue);
381```
Daniel Chengd21b551f2017-10-30 08:43:23382
383
384### Use structured types
385
386Where possible, use structured types: this allows the type system to help
387enforce that the input data is valid. Common ones to watch out for:
388
Oksana Zhuravlovac70ec7ef2018-03-13 21:08:42389* Files: use `mojo_base.mojom.File`, not raw descriptor types like `HANDLE`
Daniel Chengd21b551f2017-10-30 08:43:23390 and `int`.
Oksana Zhuravlovac70ec7ef2018-03-13 21:08:42391* File paths: use `mojo_base.mojom.FilePath`, not `string`.
Oksana Zhuravlova38321d082018-04-27 23:59:01392* JSON: use `mojo_base.mojom.Value`, not `string`.
Daniel Chengd21b551f2017-10-30 08:43:23393* Mojo interfaces: use `Interface` or `Interface&`, not `handle` or
394 `handle<message_pipe>`.
Oksana Zhuravlova34579e912018-03-23 00:18:49395* Nonces: use `mojo_base.mojom.UnguessableToken`, not `string`.
Daniel Chengd21b551f2017-10-30 08:43:23396* Origins: use `url.mojom.Origin`, not `url.mojom.Url` and certainly not
397 `string`.
Oksana Zhuravlova57e8dca2018-03-20 03:21:34398* Time types: use `mojo_base.mojom.TimeDelta` /
399 `mojo_base.mojom.TimeTicks` / `mojo_base.mojom.Time`, not `int64` /
Daniel Chengd21b551f2017-10-30 08:43:23400 `uint64` / `double` / et cetera.
401* URLs: use `url.mojom.Url`, not `string`.
Daniel Cheng876c66322018-08-29 06:42:31402* `array<uint8>` or `string` and `memcpy()`: use a Mojo struct and statically
403 define the serialized fields. While `memcpy()` may be tempting for its
404 simplicity, it can leak info in padding. Even worse, `memcpy()` can easily
405 copy [undocumented fields][serialize-struct-tm-safely] or newly introduced
406 fields that were never evaluated for safety by the developer or reviewer.
Daniel Chengd21b551f2017-10-30 08:43:23407
408**_Good_**
409
410```c++
411interface ReportingService {
Oksana Zhuravlova57e8dca2018-03-20 03:21:34412 ReportDeprecation(mojo_base.mojom.TimeTicks time,
Daniel Chengd21b551f2017-10-30 08:43:23413 url.mojom.Url resource,
414 uint32 line_number);
415};
416```
417
418**_Bad_**
419
420```c++
421interface ReportingService {
422 // Bad: unclear what units |time| is or what |data| contains.
Oksana Zhuravlova38321d082018-04-27 23:59:01423 ReportDeprecation(double time, mojo_base.mojom.Value data);
Daniel Chengd21b551f2017-10-30 08:43:23424};
425```
426
Daniel Cheng876c66322018-08-29 06:42:31427Avoid parallel arrays of data that require the receiver to validate that the
428arrays have matching lengths. Instead, bundle the data together in a struct so
429it is impossible to have a mismatch:
Daniel Chengd21b551f2017-10-30 08:43:23430
431**_Good_**
432
433```c++
434struct Pixel {
435 int8 reds;
436 int8 greens;
437 int8 blues;
438 int8 alphas;
439};
440
441struct Bitmap {
442 // Good: it is impossible for there to be mismatched data.
443 array<Pixel> pixels;
444};
445```
446
447**_Bad_**
448
449```c++
450// Bad: code using this struct will need to validate that all the arrays have
451// matching sizes.
452struct Bitmap {
453 array<int8> reds;
454 array<int8> greens;
455 array<int8> blues;
456 array<int8> alphas;
457};
458```
459
460
461### Beware of arithmetic overflow
462
463> TODO(dcheng): Import the guidance from the legacy IPC doc.
464
465Signed overflow is undefined in C++. If unsure about whether or not something
466will overflow, use the safe numeric helpers from `//base/numerics`!
467
468**_Good_**
469
470```c++
471base::CheckedNumeric<int32_t> size = mojo_rect->width();
472size *= mojo_rect.height();
473if (!size.IsValid()) {
474 mojo::ReportBadMessage("Bad size from renderer!");
475}
476```
477
478**_Bad_**
479
480```c++
481// Bad: Signed overflow is undefined in C++!
482int32_t size = mojo_rect->width() * mojo_rect.height();
483```
484
485Note that even if the types have defined overflow semantics, it is almost always
486a good idea to check for overflow.
487
488**_Good_**
489
490```c++
491uint32_t alloc_size;
492if (!CheckMul(request->elements(), request->element_size())
493 .AssignIfValid(&alloc_size)) {
494 // Safe: avoids allocating with a bogus size that overflowed to a smaller than
495 // expected value.
Rayan Kanso81aeee32019-07-11 18:50:44496 mojo::ReportBadMessage("Invalid allocation size");
Daniel Chengd21b551f2017-10-30 08:43:23497}
498
499Element* array = CreateArray(alloc_size);
500for (size_t i = 0; i < request->element_size(); ++i) {
501 array[i] = PopulateArray(i);
502}
503```
504
505**_Bad_**
506
507```c++
508uint32_t alloc_size = request->elements() * request->element_size();
509
510// Dangerous: alloc_size can overflow so that CreateArray allocates too little
511// memory. Subsequent assignments will turn into an out-of-bound write!
512Element* array = CreateArray(alloc_size);
513for (size_t i = 0; i < request->element_size(); ++i) {
514 array[i] = PopulateArray(i);
515}
516```
517
518
Erik Chen14a4bad2021-04-21 17:53:13519### All possible message values are semantically valid
520
521When possible, messages should be defined in such a way that all possible values
522are semantically valid. As a corollary, avoid having the value of one field
523dictate the validity of other fields.
524
525**_Good_**
526
527```c++
528union CreateTokenResult {
529 // Implies success.
530 string token;
531
532 // Implies failure.
533 string error_message;
534};
535
536struct TokenManager {
537 CreateToken() => (CreateTokenResult result);
538};
539```
540
541**_Bad_**
542```c++
543struct TokenManager {
544 // Requires caller to handle edge case where |success| is set to true, but
545 // |token| is null.
546 CreateToken() => (bool success, string? token, string? error_message);
547
548 // Requires caller to handle edge case where both |token| and |error_message|
549 // are set, or both are null.
550 CreateToken() => (string? token, string? error_message);
551};
552```
553
554There are some known exceptions to this rule because mojo does not handle
555optional primitives.
556
557**_Allowed because mojo has no support for optional primitives_**
558```c++
559 struct Foo {
560 int32 x;
561 bool has_x; // does the value of `x` have meaning?
562 int32 y;
563 bool has_y; // does the value of `y` have meaning?
564 };
565```
566
567Another common case where we tolerate imperfect message semantics is
568with weakly typed integer [bitfields](#handling-bitfields).
569
570### Handling bitfields
571
572Mojom has no native support for bitfields. There are two common approaches: a
573type-safe struct of bools which is a bit clunky (preferred) and an integer-based
574approach (allowed but not preferred).
575
576**_Type-safe bitfields_**
577```c++
578struct VehicleBits {
579 bool has_car;
580 bool has_bicycle;
581 bool has_boat;
582};
583
584struct Person {
585 VehicleBits bits;
586};
587```
588
589**_Integer based approach_**
590```c++
591struct Person {
592 const uint64 kHasCar = 1;
593 const uint64 kHasBicycle = 2;
594 const uint64 kHasGoat= 4;
595
596 uint32 vehicle_bitfield;
597};
598```
599
Andrew Williams304e9cf52022-01-30 23:57:16600### Avoid object lifetime issues with self-owned receivers
601
602When creating new
603[Mojo services](https://chromium.googlesource.com/chromium/src/+/HEAD/docs/mojo_and_services.md)
604in the browser process (exposed to the renderer via `BrowserInterfaceBrokers` in
605a host object like `RenderFrameHostImpl`, `DedicatedWorkerHost`, etc.), one
606approach is to have the interface implementation be owned by the `Receiver`
607using `mojo::MakeSelfOwnedReceiver`. From the
608[`mojo::MakeSelfOwnedReceiver` declaration](https://source.chromium.org/chromium/chromium/src/+/main:mojo/public/cpp/bindings/self_owned_receiver.h;l=129;drc=643cdf61903e99f27c3d80daee67e217e9d280e0):
609```
610// Binds the lifetime of an interface implementation to the lifetime of the
611// Receiver. When the Receiver is disconnected (typically by the remote end
612// closing the entangled Remote), the implementation will be deleted.
613```
614Consider such an interface created in `RenderFrameHostImpl`, and consider that
615and a corresponding `Remote` was created for this interface and owned by
616`RenderFrame`. It may seem logical to think that:
617 1. (true) The `Receiver` owns the interface implementation
618 2. (true) The lifetime of the `Receiver` is based on the lifetime of the
619 `Remote` in the renderer
620 3. (true) The `Remote` is owned by the `RenderFrame`
621 4. (true) The lifetime of the `RenderFrameHostImpl` is based on the lifetime of
622 the `RenderFrame`
623 5. (true) Destroying the `RenderFrame` will cause the `Remote` to be destroyed,
624 ultimately causing the `Receiver` and the interface implementation to be
625 destroyed. The `RenderFrameHostImpl` will likely be destroyed at some point
626 as well.
627 6. (false) It's safe to assume that `RenderFrameHostImpl` will outlive the
628 self-owned `Receiver` and interface implementation
629
630A
631[common](https://microsoftedge.github.io/edgevr/posts/yet-another-uaf/)
632mistake based on the last assumption above is to store and use a raw pointer
633to the `RenderFrameHostImpl` object in the interface implementation. If the
634`Receiver` outlives the `RenderFrameHostImpl` and uses the pointer to it, a
635Use-After-Free will occur. One way a malicious site or compromised renderer
636could make this happen is to generate lots of messages to the interface and then
637close the frame. The `Receiver` might have a backlog of messages to process
638before it gets the message indicating that the renderer's `Remote` was closed,
639and the `RenderFrameHostImpl` can be destroyed in the meantime.
640
641Similarly, it's not safe to assume that the `Profile` object (and objects owned
642by it; `StoragePartitionImpl`, for instance) will outlive the `Receiver`. This
643has been observed to be true for at least incognito windows, where a renderer
644can generate messages, close the page, and cause the entire window to close
645(assuming no other pages are open), ultimately causing the
646`OffTheRecordProfileImpl` object to be destroyed before the `Receiver` object.
647
648To avoid these types of issues, some solutions include:
649
650 - Using `DocumentService` or `DocumentUserData` instead of
651 `mojo::MakeSelfOwnedReceiver` for document-based interfaces where the
652 interface implementation needs access to a `RenderFrameHostImpl` object. See
653 the
654 [`DocumentService` declaration](https://source.chromium.org/chromium/chromium/src/+/main:content/public/browser/document_service.h;l=27;drc=d4bf612a0258dd80cfc6d17d49419dd878ebaeb0)
655 for more details.
656
657 - Having the `Receiver` and/or interface implementation be owned by the object
658 it relies on (for instance, store the `Receiver` in a private member or
659 use a `mojo::UniqueReceiverSet` for storing multiple `Receiver` /
660 interface implementation pairs).
661
662 - Using `WeakPtr`s instead of raw pointers to provide a way to check whether
663 an object has been destroyed.
664
Daniel Chengd21b551f2017-10-30 08:43:23665## C++ Best Practices
666
667
Daniel Chengd8df82002018-02-09 20:52:15668### Use mojo::WrapCallbackWithDefaultInvokeIfNotRun And mojo::WrapCallbackWithDropHandler sparingly
Daniel Cheng9ae28e02018-01-17 18:44:10669
670Mojo provides several convenience helpers to automatically invoke a callback if
671the callback has not already been invoked in some other way when the callback is
672destroyed, e.g.:
673
674```c++
675 {
Dan Sandersce2b98e2021-02-03 06:46:10676 base::OnceCallback<int> cb = mojo::WrapCallbackWithDefaultInvokeIfNotRun(
677 base::BindOnce([](int) { ... }), -1);
Daniel Cheng9ae28e02018-01-17 18:44:10678 } // |cb| is automatically invoked with an argument of -1.
679```
680
Daniel Chengd8df82002018-02-09 20:52:15681This can be useful for detecting interface errors:
Daniel Cheng9ae28e02018-01-17 18:44:10682
Daniel Chengd8df82002018-02-09 20:52:15683```c++
684 process->GetMemoryStatistics(
685 mojo::WrapCallbackWithDefaultInvokeIfNotRun(
Dan Sandersce2b98e2021-02-03 06:46:10686 base::BindOnce(&MemoryProfiler::OnReplyFromRenderer), <failure args>));
Daniel Chengd8df82002018-02-09 20:52:15687 // If the remote process dies, &MemoryProfiler::OnReplyFromRenderer will be
688 // invoked with <failure args> when Mojo drops outstanding callbacks due to
689 // a connection error on |process|.
690```
691
692However, due to limitations of the current implementation, it's difficult to
693tell if a callback object has invoke-on-destroy behavior. In general:
694
6951. Prefer error connection handlers where possible.
6961. Only use the callback helpers for detecting interface errors. These
697 callbacks may be invoked during destruction and must carefully consider
698 receiver object lifetime. For more information, please see the
699 [Mojo documentation][mojo-doc-process-crashes].
700
701> Note that using the callback wrappers in the renderer is often unnecessary.
702> Message pipes are typically closed as part of a Document shutting down; since
703> many Blink objects already inherit `blink::ContextLifecycleObserver`, it is
704> usually more idiomatic to use this signal to perform any needed cleanup work.
Daniel Cheng9ae28e02018-01-17 18:44:10705
Daniel Chengd21b551f2017-10-30 08:43:23706### Use StructTraits
707
708Creating a typemap and defining a `StructTraits` specialization moves the
709complexity of serialization, deserialization, and validation into a central
710location. We universally recommend this over defining `TypeConverter`
711specializations: when a value fails deserialization, the receiver method will
712never even be invoked. As a bonus, it also reduces the number of copies during
713serialization and deserialization. 😄
714
715**_Good_**
716
717```c++
Daniel Cheng1f386932018-01-29 19:56:47718// In url_gurl_mojom_traits.h:
Daniel Chengd21b551f2017-10-30 08:43:23719template <>
720struct StructTraits<url::mojom::UrlDataView, GURL> {
721 static base::StringPiece url(const GURL& r);
722
723 // If Read() returns false, Mojo will discard the message.
724 static bool Read(url::mojom::UrlDataView data, GURL* out);
725};
726
Daniel Cheng1f386932018-01-29 19:56:47727// In url_gurl_mojom_traits.cc:
Daniel Chengd21b551f2017-10-30 08:43:23728// Note that methods that aren't simple getters should be defined
729// out-of-line to avoid code bloat.
730base::StringPiece StructTraits<url::mojom::UrlDataView, GURL>::url(
731 const GURL& r) {
732 if (r.possibly_invalid_spec().length() > url::kMaxURLChars ||
733 !r.is_valid()) {
734 return base::StringPiece();
735 }
736 return base::StringPiece(r.possibly_invalid_spec().c_str(),
737 r.possibly_invalid_spec().length());
738}
739
740bool StructTraits<url::mojom::UrlDataView, GURL>::Read(
741 url::mojom::UrlDataView data, GURL* out) {
742 base::StringPiece url_string;
743 if (!data.ReadUrl(&url_string))
744 return false;
745 if (url_string.length() > url::kMaxURLChars)
746 return false;
747 *out = GURL(url_string);
Dominic Farolino1f92f2a2021-05-27 21:22:41748 if (!url_string.empty() && !out->is_valid())
749 return false;
750 return true;
Daniel Chengd21b551f2017-10-30 08:43:23751}
752```
753
754**_Bad_**
755
756```c++
757template <>
758struct TypeConverter<url::mojom::UrlPtr, GURL> {
759 // Inefficient: this copies data once off the wire to create a
760 // url.mojom.Url object, then copies it again to create a GURL.
761 static GURL Convert(const url::mojom::UrlPtr url) {
762 if (url.url.is_empty()) return GURL();
763 // Not good: no way to signal errors, so any code that converts the
764 // Mojo struct to a GURL will somehow need to check for errors…
765 // but it can't even be distinguished from the empty URL case!
766 if (url.url.size() > url::kMaxURLChars) return GURL();
767 return GURL(url.url);
768 }
769};
770```
771
772There are also corresponding `EnumTraits` and `UnionTraits` specializations for
773mojo enums and unions respectively.
774
775
776### StructTraits getters should be simple
777
778Where possible, `StructTraits` should be returning const references or simple
779read-only views of the data. Having to create temporary data structures during
780serialization should be rare, and it should be even rarer to mutate the input
781argument.
782
783
784### Out-of-line complex serialization/deserialization logic
785
786A `StructTraits` specialization is almost always fully specialized. Only define
787`StructTraits` methods inline in the header if the method is a simple getter
788that returns a reference, pointer, or other simple POD. Define all other
789methods out-of-line to avoid code bloat.
790
791
792### Do not write one-off functions to convert to/from Mojo types
793
794There are some instances where it is simply not possible to define a
795`StructTraits` for type mapping: this commonly occurs with Blink IDL and Oilpan
796types. In these instances, add a `TypeConverter` specialization rather than
797defining a one-off conversion function. This makes it easier to search for and
798audit code that does potentially risky type conversions.
799
800> The use of `TypeConverter` should be limited as much as possible: ideally,
801> only use it in renderers.
802
803**_Good_**
804
805```c++
806template <>
807struct TypeConverter<IDLDictionary, mojom::blink::DictionaryPtr> {
808 static IDLDictionary* Convert(const mojom::blink::DictionaryPtr& in) {
809 // Note that unlike StructTraits, there is no out-of-band way to signal
810 // failure.
811 IDLDictionary* out = new IDLDictionary;
812 out->int_value = in->int_value;
813 out->str_value = in->str_value;
814 return out;
815 }
816};
817```
818
819**_Bad_**
820
821```c++
822// Using a custom one-off function makes this hard to discover in security
823// audits.
824IDLDictionary* FromMojo(const mojom::blink::DictionaryPtr& in) {
825 IDLDictionary* out = new IDLDictionary;
826 out->int_value = in->int_value;
827 out->str_value = in->str_value;
828 return out;
829}
830```
831
832
833### Use the proper abstractions
834
Oksana Zhuravlova9f3b8ef2019-08-26 20:27:40835`mojo::ReceiverSet` implies multiple clients may connect. If this actually isn't
Daniel Chengd21b551f2017-10-30 08:43:23836the case, please do not use it. For example, if an interface can be rebound,
Oksana Zhuravlova9f3b8ef2019-08-26 20:27:40837then use the singular `mojo::Receiver` and simply `reset()` the existing receiver
Daniel Chengd21b551f2017-10-30 08:43:23838before reusing it.
839
840
Daniel Chengf48fd972017-11-21 19:15:53841### Explicitly reject bad input
842
843While validation should be done inside `StructTraits` specializations when
844possible, there are situations where additional checks, e.g. overflow checks,
845are needed outside of `StructTraits` specializations. Use
846`mojo::ReportBadMessage()` or `mojo::GetBadMessageCallback()` to reject bad
847input in these situations. Under the hood, this may record UMAs, kill the
848process sending bad input, et cetera.
849
850* `mojo::ReportBadMessage()`: use to report bad IPC input while a message is
851 being dispatched on the stack.
852* `mojo::GetBadMessageCallback()`: use to generate a callback to report bad
853 IPC input. The callback must be generated while a message is being
854 dispatched on the stack; however, the returned callback may be invoked be
855 freely invoked in asynchronously posted callbacks.
856
857
Daniel Chengd21b551f2017-10-30 08:43:23858## Java Best Practices
859
860Unfortunately, there are no strongly established conventions here. Most code
861tends to write manual conversion helpers and throw an exception on conversion
862failure. See [NfcTypeConverter.java] as one example of how to write conversion
863code.
864
865
866## General Code Health
867
868
869### Naming Conventions
870
871Place mojo types in `<top-level namespace>.mojom`. Directories for Mojo traits
872should be named `mojom` (preferable) or `ipc`. Legacy names that are also
873encountered are `public/interfaces`, `interfaces`, or just `mojo`.
874
875`mojom` is preferred for consistency between the directory name and the nested
876namespace name.
877
878
879### Do not handle impossible situations
880
881Do not clutter the code by handling impossible situations. Omitting it makes
882the invariants clear. This takes two different forms:
883
884* A less trustworthy process can be compromised by an adversary and send
885 arbitrary data. When processing data from a less trustworthy process, do
886 not attempt to handle this invalid data: just call
887 `mojo::ReportBadMessage()`. When invoked in the context of processing an
888 IPC from the renderer, this will kill the renderer process.
889* A more trustworthy process must be trusted, by definition. Do not write
890 code to handle impossible situations "just in case" there's a bug. For
891 example, the renderer class `content::RenderFrameImpl` must always be
892 connected to certain control interfaces in the browser. It does not makes
893 sense to handle a Mojo connection error and try to reconnect: a connection
894 error signals that the browser process is in the process of deleting the
895 frame, and any attempt at reconnecting will never succeed.
896
897
898### Using mojo enums directly when possible
899
900`EnumTraits` generally do not add much value: incoming Mojo enum values are
901already validated before typemapping, so it is guaranteed that the input value
902to `EnumTraits<T>::FromMojom()` is already a valid enum value, so the method
903itself is just a bunch of boilerplate to map between two very similarly named,
904yet slightly different, enums.
905
906To avoid this, prefer to use the Mojo enum directly when possible.
907
908
909### Consider the cost of setting up message pipes
910
911Message pipes are fairly inexpensive, but they are not free either: it takes 6
912control messages to establish a message pipe. Keep this in mind: if the
913interface is used relatively frequently, connecting once and reusing the
914interface pointer is probably a good idea.
915
Alex Gough303f5822022-01-27 05:35:16916## Copy data out of BigBuffer before parsing
917
918[BigBuffer](mojo/public/mojom/base/big_buffer.mojom) uses shared memory to make
919passing large messages fast. When shmem is backing the message, it may be
920writable in the sending process while being read in the receiving process. If a
921BigBuffer is received from an untrustworthy process, you should make a copy of
922the data before processing it to avoid time-of-check time-of-use (TOCTOU) bugs.
923The |size()| of the data cannot be manipulated.
924
Daniel Chengd21b551f2017-10-30 08:43:23925
Chris Palmer76482e62017-12-04 21:05:06926## Ensure An Explicit Grant For WebUI Bindings
927
928WebUI renderers sometimes need to call special, powerful IPC endpoints in a
929privileged process. It is important to enforce the constraint that the
930privileged callee previously created and blessed the calling process as a WebUI
931process, and not as a (potentially compromised) web renderer or other
932low-privilege process.
933
934* Use the standard pattern for instantiating `MojoWebUIController`. WebUI
Chris Palmerd40cda92017-12-04 21:12:46935Mojo interfaces must only be exposed through a `MojoWebUIController` subclass.
Chris Palmer76482e62017-12-04 21:05:06936* If there is external functionality that the WebUI needs, make sure to route
937it through the Mojo interfaces implemented by the `MojoWebUIController`, to
938avoid circumventing access checks.
939
940
941## Not-Yet-Shipped Features Should Be Feature-Checked On The Privileged Side
942
943Sometimes, there will be powerful new features that are not yet turned on by
944default, such as behind a flag, Finch trial, or [origin
945trial](https://www.chromium.org/blink/origin-trials). It is not safe to check
946for the feature's availability on the renderer side (or in another low-privilege
947process type). Instead, ensure that the check is done in the process that has
948power to actually enact the feature. Otherwise, a compromised renderer could opt
949itself in to the feature! If the feature might not yet be fully developed and
950safe, vulnerabilities could arise.
951
952
Daniel Chengd21b551f2017-10-30 08:43:23953[security-tips-for-ipc]: https://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc
954[NfcTypeConverter.java]: https://chromium.googlesource.com/chromium/src/+/e97442ee6e8c4cf6bcf7f5623c6fb2cc8cce92ac/services/device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java
Oksana Zhuravlovaa77a9a12021-05-07 18:02:10955[mojo-doc-process-crashes]: https://chromium.googlesource.com/chromium/src/+/main/mojo/public/cpp/bindings#Best-practices-for-dealing-with-process-crashes-and-callbacks
Lukasz Anforowiczbb99a8562019-01-25 19:19:38956[serialize-struct-tm-safely]: https://chromium-review.googlesource.com/c/chromium/src/+/679441