Closed Bug 974353 Opened 6 years ago Closed 6 years ago

Shmem should check that the segment size is large enough

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox27 --- unaffected
firefox28 --- unaffected
firefox29 --- unaffected
firefox30 --- unaffected
b2g18 --- affected
b2g-v1.2 --- affected
b2g-v1.3 --- affected

People

(Reporter: bjacob, Assigned: bjacob)

References

(Blocks 1 open bug)

Details

(Keywords: sec-high)

Attachments

(2 files, 3 obsolete files)

Attached patch Check segment size in Shmem (obsolete) — Splinter Review
While working on bug 971695, I got to a point where there was no obvious unsafety anymore in the way that graphics code uses Shmem, yet when running with the fuzzer we got Shmem that were effectively smaller than their mSize said, leading to buffer overruns.

It turns out that mSize is initialized from a message parameter value, whereas the actual size of the shmem seems to be encoded directly in a header inside of the mSegment SharedMemory object. In my understanding, then, the only thing that we need to do is validate that mSize is not too large for that actual size of mSegment. This is done by moving the code computing the segment size to a helper function. This seems safe, then, because while mSize is still untrusted,
 - if it is too large, we now detect that and put the Shmem in an error state (see remark below).
 - if it is too small, well, user code will get a too small Size() value but at least that should allow it to deal with it and not crash.

The nontrivial part here is that Shmem does not seem to already have an "error state", so this patch makes !mData && !mSize be the error case, adapting AssertInvariants() accordingly.
Attachment #8378248 - Flags: review?(bent.mozilla)
Will the error state cause us to abort the child connection? I suspect it should.
In the present patch, as far as I can see, nothing makes it abort the child connection by itself. How would you write this code to cause that to happen? This is being checked in the Shmem constructor, where I don't see an obvious way to touch the IPC connection.
One thing I'm confused about. Once we have a shmem, passing it through IPDL only passes the ID, not any other length parameter. Is the problem here that the initial setup of the shmem is passing the wrong length? And we're solving that by comparing that length to a known-good length we got from the OS itself?

In normal practice the way to valid Shmem at runtime is to make the PSomeProtocol::Read(Shmem* __v...) method validate its parameter and return false if the parameter is incorrect.

But if we're talking about setting up a shmem at all, then the validation needs to be in PToplevelInterface::OnMessageReceived method, the SHMEM_CREATED_MESSAGE_TYPE case, which forwards into Shmem::OpenExisting. If the lengths don't match, probably OpenExisting should return null.

There's also a NS_RUNTIMEABORT in Shmem::OpenExisting which probably shouldn't be there, since the child could pass an invalid parameter and cause the parent to abort.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> One thing I'm confused about. Once we have a shmem, passing it through IPDL
> only passes the ID, not any other length parameter. Is the problem here that
> the initial setup of the shmem is passing the wrong length? And we're
> solving that by comparing that length to a known-good length we got from the
> OS itself?

See this ReadInfo helper function:

http://hg.mozilla.org/mozilla-central/file/bb030d47c946/ipc/glue/Shmem.cpp#l44

In particular this line:

    IPC::ReadParam(msg, iter, aSize)

Using the IPC fuzzer, which changes pickle values, I sometimes get this ReadInfo function to read the bogus value "1" for aSize, instead of the correct larger value.

This ReadInfo helper is called by OpenExisting to determine how big a segment to create. Since it returns a size of 1, OpenExisting then decides to create a small segment of only 3 pages (see segmentSize).

But this Shmem's Size<uint8_t>() still returns the correct larger value. That is because Size() returns mSize which is read from the header, in the Shmem constructor.

As a result, compositor code, which is using Size() to determine if the Shmem is big enough, doesn't catch the problem, tries to use the entire shmem, and crashes, as only 3 pages of it were actually mapped.

Thus, there are two distinct size notions: the Shmem::mSize, which is indeed stored inside the header, and the segment size, which seems to be serialized separately in ReadInfo().

The present patch is about validating that these two different variables are mutually consistent, but I could be totally wrong as I don't understand much of this code.

> 
> There's also a NS_RUNTIMEABORT in Shmem::OpenExisting which probably
> shouldn't be there, since the child could pass an invalid parameter and
> cause the parent to abort.

Yes, this is the subject of bug 968823.
Assignee: nobody → bjacob
Keywords: sec-high
Comment on attachment 8378248 [details] [diff] [review]
Check segment size in Shmem

Yeah, I'm in agreement with bsmedberg that we should somehow force-kill the child here rather than try to deal with bad data.
Attachment #8378248 - Flags: review?(bent.mozilla)
I don't understand this sentence:  "But this Shmem's Size<uint8_t>() still returns the correct larger value." Shmem::Size returns mSize, which is the "fake" value.

The only real way to know the correct size would be to ask the shared memory block itself what size it is. We may already be doing this in the implementation of SharedMemory::Map

http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/shared_memory_posix.cc#240
http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/shared_memory_win.cc#104

