932490 - ObexBase.cpp buffer overflow(s)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Rob Fletcher [:omerta]]

ObexBase.cpp assigns and copies into buffers without regard to their size,
causing overflow vulnerabilities.

In general, the file needs to be reviewed and a method to verify length checks
must be implemented. It seems the callee is agnostic to the buffer size, so it
seems the caller must check or an additional function parameter must be passed
in so the callee can verify.

Not all instances are exploitable, but at least one appears to be an exploitable
heap overflow. The example below uses ObexBase::AppendHeaderName() which
performs a dangerous memcpy:

BluetoothOppManager.cpp
index += AppendHeaderName(&req[index], (char*)fileName, (len + 1) * 2);

1.req is an array defined on line 1019 with length of mRemoteMaxPacketLength

2.mRemoteMaxPacketLength is a 2-byte, device controlled value.

3.req is passed into AppendHeaderName(&req[3], filename, 2len + 2)

4.len is aFileName.Length()

5.AppendHeaderName makes a dangerous memcpy (replacing variable names):
  - memcpy(req[6], filename, 2len + 2)

6.We can control the size of req via mRemoteMaxPacketLength (even if we can't,
  we just need a filename length of 0x7FFF) and we can control the characters
  (filename) copied into the buffer. 

[1] and [2] are instances of unsafe memcpy's into buffers.

Below are instances of static assignments into buffers. These are seemingly
benign, but often times buffers are passed in as buffer+offset so it is possible
for these assignments to overflow allocated memory. The following lines, inside
[3], perform dangerous assignments into buffers: 16-18, 30-32, 42-44, 52-56,
64-68, 76-78.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/ObexBase.cpp#20
[2] http://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/ObexBase.cpp#34
[3] http://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/ObexBase.cpp

Comment 1

[Andrew McCreight [:mccr8]]

echou is this something you can look at?  Thanks.

Comment 2

[Paul Theriault [:pauljt] (no longer reading bugmail)]

Rob, do you have any idea what the likely attack scenario here is? Would a user have to be be paired with to a malicious bluetooth device? Or does bluetooth just have to be enabled? If its the former, I am thinking this doesn't need to block koi. But need more details really.

Comment 3

[Rob Fletcher [:omerta]]

The OBEX base code "is a communications protocol that facilitates the exchange of binary objects between devices".

It requires the phone to be paired to a device.

Comment 4

[Jason Smith [:jsmith]]

Based on comment 2 & comment 3 - not a blocker for release then.

Comment 5

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Jason Smith [:jsmith] from comment #4)
> Based on comment 2 & comment 3 - not a blocker for release then.

Yep, this isnt a release blocker.

Comment 6

[Daniel Veditz [:dveditz]]

If this isn't a blocker then it's probably also not sec-critical. Mitigation of needing to be paired with a presumably somewhat-trusted device is maybe sec-high?

Is Obex new to b2g 1.2, or is b2g18 really affected?

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

Eric can confirm but looks like his ObexBase introduction patch landed on trunk Sept 28?
https://hg.mozilla.org/mozilla-central/rev/9386cf1dbde2

Comment 8

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Fix the buffer overflows of OBEX packet. (draft patch)

Comment 9

[Rob Fletcher [:omerta]]

A little late to the party, but this code is present in mozilla-b2g18_v1_1_0_hd and was committed on Sat Nov 03 09:36:34 2012 +0800.

It appears to affect b2g18.

Comment 10

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8339178 [details] [diff] [review]
Fix the buffer overflows of OBEX packet. (draft patch)

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

Hi Jamin, please see my comments below.

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +999,5 @@
> +    // According to IrOBEX 1.2, the minimum size of the OBEX Maximum packet
> +    // length allowed for negotiation is 255 bytes.
> +    // If the body of OBEX packet size is less than 255 bytes, the object
> +    // transfer would be very inefficient.
> +    if (mFileName.Length() > mRemoteMaxPacketLength - 255) {

Actually this may be incorrect. mFileName.Length() represents the number of characters, not byte count. Therefore they shouldn't be compared.

I think we don't really need this check since it would be protected by checking length in ObexBase.cpp. Instead, we can get the min(255, (((int)(aMessage->mData[5]) << 8) | aMessage->mData[6])) to ensure mRemoteMaxPacketLength >= 255.

::: dom/bluetooth/ObexBase.cpp
@@ +17,5 @@
>    aRetBuf[0] = ObexHeaderId::Name;
>    aRetBuf[1] = (headerLength & 0xFF00) >> 8;
>    aRetBuf[2] = headerLength & 0x00FF;
>  
> +  memcpy(&aRetBuf[3], aName, (aLength < aBufferSize)? aLength : aBufferSize);

Should we check aBufferSize - 3?

@@ +32,5 @@
>    aRetBuf[0] = ObexHeaderId::Body;
>    aRetBuf[1] = (headerLength & 0xFF00) >> 8;
>    aRetBuf[2] = headerLength & 0x00FF;
>  
> +  memcpy(&aRetBuf[3], aData, (aLength < aBufferSize)? aLength : aBufferSize);

Same question as above.

Comment 11

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Fix the buffer overflows of OBEX packet. (v1)

Eric,
Thank you for your helpful comments and corrections.
I uploaded a new patch.

Comment 12

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8342807 [details] [diff] [review]
Fix the buffer overflows of OBEX packet. (v1)

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

Hey Jamin, please see my comments below. Thanks.

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +1000,5 @@
> +    // length allowed for negotiation is 255 bytes.
> +    // If the body of OBEX packet size is less than 255 bytes, the object
> +    // transfer would be very inefficient.
> +    int fileNameLen = mFileName.Length();
> +    if ((fileNameLen + 1) * 2 > mRemoteMaxPacketLength - 255) {

I still don't think this is right. Could you explain why you write it this way?

I think there are two things you may want to do:

1) Handle the case if mRemoteMaxPacketLength is smaller than 255, which violates the restriction of this field.
  1-1) Call SendDisconnectRequest() to terminate this file transfer session.
   or
  1-2) Set mRemoteMaxPacketLength to 255 once it's smaller than 255.
