Bug 1321814 (CVE-2017-5409)

Maintenance Service Updater Callback Parameter File Deletion Elevation of Privilege

RESOLVED FIXED in Firefox -esr45

Status

()

RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: hofusec, Assigned: agashlin)

Tracking

({csectype-priv-escalation, sec-moderate})

unspecified
mozilla54
csectype-priv-escalation, sec-moderate
Points:
---
Bug Flags:
sec-bounty +
qe-verify -

Firefox Tracking Flags

(firefox-esr4552+ fixed, firefox51 wontfix, firefox52+ fixed, firefox-esr5252+ fixed, firefox53+ fixed, firefox54+ fixed)

Details

(Whiteboard: [adv-main52+][adv-esr45.8+] local attack)

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8816459 [details]
poc.rb

It is possible to exploit the updater as a non privileged user to delete an arbitrary file by passing a special path to the callback parameter of the updater via the Mozilla Maintenance Service. 

During the update the callback parameter is used to generate a path to delete a file by appending a ".moz-callback" suffix (updater.cpp:3505). The result buffer of this operation has the length MAX_PATH (260) if the result is larger it will truncated. This can be exploited to bypass the appending operation and to delete a file, which has a path with the length 259. Further with relative path components this can be exploited to delete files with a smaller absolute path. With the ability to delete arbitrary files it´s possible to elevate privilege further.

I have tested the poc with the current nightly version of firefox and windows 7. It deletes the firefox.exe at 'C:\Program Files (x86)\Nightly\firefox.exe'.
Can this delete files outside of the installation directory?
Flags: needinfo?(hofusec)
(Reporter)

Comment 2

2 years ago
Yes I have only chosen the firefox.exe because it is in a protected directory and it is not too bad when the file is deleted.
Flags: needinfo?(hofusec)
Keywords: csectype-priv-escalation, sec-high
Whiteboard: local attack
(Assignee)

Comment 3

2 years ago
Created attachment 8826427 [details] [diff] [review]
Abort if callback backup path is truncated

Just went through try successfully:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f83565b1d90dbbd2a0a80390d3a77f626d5b69a
Attachment #8826427 - Flags: review?(robert.strong.bugs)
(Assignee)

Updated

2 years ago
Assignee: nobody → agashlin
(Assignee)

Comment 4

2 years ago
Created attachment 8826808 [details] [diff] [review]
Abort if callback backup path is truncated

Fixed comparison across signedness. Now properly anticipates NS_tsnprintf returning -1 on truncation on Windows. Added comment.

Try results are here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=62be787b2335b3019780f0a92a855eb09fa49c04
Attachment #8826427 - Attachment is obsolete: true
Attachment #8826427 - Flags: review?(robert.strong.bugs)
Attachment #8826808 - Flags: review?(robert.strong.bugs)
Comment on attachment 8826808 [details] [diff] [review]
Abort if callback backup path is truncated

>diff --git a/toolkit/mozapps/update/updater/updater.cpp b/toolkit/mozapps/update/updater/updater.cpp
>--- a/toolkit/mozapps/update/updater/updater.cpp
>+++ b/toolkit/mozapps/update/updater/updater.cpp
>@@ -3497,19 +3497,37 @@ int NS_main(int argc, NS_tchar **argv)
>           *d = *s;
>         ++s;
>         ++d;
>       } while (*s);
>       *d = NS_T('\0');
>       ++d;
> 
>       // Make a copy of the callback executable so it can be read when patching.
nit: this comment is now more applicable to the code after the new code. Please move it.

>-      NS_tsnprintf(gCallbackBackupPath,
>-                   sizeof(gCallbackBackupPath)/sizeof(gCallbackBackupPath[0]),
>-                   NS_T("%s" CALLBACK_BACKUP_EXT), argv[callbackIndex]);
>+      {
>+        const size_t bufsize =
>+          sizeof(gCallbackBackupPath)/sizeof(gCallbackBackupPath[0]);
>+        const int len =
>+          NS_tsnprintf(gCallbackBackupPath, bufsize,
>+                       NS_T("%s" CALLBACK_BACKUP_EXT), argv[callbackIndex]);
>+
>+        if (len < 0 || len >= static_cast<int>(bufsize)) {
>+          // The provided path was too long, do not perform the update or
>+          // launch the callback.
>+
>+          LOG(("NS_main: callback backup path truncated"));
>+          WriteStatusFile(USAGE_ERROR);
>+          LogFinish();
>+
>+          EXIT_WHEN_ELEVATED(elevatedLockFilePath, updateLockFileHandle, 1);
Please add a comment about why this doesn't try to relaunch the callback since everywhere else we do.

>+
No need for this empty line

>+          return 1;
>+        }
>+      }
>+
Don't add the additional scope. If we are going to do this for variables that are only used in one place I'd prefer it if it were done throughout the file in a separate bug.

