1236724 - Improper unserialization of bluetooth::BluetoothGattResponse leads to memory corruption

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

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

Attachment: GDB backtrace

When the type BluetoothGattResponse, is unserialized on the parent side [1], the code trusts the length specified by the user [2] and uses it to read into a buffer of fixed size [3]. This leads to memory corruption on the stack.

A compromised content process with the 'bluetooth' permission can exploit this bug and gain control over the instruction pointer of the parent.

I did some extensive testing of this (on my Aries device), it seems that the stack frame of PBluetoothParent::OnMessageReceived is corrupted, it took me a while to get a proper message sent across the channel.

So far, I seem to mess up some local variables of that function, which causes an early crash, but as can be seen in the back trace, the return address of the OnMessageReceived function is corrupt (0x04030200).

The bug also allows enough stack control so once control over the instruction pointer is gained a technique such as ROP can be used to further exploit the vulnerability. I used the default build settings for master and no stack cookie is present (which would make it a little harder to exploit)

As far as I can tell right now, this could be used to escape the sandbox once a content process (with 'bluetooth' permission) is compromised.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/ipc/BluetoothMessageUtils.h?rev=8cb55973ad98#242
[2] http://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/ipc/BluetoothMessageUtils.h?rev=8cb55973ad98#246
[3] http://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/ipc/BluetoothMessageUtils.h?rev=8cb55973ad98#251

Comment 1

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

Attachment: Patch to send malicious message

I patched gecko (ease of use) in order to send a malformed message, it is not ideal but should be enough to demonstrate the severity.

Comment 2

[Andrew McCreight [:mccr8]]

I'm marking this sec-high instead of sec-critical because it seems to require that the content process be compromised. If that makes sense.

Comment 3

[Bruce Sun [:brsun]]

Attachment: bug1236724_bluetooth_gatt_response_boundary_checking_flaw.patch

Comment 4

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

This is what it looks like at first, but, I browsed the code a bit, and there is a webapi[4] which seems to trigger that particular message[1], mLength and mValue (the ones that trigger the vuln) are set here[2] where the requestData seems to be set here[3] and there also seems to be a webapi to add such 'characteristics' (I have no idea what that is) here [5].

Unfortunately, I don't know anything about those Bluetooth components, some example code would help. I don't want to rule out the possibility that it may be reachable via webapi (given the above code).

It is pretty late over here, I might take a look at it again later.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/webapi/BluetoothGattServer.cpp?rev=8cb55973ad98#863
[2] http://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/webapi/BluetoothGattServer.cpp?rev=8cb55973ad98#843
[3] http://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/webapi/BluetoothGattServer.cpp?rev=8cb55973ad98#235
[4] https://dxr.mozilla.org/mozilla-central/rev/0771c5eab32f0cee4f7d12bc382298a81e0eabb2/dom/webidl/BluetoothGattServer.webidl#63
[5] http://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/webapi/BluetoothGattService.cpp?rev=e8c7dfe727cd#189

Comment 5

[Bruce Sun [:brsun]]

Thank you so much to dig this security flaw out.

|BluetoothGattResponse| or |RequestData| is not accessible from users. They are just internal structs in Gecko.

The application can
 - receive |onattributereadreq| and |onattributewritereq| event from |BluetoothGattServer|, and
 - try to use |writeValue()| of |BluetoothGattCharacteristic| (or |BluetoothGattDescriptor|) to update the value of the characteristic (or the descriptor), and then
 - try to use |sendResponse()| to trigger the response through |BluetoothGattServer| with a correct |requestId|.

Feel free to ping me if any information would be needed. :)

Comment 6

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

Comment on attachment 8703928 [details] [diff] [review]
bug1236724_bluetooth_gatt_response_boundary_checking_flaw.patch

Thanks :brsun, I don't know how much time I will spent looking into it further, but I may come back to it.

Patch looks good to me, in fact I have seen MOZ_ARRAY_LENGTH being used in multiple locations and I think it is the proper way to do it.

Comment 7

[Ben Tian (inactive)]

Comment on attachment 8703928 [details] [diff] [review]
bug1236724_bluetooth_gatt_response_boundary_checking_flaw.patch

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

r=me with comment addressed.