So if we try to map *more* bytes than are actually present in the shmem, that method should fail and cascade up to here: http://hg.mozilla.org/mozilla-central/file/bb030d47c946/ipc/glue/Shmem.cpp#l150

But mapping fewer bytes isn't a problem, I don't think, because Shmem::Size() should just return that size and we're still all secure and consistent.
Benjamin and I had a conversation today in which we found that Shmem's mSize and mData members were redundant with the corresponding information in mSegment, and that redundancy is what is causing us trouble therem, because we have code that assumes that these two pairs of values are synchronized, which they aren't.

So I have a WIP patch that removes mData and mSize and instead queries the mSegment for this information.

But that patch is crashy at the moment because the mSegment tends to go away under our feet, leaving mSegment as a dangling pointer.

The class comment for Shmem says:

 * The |Shmem| is a "weak
 *  reference" to the underlying |SharedMemory|, which is managed by
 *  IPDL-generated code.

Great, so, if it's a "weak reference" to the mSegment, is there a way that it can know if the mSegment is still alive?
Flags: needinfo?(bent.mozilla)
Flags: needinfo?(benjamin)
Note that there is a forget() method that clears the mSegment pointer, but obviously it wasn't called for me...

Besides mSegment being dead, is there another way that the data inside it could be inaccessible (some permissions problem?)
Nevermind, I need to do more debugging before I can ask non-stupid questions...
Flags: needinfo?(bent.mozilla)
Flags: needinfo?(benjamin)
OK, here is an explanation of why yesterday's approach didn't work.

That approach was to remove the mSize and mData fields of Shmem, as they are redundant with the values stored inside the SharedMemory's header.

The problem is that outside of the Unprotect()...Protect() pair in the Shmem constructor, these headers are protected. So when I made the Size() getter try to access the field stored in this header, it segfaulted. Since mprotect is IO, it is probably expensive, so we probably dont want to do it everytime we need to get the Size().

But I have another approach that works, see next patch...
Whoever of you gets to this first :) This works locally, fixes the crashes under faulty, allows to complete a full reftest run while fuzzing. Huzzah!
Attachment #8378248 - Attachment is obsolete: true
Attachment #8382351 - Flags: review?(bent.mozilla)
Attachment #8382351 - Flags: review?(benjamin)
Now with a NS_ERROR.
Attachment #8382351 - Attachment is obsolete: true
Attachment #8382351 - Flags: review?(bent.mozilla)
Attachment #8382351 - Flags: review?(benjamin)
Attachment #8382361 - Flags: review?(bent.mozilla)
Attachment #8382361 - Flags: review?(benjamin)
Comment on attachment 8382361 [details] [diff] [review]
In OpenExisting, check that the IPC-passed Shmem size matches the size stored in the SharedMemory header

:bent noticed that this is only the DEBUG code and there is duplicated code for the non-DEBUG case.

But that non-DEBUG case already has such a check, so the present bug only ever existed in DEBUG builds. Let's un-hide this bug, then?

The non-DEBUG code leaks |segment| when doing this early-return, btw... fixing this as well.
Attachment #8382361 - Attachment is obsolete: true
Attachment #8382361 - Flags: review?(bent.mozilla)
Attachment #8382361 - Flags: review?(benjamin)
Comment on attachment 8382392 [details] [diff] [review]
In OpenExisting, check that the IPC-passed Shmem size matches the size stored in the SharedMemory header. And don't leak the segment on error.

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

::: ipc/glue/Shmem.cpp
@@ +582,2 @@
>    if (size != static_cast<size_t>(*PtrToSize(segment))) {
> +    NS_ERROR("Wrong size for this Shmem!");

This is actually useless, you're in a !DEBUG section so this will just get optimized out always.
Attachment #8382392 - Flags: review?(bent.mozilla) → review+
Yeah, I realized that. I guess I'll remove it, although what I would really really like is de-balkanize this file (not have separate DEBUG and non-DEBUG implementations with totally different behavior, one storing the size in header->mSize and the other using *PtrToSize(segment) to store the size near the end of the segment).
Since we've been leaking SharedMemory objects in non-DEBUG builds when the size check failed...
Attachment #8382405 - Flags: review?(bent.mozilla)
Un-hiding, because we agree that the security bug here only ever existed in DEBUG builds. The present file, Shmem.cpp, has large #ifdef DEBUG blocks making it quite different between DEBUG and non-DEBUG cases.
Group: core-security
Comment on attachment 8382405 [details] [diff] [review]
Add MOZ_COUNT_[CD]TOR to SharedMemory

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

::: ipc/glue/SharedMemory.h
@@ +35,5 @@
>      TYPE_SYSV,
>      TYPE_UNKNOWN
>    };
>  
> +  virtual ~SharedMemory()

Nit: I'd move this to the cpp while you're at it.
Attachment #8382405 - Flags: review?(bent.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/a4c3a2e400b2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.