I'd like one more look at this before r+ing it.

>       NS_tremove(gCallbackBackupPath);
>       if(!CopyFileW(argv[callbackIndex], gCallbackBackupPath, true)) {
>         DWORD copyFileError = GetLastError();
>         LOG(("NS_main: failed to copy callback file " LOG_S
>              " into place at " LOG_S, argv[callbackIndex], gCallbackBackupPath));
>         LogFinish();
>         if (copyFileError == ERROR_ACCESS_DENIED) {
>           WriteStatusFile(WRITE_ERROR_ACCESS_DENIED);
Attachment #8826808 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 6

2 years ago
Created attachment 8828571 [details] [diff] [review]
Abort if callback backup path is truncated

Updated as per comment 5, also placed the LogFinish call immediately after the LOG for clarity.

Try from last night, identical to this patch but for a changed comment:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7fdbf5baf9bd591cf62c8d72017d498e8242dae
Attachment #8826808 - Attachment is obsolete: true
Attachment #8828571 - Flags: review?(robert.strong.bugs)
Comment on attachment 8828571 [details] [diff] [review]
Abort if callback backup path is truncated

Looks good to me and thanks!

Note: either Matt or myself can help you with getting security approval and landing this.
Attachment #8828571 - Flags: review?(robert.strong.bugs) → review+
Comment on attachment 8828571 [details] [diff] [review]
Abort if callback backup path is truncated

[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not easily and it would require local access to the system

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? N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Not yet and creating them should be simple and not risky.

How likely is this patch to cause regressions; how much testing does it need? Not likely. Manual testing.

As with most update patches this should bake a few days on nightly before uplifting so please keep that in mind.
Attachment #8828571 - Flags: sec-approval?
sec-approval for checkin on February 7, two weeks into the new cycle. 
After it goes in, we'll want branch patches made and nominated (well, they could be made any time but won't go in until after trunk).
status-firefox51: --- → wontfix
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox-esr45: --- → affected
tracking-firefox52: --- → +
tracking-firefox53: --- → +
tracking-firefox54: --- → +
tracking-firefox-esr45: --- → ?
Whiteboard: local attack → [checkin on 2/7] local attack
Attachment #8828571 - Flags: sec-approval? → sec-approval+
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/7205aedc3051
Whiteboard: [checkin on 2/7] local attack → local attack
https://hg.mozilla.org/mozilla-central/rev/7205aedc3051
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Please request uplift to aurora and beta so this can ship in beta5 later this week.
Flags: needinfo?(agashlin)
(Assignee)

Comment 13

2 years ago
I think we want to wait for this to hit nightly for a few days first to be safe, can we shoot for beta6 instead?
Flags: needinfo?(agashlin) → needinfo?(jcristau)
Sure, beta6 works.  Thanks Adam :)
Flags: needinfo?(jcristau)
Group: toolkit-core-security → core-security-release
(Assignee)

Comment 15

2 years ago
Comment on attachment 8828571 [details] [diff] [review]
Abort if callback backup path is truncated

Approval Request Comment
[Feature/Bug causing the regression]: None, not a regression
[User impact if declined]: Local malicious code could delete some files that it would not have permission to delete otherwise
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Unlikely
[Why is the change risky/not risky?]: Paths shouldn't normally be this long, if this check prevents an update due to truncation, the callback is so close to the path limit that the rest of the directory structure would be past the limit. If the check succeeds there will be no effect.
[String changes made/needed]: None (a log message was added but is not end-user-visible)
Attachment #8828571 - Flags: approval-mozilla-beta?
Attachment #8828571 - Flags: approval-mozilla-aurora?
Comment on attachment 8828571 [details] [diff] [review]
Abort if callback backup path is truncated

Fix a sec-high. Aurora53+.
Attachment #8828571 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8828571 [details] [diff] [review]
Abort if callback backup path is truncated

detect path truncation, beta52+
Attachment #8828571 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Setting qe-verify- per comment 15.
Flags: qe-verify-
https://hg.mozilla.org/releases/mozilla-esr52/rev/509d5333e21a

Adam, were we intending to uplift this to esr45 as well? Need an approval request if so :)
status-firefox-esr52: --- → fixed
tracking-firefox-esr52: --- → ?
Flags: needinfo?(agashlin)
(Assignee)

Comment 22

2 years ago
I'm not sure. It is a low-risk patch, and it seems like it would be desirable to get fixed in esr45 if we're fixing it elsewhere, but I think I'd defer to abillings as he marked it tracking-firefox-esr45?
Flags: needinfo?(agashlin) → needinfo?(abillings)
I'd like to see it there. You need to nominate a patch. I am not a developer (IANAD) so I don't write or nominate patches normally.
Flags: needinfo?(abillings) → needinfo?(agashlin)
(Assignee)