2) See if this if-statement is still necessary.

::: dom/bluetooth/ObexBase.cpp
@@ +17,5 @@
>    aRetBuf[0] = ObexHeaderId::Name;
>    aRetBuf[1] = (headerLength & 0xFF00) >> 8;
>    aRetBuf[2] = headerLength & 0x00FF;
>  
> +  memcpy(&aRetBuf[3], aName, (aLength < aBufferSize - 3)? aLength : aBufferSize - 3);

Since the buffer size passed into this function was actually the 'left size' of aRetBuf, then no need to minus 3.

@@ +32,5 @@
>    aRetBuf[0] = ObexHeaderId::Body;
>    aRetBuf[1] = (headerLength & 0xFF00) >> 8;
>    aRetBuf[2] = headerLength & 0x00FF;
>  
> +  memcpy(&aRetBuf[3], aData, (aLength < aBufferSize - 3)? aLength : aBufferSize - 3);

Ditto.

Comment 13

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Fix the buffer overflows of OBEX packet. (v2)

Comment 14

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Fix the buffer overflows of OBEX packet. (v3)

Comment 15

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8343672 [details] [diff] [review]
Fix the buffer overflows of OBEX packet. (v3)

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

LGTM. Thanks.

Comment 16

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [final] Fix the buffer overflows of OBEX packet. (v4), r=echou

It looks good on try server.
https://tbpl.mozilla.org/?tree=Try&rev=e87347951362

Comment 17

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8345173 [details] [diff] [review]
[final] Fix the buffer overflows of OBEX packet. (v4), r=echou

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

r=me with nit addressed.

::: dom/bluetooth/bluedroid/BluetoothOppManager.cpp
@@ +49,5 @@
> +/*
> +* The format of the appended header of an PUT request is
> +* [headerId:1][header length:4]
> +* P.S. Length of name header is 4 since unicode is 2 bytes per char.
> +*/

super-nit: please line up those '*'

Comment 18

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [final] Fix the buffer overflows of OBEX packet. (v5), r=echou

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/f6c4761e717c

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f6c4761e717c

Please nominate this for b2g18 and b2g26 uplift.

Comment 21

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [v1.3] Fix the buffer overflows of OBEX packet. , r=echou

Comment 22

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [koi] Fix the buffer overflows of OBEX packet. , r=echou

Comment 23

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [hd] Fix the buffer overflows of OBEX packet. , r=echou

Comment 24

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [leo] Fix the buffer overflows of OBEX packet. , r=echou

Comment 25

[Eric Chou [:ericchou] [:echou]]

Nominate this as leo+ for merging into b2g18 / b2g26. (Please see bug 932543 comment 28 for the reason why we don't request for uplift approval)

Comment 26

[Eric Chou [:ericchou] [:echou]]

Talked to our EPM Joe yesterday and he told me that we don't do leo triage anymore. So if we'd like to apply this fix to b2g18 or b2g26 we should find someone to give the approval. According to bug 932543 I think Bhavana may be the right person to look for, so ni? Bhavana.

Hi Bhavana,

This is another security issue we should fix in previous version of Gecko just like what we've done to bug 932543. Would you mind assisting with the uplift? In fact we still have another similar one (bug 932496), and I think we should treat these two issues with the same strategy.

Comment 27

[bhavana bajaj [:bajaj]]

(In reply to Eric Chou [:ericchou] [:echou] from comment #26)
> Talked to our EPM Joe yesterday and he told me that we don't do leo triage
> anymore. So if we'd like to apply this fix to b2g18 or b2g26 we should find
> someone to give the approval. According to bug 932543 I think Bhavana may be
> the right person to look for, so ni? Bhavana.
> 
> Hi Bhavana,
> 
> This is another security issue we should fix in previous version of Gecko
> just like what we've done to bug 932543. Would you mind assisting with the
> uplift? In fact we still have another similar one (bug 932496), and I think
> we should treat these two issues with the same strategy.

Thanks Eric. This looks good to be uplifted on impacted branches, NI :RyanVM to help here.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/63f68740f111
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/68aba07ca0f8
https://hg.mozilla.org/releases/mozilla-b2g18/rev/7d34eb8b9fe8

Comment 29

[Ryan VanderMeulen [:RyanVM]]

I had to back this out from b2g26 for build bustage.
https://tbpl.mozilla.org/php/getParsedLog.php?id=32838853&tree=Mozilla-B2g26-v1.2

I also hit build bustage on b2g18, but was able to land a fixed version with s/mFileName/sFileName.
https://hg.mozilla.org/releases/mozilla-b2g18/rev/72322f5602a6
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/72322f5602a6

Comment 30

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [koi] Fix the buffer overflows of OBEX packet. , r=echou (v2)

Hi Ryan,
I am sorry for the mistake.
I've uploaded a new patch for b2g-v1.2.
Thank you for your help.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Looks like you had a few extra hunks in this patch, but I just left them out. Thanks :)

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/7351d473030b

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Still busted (same as before). Did you attach the right patch? Backed out.
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/87758b0e695f

Comment 33

[Jamin Liu [:jaliu][:Jamin]]

Attachment: [koi] Fix the buffer overflows of OBEX packet. , r=echou (v3)

Hi Ryan,
I apologize for uploading the wrong patch.
Sorry for any inconvenience caused
I've uploaded a new patch and just built it on b2g-v1.2.
Thank you for your help.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/aea6bd0b69af