::: dom/bluetooth/ipc/BluetoothMessageUtils.h
@@ +259,5 @@
>      }
>  
> +    auto maxLength = MOZ_ARRAY_LENGTH(aResult->mValue);
> +
> +    if (aResult->mLength > maxLength) {

Compare |aResult->mLength| and |MOZ_ARRAY_LENGTH(aResult->mValue)| directly.

  if (aResult->mLength > MOZ_ARRAY_LENGTH(aResult->mValue)) {
    return false;
  }

Comment 8

[Bruce Sun [:brsun]]

https://hg.mozilla.org/integration/b2g-inbound/rev/8fdfe4071048b427d19cb5b42ee85eb13441068f
Bug 1236724: Check the maximum length of each array in IPC; f=jhector, r=btian

Comment 9

[Ben Tian (inactive)]

Assign to Bruce since he's working on this bug.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8fdfe4071048

btw shouldn't this had security approval before landing to b2g-inbound ? cc'ing albillings as info

Comment 11

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

Thanks, Carsten.

Unless this bug is trunk only, yes, it needed sec-approval before checkin. 

https://wiki.mozilla.org/Security/Bug_Approval_Process

If it wasn't trunk only, I need the sec-approval? bit set on the patch and the template questions answered so we can see how to proceed.

Comment 12

[Bruce Sun [:brsun]]

Attachment: bug1236724_bluetooth_gatt_response_boundary_checking_flaw.b2g44.patch

Comment 13

[Bruce Sun [:brsun]]

Comment on attachment 8703928 [details] [diff] [review]
bug1236724_bluetooth_gatt_response_boundary_checking_flaw.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
 - Unlikely to have an exploit based on this patch. Boundary checking on arrays are added in this patch, so using wrong values of length to let IPC receivers access invalid memory could be avoid.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
 - No messages for referring the security flaw explicitly.

Which older supported branches are affected by this flaw?
 - mozilla-b2g44_v2_5

If not all supported branches, which bug introduced the flaw?
 - bug 1181482

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
 - attachment 8705590 [details] [diff] [review] is for mozilla-b2g44_v2_5.

How likely is this patch to cause regressions; how much testing does it need?
 - Unlikely to cause regressions because this patch just add boundary checks.

Comment 14

[Bruce Sun [:brsun]]

This is my first time to solve a security bug. Sorry for my careless of approval process. Should I backout the commit and add "security fix" wordings for it?

Comment 15

[Andrew McCreight [:mccr8]]

(In reply to Bruce Sun [:brsun] from comment #14)
> This is my first time to solve a security bug. Sorry for my careless of approval process.

It is okay. It happens.

> Should I backout the commit and add "security fix" wordings for it?

No, it doesn't need to be backed out. The idea is that people trying to hack Firefox could watch any landings, so it is too late once a patch has been publicly landed somewhere.

Comment 16

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

(In reply to Bruce Sun [:brsun] from comment #13)

> Which older supported branches are affected by this flaw?
>  - mozilla-b2g44_v2_5

I assume this bug isn't b2g specific so which of the standard branches are affected? How far back does this go?

Comment 17

[Bruce Sun [:brsun]]

Hi Al,

Bluetooth related functions are only accessible on B2G environment[1].

If we need to fix this flaw from codes' perspective, we could adapt attachment 8703928 [details] [diff] [review] on |mozilla-aurora|, and attachment 8705590 [details] [diff] [review] on |mozilla-beta| respectively.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/webidl/Navigator.webidl#355-360

Comment 18

[Bruce Sun [:brsun]]

> If we need to fix this flaw from codes' perspective, we could adapt
> attachment 8703928 [details] [diff] [review] on |mozilla-aurora|, and
> attachment 8705590 [details] [diff] [review] on |mozilla-beta| respectively.

Forget to mention, other/older branches don't have such IPC message, so they don't have this flaw.

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 8703928 [details] [diff] [review]
bug1236724_bluetooth_gatt_response_boundary_checking_flaw.patch

thanks for answering the approval questions
sec-approval+ (retroactive)

Comment 20

[Bruce Sun [:brsun]]

Comment on attachment 8705590 [details] [diff] [review]
bug1236724_bluetooth_gatt_response_boundary_checking_flaw.b2g44.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 1181482
User impact if declined: security flaw due to improper boundary checking
Testing completed: N/A
Risk to taking this patch (and alternatives if risky): Low. Just add the necessary boundary checking. 
String or UUID changes made by this patch: N/A

Comment 21

[Daniel Veditz [:dveditz]]

Comment on attachment 8705590 [details] [diff] [review]
bug1236724_bluetooth_gatt_response_boundary_checking_flaw.b2g44.patch

carrying over sec-approval+. I'm not empowered for b2g branch approval so this is not that.

Comment 22

[Bruce Sun [:brsun]]

Comment on attachment 8705590 [details] [diff] [review]
bug1236724_bluetooth_gatt_response_boundary_checking_flaw.b2g44.patch

clear the approval‑mozilla‑b2g44 flag