The upper bound check of the 'width' and 'height' field[1] can be bypassed using negative values that are supplied by a malicious child through the IPC message 'Msg_AllocateGrallocBuffer' in the field 'size'. The assembler code for the mentioned checks looks like this (flame device): > cmp.w r3, #4096 > bgt.n <destination> According to the ARM Reference Manual 'bgt' is a signed compare. I verified it using gdb. To modify the IPC message I used my hooking engine to hook the Pickle::Read* functions inside the b2g process at runtime and swapped the values (similar to the faulty fuzzer). I supplied the log file from the hooks as an attachment. 'width' and 'height' (as well as the other two parameters) are used to allocate GPU memory, by creating a 'GraphicBuffer'[2] instance, the actual allocation happens inside a device specific library called 'gralloc.<device>.so' (for the flame it is called: 'gralloc.msm8610.so'[3]) I am not sure what impact this could have. I tested a couple of values for 'width' and 'height', when I set them both to 0xffffff00 then I get a NULL pointer deref: > ldr.w r3, [r10] with r10 being 0 [4]. But I don't know how this relates to the negative size of the graphic buffer. If I set the first value (I am missing the definition of the 'IntSize' struct so I can't tell whether it is the 'height' or 'width') to 0x80001100 and the second to 0x1000, so that the product of the two gives me a higher value than the allowed 4096 and 4096, the process freezes, gdb doesn't show anything, nothing responds and after a while the process is killed. So that's what I got regarding the impact, but I assume we shouldn't allow negative values for width and the height. [1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/SharedBufferManagerParent.cpp?rev=da8b000c08c6#223 [2] ./B2G/frameworks/native/libs/ui/GraphicBuffer.cpp [3] ./B2G/hardware/qcom/display/libgralloc/gpu.cpp - gralloc_alloc(...) [4] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/TiledContentClient.cpp?rev=78dcb3a426f2#1222

I think this belongs to Core::Graphics:Layers; Core::IPC is for the general IPC infrastructure.

I might be overly cautious but rating this sec-high for now because this kind of thing can lead to memory corruption.

Comment on attachment 8582566 [details] [diff] [review] patch - Add gralloc allocation requet size check Review of attachment 8582566 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/ipc/SharedBufferManagerParent.cpp @@ +210,5 @@ > *aHandle = null_t(); > > if (aFormat == 0 || aUsage == 0) { > printf_stderr("SharedBufferManagerParent::RecvAllocateGrallocBuffer -- format and usage must be non-zero"); > return true; Should we be returning false here also?

(In reply to Ben Turner <unresponsive until 3/30> (use the needinfo flag!) from comment #4) > Comment on attachment 8582566 [details] [diff] [review] > patch - Add gralloc allocation requet size check > > Review of attachment 8582566 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/ipc/SharedBufferManagerParent.cpp > @@ +210,5 @@ > > *aHandle = null_t(); > > > > if (aFormat == 0 || aUsage == 0) { > > printf_stderr("SharedBufferManagerParent::RecvAllocateGrallocBuffer -- format and usage must be non-zero"); > > return true; > > Should we be returning false here also? Yes. "return false" is better.

Comment on attachment 8583063 [details] [diff] [review] patch - Add gralloc allocation requet size check [Security approval request comment] How easily could an exploit be constructed based on the patch? > Not easy Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? > No. It is not clear from the comment. Which older supported branches are affected by this flaw? > all b2g that uses gralloc buffer since Bug 765734 fix. If not all supported branches, which bug introduced the flaw? > all b2g since gralloc support by Bug 76573 fix. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? > The patch could be created relatively easily. How likely is this patch to cause regressions; how much testing does it need? > low risk of regressions. tryserver test is enough.

This can't affect Firefox builds, only B2G?

(In reply to Al Billings [:abillings] from comment #8) > This can't affect Firefox builds, only B2G? It affects only b2g gonk.

Comment on attachment 8583063 [details] [diff] [review] patch - Add gralloc allocation requet size check Review of attachment 8583063 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/ipc/SharedBufferManagerParent.cpp @@ +216,5 @@ > + return false; > + } > + > + if (aSize.width <= 0 || aSize.height <= 0) { > + printf_stderr("SharedBufferManagerParent::RecvAllocateGrallocBuffer -- requested gralloc buffer size is invalid"); Probably worth using gfxWarning() rather than printf_stderr, and replacing the other occurrence of the printf_stderr above as well.

(In reply to Milan Sreckovic [:milan] from comment #10) > > Probably worth using gfxWarning() rather than printf_stderr, and replacing > the other occurrence of the printf_stderr above as well. It seems better to do it in another bug.

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/021e9a34c39a https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/aa80084c1ffc https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/adb9d7679526 http://mxr.mozilla.org/mozilla-b2g30_v1_4/source/gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp#291 looks like the relevant place to make the change on b2g30, but I'm not entirely sure what that should end up looking like. Can you please attach a branch patch? :)

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/aa80084c1ffc https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/adb9d7679526

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #14) > https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/021e9a34c39a > https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/aa80084c1ffc > https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/adb9d7679526 > > http://mxr.mozilla.org/mozilla-b2g30_v1_4/source/gfx/layers/ipc/ > ShadowLayerUtilsGralloc.cpp#291 looks like the relevant place to make the > change on b2g30, but I'm not entirely sure what that should end up looking > like. Can you please attach a branch patch? :) I am going to crate a patch for v1.4.

Patch for b2g v1.4. Less risky patch. Carry "r=nical".

Maybe I am missing something, but it seems like the b2g-v1.4 doesn't have the greater than 4096 check, with this patch now, it only has the less than 0 check. Which means that 'height' and 'width' could have the maximum value of 0x7fffffff. I don't know if such a size would cause trouble, but I guess the 4096 check is there for a reason.

Never mind, sorry, it was a little further up.

I've been meaning to ask, does this bug qualify for a bounty?

(In reply to Julian Hector [:tedd] from comment #21) > I've been meaning to ask, does this bug qualify for a bounty? Please email security@mozilla.org and ask to have this bug nominated per our bounty policy process. See https://www.mozilla.org/en-US/security/bug-bounty/ for details.

This is currently rated as high, but more as a precaution. It looks exploitable for me, but can either of you take a look and tell us what you think? See also https://wiki.mozilla.org/Security_Severity_Ratings

(In reply to Frederik Braun [:freddyb] from comment #23) > This is currently rated as high, but more as a precaution. > It looks exploitable for me, but can either of you take a look and tell us > what you think? > See also https://wiki.mozilla.org/Security_Severity_Ratings As far as I can tell, the worst that can happen here is a child process asking for very big textures, which could cause the main process to run out of memory and in the worst case reboot the phone (or trigger a path where the allocation fails and we deref a null pointer on the child process which reboots the phone as well. I don't think that this can be used to inject code or steal confidential data, but it could be used to cause the phone to reboot (provided the child process is already compromised and can manipulate the values it sends in this specific message). According to the wiki page, I seems like this does not qualify as sec-critical or sec-high (I see the tag csectype-oom which looks appropriate).

I agree to nical.

tedd: this now looks like a denial of service bug and not eligible for a bug bounty. If you disagree with that assessment please provide more information about how this could be exploited and ping us on #security or via email to reconsider.

Thank you dveditz, I don't disagree, I don't know enough about the code and would have to read into it.