Holger Fuhrmannek Reporter
	Description • 8 years ago

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

	Robert Strong (they/them - no direct email)
	Comment 1 • 8 years ago

Can this delete files outside of the installation directory?

	Holger Fuhrmannek Reporter
	Comment 2 • 8 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.

	Adam Gashlin (he/him) [:agashlin] (ex-moco) Assignee
	Comment 3 • 8 years ago

Just went through try successfully:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f83565b1d90dbbd2a0a80390d3a77f626d5b69a

	Adam Gashlin (he/him) [:agashlin] (ex-moco) Assignee
	Comment 4 • 8 years ago

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

	Robert Strong (they/them - no direct email)
	Comment 5 • 8 years ago

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);

	Adam Gashlin (he/him) [:agashlin] (ex-moco) Assignee
	Comment 6 • 8 years ago

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

	Robert Strong (they/them - no direct email)
	Comment 7 • 8 years ago

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.

	Robert Strong (they/them - no direct email)
	Comment 8 • 8 years ago

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.

	Al Billings [:abillings - ex-MoCo]
	Comment 9 • 8 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/7205aedc3051

	Carsten Book [:Tomcat]
	Comment 11 • 8 years ago

https://hg.mozilla.org/mozilla-central/rev/7205aedc3051

	Julien Cristau [:jcristau]
	Comment 12 • 8 years ago

Please request uplift to aurora and beta so this can ship in beta5 later this week.

	Adam Gashlin (he/him) [:agashlin] (ex-moco) Assignee
	Comment 13 • 8 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?

	Julien Cristau [:jcristau]
	Comment 14 • 8 years ago

Sure, beta6 works.  Thanks Adam :)

	Adam Gashlin (he/him) [:agashlin] (ex-moco) Assignee
	Comment 15 • 8 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)

	Gerry Chang [:gchang]
	Comment 16 • 7 years ago

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

Fix a sec-high. Aurora53+.

	Carsten Book [:Tomcat]
	Comment 17 • 7 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/6512b480a90a

	Julien Cristau [:jcristau]
	Comment 18 • 7 years ago

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

detect path truncation, beta52+

	Carsten Book [:Tomcat]
	Comment 19 • 7 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/509d5333e21a

	Cornel Ionce [:noni] [Hubs QA]
	Comment 20 • 7 years ago

Setting qe-verify- per comment 15.

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 7 years ago

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 :)

	Adam Gashlin (he/him) [:agashlin] (ex-moco) Assignee
	Comment 22 • 7 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?

	Al Billings [:abillings - ex-MoCo]
	Comment 23 • 7 years ago

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.

	Adam Gashlin (he/him) [:agashlin] (ex-moco) Assignee
	Comment 24 • 7 years ago

For easier testing with local builds

	Adam Gashlin (he/him) [:agashlin] (ex-moco) Assignee
	Comment 26 • 7 years ago

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.

	Adam Gashlin (he/him) [:agashlin] (ex-moco) Assignee
	Comment 27 • 7 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.

	Adam Gashlin (he/him) [:agashlin] (ex-moco) Assignee
	Comment 28 • 7 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.

	Adam Gashlin (he/him) [:agashlin] (ex-moco) Assignee
	Comment 29 • 7 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)

	Gerry Chang [:gchang]
	Comment 30 • 7 years ago

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

Fix a sec-high. ESR45+.

	Ryan VanderMeulen [:RyanVM]
	Comment 31 • 7 years ago
uplift

https://hg.mozilla.org/releases/mozilla-esr45/rev/0a22becb23cd

	Daniel Veditz [:dveditz]
	Comment 32 • 7 years ago

We should have called this sec-moderate to be consistent with previous deletion-only bugs.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 33 • 7 years ago

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

	Adam Gashlin (he/him) [:agashlin] (ex-moco) Assignee
	Comment 34 • 7 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.

	Robert Strong (they/them - no direct email)
	Comment 35 • 7 years ago

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

	Robert Strong (they/them - no direct email)
	Comment 36 • 7 years ago

(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