Closed
Bug 1031647
Opened 10 years ago
Closed 10 years ago
Bug 864931 causes possible use-after-free
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
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)
1.20 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
1.34 KB,
patch
|
fabrice
:
approval-mozilla-b2g34+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8447603 -
Attachment is obsolete: true
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
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|?
Assignee | ||
Comment 5•10 years ago
|
||
If string sets NetworkUtils::onNetdMessage(NetdCommand* aCommand). join() checks length ,so it isn't over BUF_SIZE.
Assignee | ||
Comment 6•10 years ago
|
||
s/If string/mResultReason/
Comment 7•10 years ago
|
||
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
Comment 8•10 years ago
|
||
(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.
Comment 9•10 years ago
|
||
(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?
Updated•10 years ago
|
blocking-b2g: --- → 1.3?
tracking-b2g-v1.3:
--- → ?
Comment 10•10 years ago
|
||
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)
Assignee | ||
Comment 11•10 years ago
|
||
(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.
Comment 12•10 years ago
|
||
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
Comment 13•10 years ago
|
||
(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)
Assignee | ||
Updated•10 years ago
|
Attachment #8447604 -
Flags: review?(bent.mozilla)
Flags: needinfo?(m_kato)
Assignee | ||
Comment 15•10 years ago
|
||
(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
Updated•10 years ago
|
Attachment #8447604 -
Flags: review?(bent.mozilla) → review+
Comment 16•10 years ago
|
||
Can we land this?
Comment 17•10 years ago
|
||
Could this fix 1082865?
Assignee | ||
Comment 18•10 years ago
|
||
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.
Assignee | ||
Comment 19•10 years ago
|
||
backed out https://hg.mozilla.org/integration/mozilla-inbound/rev/9d7c9f78230b This landing has typo. after run try server, I land it again.
Assignee | ||
Comment 20•10 years ago
|
||
reland https://hg.mozilla.org/integration/mozilla-inbound/rev/a0e6c0f2b828
Comment 21•10 years ago
|
||
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
status-b2g-v1.4:
--- → affected
status-b2g-v2.0:
--- → affected
status-b2g-v2.0M:
--- → affected
status-b2g-v2.1:
--- → affected
status-b2g-v2.2:
--- → fixed
tracking-b2g-v1.3:
? → ---
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 22•10 years ago
|
||
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? :)
Flags: needinfo?(m_kato)
Assignee | ||
Comment 23•10 years ago
|
||
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?
Updated•10 years ago
|
Attachment #8506667 -
Attachment description: for mozilla-b2g32_v2_1 → for mozilla-b2g34_v2_1
Updated•10 years ago
|
Attachment #8506667 -
Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Comment 24•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/3fec5b285061
Updated•10 years ago
|
status-firefox-esr31:
--- → unaffected
Updated•9 years ago
|
Whiteboard: [adv-main36-]
Updated•9 years ago
|
blocking-b2g: backlog → ---
tracking-b2g:
--- → backlog
Updated•9 years ago
|
Whiteboard: [adv-main36-] → [adv-main36-][b2g-adv-main2.2-]
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•8 years ago
|
Group: core-security-release
Updated•7 years ago
|
Keywords: csectype-uaf
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•