Closed Bug 1454448 Opened 2 years ago Closed 2 years ago

Free of static memory in ExecuteServiceCommand()

Categories

(Toolkit :: Application Update, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: mozillabugs, Assigned: mhowell)

Details

(Keywords: sec-low, Whiteboard: [adv-main61+][post-critsmash-triage])

Attachments

(1 file)

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: }
Flags: sec-bounty?
Flags: needinfo?(mhowell)
The maintenance service is a separate process that only processes signed update files so not too worried about this one.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: sec-low
Assignee: nobody → mhowell
Status: NEW → ASSIGNED
Flags: needinfo?(mhowell)
Attached patch bug1454448.diffSplinter Review
Attachment #8969668 - Flags: review?(robert.strong.bugs)
Attachment #8969668 - Flags: review?(robert.strong.bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/983b15929e04
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Flags: sec-bounty? → sec-bounty+
Sounds like this can ride the trains.
Group: toolkit-core-security → core-security-release
Whiteboard: [adv-main61+]
Flags: qe-verify-
Whiteboard: [adv-main61+] → [adv-main61+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.