Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 1 | # Mojo "Style" Guide |
| 2 | |
| 3 | Mojo is Chrome's new IPC system and provides lots of useful abstractions. These |
| 4 | abstractions can make it easier to write code that makes interprocess calls, |
| 5 | but can also add significant complexity. Below are some recommendation from |
| 6 | Mojo and IPC reviewers for best practices. |
| 7 | |
| 8 | For 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 | |
| 17 | Strive to write simple interfaces. Minimize the amount of cross-process state |
| 18 | that needs to be maintained in sync. |
| 19 | |
| 20 | **_Good_** |
| 21 | |
| 22 | ```c++ |
Daniel Cheng | aa8a267 | 2017-11-02 08:05:01 | [diff] [blame] | 23 | interface TeleporterFactory { |
| 24 | Create(Location start, Location end) => (Teleporter); |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 25 | }; |
Daniel Cheng | aa8a267 | 2017-11-02 08:05:01 | [diff] [blame] | 26 | |
| 27 | interface Teleporter { |
| 28 | TeleportGoat(Goat) = (); |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 29 | }; |
| 30 | ``` |
| 31 | |
| 32 | **_Bad_** |
| 33 | |
| 34 | ```c++ |
Daniel Cheng | aa8a267 | 2017-11-02 08:05:01 | [diff] [blame] | 35 | interface Teleporter { |
| 36 | // Bad: comments will need to explicitly call out that both locations need to |
| 37 | // be bound before calling TeleportGoat()! |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 38 | // |
Daniel Cheng | aa8a267 | 2017-11-02 08:05:01 | [diff] [blame] | 39 | // 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 Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 45 | }; |
| 46 | ``` |
| 47 | |
| 48 | Similarly, strive to make methods focused. Do not overuse optional types. |
| 49 | |
| 50 | **_Good_** |
| 51 | |
| 52 | ```c++ |
Daniel Cheng | aa8a267 | 2017-11-02 08:05:01 | [diff] [blame] | 53 | struct TeleporterStats { |
| 54 | AnimalStats animal_stats; |
| 55 | FungiStats fungi_stats; |
| 56 | GoatStats goat_stats; |
| 57 | PlantStats plant_stats; |
| 58 | }; |
| 59 | |
| 60 | interface 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 Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 68 | }; |
| 69 | ``` |
| 70 | |
| 71 | **_Bad_** |
| 72 | |
| 73 | ```c++ |
Daniel Cheng | aa8a267 | 2017-11-02 08:05:01 | [diff] [blame] | 74 | interface 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 Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 85 | }; |
| 86 | ``` |
| 87 | |
| 88 | |
| 89 | ## Documentation |
| 90 | |
| 91 | Mojo structs, interfaces, and methods should all have comments. Make sure the |
| 92 | comments cover the "how" and the "why" of using an interface and its methods, |
| 93 | and not just the "what". Document preconditions, postconditions, and trust: if |
| 94 | an interface is implemented in the browser process and handles requests from |
| 95 | the renderer process, this should be mentioned in the comments. Complex features |
| 96 | should also have an external `README.md` that covers the high-level flow of |
| 97 | information through interfaces and how they interact to implement the feature. |
| 98 | |
| 99 | **_Good_** |
| 100 | |
| 101 | ```c++ |
Daniel Cheng | aa8a267 | 2017-11-02 08:05:01 | [diff] [blame] | 102 | // Interface for controlling a teleporter. Lives in the browser process, and |
| 103 | // used to implement the Teleportation over Mojo IPC RFC. |
| 104 | interface 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 Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 115 | |
Daniel Cheng | aa8a267 | 2017-11-02 08:05:01 | [diff] [blame] | 116 | // 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 Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 121 | }; |
| 122 | ``` |
| 123 | |
| 124 | |
| 125 | ## Security |
| 126 | |
| 127 | Policy should be controlled solely by the browser process. "Policy" can mean |
| 128 | any number of things, such as sizes, addresses, permissions, URLs, origins, |
| 129 | etc. In an ideal world: |
| 130 | |
| 131 | 1. Unprivileged process asks for a capability from the privileged process that |
| 132 | owns the resource. |
| 133 | 1. Privileged process applies policy to find an implementation for the |
| 134 | capability. |
| 135 | 1. Unprivileged process performs operations on the capability, constrained in |
| 136 | scope. |
| 137 | |
| 138 | The privileged process must own the capability lifecycle. |
| 139 | |
| 140 | |
| 141 | ### Do not trust less privileged processes |
| 142 | |
| 143 | This is the overriding principle for all guidelines in this section. When |
| 144 | receiving data from a less trusted process, treat the data as if it were |
| 145 | generated by a malicious adversary. Message handlers cannot assume that offsets |
| 146 | are valid, calculations won't overflow, et cetera. |
| 147 | |
| 148 | In 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 | |
| 156 | When passing objects up a privilege gradient (from less → more privileged), the |
| 157 | callee must validate the inputs before acting on them. When passing objects |
| 158 | down a privilege gradient, such as from browser → renderer, it is OK for the |
| 159 | callee 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 | |
| 170 | For example, the browser process must not (fully) trust the renderer's claims |
| 171 | about origins. The browser process should already know what origin the renderer |
| 172 | is evaluating, and thus should already have this data (for example, see |
| 173 | `RenderFrameHost::GetLastCommittedOrigin()`). Thus, a method that requires |
| 174 | passing an origin from the renderer to the browser process has a conceptual |
| 175 | error, 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 | |
| 180 | Similarly, the browser process must not trust the renderer's claims about file |
| 181 | pathnames. It would be unsafe for the browser process to save a downloaded file |
| 182 | to `~/.bashrc` just because the renderer asked. Instead, it would be better for |
| 183 | the browser process to: |
| 184 | |
| 185 | 1. Kill the renderer if `basename(pathname) != pathname`, since the renderer is |
| 186 | obviously compromised if it makes this mistake. |
| 187 | 1. Defang the basename, by removing leading dots, et cetera. Note that the |
| 188 | definition of proper defanging varies per platform. |
| 189 | 1. 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 | |
| 197 | Platform-specific functionality should only be defined on the platforms where |
| 198 | it is used. |
| 199 | |
| 200 | > The work to make this possible is in progress: <https://crbug.com/676224> |
| 201 | |
| 202 | For enums, avoid the pattern of defining a `LAST` or `MAX` value. The `LAST` |
| 203 | value is typically used in conjunction with legacy IPC macros to validate enums; |
| 204 | this is not needed with Mojo enums, which automatically validated. |
| 205 | |
| 206 | The `MAX` value is typically used as an invalid sentinel value for UMA |
| 207 | histograms: unfortunately, simply defining a `MAX` value in a Mojo enum will |
| 208 | cause Mojo to treat it as valid. This forces all IPC handling to do manual |
| 209 | checks that the semantically invalid `MAX` value isn't accidentally or |
| 210 | maliciously passed around. |
| 211 | |
| 212 | > Improving UMA logging is tracked in <https://crbug.com/742517>. |
| 213 | |
| 214 | |
| 215 | ### Use structured types |
| 216 | |
| 217 | Where possible, use structured types: this allows the type system to help |
| 218 | enforce 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++ |
| 237 | interface 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++ |
| 247 | interface ReportingService { |
| 248 | // Bad: unclear what units |time| is or what |data| contains. |
| 249 | ReportDeprecation(double time, mojo.common.mojom.Value data); |
| 250 | }; |
| 251 | ``` |
| 252 | |
| 253 | Another anti-pattern to avoid is parallel arrays of data: this requires the |
| 254 | receiver to validate that all the arrays have the same length. Instead, prefer |
| 255 | to pass the data so that it is impossible to have a mismatch. |
| 256 | |
| 257 | **_Good_** |
| 258 | |
| 259 | ```c++ |
| 260 | struct Pixel { |
| 261 | int8 reds; |
| 262 | int8 greens; |
| 263 | int8 blues; |
| 264 | int8 alphas; |
| 265 | }; |
| 266 | |
| 267 | struct 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. |
| 278 | struct 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 | |
| 291 | Signed overflow is undefined in C++. If unsure about whether or not something |
| 292 | will overflow, use the safe numeric helpers from `//base/numerics`! |
| 293 | |
| 294 | **_Good_** |
| 295 | |
| 296 | ```c++ |
| 297 | base::CheckedNumeric<int32_t> size = mojo_rect->width(); |
| 298 | size *= mojo_rect.height(); |
| 299 | if (!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++! |
| 308 | int32_t size = mojo_rect->width() * mojo_rect.height(); |
| 309 | ``` |
| 310 | |
| 311 | Note that even if the types have defined overflow semantics, it is almost always |
| 312 | a good idea to check for overflow. |
| 313 | |
| 314 | **_Good_** |
| 315 | |
| 316 | ```c++ |
| 317 | uint32_t alloc_size; |
| 318 | if (!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 | |
| 325 | Element* array = CreateArray(alloc_size); |
| 326 | for (size_t i = 0; i < request->element_size(); ++i) { |
| 327 | array[i] = PopulateArray(i); |
| 328 | } |
| 329 | ``` |
| 330 | |
| 331 | **_Bad_** |
| 332 | |
| 333 | ```c++ |
| 334 | uint32_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! |
| 338 | Element* array = CreateArray(alloc_size); |
| 339 | for (size_t i = 0; i < request->element_size(); ++i) { |
| 340 | array[i] = PopulateArray(i); |
| 341 | } |
| 342 | ``` |
| 343 | |
| 344 | |
| 345 | ## C++ Best Practices |
| 346 | |
| 347 | |
Daniel Cheng | 9ae28e0 | 2018-01-17 18:44:10 | [diff] [blame^] | 348 | ### Use mojo::WrapCallbackWithDefaultInvokeIfNotRun And mojo::WrapCallbackWithDropHandler Sparingly |
| 349 | |
| 350 | Mojo provides several convenience helpers to automatically invoke a callback if |
| 351 | the callback has not already been invoked in some other way when the callback is |
| 352 | destroyed, e.g.: |
| 353 | |
| 354 | ```c++ |
| 355 | { |
| 356 | base::Callback<int> cb = mojo::WrapCallbackWithDefaultInvokeIfNotRun( |
| 357 | base::Bind([](int) { ... }), -1); |
| 358 | } // |cb| is automatically invoked with an argument of -1. |
| 359 | ``` |
| 360 | |
| 361 | Unfortunately, the fact that this callback is guaranteed to always run is hidden |
| 362 | from the type system and can propagate in surprising ways. Avoid using this |
| 363 | construction unless there are no better alternatives. Uses of these helpers must |
| 364 | be well-commented to describe why this behavior is required. |
| 365 | |
| 366 | Note that using this from the renderer is often unnecessary. Message pipes are |
| 367 | often closed as part of a Document shutting down; since many Blink objects |
| 368 | already inherit `blink::ContextLifecycleObserver`, it is usually more idiomatic |
| 369 | to use this signal to perform any needed cleanup work. |
| 370 | |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 371 | ### Use StructTraits |
| 372 | |
| 373 | Creating a typemap and defining a `StructTraits` specialization moves the |
| 374 | complexity of serialization, deserialization, and validation into a central |
| 375 | location. We universally recommend this over defining `TypeConverter` |
| 376 | specializations: when a value fails deserialization, the receiver method will |
| 377 | never even be invoked. As a bonus, it also reduces the number of copies during |
| 378 | serialization and deserialization. 😄 |
| 379 | |
| 380 | **_Good_** |
| 381 | |
| 382 | ```c++ |
| 383 | // In url_gurl_struct_traits.h: |
| 384 | template <> |
| 385 | struct StructTraits<url::mojom::UrlDataView, GURL> { |
| 386 | static base::StringPiece url(const GURL& r); |
| 387 | |
| 388 | // If Read() returns false, Mojo will discard the message. |
| 389 | static bool Read(url::mojom::UrlDataView data, GURL* out); |
| 390 | }; |
| 391 | |
| 392 | // In url_gurl_struct_traits.cc: |
| 393 | // Note that methods that aren't simple getters should be defined |
| 394 | // out-of-line to avoid code bloat. |
| 395 | base::StringPiece StructTraits<url::mojom::UrlDataView, GURL>::url( |
| 396 | const GURL& r) { |
| 397 | if (r.possibly_invalid_spec().length() > url::kMaxURLChars || |
| 398 | !r.is_valid()) { |
| 399 | return base::StringPiece(); |
| 400 | } |
| 401 | return base::StringPiece(r.possibly_invalid_spec().c_str(), |
| 402 | r.possibly_invalid_spec().length()); |
| 403 | } |
| 404 | |
| 405 | bool StructTraits<url::mojom::UrlDataView, GURL>::Read( |
| 406 | url::mojom::UrlDataView data, GURL* out) { |
| 407 | base::StringPiece url_string; |
| 408 | if (!data.ReadUrl(&url_string)) |
| 409 | return false; |
| 410 | if (url_string.length() > url::kMaxURLChars) |
| 411 | return false; |
| 412 | *out = GURL(url_string); |
| 413 | return !url_string.empty() && out->is_valid(); |
| 414 | } |
| 415 | ``` |
| 416 | |
| 417 | **_Bad_** |
| 418 | |
| 419 | ```c++ |
| 420 | template <> |
| 421 | struct TypeConverter<url::mojom::UrlPtr, GURL> { |
| 422 | // Inefficient: this copies data once off the wire to create a |
| 423 | // url.mojom.Url object, then copies it again to create a GURL. |
| 424 | static GURL Convert(const url::mojom::UrlPtr url) { |
| 425 | if (url.url.is_empty()) return GURL(); |
| 426 | // Not good: no way to signal errors, so any code that converts the |
| 427 | // Mojo struct to a GURL will somehow need to check for errors… |
| 428 | // but it can't even be distinguished from the empty URL case! |
| 429 | if (url.url.size() > url::kMaxURLChars) return GURL(); |
| 430 | return GURL(url.url); |
| 431 | } |
| 432 | }; |
| 433 | ``` |
| 434 | |
| 435 | There are also corresponding `EnumTraits` and `UnionTraits` specializations for |
| 436 | mojo enums and unions respectively. |
| 437 | |
| 438 | |
| 439 | ### StructTraits getters should be simple |
| 440 | |
| 441 | Where possible, `StructTraits` should be returning const references or simple |
| 442 | read-only views of the data. Having to create temporary data structures during |
| 443 | serialization should be rare, and it should be even rarer to mutate the input |
| 444 | argument. |
| 445 | |
| 446 | |
| 447 | ### Out-of-line complex serialization/deserialization logic |
| 448 | |
| 449 | A `StructTraits` specialization is almost always fully specialized. Only define |
| 450 | `StructTraits` methods inline in the header if the method is a simple getter |
| 451 | that returns a reference, pointer, or other simple POD. Define all other |
| 452 | methods out-of-line to avoid code bloat. |
| 453 | |
| 454 | |
| 455 | ### Do not write one-off functions to convert to/from Mojo types |
| 456 | |
| 457 | There are some instances where it is simply not possible to define a |
| 458 | `StructTraits` for type mapping: this commonly occurs with Blink IDL and Oilpan |
| 459 | types. In these instances, add a `TypeConverter` specialization rather than |
| 460 | defining a one-off conversion function. This makes it easier to search for and |
| 461 | audit code that does potentially risky type conversions. |
| 462 | |
| 463 | > The use of `TypeConverter` should be limited as much as possible: ideally, |
| 464 | > only use it in renderers. |
| 465 | |
| 466 | **_Good_** |
| 467 | |
| 468 | ```c++ |
| 469 | template <> |
| 470 | struct TypeConverter<IDLDictionary, mojom::blink::DictionaryPtr> { |
| 471 | static IDLDictionary* Convert(const mojom::blink::DictionaryPtr& in) { |
| 472 | // Note that unlike StructTraits, there is no out-of-band way to signal |
| 473 | // failure. |
| 474 | IDLDictionary* out = new IDLDictionary; |
| 475 | out->int_value = in->int_value; |
| 476 | out->str_value = in->str_value; |
| 477 | return out; |
| 478 | } |
| 479 | }; |
| 480 | ``` |
| 481 | |
| 482 | **_Bad_** |
| 483 | |
| 484 | ```c++ |
| 485 | // Using a custom one-off function makes this hard to discover in security |
| 486 | // audits. |
| 487 | IDLDictionary* FromMojo(const mojom::blink::DictionaryPtr& in) { |
| 488 | IDLDictionary* out = new IDLDictionary; |
| 489 | out->int_value = in->int_value; |
| 490 | out->str_value = in->str_value; |
| 491 | return out; |
| 492 | } |
| 493 | ``` |
| 494 | |
| 495 | |
| 496 | ### Use the proper abstractions |
| 497 | |
| 498 | `mojo::BindingSet` implies multiple clients may connect. If this actually isn't |
| 499 | the case, please do not use it. For example, if an interface can be rebound, |
| 500 | then use the singular `mojo::Binding` and simply `Close()` the existing binding |
| 501 | before reusing it. |
| 502 | |
| 503 | |
Daniel Cheng | f48fd97 | 2017-11-21 19:15:53 | [diff] [blame] | 504 | ### Explicitly reject bad input |
| 505 | |
| 506 | While validation should be done inside `StructTraits` specializations when |
| 507 | possible, there are situations where additional checks, e.g. overflow checks, |
| 508 | are needed outside of `StructTraits` specializations. Use |
| 509 | `mojo::ReportBadMessage()` or `mojo::GetBadMessageCallback()` to reject bad |
| 510 | input in these situations. Under the hood, this may record UMAs, kill the |
| 511 | process sending bad input, et cetera. |
| 512 | |
| 513 | * `mojo::ReportBadMessage()`: use to report bad IPC input while a message is |
| 514 | being dispatched on the stack. |
| 515 | * `mojo::GetBadMessageCallback()`: use to generate a callback to report bad |
| 516 | IPC input. The callback must be generated while a message is being |
| 517 | dispatched on the stack; however, the returned callback may be invoked be |
| 518 | freely invoked in asynchronously posted callbacks. |
| 519 | |
| 520 | |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 521 | ## Java Best Practices |
| 522 | |
| 523 | Unfortunately, there are no strongly established conventions here. Most code |
| 524 | tends to write manual conversion helpers and throw an exception on conversion |
| 525 | failure. See [NfcTypeConverter.java] as one example of how to write conversion |
| 526 | code. |
| 527 | |
| 528 | |
| 529 | ## General Code Health |
| 530 | |
| 531 | |
| 532 | ### Naming Conventions |
| 533 | |
| 534 | Place mojo types in `<top-level namespace>.mojom`. Directories for Mojo traits |
| 535 | should be named `mojom` (preferable) or `ipc`. Legacy names that are also |
| 536 | encountered are `public/interfaces`, `interfaces`, or just `mojo`. |
| 537 | |
| 538 | `mojom` is preferred for consistency between the directory name and the nested |
| 539 | namespace name. |
| 540 | |
| 541 | |
| 542 | ### Do not handle impossible situations |
| 543 | |
| 544 | Do not clutter the code by handling impossible situations. Omitting it makes |
| 545 | the invariants clear. This takes two different forms: |
| 546 | |
| 547 | * A less trustworthy process can be compromised by an adversary and send |
| 548 | arbitrary data. When processing data from a less trustworthy process, do |
| 549 | not attempt to handle this invalid data: just call |
| 550 | `mojo::ReportBadMessage()`. When invoked in the context of processing an |
| 551 | IPC from the renderer, this will kill the renderer process. |
| 552 | * A more trustworthy process must be trusted, by definition. Do not write |
| 553 | code to handle impossible situations "just in case" there's a bug. For |
| 554 | example, the renderer class `content::RenderFrameImpl` must always be |
| 555 | connected to certain control interfaces in the browser. It does not makes |
| 556 | sense to handle a Mojo connection error and try to reconnect: a connection |
| 557 | error signals that the browser process is in the process of deleting the |
| 558 | frame, and any attempt at reconnecting will never succeed. |
| 559 | |
| 560 | |
| 561 | ### Using mojo enums directly when possible |
| 562 | |
| 563 | `EnumTraits` generally do not add much value: incoming Mojo enum values are |
| 564 | already validated before typemapping, so it is guaranteed that the input value |
| 565 | to `EnumTraits<T>::FromMojom()` is already a valid enum value, so the method |
| 566 | itself is just a bunch of boilerplate to map between two very similarly named, |
| 567 | yet slightly different, enums. |
| 568 | |
| 569 | To avoid this, prefer to use the Mojo enum directly when possible. |
| 570 | |
| 571 | |
| 572 | ### Consider the cost of setting up message pipes |
| 573 | |
| 574 | Message pipes are fairly inexpensive, but they are not free either: it takes 6 |
| 575 | control messages to establish a message pipe. Keep this in mind: if the |
| 576 | interface is used relatively frequently, connecting once and reusing the |
| 577 | interface pointer is probably a good idea. |
| 578 | |
| 579 | |
Chris Palmer | 76482e6 | 2017-12-04 21:05:06 | [diff] [blame] | 580 | ## Ensure An Explicit Grant For WebUI Bindings |
| 581 | |
| 582 | WebUI renderers sometimes need to call special, powerful IPC endpoints in a |
| 583 | privileged process. It is important to enforce the constraint that the |
| 584 | privileged callee previously created and blessed the calling process as a WebUI |
| 585 | process, and not as a (potentially compromised) web renderer or other |
| 586 | low-privilege process. |
| 587 | |
| 588 | * Use the standard pattern for instantiating `MojoWebUIController`. WebUI |
Chris Palmer | d40cda9 | 2017-12-04 21:12:46 | [diff] [blame] | 589 | Mojo interfaces must only be exposed through a `MojoWebUIController` subclass. |
Chris Palmer | 76482e6 | 2017-12-04 21:05:06 | [diff] [blame] | 590 | * If there is external functionality that the WebUI needs, make sure to route |
| 591 | it through the Mojo interfaces implemented by the `MojoWebUIController`, to |
| 592 | avoid circumventing access checks. |
| 593 | |
| 594 | |
| 595 | ## Not-Yet-Shipped Features Should Be Feature-Checked On The Privileged Side |
| 596 | |
| 597 | Sometimes, there will be powerful new features that are not yet turned on by |
| 598 | default, such as behind a flag, Finch trial, or [origin |
| 599 | trial](https://www.chromium.org/blink/origin-trials). It is not safe to check |
| 600 | for the feature's availability on the renderer side (or in another low-privilege |
| 601 | process type). Instead, ensure that the check is done in the process that has |
| 602 | power to actually enact the feature. Otherwise, a compromised renderer could opt |
| 603 | itself in to the feature! If the feature might not yet be fully developed and |
| 604 | safe, vulnerabilities could arise. |
| 605 | |
| 606 | |
Daniel Cheng | d21b551f | 2017-10-30 08:43:23 | [diff] [blame] | 607 | [security-tips-for-ipc]: https://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc |
| 608 | [NfcTypeConverter.java]: https://chromium.googlesource.com/chromium/src/+/e97442ee6e8c4cf6bcf7f5623c6fb2cc8cce92ac/services/device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java |