Closed
Bug 1394331
Opened 7 years ago
Closed 7 years ago
Missing return value check of mozilla::Vector::reserve() could lead to memory corruption
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | unaffected |
firefox57 | --- | fixed |
People
(Reporter: tedd, Assigned: nika)
References
Details
(Keywords: regression)
Attachments
(1 file, 1 obsolete file)
2.81 KB,
patch
|
Details | Diff | Splinter Review |
The ParamTraits<> specialization of mozilla::HangStack is missing a return value check after the call to reserve() [1], combined with for looping following immediately after, this could lead to memory corruption due to insufficient memory alloction. If |length| is specified as for example -1, mozilla::Vector::reserve() is likely to fail here [2] meaning the reserved memory for the vector stays the same. However, the for loop operates on unsigned integer leading to an iteration of 0 to size_t max. Calls to infallibleAppend() [3] rely on a successful call to reserve(). I came across this while checking the diff between my local branch and upstream for changes to ParamTraits<>, I don't know where this is used, but if the deserialization happens in the parent, a compromised child process could potentially leverage as a privilege escalation vector. [1] http://searchfox.org/mozilla-central/rev/18c16ebf818abb86805ce08a6e537e4cd826f044/toolkit/components/backgroundhangmonitor/HangStack.cpp#253-254 [2] http://searchfox.org/mozilla-central/rev/18c16ebf818abb86805ce08a6e537e4cd826f044/mfbt/Vector.h#1085 [3] http://searchfox.org/mozilla-central/rev/18c16ebf818abb86805ce08a6e537e4cd826f044/mfbt/Vector.h#707-715
Updated•7 years ago
|
Blocks: 1367406
Group: core-security → dom-core-security
Component: General → XPCOM
Flags: needinfo?(michael)
Keywords: regression
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(michael)
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8901959 -
Flags: review?(nfroyd)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → michael
Comment 2•7 years ago
|
||
Comment on attachment 8901959 [details] [diff] [review] Check allocation of hang stack vector Review of attachment 8901959 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the below fixed. ::: toolkit/components/backgroundhangmonitor/HangStack.cpp @@ +256,5 @@ > if (!ReadParam(aMsg, aIter, &length)) { > return false; > } > > + if (!aResult->reserve(length)) { reserve() and any other functions in HangStack should be made MOZ_MUST_USE.
Attachment #8901959 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 3•7 years ago
|
||
Added MOZ_MUST_USE annotations
Attachment #8901964 -
Flags: review?(nfroyd)
Assignee | ||
Comment 4•7 years ago
|
||
Comment on attachment 8901964 [details] [diff] [review] Check allocation of hang stack vector oops - just carrying over the flag. [Security approval request comment] How easily could an exploit be constructed based on the patch? Other vulnerability (takeover of content process) would be required. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Yes, the security problem is pretty obvious by looking at the patch. Which older supported branches are affected by this flaw? None, only nightly If not all supported branches, which bug introduced the flaw? bug 1367406 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? N/A How likely is this patch to cause regressions; how much testing does it need? Very unlikely, no testing.
Attachment #8901964 -
Flags: review?(nfroyd) → sec-approval?
Comment 5•7 years ago
|
||
Security issues that only affect Nightly don't need sec-approval.
Assignee | ||
Comment 6•7 years ago
|
||
Comment on attachment 8901964 [details] [diff] [review] Check allocation of hang stack vector (In reply to Andrew McCreight [:mccr8] from comment #5) > Security issues that only affect Nightly don't need sec-approval. Oops - didn't realize that - thanks.
Attachment #8901964 -
Flags: sec-approval?
Updated•7 years ago
|
status-firefox55:
--- → unaffected
status-firefox56:
--- → unaffected
status-firefox57:
--- → affected
status-firefox-esr52:
--- → unaffected
Version: unspecified → Trunk
https://hg.mozilla.org/mozilla-central/rev/2c46386c1b19
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Updated•7 years ago
|
Attachment #8901959 -
Attachment is obsolete: true
Updated•7 years ago
|
Group: dom-core-security → core-security-release
Updated•6 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•