1510494 - (CVE-2019-17009) Write log files and update.status file to restricted directory and copy it to the final location after the update finishes

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect, P1)

Description

[Robert Strong (they/them - no direct email)]

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/

Comment 1

[Robert Strong (they/them - no direct email)]

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

Comment 2

[Robert Strong (they/them - no direct email)]

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

Comment 3

[Robert Strong (they/them - no direct email)]

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

Comment 4

[Daniel Veditz [:dveditz]]

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

Comment 5

[Robert Strong (they/them - no direct email)]

Pushed to autoland
https://hg.mozilla.org/integration/autoland/rev/27df5399e032451d3bdebcd86155ec69da504c99

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 7

[Robert Strong (they/them - no direct email)]

Attachment: Bug 1510494 - fix crash when running xpcshell tests on Windows 7 debug builds. r=mhowell

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 9

[Robert Strong (they/them - no direct email)]

I'm fine with this riding with 71.

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Thanks!

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/de2cf52f4c1cfa0b91d5375763f43e1c23206a7d
https://hg.mozilla.org/integration/autoland/rev/ec7a084b1a741b1d62f882770cb53ee064bdbae5
https://hg.mozilla.org/mozilla-central/rev/de2cf52f4c1c
https://hg.mozilla.org/mozilla-central/rev/ec7a084b1a74

Comment 12

[Robert Strong (they/them - no direct email)]

Attachment: Patch for ESR 68

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.

Comment 13

[ovidiu boca[:Ovidiu]]

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

Comment 14

[Robert Strong (they/them - no direct email)]

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

Comment 15

[ovidiu boca[:Ovidiu]]

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.

Comment 16

[Robert Strong (they/them - no direct email)]

Attachment: Proof of concept #3 from bug 1481907

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.

Comment 17

[Robert Strong (they/them - no direct email)]

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?

Comment 18

[ovidiu boca[:Ovidiu]]

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.

Comment 19

[Robert Strong (they/them - no direct email)]

Thank you and that is the expected result.

Comment 20

[ovidiu boca[:Ovidiu]]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Is this ready for an ESR68 uplift request?

Comment 22

[Robert Strong (they/them - no direct email)]

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?

Comment 23

[Ryan VanderMeulen [: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?

Comment 24

[Robert Strong (they/them - no direct email)]

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.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

(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?

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

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)

Comment 28

[Robert Strong (they/them - no direct email)]

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.

Comment 29

[Robert Strong (they/them - no direct email)]

(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,

Comment 30

[Robert Strong (they/them - no direct email)]

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.

Comment 31

[Robert Strong (they/them - no direct email)]

Attachment: Patch to disable staging on Beta

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

Comment 32

[Pascal Chevrel:pascalc]

(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

Comment 33

[Molly Howell (she/her) (mostly inactive)]

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:

Comment 34

[Pascal Chevrel:pascalc]

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.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f151a13c27d9fe0f7b65da2ac1c1342abdfecf32

Comment 36

[Timea Cernea [:tbabos][inactive]]

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.

Comment 37

[Robert Strong (they/them - no direct email)]

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?

Comment 38

[Daniel Veditz [:dveditz]]

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.

Comment 39

[Daniel Veditz [:dveditz]]

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.

Comment 40

[Robert Strong (they/them - no direct email)]

Attachment: Patch to disable staging on ESR

[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.

Comment 41

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 42

[Robert Strong (they/them - no direct email)]

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

Comment 43

[Robert Strong (they/them - no direct email)]

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

Comment 44

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 45

[Timea Cernea [:tbabos][inactive]]

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.

Comment 46

[Robert Strong (they/them - no direct email)]

(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.

Comment 47

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 48

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 49

[clavoillotte]

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.

Comment 50

[Tom Ritter [:tjr]]

CVE minutia, redirecting to Dan....

Comment 51

[Daniel Veditz [: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.

Comment 52

[Daniel Veditz [:dveditz]]

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

Comment 53

[Tom Ritter [:tjr]]

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

Comment 54

[Daniel Veditz [:dveditz]]

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