1042387 - Possible memory corruption when Read()ing FenceHandleFromChild or FenceHandle

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Julian Hector [:tedd] [:jhector]]

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

Comment 1

[Sotaro Ikeda [:sotaro]]

The implementations are borrowed from ParamTraits<MagicGrallocBufferHandle>::Read(). So, its implementations also seems to have same problem.

Comment 2

[Sotaro Ikeda [:sotaro]]

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

Comment 3

[Sotaro Ikeda [:sotaro]]

Message::file_descriptor_set() is protected function. It seems better to check the size by using FileDescriptorSet::MAX_DESCRIPTORS_PER_MESSAGE.

Comment 4

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add file descriptors count check

Comment 5

[Sotaro Ikeda [:sotaro]]

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?

Comment 6

[Julian Hector [:tedd] [:jhector]]

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

Comment 7

[Sotaro Ikeda [:sotaro]]

(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.

Comment 8

[Sotaro Ikeda [:sotaro]]

(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 :-(

Comment 9

[Julian Hector [:tedd] [:jhector]]

(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

Comment 10

[Sotaro Ikeda [:sotaro]]

I am thinking to add Message::num_fds() function to access header()->num_fds. It could fix the problem.

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add file descriptors count check

Add Message::num_fds().

Comment 12

[Sotaro Ikeda [:sotaro]]

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.

Comment 13

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add file descriptors count check

Fix Message::num_fds()'s problem.

Comment 14

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 15

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add file descriptors count check

Remove FileDescriptorSet::MAX_DESCRIPTORS_PER_MESSAGE check based on comment from Jeff. It is already checked in ipc code.

Comment 16

[Sotaro Ikeda [:sotaro]]

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.

Comment 17

[Sotaro Ikeda [:sotaro]]

(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.

Comment 18

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add file descriptors count check

Remove redundant data.

Comment 19

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=d7319d374fc9

Comment 20

[Sotaro Ikeda [:sotaro]]

(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() :-(

Comment 21

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add file descriptors count check r=jrmuizel

Add nullprt check. Carry "r=jrmuizel".

Comment 22

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=7158be269f6d

Comment 23

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/403203e27deb

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 25

[Sotaro Ikeda [:sotaro]]

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.

Comment 26

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8474551 [details] [diff] [review]
patch - Add file descriptors count check r=jrmuizel

sec-approval+

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/3cd520e7358d
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/be1c4865eada
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/8160021ea49f