Skip to content

Commit

Permalink
Ban memcpy() in Mojo serialization/deserialization.
Browse files Browse the repository at this point in the history
Change-Id: I93f4db4f03bc210c1804ecc97dde711249eb1d2f
Reviewed-on: https://chromium-review.googlesource.com/858301
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Chris Palmer <palmer@chromium.org>
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587027}
  • Loading branch information
zetafunction authored and Commit Bot committed Aug 29, 2018
1 parent 68a13dc commit 876c663
Showing 1 changed file with 9 additions and 3 deletions.
12 changes: 9 additions & 3 deletions docs/security/mojo.md
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,11 @@ enforce that the input data is valid. Common ones to watch out for:
`mojo_base.mojom.TimeTicks` / `mojo_base.mojom.Time`, not `int64` /
`uint64` / `double` / et cetera.
* URLs: use `url.mojom.Url`, not `string`.
* `array<uint8>` or `string` and `memcpy()`: use a Mojo struct and statically
define the serialized fields. While `memcpy()` may be tempting for its
simplicity, it can leak info in padding. Even worse, `memcpy()` can easily
copy [undocumented fields][serialize-struct-tm-safely] or newly introduced
fields that were never evaluated for safety by the developer or reviewer.

**_Good_**

Expand All @@ -402,9 +407,9 @@ interface ReportingService {
};
```

Another anti-pattern to avoid is parallel arrays of data: this requires the
receiver to validate that all the arrays have the same length. Instead, prefer
to pass the data so that it is impossible to have a mismatch.
Avoid parallel arrays of data that require the receiver to validate that the
arrays have matching lengths. Instead, bundle the data together in a struct so
it is impossible to have a mismatch:

**_Good_**

Expand Down Expand Up @@ -774,3 +779,4 @@ safe, vulnerabilities could arise.
[security-tips-for-ipc]: https://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc
[NfcTypeConverter.java]: https://chromium.googlesource.com/chromium/src/+/e97442ee6e8c4cf6bcf7f5623c6fb2cc8cce92ac/services/device/nfc/android/java/src/org/chromium/device/nfc/NfcTypeConverter.java
[mojo-doc-process-crashes]: https://chromium.googlesource.com/chromium/src/+/master/mojo/public/cpp/bindings#Best-practices-for-dealing-with-process-crashes-and-callbacks
[serialize-struct-tm-safely]: https://chromium-review.googlesource.com/c/chromium/src/+/679441

0 comments on commit 876c663

Please sign in to comment.