BluetoothOppManager.cpp heap buffer overflow

RESOLVED FIXED in Firefox 28

Status

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: rfletcher, Assigned: echou)

Tracking

({csectype-bounds, sec-critical})

unspecified
1.3 Sprint 5 - 11/22
csectype-bounds, sec-critical
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(blocking-b2g:koi+, firefox26 disabled, firefox27 disabled, firefox28+ fixed, firefox-esr17 unaffected, firefox-esr24 unaffected, b2g18 fixed, b2g-v1.1hd fixed, b2g-v1.2 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
BluetoothOppManager.cpp: [1]
memcpy(mReceivedDataBuffer.get() + mReceivedDataBufferOffset,
       &aMessage->mData[headerStartIndex],
       receivedLength - headerStartIndex);

mReceivedDataBuffer is a heap buffer of size packetLength, a device controlled
2-byte value.  Initially, mReceivedDataBufferOffset will be 0.  If
receivedLength-headerStartIndex > packetLength, then the memcpy will overflow
the buffer.

A check to verify receivedLength-headerStartIndex <= packetLength should
suffice.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/BluetoothOppManager.cpp#768
Is this newer code or does it affect 1.1 devices as well?
blocking-b2g: --- → koi?
Keywords: csec-bounds, sec-critical
(Reporter)

Comment 2

5 years ago
:dveditz, it's present in Mozilla Central, so I _think_ that means it affects 1.1, right? If that's not the case, I can look more into it.
(In reply to Rob Fletcher [:omerta] from comment #2)
> :dveditz, it's present in Mozilla Central, so I _think_ that means it
> affects 1.1, right? If that's not the case, I can look more into it.

Not necessarily. Mozilla Central = 1.3.

Can you try testing this on a 1.1 & 1.2 build?
status-b2g18: --- → affected
status-firefox28: --- → affected
tracking-firefox28: --- → +
(Assignee)

Updated

5 years ago
Assignee: nobody → echou
(Assignee)

Comment 4

5 years ago
Created attachment 830708 [details] [diff] [review]
patch 1: v1: Check data size before memcpy in BluetoothOppManager

receivedLength should be always less than mPacketLeftLength, except the last piece of data which should be equal to mPacketLeftLength. To prevent from memory pollution, we should check

  mPacketLeftLength < receivedLength

before memcpy.
Attachment #830708 - Flags: review?(gyeh)
Attachment #830708 - Flags: feedback?(rfletcher)
Attachment #830708 - Flags: feedback?
(Assignee)

Updated

5 years ago
Attachment #830708 - Flags: feedback? → feedback?(btian)
Comment on attachment 830708 [details] [diff] [review]
patch 1: v1: Check data size before memcpy in BluetoothOppManager

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

f=me with nits addressed. Also we should remove variable packetLength in ClientDataHandler() as it's not used at all.

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +818,5 @@
>         */
>        mPutFinalFlag = (opCode == ObexRequestCode::PutFinal);
>      }
>  
> +    // To prevent from memory pollution

Please explain in comment that (mPacketLeftLength >= receivedLength) implies (mPacketLeftLength >= receivedLength - headerStartIndex) so that memory pollution in memcpy below is prevented.

@@ +820,5 @@
>      }
>  
> +    // To prevent from memory pollution
> +    if (mPacketLeftLength < receivedLength) {
> +      BT_LOGR("%s: Received packet size is larger than expected", __FUNCTION__);

Should print BT_WARNING instead of BT_LOGR.
Attachment #830708 - Flags: feedback?(btian) → feedback+
Comment on attachment 830708 [details] [diff] [review]
patch 1: v1: Check data size before memcpy in BluetoothOppManager

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

Per offline discussion, I see your point. However, it's a bit hard to understand the logic. Eric, can you try some other ways?
(Assignee)

Comment 7

5 years ago
(In reply to Ben Tian [:btian] from comment #5)
> Comment on attachment 830708 [details] [diff] [review]
> patch 1: v1: Check data size before memcpy in BluetoothOppManager
> 
> Review of attachment 830708 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> f=me with nits addressed. Also we should remove variable packetLength in
> ClientDataHandler() as it's not used at all.
> 
> ::: dom/bluetooth/BluetoothOppManager.cpp
> @@ +818,5 @@
> >         */
> >        mPutFinalFlag = (opCode == ObexRequestCode::PutFinal);
> >      }
> >  
> > +    // To prevent from memory pollution
> 
> Please explain in comment that (mPacketLeftLength >= receivedLength) implies
> (mPacketLeftLength >= receivedLength - headerStartIndex) so that memory
> pollution in memcpy below is prevented.
> 

I've re-written the logic around this part.

> @@ +820,5 @@
> >      }
> >  
> > +    // To prevent from memory pollution
> > +    if (mPacketLeftLength < receivedLength) {
> > +      BT_LOGR("%s: Received packet size is larger than expected", __FUNCTION__);
> 
> Should print BT_WARNING instead of BT_LOGR.

ok.
(Assignee)

Comment 8

5 years ago
Created attachment 831358 [details] [diff] [review]
patch 1: v2: Check data size before memcpy in BluetoothOppManager

* Patch updated according to comments from Gina and Ben. It's more readable.
* The usage of mPacketLeftLength has been modified.
Attachment #830708 - Attachment is obsolete: true
Attachment #830708 - Flags: review?(gyeh)
Attachment #830708 - Flags: feedback?(rfletcher)
Attachment #831358 - Flags: review?(gyeh)
Attachment #831358 - Flags: feedback?(rfletcher)
Attachment #831358 - Flags: feedback?(btian)
Comment on attachment 831358 [details] [diff] [review]
patch 1: v2: Check data size before memcpy in BluetoothOppManager

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

It's much easier to understand. Good job. Just a few nits and suggestions to you.

Seem like |mReceivedDataBufferOffset| and |mPacketLeftLength| are paired, and the sum of them should be a constant for a single packet, am I right? In my humble opinion, could we give them new names to make it clearer?

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +794,5 @@
>      AfterOppDisconnected();
>      FileTransferComplete();
>    } else if (opCode == ObexRequestCode::Put ||
>               opCode == ObexRequestCode::PutFinal) {
> +    const int cFrameHeaderLength = 3;

How about using #define?

@@ +795,5 @@
>      FileTransferComplete();
>    } else if (opCode == ObexRequestCode::Put ||
>               opCode == ObexRequestCode::PutFinal) {
> +    const int cFrameHeaderLength = 3;
> +    uint16_t receivedBodyLength = receivedLength;

Instead of writing two memcpy below, I would recommend to add a local variable.

int startReadPos = 0;

@@ +806,2 @@
>  
> +      int packetLength = (((int)aMessage->mData[1]) << 8) | aMessage->mData[2];

startReadPos = cFrameHeaderLength;

@@ +833,5 @@
> +             &aMessage->mData[cFrameHeaderLength], receivedBodyLength);
> +    } else {
> +      memcpy(mReceivedDataBuffer.get() + mReceivedDataBufferOffset,
> +             &aMessage->mData[0], receivedBodyLength);
> +    }

