Closed
Bug 1454448
Opened 6 years ago
Closed 6 years ago
Free of static memory in ExecuteServiceCommand()
Categories
(Toolkit :: Application Update, enhancement)
Toolkit
Application Update
Tracking
()
RESOLVED
FIXED
mozilla61
People
(Reporter: mozillabugs, Assigned: molly)
Details
(Keywords: sec-low, Whiteboard: [adv-main61+][post-critsmash-triage])
Attachments
(1 file)
1.89 KB,
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
ExecuteServiceCommand() (toolkit\components\maintenanceservice\workmonitor.cpp) can attempt to free static memory if it experiences an OOM. The bug is that line 584 doesn't check the status from UuidToString(), which can fail with OOM (see https://msdn.microsoft.com/en-us/library/windows/desktop/aa379352(v=vs.85).aspx ). If that occurs, line 588 unconditionally frees |guidString|, which contains a pointer to static memory: 570: BOOL 571: ExecuteServiceCommand(int argc, LPWSTR *argv) 572: { 573: if (argc < 3) { 574: LOG_WARN(("Not enough command line arguments to execute a service command")); 575: return FALSE; 576: } 577: 578: // The tests work by making sure the log has changed, so we put a 579: // unique ID in the log. 580: RPC_WSTR guidString = RPC_WSTR(L""); 581: GUID guid; 582: HRESULT hr = CoCreateGuid(&guid); 583: if (SUCCEEDED(hr)) { 584: UuidToString(&guid, &guidString); 585: } 586: LOG(("Executing service command %ls, ID: %ls", 587: argv[2], reinterpret_cast<LPCWSTR>(guidString))); 588: RpcStringFree(&guidString); ... 774: }
Updated•6 years ago
|
Flags: sec-bounty?
Updated•6 years ago
|
Flags: needinfo?(mhowell)
Comment 1•6 years ago
|
||
The maintenance service is a separate process that only processes signed update files so not too worried about this one.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → mhowell
Status: NEW → ASSIGNED
Flags: needinfo?(mhowell)
Assignee | ||
Comment 2•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa297a2cf0fadf42be1c3ed7da93dff54df7a43c
Assignee | ||
Comment 3•6 years ago
|
||
Attachment #8969668 -
Flags: review?(robert.strong.bugs)
Updated•6 years ago
|
Attachment #8969668 -
Flags: review?(robert.strong.bugs) → review+
Assignee | ||
Comment 4•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/983b15929e0424b2346df3d39af2d352ed4db0b9 Bug 1454448 - Properly handle an allocation failure. r=rstrong
Comment 5•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/983b15929e04
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•6 years ago
|
Flags: sec-bounty? → sec-bounty+
Comment 6•6 years ago
|
||
Sounds like this can ride the trains.
status-firefox59:
--- → wontfix
status-firefox60:
--- → wontfix
status-firefox-esr52:
--- → wontfix
status-firefox-esr60:
--- → wontfix
Updated•6 years ago
|
Group: toolkit-core-security → core-security-release
Updated•6 years ago
|
Whiteboard: [adv-main61+]
Updated•6 years ago
|
Flags: qe-verify-
Whiteboard: [adv-main61+] → [adv-main61+][post-critsmash-triage]
Updated•5 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•