Closed
Bug 1042387
Opened 10 years ago
Closed 10 years ago
Possible memory corruption when Read()ing FenceHandleFromChild or FenceHandle
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla34
Tracking | Status | |
---|---|---|
firefox32 | --- | unaffected |
firefox33 | --- | unaffected |
firefox34 | --- | fixed |
firefox-esr24 | --- | unaffected |
firefox-esr31 | --- | unaffected |
b2g-v1.3 | --- | fixed |
b2g-v1.3T | --- | fixed |
b2g-v1.4 | --- | fixed |
b2g-v2.0 | --- | fixed |
b2g-v2.1 | --- | fixed |
People
(Reporter: tedd, Assigned: sotaro)
References
Details
(Keywords: csectype-sandbox-escape, sec-high)
Attachments
(1 file, 5 obsolete files)
(deleted),
patch
|
sotaro
:
review+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
I just did a quick search on mxr for custom serialization/deserialization functions and came across the Read() function for FenceHandle[1] and FenceHandleFromChild[2]
I believe both functions are vulnerable to an arbitrary memory write. (I am not sure so this could all be safe, but to be sure I filed the bug anyways. I wrote a local test program to test a little)
To my understanding |aMsg| is considered unsafe, so we could control |nfds| which is read in line 173. This is used to allocate a stack array in line 178.
|nfds| is of type size_t (unsigned int), so -1 will be 4294967295. We can't allocate that much space on the stack. It doesn't fail though (I think 0 bytes are actually allocated)
In the for-loop in line 180, |n<nfds| is an unsigned compare, so |n| could iterate from 0 to 4294967295 and in line 185 we write to |fds| at the given index |n| using |fd.fd| which comes from |aMsg| as well but might be restricted in value, depending on what ReadFileDescriptor() does.
Locally the first write operation, overwrites the pointer to |fds| which gives us the ability to let it point anywhere in memory, so that in the second iteration we overwrite that location.
We can also trigger a controlled exit of the for loop (to avoid the DOS) with the number of file descriptors we actually supply (see line 183)
I hope someone has a better understanding where this function is actually called so it can be tested if it is vulnerable or not.
[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/FenceUtilsGonk.cpp#63
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/FenceUtilsGonk.cpp#164
Updated•10 years ago
|
Component: General → Graphics: Layers
Product: Firefox OS → Core
Updated•10 years ago
|
Flags: needinfo?(sotaro.ikeda.g)
Assignee | ||
Comment 1•10 years ago
|
||
The implementations are borrowed from ParamTraits<MagicGrallocBufferHandle>::Read(). So, its implementations also seems to have same problem.
Flags: needinfo?(sotaro.ikeda.g)
Assignee | ||
Comment 2•10 years ago
|
||
They stores a number of fds in the Message. But Message have a number of fd information in FileDescriptorSet. It can be used for sanity checking.
http://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/chrome/common/file_descriptor_set_posix.h#
By the way, current IPC message's file descriptor limit is 250. It also can be used for sanity checking.
http://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/chrome/common/file_descriptor_set_posix.h#27
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → sotaro.ikeda.g
Assignee | ||
Comment 3•10 years ago
|
||
Message::file_descriptor_set() is protected function. It seems better to check the size by using FileDescriptorSet::MAX_DESCRIPTORS_PER_MESSAGE.
Assignee | ||
Comment 4•10 years ago
|
||
Assignee | ||
Comment 5•10 years ago
|
||
Comment on attachment 8468743 [details] [diff] [review]
patch - Add file descriptors count check
Jeff, I create the patch based on your comment. Can you review the patch?
Attachment #8468743 -
Flags: review?(jmuizelaar)
Reporter | ||
Comment 6•10 years ago
|
||
As a note, there is also a header field in the message, that contains the amount of file descriptors send, which is sanitized when receiving the message here:
http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/chrome/common/ipc_channel_posix.cc?rev=4c5c545d7874#564
which could be used for further implementations
Assignee | ||
Updated•10 years ago
|
Attachment #8468743 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Julian Hector [:tedd] from comment #6)
> As a note, there is also a header field in the message, that contains the
> amount of file descriptors send, which is sanitized when receiving the
> message here:
>
> http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/chrome/common/
> ipc_channel_posix.cc?rev=4c5c545d7874#564
>
> which could be used for further implementations
Thanks! I am going to update the patch.
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to Julian Hector [:tedd] from comment #6)
> As a note, there is also a header field in the message, that contains the
> amount of file descriptors send, which is sanitized when receiving the
> message here:
>
> http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/chrome/common/
> ipc_channel_posix.cc?rev=4c5c545d7874#564
>
> which could be used for further implementations
header() is also protected :-(
Reporter | ||
Comment 9•10 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> (In reply to Julian Hector [:tedd] from comment #6)
> > As a note, there is also a header field in the message, that contains the
> > amount of file descriptors send, which is sanitized when receiving the
> > message here:
> >
> > http://mxr.mozilla.org/mozilla-central/source/ipc/chromium/src/chrome/common/
> > ipc_channel_posix.cc?rev=4c5c545d7874#564
> >
> > which could be used for further implementations
>
> header() is also protected :-(
oh, sorry, didn't see the protected there :/, I guess then the original patch is the better way to go
Assignee | ||
Comment 10•10 years ago
|
||
I am thinking to add Message::num_fds() function to access header()->num_fds. It could fix the problem.
Assignee | ||
Comment 11•10 years ago
|
||
Add Message::num_fds().
Assignee | ||
Updated•10 years ago
|
Attachment #8468743 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8472589 -
Flags: review?(jmuizelaar)
Assignee | ||
Updated•10 years ago
|
Attachment #8472589 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 12•10 years ago
|
||
attachment 8472589 [details] [diff] [review] does not work Message::num_fds() is valid only when IPC is cross process IPC. IF ipc is cross thread ipc, Message::num_fds() returns 0.
Assignee | ||
Comment 13•10 years ago
|
||
Fix Message::num_fds()'s problem.
Attachment #8472589 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8472706 -
Flags: review?(jmuizelaar)
Comment 14•10 years ago
|
||
Comment on attachment 8472706 [details] [diff] [review]
patch - Add file descriptors count check
Review of attachment 8472706 [details] [diff] [review]:
-----------------------------------------------------------------
::: gfx/layers/ipc/FenceUtilsGonk.cpp
@@ +75,5 @@
> }
>
> + if (nfds != aMsg->num_fds() ||
> + nfds > FileDescriptorSet::MAX_DESCRIPTORS_PER_MESSAGE)
> + {
Just checking that nfds == aMsg->num_fds() should be sufficient because num_fds() is realdy checked against MAX_DESCRIPTORS_PER_MESSAGE. If we're exposing num_fds() we can also remove nfds because we're sending the same information twice.
Attachment #8472706 -
Flags: review?(jmuizelaar) → review-
Assignee | ||
Comment 15•10 years ago
|
||
Remove FileDescriptorSet::MAX_DESCRIPTORS_PER_MESSAGE check based on comment from Jeff. It is already checked in ipc code.
Attachment #8472706 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8473046 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 16•10 years ago
|
||
attachment 8473046 [details] [diff] [review] exposes a number of fds in Message. Therefore ParamTraits<FenceHandle>::Write() do not need to store a number of fds as message data. This bug is a security bug. Therefore, it seems better to remove redundant "a number of fds" message data in a new bug.
Assignee | ||
Updated•10 years ago
|
Attachment #8473046 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 17•10 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #16)
> attachment 8473046 [details] [diff] [review] exposes a number of fds in
> Message. Therefore ParamTraits<FenceHandle>::Write() do not need to store a
> number of fds as message data. This bug is a security bug. Therefore, it
> seems better to remove redundant "a number of fds" message data in a new bug.
The above change is very simple. I am going to fix it in this bug.
Assignee | ||
Comment 18•10 years ago
|
||
Remove redundant data.
Attachment #8473046 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8473062 -
Flags: review?(jmuizelaar)
Updated•10 years ago
|
Attachment #8473062 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 19•10 years ago
|
||
Assignee | ||
Comment 20•10 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #19)
> https://tbpl.mozilla.org/?tree=Try&rev=d7319d374fc9
some tryserver tests failed at IPC::Message::num_fds() :-(
Assignee | ||
Comment 21•10 years ago
|
||
Add nullprt check. Carry "r=jrmuizel".
Attachment #8473062 -
Attachment is obsolete: true
Attachment #8474551 -
Flags: review+
Assignee | ||
Comment 22•10 years ago
|
||
Assignee | ||
Comment 23•10 years ago
|
||
Comment 24•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/403203e27deb
This should have had security approval before landing, no? There's no indication that it's trunk-only.
https://wiki.mozilla.org/Security/Bug_Approval_Process
Please fill that out retroactively.
Status: UNCONFIRMED → RESOLVED
Closed: 10 years ago
status-b2g-v2.1:
--- → fixed
status-firefox34:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Updated•10 years ago
|
Flags: needinfo?(sotaro.ikeda.g)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(sotaro.ikeda.g)
Assignee | ||
Comment 25•10 years ago
|
||
Comment on attachment 8474551 [details] [diff] [review]
patch - Add file descriptors count check r=jrmuizel
[Security approval request comment]
How easily could an exploit be constructed based on the patch?
- It is hard to exploit. A web content can not hit this problem.
To hit this, modification to native code or compromise child process becomes necessary.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
- No comment and no tests included.
Which older supported branches are affected by this flaw?
- All Firefox os gonks are affected.
If not all supported branches, which bug introduced the flaw?
- This bug exist since gonk is implemented.
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
- I do not have the backports. They are easy to be created.
How likely is this patch to cause regressions; how much testing does it need?
- This is low risk of regression. If firefox os works correctly, there should be no regression.
Attachment #8474551 -
Flags: sec-approval?
Updated•10 years ago
|
status-b2g-v1.3:
--- → affected
status-b2g-v1.3T:
--- → affected
status-b2g-v1.4:
--- → affected
status-b2g-v2.0:
--- → affected
status-firefox32:
--- → unaffected
status-firefox33:
--- → unaffected
status-firefox-esr24:
--- → unaffected
status-firefox-esr31:
--- → unaffected
Comment 26•10 years ago
|
||
Comment on attachment 8474551 [details] [diff] [review]
patch - Add file descriptors count check r=jrmuizel
sec-approval+
Attachment #8474551 -
Flags: sec-approval? → sec-approval+
Comment 27•10 years ago
|
||
Updated•10 years ago
|
Updated•10 years ago
|
Group: core-security
Updated•3 years ago
|
Keywords: csectype-sandbox-escape
You need to log in
before you can comment on or make changes to this bug.
Description
•