memcpy(mReceivedDataBuffer.get(),
       &aMessage->mData[startPos], receivedBodyLength);
Attachment #831358 - Flags: review?(gyeh) → review+
(Assignee)

Comment 10

5 years ago
Created attachment 832156 [details] [diff] [review]
patch 1: v3: Check data size before memcpy in BluetoothOppManager

* Revised by Ben and Gina's suggestion.
Attachment #831358 - Attachment is obsolete: true
Attachment #831358 - Flags: feedback?(rfletcher)
Attachment #831358 - Flags: feedback?(btian)
Attachment #832156 - Flags: review?(gyeh)
Attachment #832156 - Flags: feedback?(btian)
Comment on attachment 832156 [details] [diff] [review]
patch 1: v3: Check data size before memcpy in BluetoothOppManager

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

f=me with nits addressed. Thanks.

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +761,5 @@
> +                      frameHeaderLength;
> +    /**
> +     * A PUT request from remote devices may be divided into multiple parts.
> +     * In other words, one request may need to be received multiple times,
> +     * so here we keep a variable mPacketLeftLength to indicate if current

Please revise the comment as mPacketLeftLength no longer exists.

@@ +842,4 @@
>      FileTransferComplete();
>    } else if (opCode == ObexRequestCode::Put ||
>               opCode == ObexRequestCode::PutFinal) {
> +    NS_ENSURE_TRUE_VOID(ComposePacket(opCode, aMessage) > 0);

No need to '> 0' as the function is boolean.
Attachment #832156 - Flags: feedback?(btian) → feedback+
(Assignee)

Comment 12

5 years ago
(In reply to Ben Tian [:btian] from comment #11)
> Comment on attachment 832156 [details] [diff] [review]
> patch 1: v3: Check data size before memcpy in BluetoothOppManager
> 
> Review of attachment 832156 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> f=me with nits addressed. Thanks.
> 
> ::: dom/bluetooth/BluetoothOppManager.cpp
> @@ +761,5 @@
> > +                      frameHeaderLength;
> > +    /**
> > +     * A PUT request from remote devices may be divided into multiple parts.
> > +     * In other words, one request may need to be received multiple times,
> > +     * so here we keep a variable mPacketLeftLength to indicate if current
> 
> Please revise the comment as mPacketLeftLength no longer exists.
> 
> @@ +842,4 @@
> >      FileTransferComplete();
> >    } else if (opCode == ObexRequestCode::Put ||
> >               opCode == ObexRequestCode::PutFinal) {
> > +    NS_ENSURE_TRUE_VOID(ComposePacket(opCode, aMessage) > 0);
> 
> No need to '> 0' as the function is boolean.

All suggestions would be taken. Thanks.
(Assignee)

Updated

5 years ago
Attachment #832156 - Flags: feedback?(rfletcher)
As per the logic in 932490, I assume the exploit scenario here requires a victim to be paired with a malicious bluetooth device. Given how rare that case would be, I think this is not a blocker for koi.
blocking-b2g: koi? → ---
Rob: did you check to see whether this really did affect 1.1 (the b2g18 branch)?
status-b2g-v1.2: --- → affected
status-firefox26: --- → disabled
status-firefox27: --- → disabled
status-firefox28: affected → disabled
status-firefox-esr17: --- → unaffected
status-firefox-esr24: --- → unaffected
Flags: needinfo?(rfletcher)
(Reporter)

Comment 15

5 years ago
:echou

Depending on the size of aMessage->mSize, dataLength can become negative.  If
dataLength becomes negative, the `if` check becomes insufficient. We should
also check to see if dataLength is less than 0.

After that, LGTM! Thanks for the fix.
Comment on attachment 832156 [details] [diff] [review]
patch 1: v3: Check data size before memcpy in BluetoothOppManager

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

The patch is getting better and better. ;) Please check my comments below.

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +842,4 @@
>      FileTransferComplete();
>    } else if (opCode == ObexRequestCode::Put ||
>               opCode == ObexRequestCode::PutFinal) {
> +    NS_ENSURE_TRUE_VOID(ComposePacket(opCode, aMessage) > 0);

A little bit worried about using NS_ENSURE_TRUE_VOID in this case. When we receive files, a great number of logs are produced by NS_WARNING in debug build.

On the other hand, the function name |ComposePacket| is a bit confusing. For me, it seems like a failure/error is occurred when it returns false. However, it could also mean that we're still waiting for the following parts of the packet. I suggest to write some comments in header file or rename the function.
Attachment #832156 - Flags: review?(gyeh) → review+
(Assignee)

Comment 17

5 years ago
Hi omerta, 

(In reply to Rob Fletcher [:omerta] from comment #15)
> :echou
> 
> Depending on the size of aMessage->mSize, dataLength can become negative.  If
> dataLength becomes negative, the `if` check becomes insufficient. We should
> also check to see if dataLength is less than 0.
>
> After that, LGTM! Thanks for the fix.

Thanks for your comments! I was about to send you a mail to inform you that I need your feedback because I didn't notice you've left a comment already. :)
Flags: needinfo?(rfletcher)
(Assignee)

Comment 18

5 years ago
(In reply to Gina Yeh [:gyeh] [:ginayeh] (PTO: 11/21 ~ 11/26) from comment #16)
> Comment on attachment 832156 [details] [diff] [review]
> patch 1: v3: Check data size before memcpy in BluetoothOppManager
> 
> Review of attachment 832156 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patch is getting better and better. ;) Please check my comments below.
> 
> ::: dom/bluetooth/BluetoothOppManager.cpp
> @@ +842,4 @@
> >      FileTransferComplete();
> >    } else if (opCode == ObexRequestCode::Put ||
> >               opCode == ObexRequestCode::PutFinal) {
> > +    NS_ENSURE_TRUE_VOID(ComposePacket(opCode, aMessage) > 0);
> 
> A little bit worried about using NS_ENSURE_TRUE_VOID in this case. When we
> receive files, a great number of logs are produced by NS_WARNING in debug
> build.

You're right. Should use normal if-statement in this case.

> 
> On the other hand, the function name |ComposePacket| is a bit confusing. For
> me, it seems like a failure/error is occurred when it returns false.
> However, it could also mean that we're still waiting for the following parts
> of the packet. I suggest to write some comments in header file or rename the
> function.

Great suggestion. Thanks.
landed on central -> https://hg.mozilla.org/mozilla-central/rev/05ca2beb39ce
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Can we get the appropriate noms for uplift?
status-firefox28: disabled → fixed
Target Milestone: --- → 1.3 Sprint 5 - 11/22
Flags: needinfo?(echou)
(Assignee)

Updated

5 years ago
Attachment #832156 - Flags: feedback?(rfletcher)
(Assignee)

Comment 22

5 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-5][Back 2-Dec] from comment #21)
> Can we get the appropriate noms for uplift?

Sure. I'll prepare patch for v1.2 and b2g18 and request for uplift.
Flags: needinfo?(echou)
(Assignee)

Comment 23

5 years ago
Created attachment 8339731 [details] [diff] [review]
patch 1: patch for v1.2, r=gyeh

Actually I don't know how to request for approval-v1.2. There is no such flag on the patch attachment page. Should I nominate as koi+?
(Assignee)

Comment 24

5 years ago
Worth to give it a try :-)
blocking-b2g: --- → koi?
(In reply to Eric Chou [:ericchou] [:echou] from comment #24)
> Worth to give it a try :-)

Use approval b2g26 for this.
blocking-b2g: koi? → ---
(Assignee)

Comment 26

5 years ago
(In reply to Jason Smith [:jsmith] from comment #25)
> (In reply to Eric Chou [:ericchou] [:echou] from comment #24)
> > Worth to give it a try :-)
> 
> Use approval b2g26 for this.

Ah, thanks.
(Assignee)

Comment 27

5 years ago
Comment on attachment 8339731 [details] [diff] [review]
patch 1: patch for v1.2, r=gyeh

NOTE: This flag is now for security issues only. 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 #): No specific bug. This issue should have existed from the beginning.
User impact if declined: no user impact. Security issue.
Testing completed: m-c and manual testing by transferring files.
Risk to taking this patch (and alternatives if risky): Fairly low. Just added length-checking to prevent from potential memory pollution. Code logic should not be affected.
String or UUID changes made by this patch: no
Attachment #8339731 - Flags: approval-mozilla-b2g26?
(In reply to Eric Chou [:ericchou] [:echou] from comment #27)
> Comment on attachment 8339731 [details] [diff] [review]
> patch 1: patch for v1.2, r=gyeh
> 
> NOTE: This flag is now for security issues only. 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 #): No specific bug. This issue should
> have existed from the beginning.
> User impact if declined: no user impact. Security issue.
> Testing completed: m-c and manual testing by transferring files.
> Risk to taking this patch (and alternatives if risky): Fairly low. Just
> added length-checking to prevent from potential memory pollution. Code logic
> should not be affected.
> String or UUID changes made by this patch: no

I am rather koi+'ing this bug as that is the right thing to do here. We do not want to have approvals in the last six weeks of convergence. Also clarified this with Jason in person.
blocking-b2g: --- → koi+
Attachment #8339731 - Flags: approval-mozilla-b2g26?
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/27aba4a73084

Looks like we need this on b2g18 as well?
status-b2g-v1.1hd: --- → affected
status-b2g-v1.2: affected → fixed
Flags: needinfo?(bbajaj)
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #29)
> https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/27aba4a73084
> 
> Looks like we need this on b2g18 as well?

Yes.Thanks for that Ryan ! a=bajaj if the same patch applies which may be the case per comment #22.
Flags: needinfo?(bbajaj)
(Assignee)

Comment 32

5 years ago
Oops. Thanks, Ryan.
Group: core-security
You need to log in before you can comment on or make changes to this bug.