Merge branch 'master' into review-update-3

This commit is contained in:
Myk Melez 2018-03-14 15:20:07 -07:00 коммит произвёл GitHub
Родитель fec7dc6c24 efcfa18c3c
Коммит 54982852f7
Не найден ключ, соответствующий данной подписи
Идентификатор ключа GPG: 4AEE18F83AFDEB23
6 изменённых файлов: 970 добавлений и 4 удалений

Просмотреть файл

@ -27,6 +27,7 @@ This is a list of our findings that we're reasonably happy with so far.
* [JavaScript Type Safety Systems](text/0009-type-safety-systems.md) are some conclusions of an investigation into the use of JavaScript type safety systems.
* [Firefox Data Stores Documentation](text/0010-firefox-data-stores.md) documents the existing data stores across all current Firefox platforms.
* [Fluent in Prefs Design Review](text/0011-fluent-in-prefs-design-review.md) describes the lightweight design review for Fluent in Prefs.
* [A brief analysis of JSON file-backed storage](text/0012-jsonfile.md) outlines some of the pros and cons of using a flat file (particularly via `JSONFile.jsm`) to store data in Firefox.
## Posts

Просмотреть файл

@ -40,9 +40,9 @@ Now let's create our standalone NDKs. There is no need to be inside the `NDK` di
```
mkdir NDK
${NDK_HOME}/build/tools/make_standalone_toolchain.py --unified-headers --api 21 --arch arm64 --install-dir NDK/arm64
${NDK_HOME}/build/tools/make_standalone_toolchain.py --unified-headers --api 14 --arch arm --install-dir NDK/arm
${NDK_HOME}/build/tools/make_standalone_toolchain.py --unified-headers --api 14 --arch x86 --install-dir NDK/x86
${NDK_HOME}/build/tools/make_standalone_toolchain.py --api 26 --arch arm64 --install-dir NDK/arm64
${NDK_HOME}/build/tools/make_standalone_toolchain.py --api 26 --arch arm --install-dir NDK/arm
${NDK_HOME}/build/tools/make_standalone_toolchain.py --api 26 --arch x86 --install-dir NDK/x86
```
Create a new file, `cargo-config.toml`. This file will tell cargo where to look for the NDKs during cross compilation. Add the following content to the file, remembering to replace instances of `<project path>` with the path to your project directory.

Просмотреть файл

@ -28,7 +28,7 @@ For both a Roadmap Review and a Design Review the **timetable** is as follows:
- For a Design Review this will be an experienced engineer in the problem domain outside of the team willing to put time into the problem. As engineers dont review their own code they shouldnt review their own designs. The principle is to get the most informed and least biased feedback possible.
* Optionally, for larger reviews, find someone to **chair** the review. This can facilitate the process (i.e. scheduling the meeting, managing the clock, ensuring minutes are taken, etc.) and enables the reviewer to concentrate on the review itself. The rest of this document uses 'chair' for the administrative role. For smaller reviews the reviewer also does the tasks of the chair.
2. The team should produce a **Review Packet** designed to document the proposal identified in step 1. The Review Packet includes:
2. The team produces a **Review Packet** designed to document the proposal identified in step 1. The Review Packet includes:
* A lay summary of the problem space which is focused on defining a shared language and identifying the key forces behind the problem.
* A list of the groups directly affected by the proposal to ensure that the review meeting includes representatives from those groups.
* A vision of what the project will achieve on completion.

Просмотреть файл