Comment 24

2 years ago
Created attachment 8837674 [details]
poc for Developer Preview path

For easier testing with local builds
(Assignee)

Comment 25

2 years ago
Created attachment 8837675 [details]
poc for release (x86) path
(Assignee)

Comment 26

2 years ago
Created attachment 8837679 [details] [diff] [review]
esr45 version of Abort if callback backup path truncated

The earlier patch doesn't apply cleanly to esr45 due to changes elsewhere in the file; these do not affect the behavior of the patch and it removes and adds exactly the same code. Confirmed to fix the issue with local builds.
Attachment #8837679 - Flags: review?(mhowell)
Attachment #8837679 - Flags: review?(mhowell) → review+
(Assignee)

Comment 27

2 years ago
Comment on attachment 8837679 [details] [diff] [review]
esr45 version of Abort if callback backup path truncated

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
It wouldn't take too much imagination to reproduce the proof of concept given the patch, but an exploit would only be possible with local access to the system.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No, the most suspicious word is "truncated".

Which older supported branches are affected by this flaw?
This is specifically for esr45, all branches were affected.

If not all supported branches, which bug introduced the flaw?
N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This is the backport. It is essentially identical to the patch that already landed safely on Nightly, Aurora, and Beta.

How likely is this patch to cause regressions; how much testing does it need?
Unlikely, it is a small change and the code has been on Nightly for a week. Manual testing should be sufficient, and the code path is covered by existing automated tests.
Flags: needinfo?(agashlin)
Attachment #8837679 - Flags: sec-approval?
(Assignee)

Comment 28

2 years ago
Comment on attachment 8837679 [details] [diff] [review]
esr45 version of Abort if callback backup path truncated

I've been informed that I would only need sec-approval for landing on trunk.
Attachment #8837679 - Flags: sec-approval?
(Assignee)

Comment 29

2 years ago
Comment on attachment 8837679 [details] [diff] [review]
esr45 version of Abort if callback backup path truncated

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-high
User impact if declined: Local exploit could delete files that it would not have permission to delete otherwise, possibly leading to further elevation of privileges
Fix Landed on Version: 52 (and 53, 54)
Risk to taking this patch (and alternatives if risky): This should be low risk as it is a small change affecting an extreme situation which should only happen when this bug is exploited. The same code has lived on 54 for about a week.
String or UUID changes made by this patch: None significant (a log message was added but is not end-user-visible)
Attachment #8837679 - Flags: approval-mozilla-esr45?
tracking-firefox-esr52: ? → 52+
Comment on attachment 8837679 [details] [diff] [review]
esr45 version of Abort if callback backup path truncated

Fix a sec-high. ESR45+.
Attachment #8837679 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
tracking-firefox-esr45: ? → 52+
Whiteboard: local attack → [adv-main52+][adv-esr45.8+] local attack
Alias: CVE-2017-5409
Flags: sec-bounty?
We should have called this sec-moderate to be consistent with previous deletion-only bugs.
Flags: sec-bounty? → sec-bounty+
Keywords: sec-high → sec-moderate
(In reply to Adam Gashlin [:agashlin] from comment #15)
> Approval Request Comment
> [Is this code covered by automated tests?]: Yes

Would you mind to point to those automated tests? I don't see any new ones created with this patch, and previously our automation didn't catch this bug.

I currently wonder if bug 1355818 could be a regression by this change.
Flags: needinfo?(hofusec)
(Assignee)

Comment 34

2 years ago
Sorry, I must have been mistaken about this being covered by automated tests. There's no test which should exercise the new error checking here, I had perhaps misunderstood that a test would use the copied callback to at least ensure the copying wasn't always broken. I can't point to it.
Flags: needinfo?(hofusec)
(In reply to Adam Gashlin [:agashlin] from comment #34)
> Sorry, I must have been mistaken about this being covered by automated
> tests. There's no test which should exercise the new error checking here, I
> had perhaps misunderstood that a test would use the copied callback to at
> least ensure the copying wasn't always broken. I can't point to it.
Existing tests do exercise that code but there is no new test to exercise the error condition.

Also, this bug affects the callback path which isn't part of staging an update.
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #35)
> (In reply to Adam Gashlin [:agashlin] from comment #34)
> > Sorry, I must have been mistaken about this being covered by automated
> > tests. There's no test which should exercise the new error checking here, I
> > had perhaps misunderstood that a test would use the copied callback to at
> > least ensure the copying wasn't always broken. I can't point to it.
> Existing tests do exercise that code but there is no new test to exercise
> the error condition.
Note: I added code that catches this earlier as well as a test in bug 1348645.
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/tests/unit_base_updater/invalidArgCallbackFilePathTooLongFailure.js
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.