Closed Bug 1510494 (CVE-2019-17009) Opened 2 years ago Closed 1 year ago

Write log files and update.status file to restricted directory and copy it to the final location after the update finishes

Categories

(Toolkit :: Application Update, defect, P1)

59 Branch
All
Windows
defect

Tracking

()

VERIFIED FIXED
mozilla71
Tracking Status
firefox-esr60 --- wontfix
firefox-esr68 71+ verified
firefox69 --- wontfix
firefox70 + wontfix
firefox71 + verified
firefox72 + verified

People

(Reporter: robert.strong.bugs, Assigned: robert.strong.bugs)

References

Details

(Keywords: sec-moderate, Whiteboard: [fixes bug 1486637/1481907#c19][post-critsmash-triage][adv-main71+][adv-esr68.3+])

Attachments

(7 files, 1 obsolete file)

To lessen the attack surface when writing files in unrestricted locations such as the log file and the update.status file the updater should write these files to a restricted location and then copy them to the final location.

*edit* s/then copy them to the final location/the unelevated updater should copy them to the update directory/
Priority: -- → P2
Keywords: sec-want
Group: toolkit-core-security → firefox-core-security
Priority: P2 → P1
Assignee: nobody → robert.strong.bugs
Keywords: sec-wantsec-moderate
Whiteboard: [fixes bug 1486637/1481907#c19]
Type: enhancement → defect
Status: NEW → ASSIGNED
OS: Unspecified → Windows
Hardware: Unspecified → All

[Tracking Requested - why for this release]:
Long standing security bug.

Attachment #9094251 - Attachment description: Bug 1510494 - write elevated updater log and status files to a new directory in the Maintenance Service directory. r=agashlin,mhowell → Bug 1510494 - write elevated updater log and status file to a new directory in the Maintenance Service directory. r=agashlin,mhowell
Attachment #9094251 - Attachment description: Bug 1510494 - write elevated updater log and status file to a new directory in the Maintenance Service directory. r=agashlin,mhowell → Bug 1510494 - write elevated updater log and status files to a new directory in the Maintenance Service directory. r=agashlin,mhowell

Comment on attachment 9094251 [details]
Bug 1510494 - write elevated updater log and status files to a new directory in the Maintenance Service directory. r=agashlin,mhowell

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Not easily. Though it references secure output path it doesn't point out how the old path could be exploited.
  • Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
  • Which older supported branches are affected by this flaw?: All
  • If not all supported branches, which bug introduced the flaw?: None
  • Do you have backports for the affected branches?: No
  • If not, how different, hard to create, and risky will they be?: Easy
  • How likely is this patch to cause regressions; how much testing does it need?: Moderate. I think having this on Nightly for a couple of days will provide the largest benefit. It would be good if QA can verify that the POC from bug 1481907 comment #19
Attachment #9094251 - Flags: sec-approval?

Comment on attachment 9094251 [details]
Bug 1510494 - write elevated updater log and status files to a new directory in the Maintenance Service directory. r=agashlin,mhowell

sec-approval+, a=dveditz

Attachment #9094251 - Flags: sec-approval? → sec-approval+

Backed out for causing xpcshell test failures in toolkit/mozapps/update/tests/unit_service_updater/marStageSuccessCompleteSvc.js on Windows 7 debug:

https://hg.mozilla.org/integration/autoland/rev/6b9cf21a84bcde09d3dc46760784fc95b55818b8

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Csuperseded%2Cretry%2Cusercancel&revision=27df5399e032451d3bdebcd86155ec69da504c99
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=269733547&repo=autoland

[task 2019-10-04T07:16:56.144Z] 07:16:56 INFO - TEST-PASS | toolkit/mozapps/update/tests/unit_service_updater/marStageSuccessCompleteSvc.js | runUpdate - [runUpdate : 1974] the file or directory should exist, path: Z:\task_1570168904\build\tests\xpcshell\tests\toolkit\mozapps\update\tests\unit_service_updater\marStageSuccessCompleteSvc\dir.app\callback_app.exe - true == true
[task 2019-10-04T07:16:56.145Z] 07:16:56 INFO - "07:16:55:017 | TEST-INFO | xpcshellUtilsAUS.js | [runUpdate : 2002] launching the program: Z:\task_1570168904\build\tests\xpcshell\tests\toolkit\mozapps\update\tests\unit_service_updater\marStageSuccessCompleteSvc\dir.app\updater.exe C:\ProgramData\Mozilla\updates\marStageSuccessCompleteSvc\updates\0 Z:\task_1570168904\build\tests\xpcshell\tests\toolkit\mozapps\update\tests\unit_service_updater\marStageSuccessCompleteSvc\dir.app Z:\task_1570168904\build\tests\xpcshell\tests\toolkit\mozapps\update\tests\unit_service_updater\marStageSuccessCompleteSvc\dir.app\updated 0/replace Z:\task_1570168904\build\tests\xpcshell\tests\toolkit\mozapps\update\tests\unit_service_updater\marStageSuccessCompleteSvc\dir.app Z:\task_1570168904\build\tests\xpcshell\tests\toolkit\mozapps\update\tests\unit_service_updater\marStageSuccessCompleteSvc\dir.app\callback_app.exe ./ callback.log Test Arg 2 Test Arg 3"
[task 2019-10-04T07:16:56.145Z] 07:16:56 INFO - "07:16:55:017 | TEST-INFO | xpcshellUtilsAUS.js | [setEnvironment : 4604] setting the XRE_NO_WINDOWS_CRASH_DIALOG environment variable to 1... previously it didn't exist"
[task 2019-10-04T07:16:56.145Z] 07:16:56 INFO - "07:16:55:018 | TEST-INFO | xpcshellUtilsAUS.js | [setEnvironment : 4623] setting the XPCOM_DEBUG_BREAK environment variable to warn... previous value stack-and-abort"
[task 2019-10-04T07:16:56.146Z] 07:16:56 INFO - "07:16:55:018 | TEST-INFO | xpcshellUtilsAUS.js | [setEnvironment : 4638] setting MOZ_NO_SERVICE_FALLBACK environment variable to 1"
[task 2019-10-04T07:16:56.146Z] 07:16:56 INFO - "07:16:55:703 | TEST-INFO | xpcshellUtilsAUS.js | [resetEnvironment : 4664] setting the XPCOM_DEBUG_BREAK environment variable back to stack-and-abort"
[task 2019-10-04T07:16:56.146Z] 07:16:56 INFO - "07:16:55:704 | TEST-INFO | xpcshellUtilsAUS.js | [resetEnvironment : 4675] removing the XRE_NO_WINDOWS_CRASH_DIALOG environment variable"
[task 2019-10-04T07:16:56.146Z] 07:16:56 INFO - "07:16:55:704 | TEST-INFO | xpcshellUtilsAUS.js | [resetEnvironment : 4680] removing MOZ_NO_SERVICE_FALLBACK environment variable"
[task 2019-10-04T07:16:56.147Z] 07:16:56 WARNING - TEST-UNEXPECTED-FAIL | toolkit/mozapps/update/tests/unit_service_updater/marStageSuccessCompleteSvc.js | runUpdate - [runUpdate : 2059] the update status should equal the expected value - "failed: 30" == "succeeded"
[task 2019-10-04T07:16:56.147Z] 07:16:56 INFO - xpcshellUtilsAUS.js:runUpdate:2059
[task 2019-10-04T07:16:56.147Z] 07:16:56 INFO - Z:/task_1570168904/build/tests/xpcshell/tests/toolkit/mozapps/update/tests/unit_service_updater/marStageSuccessCompleteSvc.js:run_test:25
[task 2019-10-04T07:16:56.147Z] 07:16:56 INFO - exiting test
[task 2019-10-04T07:16:56.148Z] 07:16:56 INFO - PID 7492 | JavaScript error: Z:\task_1570168904\build\tests\xpcshell\head.js, line 789: NS_ERROR_ABORT
[task 2019-10-04T07:16:56.148Z] 07:16:56 INFO - "CONSOLE_MESSAGE: (error) [JavaScript Error: "NS_ERROR_ABORT" {file: "Z:\task_1570168904\build\tests\xpcshell\head.js" line: 789}]"
[task 2019-10-04T07:16:56.148Z] 07:16:56 INFO - PID 7492 | *** AUS:SVC UpdateManager:_writeUpdatesToXMLFile - no updates to write. removing file: C:\ProgramData\Mozilla\updates\marStageSuccessCompleteSvc\updates.xml

Flags: needinfo?(robert.strong.bugs)
Flags: needinfo?(robert.strong.bugs)

Can this ride with 71? It is kind of late in 70 so I wonder if there is anything (other than this being a cluster of sec-moderate issues) that makes this urgent to fix in 70.

Flags: needinfo?(robert.strong.bugs)

I'm fine with this riding with 71.

Flags: needinfo?(robert.strong.bugs)
Flags: qe-verify+
Whiteboard: [fixes bug 1486637/1481907#c19] → [fixes bug 1486637/1481907#c19][post-critsmash-triage]
Attached patch Patch for ESR 68Splinter Review

This includes both patches from this bug and the patch from bug 1587841. I'll verify this still applies cleanly and update the patch if necessary when 71 makes it to Beta. Adding needinfo for myself as a reminder.

Flags: needinfo?(robert.strong.bugs)
Attachment #9100513 - Flags: review+

Can you give us some guidance in order to be able to verify this issue, maybe some steps? Thanks

There is a POC in bug 1481907 comment #19 along with details

Flags: needinfo?(robert.strong.bugs)

Thanks, Robert for pointing out bug 1481907 comment #19, I investigate it and I tried to reproduce the initial issue there. Giving the fact that this issue is marked as security, for a better understanding I would prefer to let me know what exactly I need to verify here, how, and what is the expected result.

Flags: needinfo?(robert.strong.bugs)

From bug 1481907 comment #19

The linked patch does not seem to prevent directory junctions, the following PowerShell commands from the original report still create the update.status file in C:\Windows\System32:
New-Item -Type Junction -Force -Path C:\Temp\updates\0 -Value C:\Windows\System32
(Get-Service MozillaMaintenance).Start(@("MozillaMaintenance", "software-update", "updater.exe", "C:\Temp\updates\0", "C:\X", "C:\X"))

Tested with an install of https://archive.mozilla.org/pub/firefox/nightly/2018/08/2018-08-16-10-00-35-mozilla-central/firefox-63.0a1.en-US.win32.installer.exe on Windows 1703 x64.

To back the point I was trying to make in comment #11, please find attached a new PoC that works with this version, and demonstrates the use of OpLock to place object manager symbolic links after the initial checks and the temporary file creation, but before the file is moved.

(Note: the PoC expects the standard install path "C:\Program Files (x86)\Firefox Nightly", or the script can be adjusted accordingly.)

As for a full fix suggestion, it would be safer IMHO to change the location of the status file (e.g. to C:\Program Files\Mozilla Maintenance Service\status) when it's generated by the service/updater, because it directly tackles the root cause / risky behaviour (manipulating user-controlable resources from a privileged process) instead of trying to address individual exploitation techniques (junctions/symlinks) which requires checking for all possible bypasses and corner cases.

Flags: needinfo?(robert.strong.bugs)

Hi Ovidiu, this bug is to specifically fix the exploit demonstrated in bug 1481907 comment #19 which wasn't fixed in bug 1481907. What needs to be verified is that the proof of concept is no longer reproducible.

For context, the patch in bug 1481907 fixed the exploit as originally reported and the author of the exploit came up with a slightly different way for an exploit that wasn't addressed by the patch in bug 1481907. This was addressed by bug 1486637 which had issues and was backed out. The patch in this bug addresses the exploit demonstrated by the proof of concept.

(In reply to ovidiu boca[:Ovidiu] from comment #15)

Thanks, Robert for pointing out bug 1481907 comment #19, I investigate it and I tried to reproduce the initial issue there. Giving the fact that this issue is marked as security, for a better understanding I would prefer to let me know what exactly I need to verify here, how, and what is the expected result.

Are you unable to reproduce the issue?

Robert please look over my results and let me know if this is the expected result.

I took a Nightly build form 2019-10-01(before the fix was uplifted) and run the PowerShell commands from https://bugzilla.mozilla.org/show_bug.cgi?id=1481907#c19, I verified if the update.status file is created in C:\Windows\System32 and I can confirm that the file is created. After that, I installed A Nightly build with the fix (version 2019-10-22) and run again the PowerShell commands, the update.status file is no longer created in the C:\Windows\System32. Please let me know if this is the expected result.

Flags: needinfo?(robert.strong.bugs)

Thank you and that is the expected result.

Flags: needinfo?(robert.strong.bugs)

I verified this issue on Windows 10 x64 with FF Nightly 72.0a1(2019-10-22) and I can confirm the fix.

Is this ready for an ESR68 uplift request?

Flags: needinfo?(robert.strong.bugs)
Depends on: 1596778
Regressions: 1596778

After bug 1596778 is approved for Beta I'll request uplift to ESR for this bug and bug 1596778.

Ryan, does this sound like a good path forward?

Flags: needinfo?(robert.strong.bugs) → needinfo?(ryanvm)

Today was the the final go-to-build for Beta71. The next builds we create will be RCs. Not sure if that's enough bake time?

Flags: needinfo?(ryanvm)

The possible paths forward from my perspective are either bug 1596778 will need to be uplifted to beta or this bug along with the followups to this bug will need to be backed out of beta.

NI Pascal as the 71 release owner and Liz as the 68.3esr owner to get involved in that decision then.

Flags: needinfo?(pascalc)
Flags: needinfo?(lhenry)

(In reply to Robert Strong [:rstrong] (Robert they/them - use needinfo to contact me) from comment #24)

. . . or this bug along with the followups to this bug will need to be backed out of beta.

Which followup patches exactly?

Flags: needinfo?(lhenry) → needinfo?(robert.strong.bugs)

Do we really need to back this out for bug 1596778 (looks like an edge case that QA ran into that may be rare for users to encounter)

I highly suspect that it affects a small number of clients but that these clients could easily become orphaned.

Another option I thought of this morning is to disable update staging on Beta for Windows only for this release. This is only a pref flip, is something we've done in the past, and is extremely safe. This would ride the train from Beta to Release and staging would be re-enabled when the current Nightly that contains the fix for bug 1596778 makes it to Beta and Release.

Flags: needinfo?(robert.strong.bugs) → needinfo?(lhenry)

(In reply to Liz Henry (:lizzard) from comment #26)

(In reply to Robert Strong [:rstrong] (Robert they/them - use needinfo to contact me) from comment #24)

. . . or this bug along with the followups to this bug will need to be backed out of beta.

Which followup patches exactly?

I think just bug 1587841 and I'll verify if there are other bugs if this is the direction that is chosen. Personally, I think disabling staging is the safest path forward,

Note: I won't be around this afternoon so if anything further is needed from me at that time I'll address it on Monday.

Attaching a patch to disable staging on Beta in case it is needed and I'm not around.

(In reply to Robert Strong [slack rstrong] (Robert they/them - use needinfo to contact me) from comment #31)

Created attachment 9110914 [details] [diff] [review]
Patch to disable staging on Beta

Attaching a patch to disable staging on Beta in case it is needed and I'm not around.

That sounds like a good solution I would be OK with for RC, Molly could you request the uplift please? Thanks

Flags: needinfo?(pascalc) → needinfo?(mhowell)

Comment on attachment 9110914 [details] [diff] [review]
Patch to disable staging on Beta

Beta/Release Uplift Approval Request

  • User impact if declined: Potential failures to apply updates under certain system conditions not within our control.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This patch is just a pref flip from one tested configuration to another; we've had the staging pref flipped off for extended periods of time in the past without issue, and this is intended only as a temporary (~ 1 release) solution.
    This also avoids having to back out a security patch.
  • String changes made/needed:
Flags: needinfo?(mhowell)
Attachment #9110914 - Flags: approval-mozilla-beta?
Comment on attachment 9110914 [details] [diff] [review]
Patch to disable staging on Beta

That's a good solution thanks, uplift approved for the beta branch  before RC.
Attachment #9110914 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

Verified on latest Beta 71.0b13 (32-bit) on Windows 10 x64. The pref "app.update.staging.enabled" is disabled by default.
Waiting for potential ESR uplift.

We can either:
a) uplift both patches in this bug to ESR and during the next Beta cycle uplift the patch in bug 1596778 and backout the patch disabling staging on ESR in this bug.
or
b) wait for the next Beta cycle and uplift to ESR the "Patch for ESR 68" patch and the patch in bug 1596778.

Liz, do you have a preference?

Flags: needinfo?(lhenry)

Am I right in understanding that this bug is not "disabled" on Fx71 (beta), the fix for this bug was disabled which means Fx71 is "affected"?

It's caused at least one known regression, we probably shouldn't try to rush the fixes into 71, and then back on to ESR, last minute.

backout the patch disabling staging on ESR

You mean beta, right? Looks like nothing has landed on ESR. But that's option a) and b) seems safer.

OK, rstrong says this is fixed on beta. It's just that staging is disabled to avoid this fix causing a known regression in that case. We should fix ESR 68.3 to match what we're shipping in Fx71.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This patch disables staging on ESR to support this security bug.
User impact if declined: This patch disables staging on ESR to support this security bug.
Fix Landed on Version: 71.
Risk to taking this patch (and alternatives if risky): Minimal, this patch disables staging on ESR to support this security bug.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Attachment #9111381 - Flags: approval-mozilla-esr68?
Comment on attachment 9111381 [details] [diff] [review]
Patch to disable staging on ESR

After discussion with rstrong and dveditz, taking this for today's ESR build.
Flags: needinfo?(lhenry)
Attachment #9111381 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+

Comment on attachment 9100513 [details] [diff] [review]
Patch for ESR 68

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: This patch fixes a security bug that is part of Firefox 71.
  • User impact if declined: The 2 exploits in bug 1481907 won't be fixed.
  • Fix Landed on Version: 71
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This patch has baked on Nightly since 10/8
  • String or UUID changes made by this patch: None
Attachment #9100513 - Flags: approval-mozilla-esr68?
Comment on attachment 9100513 [details] [diff] [review]
Patch for ESR 68

Note: this patch contains the test fix in this bug and the fix from bug 1587841
Attachment #9100513 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+

https://hg.mozilla.org/releases/mozilla-esr68/rev/8f0a5a7d37db
https://hg.mozilla.org/releases/mozilla-esr68/rev/1f8b2df42186

Please make sure we end up with a follow-up patch somewhere which re-enables background staging on ESR68 eventually.

Verified - fixed on latest 68.3.0esr (32-bit) on Windows 10.

  • Verified that "app.update.staging.enabled" is disabled by default as support needed for the security bug patch.
  • Marking Fx71 verified as well based on Comment 36.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+

(In reply to Ryan VanderMeulen [:RyanVM] from comment #44)

https://hg.mozilla.org/releases/mozilla-esr68/rev/8f0a5a7d37db
https://hg.mozilla.org/releases/mozilla-esr68/rev/1f8b2df42186

Please make sure we end up with a follow-up patch somewhere which re-enables background staging on ESR68 eventually.

Since the patch for bug 1596778 is needed to re-enable update staging I submitted an ESR patch in bug 1596778 that includes re-enabling staging.

Whiteboard: [fixes bug 1486637/1481907#c19][post-critsmash-triage] → [fixes bug 1486637/1481907#c19][post-critsmash-triage][adv-main71+]
Whiteboard: [fixes bug 1486637/1481907#c19][post-critsmash-triage][adv-main71+] → [fixes bug 1486637/1481907#c19][post-critsmash-triage][adv-main71+][adv-esr68.3+]

This also fixes bug 1481907 aka CVE-2018-12380 which was never published AFAIK (presumably because the underlying bug was not fixed?) and is still marked "RESERVED". Not sure if it can/should be made an alias of CVE-2019-17009 by MITRE.

Flags: needinfo?(tom)

CVE minutia, redirecting to Dan....

Flags: needinfo?(tom) → needinfo?(dveditz)

(In reply to clavoillotte from comment #49)

This also fixes bug 1481907 aka CVE-2018-12380 which was never published AFAIK (presumably because the underlying bug was not fixed?) and is still marked "RESERVED". Not sure if it can/should be made an alias of CVE-2019-17009 by MITRE.

It looks like neither has been reported to MITRE yet, and we never published the 2018 one because it didn't fix the problem. So yes, we should merge the two CVEs, put whichever one we pick on this bug, and remove the alias from bug 1481907. And since both CVEs are known to at least one non-MoCo person we need to inform MITRE that we "reject" (discard) the other.

Flags: needinfo?(dveditz)

needinfo=>Tom to figure out which CVE he'd like to use since he'll be writing up the advisories.

Flags: needinfo?(tom)

Looks like I never assigned an alias to this... Fixed that and added a few other missing ones.

Let's use CVE-2019-17009 and discard CVE-2018-12380

Alias: CVE-2019-17009
Flags: needinfo?(tom) → needinfo?(dveditz)

I've updated MITRE (sent rejection for -12380)

Flags: needinfo?(dveditz)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.