@ -0,0 +1,233 @@
Process Isolation in Firefox
Randell Jesup, Mozilla Browser Architecture team
## NOTE: this document is a Work in Progress!
# OVERVIEW
Weve recently moved a number of parts of Firefox into separate processes (e10s, multi-e10s, GPU process, compositor, WebExtensions process, GMP). This has produced some serious improvements in both security (sandboxing content, codecs) and stability (restarting GPU process when a GPU driver crashes, etc). This project is to evaluate how much further we can push process isolation to further improve these things.
### Problems:
* We have large processes, running many unrelated items of highly varying security and stability properties. A single bug (including in OS drivers) in many cases will take down either a major part of your tabs, or the master process and by extension the entire browser.
* In a related concern, a single exploitable bug gives access to a large part of the browser. Even if its in the Content process, it can give access to ¼ of your tabs, and because Content processes have very wide needs to access data and communicate with the Master process, the possibilities for either sandbox escape or information leakage are quite high.
* Features and capabilities often have code strewn across various parts of the tree, increasing the maintenance cost and risk of unrelated changes breaking them.
There are some secondary benefits we hope to achieve by doing this, such as decoupling parts of the system and providing more-stable interfaces to them, as well as easing some aspects of unit testing. There may be some advantages in build times or cost of maintenance; well see.
There are costs: development time, memory use, and performance all can or will be negatively impacted. Part of this project is to quantify these impacts (and hopefully reduce them) in order to guide the decisions on how far to push this process.
Chrome/Chromium has been doing similar work recently on "[Servicification](https://www.chromium.org/servicification)". This is related to their slowly replacing classic ipc/chromium-based IPC with “[Mojo](https://chromium.googlesource.com/chromium/src/+/master/mojo/README.md)”; a new IPC implementation (and ipdl-like layer) that has improved performance compared to classic Chromium IPC. Note that some ways Mozilla uses IPC might avoid some of the performance costs in Chrome (multiple channels, PBackground and the like, for example) - but we havent yet assessed how much overlap there is between Mojo and our additions to Chromium IPC. It may be that with some smaller modifications our use of Chromium IPC will be as efficient (or nearly so) as Mojo.
Chrome is also working on [Site Isolation](https://www.chromium.org/developers/design-documents/site-isolation), to avoid a compromised Renderer from breaking cross-origin isolation (more details below).
As part of this work, since it affects the performance impact of Process Isolation, we plan to explore the potential of adopting Mojo for Mozilla IPC (either wholesale, or progressively as Chrome has been doing).
Alternatively, and maybe more interestingly, we could look at using a Rust IPC crate and some added interface logic. <Need some more concrete suggestions/pointers here>
Note: we dont want to just "follow" Chromes lead, but to do whats smart for the users and Firefox, whether its similar to Chrome or not. Leapfrogging them in some manner would be great, but is not a requirement. If we can leverage work or research theyve done to reduce our cost or risks, great.
# GOAL
* Develop a browser-wide direction for Process Isolation:
* How much Servicification should we do?
* How many Content Processes should we use?
* Should we consider a Chrome-like Process-Per-Origin or Process-Per-Iframe model? What are the implications of this?
# Tentative Work Plan
1. Measure the overhead of using Process Isolation:
1. Memory cost for various scenarios (which largely depend on what part of firefox code need to be initialized/used in the process - XPCOM, etc)
1. Memory overhead for a Process on Win10 x64 appears to be circa 7-10 MB (Private) for a process with minimal XPCOM inited, IPC, etc. The GPU process (which also loads GPU drivers, etc) is on the order to 20MB, and anecdotally the GMP process uses on the order of 10MB, which is in line.
2. Performance cost - latency and throughput of calls through IPC (with classic IPC and Mojo, and perhaps a Rust IPC crate)
2. Still to be measured
3. Evaluate if we can make Chromium IPC as efficient as Mojo
3. I suspect we can; the main perf advantage Mojo has on classic IPC is one less thread-hop per message (2 less per round-trip - 4 vs 6). The overall code is probably a lot "cleaner", but it would be a fair bit of work to convert over, though probably much of it could be hidden by ipdl.
4. We believe that Mojos shutdown code may be more robust/better engineered than IPCs; shutdown has been a common source of crash/security bugs.
5. We think it might be possible in some special(?) cases to avoid a thread hop on the receiving side as well as the sending. Mojo does not do this.
4. Startup cost - cost to browser startup time
6. Unknown. Need to measure the cost in time to start a minimal process, and see how many we need at startup. We may be able to partly overlap creation and startup of service processes. Expectation is that this will not be a blocker.
5. Service startup time cost - cost on first use of a service which requires spawning a Process
7. Evaluate using "embryo" processes to allow the forked processes to be smaller but not pay new relocation/etc costs (i.e. dont fork the Master process and all that entails when we spin up a process/service). (B2G did something like this with [Nuwa](https://bugzil.la/nuwa).) Note that this doesnt help Windows apparently, and there was some sort of issue with this on Mac - these need to be examined/verified.
2. Analysis of Process Isolation
6. Analysis of the code maintenance impact
7. Analysis of the stability impact
8. Analysis of the security impact
9. Analysis of embryo processes
8. Per-OS
10. Analysis of IPC options
9. Update/improve current Chromium IPC
10. Mojo
11. Rust IPC
11. Android analysis - android will likely require different tradeoffs
3. Develop a preliminary list of potential subsystems/features to consider for Isolation
12. Necko is already planning to Isolate network socket access and protocol code (and some crypto code) after 57 or 58 -- [Bug 1322426](https://bugzilla.mozilla.org/show_bug.cgi?id=1322426)
12. They expect to land code in 61 behind a pref, and enable it in release in 63.
1. This will not be happening in the near term, due to staffing changes. Its not yet clear when or if this will occur.
13. Video camera capture code (and screensharing) is another prime target, as its already remoted over IPC even when running non-e10s. The way this works is very similar in principle to Chromes remote-services-over-Mojo approach.
14. Places in particular, eventually profile data access in general. This pushes storage asynchrony to the process boundary and decouples lifetimes (background storage and syncing, faster 'startup' and relaunch, with Places possibly living longer than a crashed browser). Future technology refactors could make this standalone process reusable outside of desktop Firefox. No bugs filed yet.
15. Font and/or Image code to avoid or reduce duplication of data between Content processes and the Compositor.
16. Printing
17. PDF display/PDFium
4. Look at the Content Process state and model (mostly this has been done in the e10s team)
18. How far do we want to push the model towards Chromes 1-per-origin/iframe model?
13. Probably not as far… note however that Chrome has closed the gap we created with them on memory use. [reference?]
14. Even Chrome cant get away with their stated goal (yet?)
2. While site isolation with a small number of sites is in the 15-20% range, this is largely because of the base overhead of Chrome (Master process, GPU processes, [zygotes, ](https://chromium.googlesource.com/chromium/src/+/lkcr/docs/linux_zygote.md)etc). WIth 17 tabs (a mixture of simple and complex sites), the measured overhead was about 50%.
19. How much does servicification help reduce Chrome process overhead (avoiding N instances of things)
15. It may not help a lot
20. How much can this work help [sandbox hardening](https://www.google.com/url?q=https://wiki.mozilla.org/Security/Sandbox/Hardening&sa=D&ust=1507349372899000&usg=AFQjCNHD1aRxpBa1fuJNcVKxytjULX6krA)?
16. Main Process will still need file access probably
5. Very speculative: examine if the current Master Process could be moved to be a child process of a thin Master Process, allowing restarts on Master Process crash without reloading all the running Content Processes.
21. **Very** likely this is too hard and/or too much work. Theres lots of state stored in the Master on behalf of the Content processes; the reconnect operation between Content and restarted Master would be considerably complex.
# Previous work
* GMP
* Very tight sandbox, one sandbox per-origin
* E10S
* GPU process
* Compositor
* WebExtensions
* Examination of the options for sandboxing Audio capture and playback, as well as other parts of the Media code: [Media, WebRTC and Audio Sandboxing Plans](https://docs.google.com/document/d/1cwc153l1Vo6CDuzCf7M7WbfFyHLqOcPq3JMwwYuJMRQ/edit)
* Background docs (needs to be updated but some useful info maybe)
* [https://wiki.mozilla.org/Security/Sandbox/Hardening](https://wiki.mozilla.org/Security/Sandbox/Hardening)
* https://wiki.mozilla.org/Security/Sandbox/Process_model
# Chromes Servicification and Mojo
Chrome has been discussing Servicification since roughly early 2016, and major work on it has begun this year (2017). This is the primary document: [Chrome Service Model](https://docs.google.com/document/d/15I7sQyQo6zsqXVNAlVd520tdGaS8FCicZHrN0yRu-oU/), and this is the primary root of the Servicification work: [Servicification](https://www.chromium.org/servicification).
An example of one item currently being moved to a Service is Video Capture: [Design Doc](https://docs.google.com/document/d/1RLlgEdvqRA_NQfSPMJLn5KR-ygVzZ2MRgIy9yd6CdFA/edit#heading=h.qjpzx8d6k7t5) and [detailed plans and measurements](https://docs.google.com/document/d/1Qw7rw1AJy0QHXjha36jZNiEuxsxWslJ_X-zpOhijvI8/edit#). Another which I think has been completed is the [Prefs Service](https://docs.google.com/document/d/1JU8QUWxMEXWMqgkvFUumKSxr7Z-nfq0YvreSJTkMVmU/edit).
[Mojo](https://chromium.googlesource.com/chromium/src/+/master/mojo/README.md) consists of a set of common IPC primitives, a [message IDL format](https://chromium.googlesource.com/chromium/src/+/master/mojo/public/tools/bindings), and a bindings library (with generation for a number of languages; undoubtedly wed need to add Rust binding generation -- [C++ bindings are here](https://chromium.googlesource.com/chromium/src/+/master/mojo/public/cpp/bindings)). Mojo has been measured in Chrome as being about ⅓ faster than classic IPC, and produces ⅓ less context switches. (Probably these two facts are related, and their [performance analysis](https://chromium.googlesource.com/chromium/src/+/master/mojo/public/cpp/bindings) indicates that not having to "hop to the IO thread" is part of why its faster, which makes sense.)
One thing we plan to experiment with is seeing if (leveraging our IPDL compiler) we can fairly transparently replace (some?) existing Chromium IPC channels/messages with Mojo.
Chrome has docs on [how they move legacy IPC code to Mojo](https://chromium.googlesource.com/chromium/src/+/master/ipc#Using-Services). This is a (somewhat dated) cheat sheet on [moving code from IPC and Mojo](https://www.chromium.org/developers/design-documents/mojo/chrome-ipc-to-mojo-ipc-cheat-sheet).
One interesting tidbit from that cheatsheet:
**IPC**
IPCs can be sent and received from any threads. If the sending/receiving thread is not the IO thread, there is always a hop and memory copy to/from the IO thread.
**Mojo**
A binding or interface pointer can only be used on one thread at a time since they're not thread-safe. However the message pipe can be unbound using either Binding::Unbind or InterfacePtr::PassInterface and then it can be bound again on a different thread. Sending an IPC message in Mojo doesn't involve a thread hop, but receiving it on a thread other than the IO thread does involve a thread hop.
Mojo has extensive support for message validation:
Regardless of target language, all interface messages are validated during deserialization before they are dispatched to a receiving implementation of the interface. This helps to ensure consistent validation across interfaces without leaving the burden to developers and security reviewers every time a new message is added.
# Overhead Details
1. Memory use. Content Process overhead is tracked in [Bug 1436250](https://bugzilla.mozilla.org/show_bug.cgi?id=1436250). Its measured both with a small patch to dump the ASAN heap state on command, and using a DMD build with specific environment options.
1. Minimal process (with IPC)
2. With XPCOM
1. 7-10MB
3. Full Content process
2. 25-30MB (varies by OS, memory model (32 vs 64), and system details (fonts, etc))
3. Content Process overhead is critical for Site Isolation
2. Performance -- measure each with small messages, large messages, and shared-memory payloads (anything else?)
4. Latency
5. Messages/second
# Security implications
Moving services and other code into sandboxed processes should generally increase the resilience of the system to security bugs. In particular, compromising the system in one sandboxed process will require a sandbox escape of some sort to leverage that into full or increased control, and generally will only expose data already flowing through the Service to the attacker - and for many sandboxed processes, exfiltrating compromised data would be much harder.
How hard exfiltration would be depends on what sort of data flows through the Service, how locked-down we can make the process, and if the output of the process normally is in some way visible to content - for example if the process did image decoding, then data could be exfiltrated by using it to break cross-domain restrictions (such as taking the content of image A in domain X, and outputting it in place of image B from domain Y (the attackers domain), allowing the attacker to render it in a canvas).
Another way that isolation will help security is by separating memory pools - memory errors such as UAFs can become much harder to exploit if you cant put the memory system under load (forcing reallocation of the memory with content you control, for example). In a Content process with JS running (and tons of other stuff), this is often not too hard; in an isolated Service it might be very hard indeed.
Once a Process is compromised, leveraging that into an exploit requires escaping into other Processes (using further bugs), or leveraging an OS bug. How hard that is depends on the OS and how locked-down we can make these Processes. "Small" processes may have much smaller OS attack surfaces, though this might be tougher to do on (say) Windows due to granularity of permissions.
# What Chrome does
Chrome doesnt actually use a Process-per-tab (or origin), though many people believe it does: see [Peter Kastings post from June](https://plus.google.com/+PeterKasting/posts/TC4ACtKevJY). (That was partially in response to some of our announcements around E10S.) The number of Render processes they use depends on the available memory - though it sounds like they have bugs there, and that may cause them to overrun and slow down the users system.
Chrome is working on [Sit](https://www.chromium.org/developers/design-documents/site-isolation)[e Isolation](https://www.chromium.org/developers/design-documents/site-isolation). Part of this is putting iframes OutOfProcess from the main page renderer, but more generally its about not using a renderer (Content process) for more than one origin. This has some serious downsides if taken to an extreme, and currently theyre planning to do this only for "high value" sites. (Its unclear what “high value” means here; one presumes banks, paypal, and other especially juicy targets.)
As mentioned above, Chrome is increasing the number of non-Render processes they use as part of Servicification.
The [Chrome Process Model](https://www.chromium.org/developers/design-documents/process-models) document is useful, but very out of date with current details - for example, the [Site Isolation](https://www.chromium.org/developers/design-documents/site-isolation) work theyve done. Some of the code for all these decisions is [here](https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_process_host_impl.cc?rcl=98c9b1d4fc903ab9dee06a98e7497b11de760449&l=341).
# What Edge does
In "[Browser security beyond sandboxing](https://blogs.technet.microsoft.com/mmpc/2017/10/18/browser-security-beyond-sandboxing/)" Microsoft goes into detail on a Chrome vulnerability, but also highlights some of what theyve done - in particular [Arbitrary Code Guard](https://blogs.windows.com/msedgedev/2017/02/23/mitigating-arbitrary-native-code-execution/#HGTKcYGQeuOxvYIB.97): “ACG, which was introduced in Windows 10 Creators Update, enforces strict Data Execution Prevention (DEP) and moves the JIT compiler to an external process. This creates a strong guarantee that attackers cannot overwrite executable code without first somehow compromising the JIT process, which would require the discovery and exploitation of additional vulnerabilities.” Their post introducing ACG is [here](https://blogs.windows.com/msedgedev/2016/09/27/application-guard-microsoft-edge/).
In more detail: "To support this, we moved the JIT functionality of Chakra into a separate process that runs in its own isolated sandbox. The JIT process is responsible for compiling JavaScript to native code and mapping it into the requesting content process. In this way, the content process itself is never allowed to directly map or modify its own JIT code pages."
This suggests that we should consider moving the JIT itself out of the main process space, and just share the result of the JIT back with the Content process requesting it. Since we already do JIT on a non-MainThread, the main issue here is probably shared-memory management. There will be some perf tests to do on this to validate if this is feasible within our current overall architecture, but it seems possible. This is being tracked in [Bug 1348341](https://bugzilla.mozilla.org/show_bug.cgi?id=1348341), and Tom Ritter has been investigating in [this doc](https://docs.google.com/document/d/13HwuPNxabIONDVTKInMkkIitXxcB_kmBjMTjA7dHtSE/edit).
Note that if the JIT process is compromised, anything running through it is as well, and any content processes it provides code for. This would imply that JIT processes may need to be tied to a single requesting Content Process to avoid stepping backwards in security here (and this also increases the potential memory cost).

60
text/0012-jsonfile.md Normal file
Просмотреть файл

@ -0,0 +1,60 @@
# A brief analysis of JSON file-backed storage
Several components in Firefox — including libpref, XULStore, autofill, and logins — follow a general pattern:
- Store data in memory, usually as a JS collection.
- Load that data from disk during initialization.
- Persist that data to disk in its entirety, usually serialized as JSON, at some point after each write, and/or at shutdown.
[JSONFile.jsm](https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/JSONFile.jsm) is a centralized implementation of this pattern. It supports Promise-based asynchronous loading, synchronous loading, and flushes asynchronously, defaulting to 1500ms after each change. libpref flushes after 500msec.
This approach is well suited to data that:
- Changes infrequently.
- Is relatively small.
- Is only ever directly read or written by a single thread.
- Needs to be kept entirely in memory for reads.
- Has a straightforward JSON encoding (encoding binary blobs, such as images, in base64 is relatively inefficient and typically avoided).
- Has limited durability requirements.
Advantages of this approach:
- It's relatively simple, with serialization and deserialization being the only steps.
- Because the file is rewritten frequently, there is some hard-to-measure resilience to gradual file corruption.
- The file on disk is — unless compressed — human-readable and editable, which is convenient for development, test, and support.
- Automatic opt-in whole-file compression is offered by JSONFile.
- The in-memory data can be queried and manipulated synchronously and quickly, assuming the representation is a good match for retrieval patterns.
- Development is 'natural' for front-end developers — get things working with only in-memory data, then add persistence on top.
Disadvantages:
- Versioning is often an afterthought: engineers rarely think to version in-memory data formats.
- Change tracking is almost always an afterthought, thanks to the 'natural' development process: adding syncing later becomes difficult.
- Users feel relatively empowered to edit readable files on disk, which can result in persistent state that was never created by the component's own logic. The use of compressed formats makes this less likely.
- Frequent writes will cause the entire file to be written to disk repeatedly, [which causes complaints about SSD impact](https://bugzilla.mozilla.org/show_bug.cgi?id=1304389).
- The entire file typically needs to be read into memory to be used, which increases memory footprint and can impact startup time.
- Writes at shutdown harm the user experience and don't happen during a crash.
- In order to achieve atomic file writes, the entire file contents briefly exist twice on disk, which could be problematic on mobile platforms for large data sets.
- The only approach to cross-process use is duplication of all or part of the in-memory data, which can increase memory footprint. It's not possible for multiple processes to collaborate on the same data via the filesystem. This is why Firefox for Android still uses the SQLite implementation of `nsILoginManagerStorage`: it allows the Android-side `ContentProvider` code to read and write saved logins without worrying about Gecko, which nominally owns the backing storage, but has a shorter lifetime than the enclosing Android code. Using desktop's JSON-backed storage on Android would require launching Gecko on each sync, using messaging for safe access to the file contents.
- The in-memory object is essentially a non-transactional write-back cache. This has several issues:
- It makes isolation (readers can't see in-progress writes) and atomicity (all writes complete or none do) difficult: we typically mutate the in-memory object directly, and so an exception can cause partial changes to 'commit', and readers will see each change as it is applied.
- Similiarly, timed flushing makes consistency difficult: it's possible for only some of a set of asynchronous writes to be flushed to disk because the flush beat the last few writes, leaving the data inconsistent after a crash.
JSONFile.jsm advises that callers do all of their work synchronously on the main thread to avoid the possibility of concurrent readers or partial writes:
> The raw data should be manipulated synchronously, without waiting for the
> event loop or for promise resolution, so that the saved file is always
> consistent.
This can cause jank.
- Synchronization of data stored in this way takes some careful thought. Syncs typically take hundreds of milliseconds or more, and involve asynchronous network operations, which makes exclusive synchronous access to the in-memory data between reads and writes infeasible.
- There is a tension between durability (that is: writes that complete are permanent) and performance. We typically choose not to flush the file immediately and synchronously after every change, but not doing so leaves a window in which a crash would cause data loss. By default, that window is 1.5 seconds, plus the time needed to write and atomically switch the files. This forces careful consumers to manually flush if they want their writes to stick, which is a bad pattern.
- In-memory objects lack the sophistication of most databases. This leads to front-end features building their own [simple query engines](https://dxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/storage-json.js#296) for [finding records by linear search](https://dxr.mozilla.org/mozilla-central/source/browser/extensions/formautofill/FormAutofillStorage.jsm#1081).
- Similarly, these stores must reinvent (or contribute to JSONFile) their own:
- Versioning and upgrade logic.
- Validation and schema checking.
- Concurrent access patterns.
- Write coalescing/async update patterns.
- Tooling, if existing general-purpose JSON tools (*e.g.*, `jq`) are not sufficient.
- Backup, if any.
- Indexing, if any.
- Datatype serialization (*e.g.*, timestamps).
- Finally, the use of `JSONFile.jsm` is intimately linked to Gecko; it's a poor choice for code that will later need to be deployed on other platforms.

Просмотреть файл

@ -0,0 +1,672 @@
# IPC Security Models and Status
Randell Jesup
Reviewers: pauljt, baku, bkelly, posidron, ehsan
## TL;DR:
* Considerable additional work could be done validating IPC parameters received by the Master process from Content (and other) processes
* More emphasis on Best Practices should be used in IPC work - especially documenting the security aspects in-tree
* There is some cruft in our IPC code left over from B2G, though some of it may need to be kept and updated instead of removed
* We should consider either moving to Mojo, or mirroring the partially-automatic validation of IPC arguments provided by Mojo
* A more in-depth look at how shmem is used might be valuable
* Improved automated fuzzing would be good
* An audit of 12 semi-randomly-chosen IPC protocols is attached (of ~144)
* One possible security issue found
## Current State:
IPC security is basically ad-hoc, implemented by each IPC protocol implementation.
The basics are that many parts of our system are gatewayed by using Principals, which encode the origin and related information associated with the document. Largely we rely on good reviewers, and thats more for avoiding sec issues like UAFs.
There are currently many components that send (child-to-parent) a Principal or a PrincipalInfo, but the parent is not really able to know if the child is allowed to use that principal. We should be able to validate what Principals a Content Process is allowed to use with data stored in the ClientManagerService. This is being worked on in [bug 1432825](https://bugzilla.mozilla.org/show_bug.cgi?id=1432825) and [bug 1432831](https://bugzilla.mozilla.org/show_bug.cgi?id=1432831), though that just covers a single case; resolving bug 1432831 would provide examples on how to do similar argument validation elsewhere.
As an example of Principal validation, when BroadcastChannel is sending a message, it sends the origin and its principal. The check is done on the parent side.
Once we have Site Isolation, the danger when Principle ownership isnt checked is that if a compromised Content Process (somehow) knows a principal thats valid in another document, it could pass it up, masquerading as that other document. Without checking if that document is loaded into that Content Process, the Master process will allow the action to proceed with an incorrect Principal. This can lead to all sorts of information leakage or spoofing. (Note: since a current Content Process could be navigating to a new origin, we cant really stop a compromised Content Process from getting a Principal for a random origin until we have support for process-per-origin (or set of origins) and necessary process switching code for navigations).
The issue isnt just Principals, however; all arguments must be checked, and where possible validated against what a given sender should be able to use.
In addition to information leakage, crafting arguments for an IPC message can also lead to considerable risk of sandbox escape - sending an IPC request with invalid arguments to trick the Master Process into taking an action that leads to the master process hitting a UAF or other security issue. Luckily, even in this case its hard to craft an escape, but this greatly increases the possibility.
Securing IPC code is important. Its more critical as we move towards site isolation - compromise of a Content Process with 1/4 your tabs (and everywhere you browse in the future in those) is pretty bad, so a sandbox escape is only incrementally worse, but with site isolation a compromised process may be worth much less, unless it can escape.
## Best Practices:
* Firefox [IPC guide](https://wiki.mozilla.org/Security/Sandbox/IPCguide)
* Googles legacy IPC [security tips](https://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc) (read for full explanations)
* (paraphrased/subsetted):
* **Trust only the ****browser**** "****Master" ****(aka “Chrome”) process.**
* **Sanitize and validate untrustworthy input.**
* **Whitelisting is better than blacklisting**
* **Safely handle known-bad input**
* Dont RELEASE_ASSERT on bad data
* **Use and validate specific, constrained types; let the compiler work for you**
* Use types IPC knows about, try to avoid bare strings and custom serializers
* **Keep it simple.**
* **Be aware of the subtleties of integer types.**
* **Be aware of the subtleties of integer types across C++ and Java, too.**
* **Don't leak information, don't pass information that would be risky to use.**
* Be careful with shared memory mappings (specifically tracking their sizes on either side of the IPC channel). Do not store and trust sizes on one side of the channel. Avoid specifying the size when calling Map.
* (many more in the doc)
* Carefully document lifetimes of objects that can receive IPC messages and ensure (and comment in the code) how we know no more messages can be received for an object
* Document the valid ranges for parameters, or validation criteria
* Provide guidance on when a Principal should be required for IPC protocols
* This primarily applies for info going to/from the Master Process, though as we get a more-complex web of Processes this might be needed elsewhere.
* An example where it isnt needed would be GMP webrtc codec processes, which are inherently per-site already and only communicate with a specific Content Process and webrtc connection within it.
* Validate parameters, lengths and existence of any data received from another Process.
* Its less critical to check validity of data received from the Master Process, but even there it may help catch problems and bugs. Also, in some cases, an attacker might be able to leverage a bug in code in the master to cause it to send a bad message (or race) to the Content Process, provoking a UAF/etc in content they can exploit. Its also possible to attack another non-Master process (Content Process, Extension Process, etc) by having the Master relay messages to it, as is often done in Extensions.
* Compromising the extension process could lead to control or disclosure of all content data
* Keep lists of things that are associated with a specific connection/Content Process to do validity checks (perhaps typically a Hashmap)
* Be very careful about overflows with data from the Content Process
* Where possible, give tokens out instead of raw data, if the other process doesnt actually need to manipulate or examine the data. This is similar in concept to things like fds, but with added validation whenever doing a lookup of a token (again, typically a hashmap, but perhaps not always). The lookup routine is a good place to put ownership checks and enforce their being checked.
* Fuzzing - weve done this before (fuzzing Pickle/etc). We can probably do more.
* Faulty
* Process bi-directional fuzzer in real-time.
* Integrated in --enable-fuzzing builds available at TaskCluster.
* Fuzzes pickled messages and/or pipe states.
* Runs in multiple instances in our fuzzing cluster at EC2.
* Upgrade of EC2 instance required which supports HW counters for rr. Important for general test-case reproducibility. There is currently no way to feed a mutated message into Firefox from the outside of the process.
* Requires targets (i.e mochitests, test-suite) to trigger individual IPC messages.
* We have no way to know if all IPC messages are hit in our testing
* We could keep stats on numbers of different IPC messages in debug builds
* [Meta bug for fuzzing](https://bugzilla.mozilla.org/show_bug.cgi?id=fuzzing-ipc-ipdl) (80+ bugs)
* Adding fuzzing for every new protocol would be useful.
* Christoph Diehl has a list of improvements for Faulty, and is also looking at how Mojo uses Googles fuzzer.
* Discovering dead/unused IPDL APIs. Weve had unused IPDL APIs left in the browser that could be used as attack vectors. It would be nice to be able to use automated tools to get rid of such code.
* As an example, the partial audit found that the DocumentRenderer IPC protocol was unused (and had a security vulnerability), and it has been removed.
## Possible Improvements to our IPC use:
* Provide more support for IPC parameter validation
* [Bug 1430159](https://bugzilla.mozilla.org/show_bug.cgi?id=1430159) - specified validator for specific IPC protocols
* [Bug 1325647](https://bugzilla.mozilla.org/show_bug.cgi?id=1325647) - Automatic validators for integers generated by the IPDL compiler
* Support classes for checking token/principal ownership
* Leveraging improved argument checking support in Mojo if we switch from IPC/chromium to Mojo
* Mojo heavily leverages C++ and types for validation, and also allows custom validators
* Add the possibility to mark each IPC method with **[principal_needed]** or something similar. Maybe that would be nice to be the default for each method. Basically, each method should have a principal or a principalInfo passed as argument. This would be interesting, if we have the possibility to validate which principal is allowed to run on the content process.
* We could move to a model closer to the dbus model, where Services are available under Well Known names, and clients can access them safely without worrying about if the target object exists (anymore). This would help remove a common failure case, where due to timing races an IPC request for a deleted object is received.
* A form of cross-IPC/process "weakptr"
* Possibly rewrite MessageManager (not in JS)?
## Further Investigations
* Examine an example protocol in great detail and document any issues, and solutions (as in [bug 1432831](https://bugzilla.mozilla.org/show_bug.cgi?id=1432831))
* Use as a guide for looking at others
* HTTPChannel Init was suggested (non-trivial, complex)
* List all IDPL protocols (~144 ipdl files)
* Document all JS users of MessageManager
# Prior work from Security Assurance Team on sandboxing (pauljt)
[Metabug](https://bugzilla.mozilla.org/show_bug.cgi?id=sandbox-sa)
The key activities that we have done are:
### IPDL auditing
Weve been through IPDL in an ongoing manner, but most recently the approach we took was [here](https://docs.google.com/document/d/1YYWGQAqKzBpXPmBnhjAJuCtr8ADtxhz7mVlbAvWorcQ/edit#). Summary:
* Audit message serialisation (param traits). DONE.See also: [Bug](https://bugzilla.mozilla.org/show_bug.cgi?id=1315840) and [Spreadsheet](https://docs.google.com/spreadsheets/d/1sTgA4bOuV0j1bP7_Q_QKsfiNNLfdIg_1CCo4-iB_cfo/edit#gid=0)
* Some issues were found
* There were a number which were "might be an issue". The follow up [bug](https://bugzilla.mozilla.org/show_bug.cgi?id=1325670) was filed by DH but then he left the org.
* Audit Recv Methods (look just at the recv function for bugs)
* This was an ongoing effort for Jhector before he left
* Bugs are filed under [https://bugzilla.mozilla.org/show_bug.cgi?id=1041862](https://bugzilla.mozilla.org/show_bug.cgi?id=1041862)
* The challenge here is that auditing this code is very time consuming and requires an understanding of the code/api in question
* Deeper audit diving into where data from recv function goes
* Weve only looked at this level in piecemeal approaches or when hunting for dangerous patterns
* We started trying to organise an [audit](https://docs.google.com/spreadsheets/d/17wvJPTfKto8abz7UD2NoPTebNYoV-dh60ejeCx84Vkw/edit#gid=1569995826) by contacting module owners but had to put this on hold due to the focus on Quantum
## External IPDL Audit
I also used some parental leave backfill to get a [timeboxed audit of IPDL](https://docs.google.com/document/d/1ad9JV6yeRcoqnS-vQR2h3A8avPDL7U4aydmzzHL7lhQ/edit) last year from a contractor (Stephen Fewer). It was only a month though, so very timeboxed.
## Message manager audit
* Weve been through all the "MessageManager" messages a couple times now, and Im pretty confident of our coverage here
* One exception is nested protocols, e.g. Web Extension message passing
* Detailed review notes are [here](https://docs.google.com/spreadsheets/d/1YnOFWatdnSBEvDKHLQV4DFngNuwC1Kkb2hZShV1cVx0/edit#gid=35305492)
* Some findings tracked under [here in bug 1040184](https://bugzilla.mozilla.org/show_bug.cgi?id=1040184)
**Other sandbox related auditing activities**
* Audit places where were run remote content in parent process: [bug 1305531](https://bugzilla.mozilla.org/show_bug.cgi?id=1305331)
* Attempt at automatic bounds checking (since this was the most common bug). [Bug 1325647](https://bugzilla.mozilla.org/show_bug.cgi?id=1325647)
## Specific Protocols and IPC messages Audit (jesup)
We have ([currently](https://searchfox.org/mozilla-central/search?q=&case=false&regexp=false&path=*.ipdl)) about 144 separate non-test .ipdl files in the tree. Looking in them for "manager XXXXXX": (This search may be comparable to mine below, but I dont trust it as much - [instances of manager in searchfox](https://searchfox.org/mozilla-central/search?q=manager%20&case=true&regexp=false&path=*.ipdl))
* About 36 managed by PBackground*
* About 9 managed by PBrowser
* 1 PCache
* 6 PClient*
* 8 PCompositor*
* About 23 managed by PContent
* 5 PGMP*
* 2 PImageBridge
* 17 PNecko
* 6 PPlugin*
* 2 PPresentation
* 2 PQuota
* 1 PServiceWorkerManager
* 1 PSpeechSynthesis
* 1 PVideoDecoderManager
* 1 PVRManager
* 2 PWebBrowserPersistManager
Looking at all the methods inside these ipdl files, there are roughly:
* ~1284 async IPCs
* 129 of these as async __delete__(...)
* Perhaps ~15-18 are async DeleteMe/DeleteSelf/RequestDelete
* ~124 sync IPCs
* ~19 nested(inside_cpow) (sync and async)
* ~146 nested(inside_sync) sync IPCs
* 1 prio(input) async
* ~10 prio(high) async
### Examination of example protocols:
* PVsync
* Child of PBackground, used between Parent (Master) and Child (Content) processes
* Only arguments are the current Timestamp on a Notify, and the timestamp rate (which could change I believe), both sent Master->Child
* Exists for the entire life of the Content process; if Content doesnt need VSync, it tells the Master to hold off on sending it.
* Security
* The only information exposed through this to the Content process is the VSync rate of the current monitor, and the exact timings of when the VSyncs happen (modulo latency in IPC).
* The exact timings might vaguely reveal some aspect of varying load on the Master process, maybe, and then only if the child can somehow time them extremely accurately. I seriously doubt this could be used to convey any significant data even with cooperation of code on the Master side and full control of the Content process (owned).
* Recommendations:
* None
* PUDPSocket
* Child of PNecko of PBackground
* PNecko for JS-visible PUDPSocket
* Was planned to move to PBackground also ([Bug 1167039](https://bugzilla.mozilla.org/show_bug.cgi?id=1167039)), but that was deprioritized with the end of B2G
* Likely PNecko support could be removed
* PBackground for WebRTC ICE network usage
* Originally designed to support B2G networked applications, such as Mail
* Web content was not allowed to use UDPSocket
* Principal used to check that udp-socket is allowed for that Principal
* All such uses have been removed with the end of B2G
* PBackground use was added to support WebRTCs need to send/receive UDP data and negotiate port usage via ICE (IETF protocol for NAT traversal)
* WebRTC doesnt pass up a Principal - Bug [1126232](https://bugzilla.mozilla.org/show_bug.cgi?id=1126232)
* Not critical since all web content is allowed to create RTCPeerConnections, which can be asked to initiate WebRTC with an arbitrary address, if (and only if) the target address responds to a STUN message which is used as a "Consent" indicator that the target is willing to exchange WebRTC data.
* No other data in the Master process is accessed via this protocol, only the network, so we dont need the principal to gate it
* All traffic on the port is filtered to ensure we see a STUN response before allowing any other traffic
* Until we see a STUN response, outgoing traffic is only allowed if the packet is a STUN packet, and incoming traffic is blocked
* This is the security requirement of the ICE & WebRTC protocols
* All methods are async
* From Content to Master, there are a number of methods to set up sockets or modify them
* Bind(), Connect(), JoinMulticast(), LeaveMulticast(), Close
* These have a number of structures as parameters
* WebRTC doesnt use the Multicast methods, but an owned Content process could send them
* The others are OutgoingData() (i.e. send()) and RequestDelete (asks the parent to send a __delete__ message to the child; avoids races)
* OutgoingData() takes a structure and a buffer
* From Master to Content, theyre all responses to Content IPCs except CallbackReceivedData() - i.e. incoming data on the port
* Security
* The only extant use (WebRTC) doesnt use a Principal
* This isnt important since the it cant be used to access data keyed by the Principal in the master, just network sockets
* Bug is filed (P4)
* Can send and receive data to arbitrary addresses
* STUN filter avoids content (even owned) being able to try to interact with websites or anything using a protocol not associated with realtime communication (e.g. STUN/ICE)
* Avoids using it to bypass other web controls or to try to DoS sites
* It can generate STUN packets, but STUN packets are rate-limited to avoid DoS-via-STUN
* Rate-limitation is in Content, so could be bypassed
* An owned process could create a UDPSocket channel without passing a filter string ("stun"), but that will cause the parent to fail Init() - it requires all UDPSockets without a Principal to use a filter (and the only valid filter is stun)
* Other than the default IPC read checks (read a string, read a uint32, read a bool, etc), generally theres no sanity checking of parameters
* However, when passed to system calls bad data *should* be rejected
* If theres an underlying library/OS bug, passing arbitrary data to bind()/connect()/etc could trigger it
* Additional sanity-checking of parameters is warranted for belt-and-suspenders security against sandbox attacks
* OutgoingData is an array or an IPCStream (not used for WebRTC)
* It has an address field, but if a filter (stun) is in place, it isnt allowed to be used.
* Recommendations
* Add validators for AddressInfo, etc per above
* Highest priority
* Remove PNecko and non-WebRTC support
* Unused and non-spec
* Lets us remove Multicast support
* Cons: disturbs working code, man-hours
* [dont do] Add Principal to WebRTC
* Removing non-WebRTC support means we can stop using Principal as a flag -- simplifies code in UDPSocketParent
* Low priority due to no obvious advantage to having the principal
* Con: man-hours, coding risk - we dont have access to the Principal down in mtransport; API changes would ripple through the stack to get access to it for no known current benefit
* PTexture
* Child of PImageBridge or PCompositorBridge or PVideoBridge
* Virtual reference to a Compositor resource (a gfx texture), which allows dropping or recycling it
* Used as part of higher-level protocols
* No arguments
* No security issues
* No recommendations
* PWebSocket
* Child of PNecko
* Supports implementing the DOM WebSocket API
* Since network resources must be used in the Master process, the Content processs WebSocket impl is basically an interface to a WebSocket that lives within the Master
* WebSocket creation involves many arguments. Once created, theres two-way communication over the channel, and the ability to close it.
* All requests as async
* Security
* Methods after creation are simple - send data, or receive data, or close.
* Master can request client delete itself
* Only security concerns here are to ensure that any data sent does not extend outside the memory the script has access to, and to avoid UAFs (especially on close/deletion)
* Open() and OnStart() are where most of the arguments are
* Open() takes 12 arguments, some of which are structures, OnStart() has 3 strings and a bool
* None of these are a Principal
* There are both original and triggering principals on the LoadInfo structure, which is passed in LoadInfoArgs (optionally null)
* Since it can be null, when converted into a LoadInfo it will succeed and put a nullptr in the nsCOMPtr<>
* It includes an optional URI, an Origin (string), and an InnerWindowID
* These get passed on the an HTTP(S)Channel AsyncOpen(), since WebSocket is a protocol run over an HTTP(S) channel
* Origin is used as a header for the HTTP/HTTPS request, to inform the server what origin URI generated this WebSocket connection request; note that its suggested that servers not use this as a form of authentication.
* Use on HTTP is as insecure as any HTTP connection, and an attacker could use this to feed bad data or commands (or collect information from) a website that uses HTTP WebSockets, via MITM attacks
* Using WebSocket as a TCP tunnel (WebSocket to Server, Server connects it to a TCP connection to some internal or external resource) is dangerous (to the resource), since a compromised Content (or Master) process could craft attacks on that resource.
* Recommendations
* Document when aLoadInfoArgs can be null, what happens, and what it implies - what will succeed and what will fail.
* Modify LoadInfoArgsToLoadInfo() (in BackgroundUtils.cpp) to verify if the principals are valid for the requesting content process/document
* Probably also affects all the other AsyncOpens (HTTP, FTP, etc)
* Verify that the InnerWindowID is associated with the principals in to LoadInfo
* PPresentation (and friends)
* Child of PContent (or PPresentationBuilder or PPresentationRequest)
* Supports the [Presentation API](https://developer.mozilla.org/en-US/docs/Web/API/Presentation_API), which allows remotely controlling a presentation using DataChannels or https/TCP
* [http://w3c.github.io/presentation-api/](http://w3c.github.io/presentation-api/)
* "enable Web content to access presentation displays and use them for presenting Web content"
* Originally implemented as part of B2G and perhaps also the TV project
* Apparently enabled in Chrome on Android since Chrome 48 (2015), and Chrome 51 (Desktop)
* Disabled by default (pref("dom.presentation.enabled", false);)
* Security
* There seems to be little or no checking in the Parent of the parameters and that theyre internally consistent, especially the session ID, window ID, tab ID, etc.
* Its unclear if this actually creates a security risk here
* Review [comment from smaug](https://bugzilla.mozilla.org/show_bug.cgi?id=1069230#c141) unaddressed by updates supposedly to address review -- requested the NS_CreatePresentationService() function check if the Presentation API is enabled/permitted
* Security review of this code has not been done: [Bug 1207051](https://bugzilla.mozilla.org/show_bug.cgi?id=1207051)
* Recommendations
* Find out the priority of completing and enabling this API in Firefox, either for Android or Desktop.
* If theres no interest in enabling it anytime soon, consider if we want to remove it
* Removing it will make it harder to bring it back (code rot); the code exists, apparently works and has tests
* Alternatively ensure that the IPC methods are gated by the enable pref, so that a compromised Content process cant try to leverage these IPC methods
* Since the API isnt even enabled, its unlikely that the Content Process can provoke any meaningful problems (other than DoS) by sending Presentation API IPC methods to the Master process, but adding a check of the preference or ensuring these methods cant do anything is a good safety move
* PMedia
* Child of PContent
* Used to manage unique deviceids for media devices (via [enumerateDevices](https://developer.mozilla.org/en-US/docs/Web/API/MediaDevices/enumerateDevices), which is speced in W3s [Media Capture and Streams](https://w3c.github.io/mediacapture-main/#mediadevices) API)
* Backed by a file mapping ids to devices on a per-origin basis
* Each origin has a different ID for the same device, to avoid them being supercookies.
* Mapping cleared when you clear cookies (per spec)
* Mapping is temporary (not saved) for private browsing
* Persistence gated by user permission to use a device
* Security
* Can cause Parent to access disk (flat file encoding deviceID/origin triples (id, time, and origin)
* File lives in profile
* A compromised Content process could probably look up the device ids for any site, instead of just the current site (if it knows or can guess what sites have called enumerateDevices())
* Just a privacy leakage in this case (supercookie), which is mostly irrelevant given then control a content process
* Could be blocked if we have Site Isolation
* Recommendations
* Once we have Site Isolation, verify the PrincipalInfo
* Ensure that the Sync & Storage team knows about this file
* PDataChannel
* Child of PNecko
* Involved in Necko redirection to data: URIs
* Necko code assumes that theres an IPC channel associated with the redirect
* The parent class wrapping the IPC channel largely does nothing
* Security
* There are no methods other than __delete__
* Recommendations
* None, other than possible future Necko rework for isolating parts of Necko might allow us to get rid of this required wart
* PColorPicker
* Child of PBrowser
* Requests a Color Picker be displayed by the Master (parent); implementing <input type=color>
* Security
* Uses the IPC Manager (PBrowser) to get the owner element, and thus the Outer Window
* No parameters from the Content Process; one method that passes a picked color from the Master Process
* Recommendations
* None
* PRenderFrame
* Child of PBrowser
* Represents one web "page"; it's used to graft content processes' layer trees into chrome's rendering path.
* Only one direct method to trigger a repaint by the Master; no parameters
* Used to connect Child layout Frames to the Parent
* Security
* Nothing direct to this protocol; the connection is passed around by a number of other parts of TabParent/Child
* Primary security risk would be lifetime management of the PRenderFrameParent/Child objects
* Per the ipdl file, the PRenderFrame is conceptually owned by the Content Process, and its lifetime is tied to the PresShell in the Content Process.
* Recommendations:
* Audit/document the lifetime management (low-ish priority)
* PDNSRequest
* Child of PNecko
* Implements DNS requests
* Most parameters are in the construction request
* Host, attributes, network interface
* Passed again to cancel
* Results are returned asynchronously once complete
* Security
* Host and Network Interface are strings
* None of the parameters are checked for validity on reception
* Relies on existing validity checks in nsDNSService::AsyncResolveExtendedNative() and functions it calls, like PreprocessHostname()
* Probably these are well-checked in general. They are (if internal checks are passed) sent to the OS
* The ability to specify a network interface might be a subtle risk, especially if were trying to route traffic through a VPN interface or proxy.
* Recommendations
* Consider validating parameters, or documenting where theyre validated
* PDocumentRenderer
* Child of PBrowser
* Implements rendering of a Canvas in the Content Process, and returns the rendered data
* Security
* Creation is Master->Content - no issues
* When rendered, the IPC protocol object is destroyed with the data being returned in the __delete__() method - size and a raw buffer as an nsCString (though it isnt really a null-terminated string
* This is concerning, since it opens the door to mishandling the data
* Depends on DataSourceSurface to avoid going past the real end of the data from the canvas, but nothing checks this
* Likely information-leakage bug possible if a crafted attack message is received in __delete__() (csectype-bounds)
* Recommendations
* Validate that the IntSize(aSize.width, aSize.height) is not larger than the length of the aData nsCString
* As a result of this review and evaluating a fix to implement the above, it was determined that PDocumentRenderer was unused, and it was instead removed.
* PGamepadEventChannel
* Child of PBackground
* Used to notify the Content process (and gamepad) of Update events
* Controllers added/removed
* Buttons pressed
* Joysticks moved
* Used by Content to tell the gamepad to vibrate (haptic feedback)
* Parameters are largely numbers (length, intensity, etc) and which controller
* Master Process appear to not actually implement haptic feedback ([bug 680289](https://bugzilla.mozilla.org/show_bug.cgi?id=680289))
* Security
* Parameters for Haptic feedback arent checked
* However, theyre not used currently
* Recommendations
* Add validation of Haptic feedback parameters (even if were not currently implementing it), or add a comment stating its needed before implementing it.
* Especially check that the index to the controller is valid
* There may be an very unlikely information leakage channel if Haptic feedback can be sensed by another Content process (causes joystick "motion", audible to an active mic, etc); however, exfiltrating data is far simpler in other manners, so I see almost no real risk here