1145389 - Upper bound check bypass due to signed compare in SharedBufferManagerParent::RecvAllocateGrallocBuffer

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

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

Attachment: logfile from the hooks (values are little endian)

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

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

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

Comment 2

[Daniel Veditz [:dveditz]]

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

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add gralloc allocation requet size check

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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?

Comment 5

[Sotaro Ikeda [:sotaro]]

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

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add gralloc allocation requet size check

Comment 7

[Sotaro Ikeda [:sotaro]]

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.

Comment 8

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

This can't affect Firefox builds, only B2G?

Comment 9

[Sotaro Ikeda [:sotaro]]

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

It affects only b2g gonk.

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 11

[Sotaro Ikeda [:sotaro]]

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

Comment 12

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/628e263e2205

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/628e263e2205

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Sotaro Ikeda [:sotaro]]

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

Comment 17

[Sotaro Ikeda [:sotaro]]

Attachment: patch forb2g v1.4 - Add gralloc allocation requet size check

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/f31e7ac5347f

Comment 19

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

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.

Comment 20

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

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

Comment 21

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

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

Comment 22

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

(In reply to Julian Hector [:tedd] from comment #21)
> I've been meaning to ask, does this bug qualify for a bounty?

Please email [e-mail] and ask to have this bug nominated per our bounty policy process. See https://www.mozilla.org/en-US/security/bug-bounty/ for details.

Comment 23

[Frederik Braun [:freddy]]

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

Comment 24

[Nicolas Silva [:nical]]

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

Comment 25

[Sotaro Ikeda [:sotaro]]

I agree to nical.

Comment 26

[Daniel Veditz [:dveditz]]

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.

Comment 27

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

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