Closed Bug 1450232 Opened 6 years ago Closed 6 years ago

Check that IPC message lengths fit in the available data before allocating data

Categories

(Core :: IPC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: posidron, Assigned: Alex_Gaynor)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch set.infallible.diff (obsolete) — Splinter Review
      No description provided.
Attachment #8963902 - Flags: superreview?(jld)
Attachment #8963902 - Flags: review?(jld)
Summary: Use infallible SetLength() and SetCapacity() in --enable-fuzzing builds → Use fallible SetLength() and SetCapacity() in --enable-fuzzing builds
Summary: Use fallible SetLength() and SetCapacity() in --enable-fuzzing builds → Use fallible SetLength() and SetCapacity() in IPC messages
:baku suggested to me that these should always be fallible (not just for fuzzing), hence no #ifdef FUZZING in these patches.
Comment on attachment 8963902 [details] [diff] [review]
set.infallible.diff

Review of attachment 8963902 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/glue/IPDLParamTraits.h
@@ +110,3 @@
>        return aMsg->ReadBytesInto(aIter, elements, pickledLength.value());
>      }
> +    if (!aResult->SetCapacity(length, mozilla::fallible)) {

The comment before Read() specifically mentions why we chose infallible allocation here.  Presumably the same sort of logic applies to the string methods changed elsewhere in this patch.  I don't think changing that unconditionally is the right thing to do.
:baku, can you elaborate if and under which conditions, these string methods should be fallible? I remember we had an IRC conversation where you mentioned that they should be fallible unconditionally (at that point, they still lived in generated code).
Flags: needinfo?(amarchesini)
Often, when a string is sent via IPC, its length is sent as well as part of of the message. This happens for StructuredClone Algorithm, and elsewhere. If the child process is compromised, the IPC message could contain a huge length value.
If the parent doesn't use the fallible versions of SetLength/SetCapacity, it would crash because of OOM errors.

In general, I would like to get rid of infallible SetLength/SetCapacity methods.
Flags: needinfo?(amarchesini)
While it is not a security problem, I also think that the child shouldn't be able to crash the parent with a single wrong message and that's the case here. This could happen for numerous reasons, including memory corruptions in the child process or some unchecked flow of a large size into one of these messages, causing the whole browser to crash.

And fwiw, this is happening in the wild as well: https://crash-stats.mozilla.com/search/?signature=%40.%2ANS_ABORT_OOM.%2ASet%28Length%7CCapacity%29.%2A&proto_signature=~OnMessageReceived&product=Firefox&process_type=browser&date=%3E%3D2018-03-19T11%3A33%3A55.000Z&date=%3C2018-04-02T12%3A33%3A55.000Z&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature

While we cannot avoid OOM, we can at least avoid the parent to crash from a single wrong message.
ni?mccr8 who made this change in bug 1269365.
Flags: needinfo?(continuation)
Comment on attachment 8963902 [details] [diff] [review]
set.infallible.diff

Review of attachment 8963902 [details] [diff] [review]:
-----------------------------------------------------------------

In Gecko, the philosophy is that allocations should be infallible by default, unless they show up frequently enough on Crash Stats that they are a problem for stability. A few hundred crashes in a week does not rise to this level. The bigger concern is avoiding poorly tested error conditions. If there are particular messages that are in danger of being too large, they should be handled with a special case, not just a generic change like this, as Nathan said.
Attachment #8963902 - Flags: superreview?(jld)
Attachment #8963902 - Flags: review?(jld)
Attachment #8963902 - Flags: review-
Flags: needinfo?(continuation)
I have a proposal that I think could meet both desires:

Write the code as:

    length = ReadLength()
    if CheckedInt(length) * width >= AvailableBytes()
        return false;
    SetCapacity(length)

This would a) Not crash in fuzzing scenarios where a super long length is provided, b) In the real world we'd still get OOM crashes, c) not require fuzzing specific branches, d) make it consistent with other truncation scenarios.

The one downside is that it's a tad more verbose, but it's in like 4 place so that's life.

Is that amenable to folks?
We'd still get potential failures in the real world due to the CheckedInt usage, though, right?  (e.g. bit corruption might show up as failures elsewhere, rather than OOMs)
Yes, but that's true now as well -- a bit corruption which produces a successful SetCapacity() but there aren't that many bytes. This would simply make it slightly more deterministic (the case where a bit corruption _decreased_ the length and then you ended up with corrupted data isn't handled).

I just realized my scheme is less precise with nsArray, where you don't know how much data each ReadIPDLParam will read, so all we can really do is verify that at least length times alignment bytes are still available.
A nice side-benefit that just occurred to me is that this will make the fuzzers faster :-) length=300MB will no longer perform a giant alloc and then immediately free it, it'll just error out immediately.
Comment on attachment 8969698 [details]
Bug 1450232 - in IPC, check that lengths fit in the available data before allocating data;

https://reviewboard.mozilla.org/r/238496/#review244408

The commit message really needs to run through the different cases you are trying to address. It took me a while to figure out what you were trying to do, and I only managed to do that after reading through the bugzilla comments a number of times. You should probably also add a comment along those lines to HasBytesAvailable(), so somebody looking at the code can figure out what is going on.

Otherwise this looks reasonable to me.

::: ipc/glue/IPCMessageUtils.h:590
(Diff revision 1)
>  
>        E* elements = aResult->AppendElements(length);
>        return aMsg->ReadBytesInto(aIter, elements, pickledLength);
>      } else {
> +
> +      // Each ReadParam<E> will read more than 1 byte each, this is our best

nit: this should be a period or maybe a semi-colon, not a comma. Same with below.

::: ipc/glue/IPDLParamTraits.h:121
(Diff revision 1)
>      }
>  
> +    // Each ReadParam<T> will read more than 1 byte each, this is our best
> +    // effort to pre-check if the length field is larger than what's
> +    // available.
> +    if (!aMsg->HasBytesAvailable(aIter, length)) {

Can you use something like pickedLength (from above) here?
Attachment #8969698 - Flags: review?(continuation) → review-
Comment on attachment 8969698 [details]
Bug 1450232 - in IPC, check that lengths fit in the available data before allocating data;

https://reviewboard.mozilla.org/r/238496/#review244408

Ok, I think I got all of these. Let me know if the commit message still isn't enough.

> Can you use something like pickedLength (from above) here?

I can't, but I added a comment explaining why.
Comment on attachment 8969698 [details]
Bug 1450232 - in IPC, check that lengths fit in the available data before allocating data;

https://reviewboard.mozilla.org/r/238496/#review245028
Attachment #8969698 - Flags: review?(continuation) → review+
Keywords: checkin-needed
Pushed by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a0d5ed4a3fb6
in IPC, check that lengths fit in the available data before allocating data; r=mccr8
Keywords: checkin-needed
Attachment #8963902 - Attachment is obsolete: true
Assignee: nobody → agaynor
Summary: Use fallible SetLength() and SetCapacity() in IPC messages → Check that IPC message lengths fit in the available data before allocating data
https://hg.mozilla.org/mozilla-central/rev/a0d5ed4a3fb6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.