blob: e4cde6ef6efa43c66dcca0d77f88dc06ac5c2999 [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
8For questions, concerns, or suggestions, reach out to <[email protected]>.
9
10> For legacy IPC, please see [security tips for IPC][security-tips-for-ipc].
11
12[TOC]
13
14
15## Simplicity
16
17Strive to write simple interfaces. Minimize the amount of cross-process state
18that needs to be maintained in sync.
19
20**_Good_**
21
22```c++
Daniel Chengaa8a2672017-11-02 08:05:0123interface TeleporterFactory {
24 Create(Location start, Location end) => (Teleporter);
Daniel Chengd21b551f2017-10-30 08:43:2325};
Daniel Chengaa8a2672017-11-02 08:05:0126
27interface Teleporter {
28 TeleportGoat(Goat) = ();
Daniel Chengd21b551f2017-10-30 08:43:2329};
30```
31
32**_Bad_**
33
34```c++
Daniel Chengaa8a2672017-11-02 08:05:0135interface Teleporter {
36 // Bad: comments will need to explicitly call out that both locations need to
37 // be bound before calling TeleportGoat()!
Daniel Chengd21b551f2017-10-30 08:43:2338 //
Daniel Chengaa8a2672017-11-02 08:05:0139 // In addition, if untrustworthy processes can talk to trustworthy processes,
40 // the Teleporter implementation will need to also handle the case where the
41 // Location objects are not yet bound.
42 SetStart(Location);
43 SetEnd(Location);
44 TeleportGoat(Goat) = ();
Daniel Chengd21b551f2017-10-30 08:43:2345};
46```
47
48Similarly, strive to make methods focused. Do not overuse optional types.
49
50**_Good_**
51
52```c++
Daniel Chengaa8a2672017-11-02 08:05:0153struct TeleporterStats {
54 AnimalStats animal_stats;
55 FungiStats fungi_stats;
56 GoatStats goat_stats;
57 PlantStats plant_stats;
58};
59
60interface Teleporter {
61 TeleportAnimal(Animal) => ();
62 TeleportFungi(Fungi) => ();
63 TeleportGoat(Goat) = ();
64 TeleportPlant(Plant) => ();
65
66 // TeleportStats is only non-null if success is true.
67 GetStats() => (bool success, TeleporterStats?);
Daniel Chengd21b551f2017-10-30 08:43:2368};
69```
70
71**_Bad_**
72
73```c++
Daniel Chengaa8a2672017-11-02 08:05:0174interface Teleporter {
75 // The intent of four optional arguments is unclear: can this call teleport
76 // multiple objects of different types at once, or is the caller only
77 // supposed to only pass one non-null argument per call?
78 Teleport(Animal?, Fungi?, Goat?, Plant?) => ();
79
80 // Does this return all stats if sucess is true? Or just the categories that
81 // the teleporter already has stats for? The intent is uncertain, so wrapping
82 // the disparate values into a result struct would be cleaner.
83 GetStats() =>
84 (bool success, AnimalStats?, FungiStats?, PlantStats?, FungiStats?);
Daniel Chengd21b551f2017-10-30 08:43:2385};
86```
87
88
89## Documentation
90
91Mojo structs, interfaces, and methods should all have comments. Make sure the
92comments cover the "how" and the "why" of using an interface and its methods,
93and not just the "what". Document preconditions, postconditions, and trust: if
94an interface is implemented in the browser process and handles requests from
95the renderer process, this should be mentioned in the comments. Complex features
96should also have an external `README.md` that covers the high-level flow of
97information through interfaces and how they interact to implement the feature.
98
99**_Good_**
100
101```c++
Daniel Chengaa8a2672017-11-02 08:05:01102// Interface for controlling a teleporter. Lives in the browser process, and
103// used to implement the Teleportation over Mojo IPC RFC.
104interface Teleporter {
105 // Teleportation helpers for different taxonomic kingdoms. Teleportation is
106 // not complete until the reply callback is invoked. The caller must NOT
107 // release the sender side resources until the reply callback runs; releasing
108 // the resources early will cause splinching.
109 TeleportAnimal(Animal) => ();
110 TeleportFungi(Fungi) => ();
111 // Goats require a specialized teleportation channel distinct from
112 // TeleportAnimal to ensure goatiness isolation.
113 TeleportGoat(Goat) => ();
114 TeleportPlant(Plant) => ();
Daniel Chengd21b551f2017-10-30 08:43:23115
Daniel Chengaa8a2672017-11-02 08:05:01116 // Returns current teleportation stats. On failure (e.g. a teleportation
117 // operation is currently in progress) success will be false and a null stats
118 // object will be returned.
119 GetStats() =>
120 (bool success, TeleportationStats?);
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
166> Note: there is currently work in progress to associate origins with the
167> `InterfaceProvider`s for frames and workers: <https://crbug.com/734210> and
168> <https://crbug.com/775792/>.
169
170For example, the browser process must not (fully) trust the renderer's claims
171about origins. The browser process should already know what origin the renderer
172is evaluating, and thus should already have this data (for example, see
173`RenderFrameHost::GetLastCommittedOrigin()`). Thus, a method that requires
174passing an origin from the renderer to the browser process has a conceptual
175error, and quite possibly, a vulnerability.
176
177> Note: there are currently subtle races when using `GetLastCommittedOrigin()`
178> that will be resolved by fixing <https://crbug.com/729021>.
179
180Similarly, the browser process must not trust the renderer's claims about file
181pathnames. It would be unsafe for the browser process to save a downloaded file
182to `~/.bashrc` just because the renderer asked. Instead, it would be better for
183the browser process to:
184
1851. Kill the renderer if `basename(pathname) != pathname`, since the renderer is
186 obviously compromised if it makes this mistake.
1871. Defang the basename, by removing leading dots, et cetera. Note that the
188 definition of proper defanging varies per platform.
1891. Prepend its own parent directory to the basename, e.g. ~/Downloads.
190
191> TODO(https://crbug.com/779196): Even better would be to implement a C++ type
192> performs the appropriate sanitizations and recommend its usage directly here.
193
194
195### Do not define things that are not used
196
197Platform-specific functionality should only be defined on the platforms where
198it is used.
199
200> The work to make this possible is in progress: <https://crbug.com/676224>
201
202For enums, avoid the pattern of defining a `LAST` or `MAX` value. The `LAST`
203value is typically used in conjunction with legacy IPC macros to validate enums;
204this is not needed with Mojo enums, which automatically validated.
205
206The `MAX` value is typically used as an invalid sentinel value for UMA
207histograms: unfortunately, simply defining a `MAX` value in a Mojo enum will
208cause Mojo to treat it as valid. This forces all IPC handling to do manual
209checks that the semantically invalid `MAX` value isn't accidentally or
210maliciously passed around.
211
212> Improving UMA logging is tracked in <https://crbug.com/742517>.
213
214
215### Use structured types
216
217Where possible, use structured types: this allows the type system to help
218enforce that the input data is valid. Common ones to watch out for:
219
220* Files: use `mojo.common.mojom.File`, not raw descriptor types like `HANDLE`
221 and `int`.
222* File paths: use `mojo.common.mojom.FilePath`, not `string`.
223* JSON: use `mojo.common.mojom.Value`, not `string`.
224* Mojo interfaces: use `Interface` or `Interface&`, not `handle` or
225 `handle<message_pipe>`.
226* Nonces: use `mojo.common.mojom.UnguessableToken`, not `string`.
227* Origins: use `url.mojom.Origin`, not `url.mojom.Url` and certainly not
228 `string`.
229* Time types: use `mojo.common.mojom.TimeDelta` /
230 `mojo.common.mojom.TimeTicks` / `mojo.common.mojom.Time`, not `int64` /
231 `uint64` / `double` / et cetera.
232* URLs: use `url.mojom.Url`, not `string`.
233
234**_Good_**
235
236```c++
237interface ReportingService {
238 ReportDeprecation(mojo.common.mojom.TimeTicks time,
239 url.mojom.Url resource,
240 uint32 line_number);
241};
242```
243
244**_Bad_**
245
246```c++
247interface ReportingService {
248 // Bad: unclear what units |time| is or what |data| contains.
249 ReportDeprecation(double time, mojo.common.mojom.Value data);
250};
251```
252
253Another anti-pattern to avoid is parallel arrays of data: this requires the
254receiver to validate that all the arrays have the same length. Instead, prefer
255to pass the data so that it is impossible to have a mismatch.
256
257**_Good_**
258
259```c++
260struct Pixel {
261 int8 reds;
262 int8 greens;
263 int8 blues;
264 int8 alphas;
265};
266
267struct Bitmap {
268 // Good: it is impossible for there to be mismatched data.
269 array<Pixel> pixels;
270};
271```
272
273**_Bad_**
274
275```c++
276// Bad: code using this struct will need to validate that all the arrays have
277// matching sizes.
278struct Bitmap {
279 array<int8> reds;
280 array<int8> greens;
281 array<int8> blues;
282 array<int8> alphas;
283};
284```
285
286
287### Beware of arithmetic overflow
288
289> TODO(dcheng): Import the guidance from the legacy IPC doc.
290
291Signed overflow is undefined in C++. If unsure about whether or not something
292will overflow, use the safe numeric helpers from `//base/numerics`!
293
294**_Good_**
295
296```c++
297base::CheckedNumeric<int32_t> size = mojo_rect->width();
298size *= mojo_rect.height();
299if (!size.IsValid()) {
300 mojo::ReportBadMessage("Bad size from renderer!");
301}
302```
303
304**_Bad_**
305
306```c++
307// Bad: Signed overflow is undefined in C++!
308int32_t size = mojo_rect->width() * mojo_rect.height();
309```
310
311Note that even if the types have defined overflow semantics, it is almost always
312a good idea to check for overflow.
313
314**_Good_**
315
316```c++
317uint32_t alloc_size;
318if (!CheckMul(request->elements(), request->element_size())
319 .AssignIfValid(&alloc_size)) {
320 // Safe: avoids allocating with a bogus size that overflowed to a smaller than
321 // expected value.
322 mojo::ReportBadMessge("Invalid allocation size");
323}
324
325Element* array = CreateArray(alloc_size);
326for (size_t i = 0; i < request->element_size(); ++i) {
327 array[i] = PopulateArray(i);
328}
329```
330
331**_Bad_**
332
333```c++
334uint32_t alloc_size = request->elements() * request->element_size();
335
336// Dangerous: alloc_size can overflow so that CreateArray allocates too little
337// memory. Subsequent assignments will turn into an out-of-bound write!
338Element* array = CreateArray(alloc_size);
339for (size_t i = 0; i < request->element_size(); ++i) {
340 array[i] = PopulateArray(i);
341}
342```
343
344
345## C++ Best Practices
346
347
348### Use StructTraits
349
350Creating a typemap and defining a `StructTraits` specialization moves the
351complexity of serialization, deserialization, and validation into a central
352location. We universally recommend this over defining `TypeConverter`
353specializations: when a value fails deserialization, the receiver method will
354never even be invoked. As a bonus, it also reduces the number of copies during
355serialization and deserialization. 😄
356
357**_Good_**
358
359```c++
360// In url_gurl_struct_traits.h:
361template <>
362struct StructTraits<url::mojom::UrlDataView, GURL> {
363 static base::StringPiece url(const GURL& r);
364
365 // If Read() returns false, Mojo will discard the message.
366 static bool Read(url::mojom::UrlDataView data, GURL* out);
367};
368
369// In url_gurl_struct_traits.cc:
370// Note that methods that aren't simple getters should be defined
371// out-of-line to avoid code bloat.
372base::StringPiece StructTraits<url::mojom::UrlDataView, GURL>::url(
373 const GURL& r) {
374 if (r.possibly_invalid_spec().length() > url::kMaxURLChars ||
375 !r.is_valid()) {
376 return base::StringPiece();
377 }
378 return base::StringPiece(r.possibly_invalid_spec().c_str(),
379 r.possibly_invalid_spec().length());
380}
381
382bool StructTraits<url::mojom::UrlDataView, GURL>::Read(
383 url::mojom::UrlDataView data, GURL* out) {
384 base::StringPiece url_string;
385 if (!data.ReadUrl(&url_string))
386 return false;
387 if (url_string.length() > url::kMaxURLChars)
388 return false;
389 *out = GURL(url_string);
390 return !url_string.empty() && out->is_valid();
391}
392```
393
394**_Bad_**
395
396```c++
397template <>
398struct TypeConverter<url::mojom::UrlPtr, GURL> {
399 // Inefficient: this copies data once off the wire to create a
400 // url.mojom.Url object, then copies it again to create a GURL.
401 static GURL Convert(const url::mojom::UrlPtr url) {
402 if (url.url.is_empty()) return GURL();
403 // Not good: no way to signal errors, so any code that converts the
404 // Mojo struct to a GURL will somehow need to check for errors…
405 // but it can't even be distinguished from the empty URL case!
406 if (url.url.size() > url::kMaxURLChars) return GURL();
407 return GURL(url.url);
408 }
409};
410```
411
412There are also corresponding `EnumTraits` and `UnionTraits` specializations for
413mojo enums and unions respectively.
414
415
416### StructTraits getters should be simple
417
418Where possible, `StructTraits` should be returning const references or simple
419read-only views of the data. Having to create temporary data structures during
420serialization should be rare, and it should be even rarer to mutate the input
421argument.
422
423
424### Out-of-line complex serialization/deserialization logic
425
426A `StructTraits` specialization is almost always fully specialized. Only define
427`StructTraits` methods inline in the header if the method is a simple getter
428that returns a reference, pointer, or other simple POD. Define all other
429methods out-of-line to avoid code bloat.
430
431
432### Do not write one-off functions to convert to/from Mojo types
433
434There are some instances where it is simply not possible to define a
435`StructTraits` for type mapping: this commonly occurs with Blink IDL and Oilpan
436types. In these instances, add a `TypeConverter` specialization rather than
437defining a one-off conversion function. This makes it easier to search for and
438audit code that does potentially risky type conversions.
439
440> The use of `TypeConverter` should be limited as much as possible: ideally,
441> only use it in renderers.
442
443**_Good_**
444
445```c++
446template <>
447struct TypeConverter<IDLDictionary, mojom::blink::DictionaryPtr> {
448 static IDLDictionary* Convert(const mojom::blink::DictionaryPtr& in) {
449 // Note that unlike StructTraits, there is no out-of-band way to signal
450 // failure.
451 IDLDictionary* out = new IDLDictionary;
452 out->int_value = in->int_value;
453 out->str_value = in->str_value;
454 return out;
455 }
456};
457```
458
459**_Bad_**
460
461```c++
462// Using a custom one-off function makes this hard to discover in security
463// audits.
464IDLDictionary* FromMojo(const mojom::blink::DictionaryPtr& in) {
465 IDLDictionary* out = new IDLDictionary;
466 out->int_value = in->int_value;
467 out->str_value = in->str_value;
468 return out;
469}
470```
471
472
473### Use the proper abstractions
474
475`mojo::BindingSet` implies multiple clients may connect. If this actually isn't
476the case, please do not use it. For example, if an interface can be rebound,
477then use the singular `mojo::Binding` and simply `Close()` the existing binding
478before reusing it.
479
480
Daniel Chengf48fd972017-11-21 19:15:53481### Explicitly reject bad input
482
483While validation should be done inside `StructTraits` specializations when
484possible, there are situations where additional checks, e.g. overflow checks,
485are needed outside of `StructTraits` specializations. Use
486`mojo::ReportBadMessage()` or `mojo::GetBadMessageCallback()` to reject bad
487input in these situations. Under the hood, this may record UMAs, kill the
488process sending bad input, et cetera.
489
490* `mojo::ReportBadMessage()`: use to report bad IPC input while a message is
491 being dispatched on the stack.
492* `mojo::GetBadMessageCallback()`: use to generate a callback to report bad
493 IPC input. The callback must be generated while a message is being
494 dispatched on the stack; however, the returned callback may be invoked be
495 freely invoked in asynchronously posted callbacks.
496
497
Daniel Chengd21b551f2017-10-30 08:43:23498## Java Best Practices
499
500Unfortunately, there are no strongly established conventions here. Most code
501tends to write manual conversion helpers and throw an exception on conversion
502failure. See [NfcTypeConverter.java] as one example of how to write conversion
503code.
504
505
506## General Code Health
507
508
509### Naming Conventions
510
511Place mojo types in `<top-level namespace>.mojom`. Directories for Mojo traits
512should be named `mojom` (preferable) or `ipc`. Legacy names that are also
513encountered are `public/interfaces`, `interfaces`, or just `mojo`.
514
515`mojom` is preferred for consistency between the directory name and the nested
516namespace name.
517
518
519### Do not handle impossible situations
520
521Do not clutter the code by handling impossible situations. Omitting it makes
522the invariants clear. This takes two different forms:
523
524* A less trustworthy process can be compromised by an adversary and send
525 arbitrary data. When processing data from a less trustworthy process, do
526 not attempt to handle this invalid data: just call
527 `mojo::ReportBadMessage()`. When invoked in the context of processing an
528 IPC from the renderer, this will kill the renderer process.
529* A more trustworthy process must be trusted, by definition. Do not write
530 code to handle impossible situations "just in case" there's a bug. For
531 example, the renderer class `content::RenderFrameImpl` must always be
532 connected to certain control interfaces in the browser. It does not makes
533 sense to handle a Mojo connection error and try to reconnect: a connection
534 error signals that the browser process is in the process of deleting the
535 frame, and any attempt at reconnecting will never succeed.
536
537
538### Using mojo enums directly when possible
539
540`EnumTraits` generally do not add much value: incoming Mojo enum values are
541already validated before typemapping, so it is guaranteed that the input value
542to `EnumTraits<T>::FromMojom()` is already a valid enum value, so the method
543itself is just a bunch of boilerplate to map between two very similarly named,
544yet slightly different, enums.
545
546To avoid this, prefer to use the Mojo enum directly when possible.
547
548
549### Consider the cost of setting up message pipes
550
551Message pipes are fairly inexpensive, but they are not free either: it takes 6
552control messages to establish a message pipe. Keep this in mind: if the
553interface is used relatively frequently, connecting once and reusing the
554interface pointer is probably a good idea.
555
556
Chris Palmer76482e62017-12-04 21:05:06557## Ensure An Explicit Grant For WebUI Bindings
558
559WebUI renderers sometimes need to call special, powerful IPC endpoints in a
560privileged process. It is important to enforce the constraint that the
561privileged callee previously created and blessed the calling process as a WebUI
562process, and not as a (potentially compromised) web renderer or other
563low-privilege process.
564
565* Use the standard pattern for instantiating `MojoWebUIController`. WebUI
Chris Palmerd40cda92017-12-04 21:12:46566Mojo interfaces must only be exposed through a `MojoWebUIController` subclass.
Chris Palmer76482e62017-12-04 21:05:06567* If there is external functionality that the WebUI needs, make sure to route
568it through the Mojo interfaces implemented by the `MojoWebUIController`, to
569avoid circumventing access checks.
570
571
572## Not-Yet-Shipped Features Should Be Feature-Checked On The Privileged Side
573
574Sometimes, there will be powerful new features that are not yet turned on by
575default, such as behind a flag, Finch trial, or [origin
576trial](https://www.chromium.org/blink/origin-trials). It is not safe to check
577for the feature's availability on the renderer side (or in another low-privilege
578process type). Instead, ensure that the check is done in the process that has
579power to actually enact the feature. Otherwise, a compromised renderer could opt
580itself in to the feature! If the feature might not yet be fully developed and
581safe, vulnerabilities could arise.
582
583
Daniel Chengd21b551f2017-10-30 08:43:23584[security-tips-for-ipc]: https://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc
585[NfcTypeConverter.java]: https://chromium.googlesource.com/chromium/src/+/e97442ee6e8c4cf6bcf7f5623c6fb2cc8cce92ac/services/device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java