Patch is up for this. Copying over the comment I had in the phab here, so we have a record ``` When investigating Bug 1824892 I saw some other anti-pattern instances of removing tainting earlier. While, for the most, the issue identified in Bug 1824892 is the only major OOB read (apart from non-null terminated string reads, which we are ok with in this context), I did more generally try to fix any anti-patterns I saw in this file. ``` Additionally, I did want to discuss a separate question @bholley: Typically bugs like this are exploitable only if we find a memory safety violation in the sandboxed code. However, here, we seem to be potentially triggering the same code path because, we don't abort on sandbox OOM. This makes sense because of how Wasm sandboxed code is setup. Today, if a Wasm sandbox attempts to grow (to support a malloc call from sandboxed code) and there is no more space, the malloc returns null and the sandboxed program continuous to execute (Most of this code doesn't check that malloc return nonnull). We do interpose on memory growth/malloc failures of sandboxed code, but we use this as an only to annotate crash reports. I wanted to ask if we should consider "abort on memorygrowth/malloc fail" behavior to avoid scenarios like this in the future.
Bug 1824892 Comment 13 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
Patch is up for this. Copying over the comment I had in the phab here, so we have a record ``` When investigating Bug 1824892 I saw some other anti-pattern instances of removing tainting earlier. While, for the most, the issue identified in Bug 1824892 is the only major OOB read (apart from non-null terminated string reads, which we are ok with in this context), I did more generally try to fix any anti-patterns I saw in this file. ``` Additionally, I did want to discuss a separate question @bholley: Typically bugs like this are exploitable only if we find a memory safety violation in the sandboxed code. However, here, we seem to be potentially triggering the same code path because, we don't abort on sandbox OOM. This makes sense because of how Wasm sandboxed code is setup. Today, if a Wasm sandbox attempts to grow (to support a malloc call from sandboxed code) and there is no more space, the malloc returns null and the sandboxed program continuous to execute (Most of this code doesn't check that malloc return nonnull). This continued execution corrupts the internal sandbox memory, and the sandbox returns garbage. Assuming correct tainted checks, this shouldn't be an issue. However, I was thinking we may want to employ a defense in depth here. Should we consider "abort on memorygrowth/malloc fail" behavior to avoid scenarios like this in the future. We do interpose on memory growth/malloc failures of sandboxed code, but we use this as an only to annotate crash reports.