Closed Bug 1031647 Opened 10 years ago Closed 10 years ago

Bug 864931 causes possible use-after-free

Categories

(Core :: DOM: Core & HTML, defect)

30 Branch
All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
tracking-b2g backlog
Tracking Status
firefox34 --- wontfix
firefox35 --- wontfix
firefox36 --- fixed
firefox-esr31 --- unaffected
b2g-v1.4 --- wontfix
b2g-v2.0 --- wontfix
b2g-v2.0M --- wontfix
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: m_kato, Assigned: m_kato)

References

Details

(Keywords: csectype-uaf, regression, sec-low, Whiteboard: [adv-main36-][b2g-adv-main2.2-])

Attachments

(2 files, 1 obsolete file)

This is Gonk only.

This code is dangerous.

const char* reason = NS_ConvertUTF16toUTF8(aResult.mResultReason).get();
memcpy(buf, reason, strlen(reason)); 

This means,

{
  NS_ConvertUTF16toUTF8 utf8Reason(aResult.mResultReason);
  const char* reason = utf8Reason.get();
}
memcpy(buf, reason, strlen(reason)); 

So when using reason value, it is already freed.
Attached patch correct way to use string API (obsolete) — Splinter Review
Attachment #8447603 - Attachment is obsolete: true
Set product and component as same as original bug.
Component: DOM → General
Product: Core → Firefox OS
What guarantees that the length of |reason.Lenth() + 1| is less than or equal to |BUF_SIZE|?
If string sets NetworkUtils::onNetdMessage(NetdCommand* aCommand).  join() checks length ,so it isn't over BUF_SIZE.
s/If string/mResultReason/
Probably a blocker for 1.4 (reads off as a sec-critical). Paul - Can you confirm?
Component: General → DOM
Flags: needinfo?(ptheriault)
Product: Firefox OS → Core
Version: unspecified → 30 Branch
(In reply to Makoto Kato (:m_kato) from comment #3)
> Set product and component as same as original bug.

General isn't a good spot to put a bug in - it's a black hole. I'd keep it in DOM.
(In reply to Makoto Kato (:m_kato) from comment #5)
> If string sets NetworkUtils::onNetdMessage(NetdCommand* aCommand).  join()
> checks length ,so it isn't over BUF_SIZE.

Famous last words...  :-)
It wouldn't hurt to program defensively here.

Looking at join():
http://hg.mozilla.org/mozilla-central/annotate/afa67a2f7905/dom/system/gonk/NetworkUtils.cpp#l297
if the first CHECK_LENGTH bails out because the string was too long
then you will have random data in 'buf' here:
http://hg.mozilla.org/mozilla-central/annotate/afa67a2f7905/dom/system/gonk/NetworkUtils.cpp#l1203

Also, strtok() is used in a number of places in this file.
It's not thread-safe, isn't that a requirement for networking code?
blocking-b2g: --- → 1.3?
Use-after-free is usually dangerous enough to warrant blocking but networking APIs (NetworkStats and WifiManager are the main consumers I can think of) are certified. It looks to me like vulnerable function (postTetherInterfaceList) is only called as part of command chains related to when wifi or usb is disabled and this functionality is restricted to certified apps. (could maybe be triggered by other events though such as unplugging the device?)

Can anyone else comment who is more familiar with this code comment to the difficulty of exploitation here?
Flags: needinfo?(ptheriault)
(In reply to Paul Theriault [:pauljt] from comment #10)
> Use-after-free is usually dangerous enough to warrant blocking but
> networking APIs (NetworkStats and WifiManager are the main consumers I can
> think of) are certified. It looks to me like vulnerable function
> (postTetherInterfaceList) is only called as part of command chains related
> to when wifi or usb is disabled and this functionality is restricted to
> certified apps. (could maybe be triggered by other events though such as
> unplugging the device?)
> 
> Can anyone else comment who is more familiar with this code comment to the
> difficulty of exploitation here?

It is too difficult to create exploitation code by this because this will depend on netd daemon.  This needs user have to control netd result.  (It requires root permission to create new network device.)


(In reply to Mats Palmgren (:mats) from comment #9)
> Also, strtok() is used in a number of places in this file.
> It's not thread-safe, isn't that a requirement for networking code?

Is this network worker always run 1 thread only?   I think we should use FindCharInReadable instead.
UAF is generally bad, but in this case the use is the very next time which doesn't give much opportunity for an attacker to reallocate the space.
Keywords: regression, sec-low
(In reply to Daniel Veditz [:dveditz] from comment #12)
> UAF is generally bad, but in this case the use is the very next time which
> doesn't give much opportunity for an attacker to reallocate the space.

In that case, this isn't a blocker if it's deemed as sec-low.
blocking-b2g: 1.3? → backlog
Can we get this patch reviewed?
Flags: needinfo?(m_kato)
Attachment #8447604 - Flags: review?(bent.mozilla)
Flags: needinfo?(m_kato)
(In reply to Mats Palmgren (:mats) from comment #9)
> Also, strtok() is used in a number of places in this file.
> It's not thread-safe, isn't that a requirement for networking code?

file as bug 1061606
Attachment #8447604 - Flags: review?(bent.mozilla) → review+
Can we land this?
Could this fix 1082865?
https://hg.mozilla.org/integration/mozilla-inbound/rev/118d4c56c3c4

(In reply to Gregor Wagner [:gwagner] from comment #17)
> Could this fix 1082865?

I have no permission for that bug.
backed out
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d7c9f78230b

This landing has typo.  after run try server, I land it again.
https://hg.mozilla.org/mozilla-central/rev/a0e6c0f2b828

Not sure how far back we want to backport this.
Assignee: nobody → m_kato
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Spoke to bajaj on IRC about this. Given where we are in the v1.4/v2.0 release cycles, we aren't going to take a sec-low without good reason. However, this should still land on v2.1. Makoto, can you please request b2g34 approval on this patch when you get a chance? :)
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 864931

User impact if declined: 
crash b2g process due to use-after-free or buffer overrun.

Testing completed: 
Risk to taking this patch (and alternatives if risky): 
too low.  remove possible use-after free code and add null terminate.

String or UUID changes made by this patch:
no
Flags: needinfo?(m_kato)
Attachment #8506667 - Flags: approval-mozilla-b2g34?
Attachment #8506667 - Attachment description: for mozilla-b2g32_v2_1 → for mozilla-b2g34_v2_1
Attachment #8506667 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Whiteboard: [adv-main36-]
blocking-b2g: backlog → ---
Whiteboard: [adv-main36-] → [adv-main36-][b2g-adv-main2.2-]
Group: core-security → core-security-release
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.