Last Comment Bug 709183 - Callback path and arguments are taken from the service work item, which is untrusted
: Callback path and arguments are taken from the service work item, which is un...
Status: RESOLVED FIXED
[sg:high][qa-]
:
Product: Toolkit
Classification: Components
Component: Application Update (show other bugs)
: Trunk
: x86 Windows Vista
: -- normal (vote)
: mozilla12
Assigned To: Brian R. Bondy [:bbondy]
:
Mentors:
Depends on: 708854
Blocks: 481815
  Show dependency treegraph
 
Reported: 2011-12-09 10:25 PST by Ian Melven :imelven
Modified: 2012-05-18 13:26 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
unaffected


Attachments
pach v1. (11.98 KB, patch)
2011-12-10 12:47 PST, Brian R. Bondy [:bbondy]
ian.melven: feedback+
Details | Diff | Review
Patch v2. (9.70 KB, patch)
2011-12-14 23:02 PST, Brian R. Bondy [:bbondy]
robert.strong.bugs: review+
Details | Diff | Review
Patch v3. (9.66 KB, patch)
2011-12-15 08:57 PST, Brian R. Bondy [:bbondy]
netzen: review+
Details | Diff | Review

Description Ian Melven :imelven 2011-12-09 10:25:54 PST
The service work item for the work in bug 481185 contains the callback application to execute after the update has occurred and the arguments to pass to this application. The work item is created in a world writeable directory that other valid users on the machine will be able to create files (work items) in. 

Although the callback executable's signature is checked and it is verified to be signed by Mozilla - it could be any executable signed by Mozilla, not necessarily the newly installed version of Firefox. Additionally, arbitrary arguments can be passed to the executable - the command line options to Mozilla signed executables may have all sorts of powers/behaviours that can't be assumed to be safe or low risk. 

Marking [sg:high] due to the possibility of there being command line args to Firefox (and other Mozilla signed executables) that could cause real security problems beyond opening an arbitrary URL. Also marking as blocking bug 481185. Please discuss here in the bug if you have other opinions :)
Comment 1 Ian Melven :imelven 2011-12-09 10:29:22 PST
The proposed design is to have the unelevated updater.exe perform the callback - this removes the necessity to pass the callback path and args to the service at all, and removes the need to have them in the work item. 

See also bug 708854 which is around the session ID spoofing issue possible to the session ID being in the work item - the same fix to use the unelevated updater.exe to perform the callback should address both that bug and this bug.
Comment 2 Brian R. Bondy [:bbondy] 2011-12-10 12:47:25 PST
Created attachment 580665 [details] [diff] [review]
pach v1.

Note: In bug 708854 we stopped executing the callback application in the service.  In this bug we no longer pass the info of the callback, nor do we use it to determine anything else.  For example we used it to determine where the post update process was located. 

Changes:
- We no longer pass this information to the work item file
- We no longer use this information to determine the path of the post update process which we execute.
- Added some extra logging.
- Tested and pushed to elm as well.
Comment 3 Ian Melven :imelven 2011-12-12 16:55:30 PST
Comment on attachment 580665 [details] [diff] [review]
pach v1.

Review of attachment 580665 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good !

so the callback is now executed from the updater.exe like it sometimes was previously
(that's the this patch is mostly removing code from the service, correct ? )
Comment 4 Ian Melven :imelven 2011-12-12 16:56:24 PST
(In reply to Ian Melven :imelven from comment #3)
> Comment on attachment 580665 [details] [diff] [review]
> pach v1.
> 
> Review of attachment 580665 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good !
> 
> so the callback is now executed from the updater.exe like it sometimes was
> previously
> (that's the this patch is mostly removing code from the service, correct ? )

er 'that's why this patch is' - sorry.

mostly i'm just clarifying my understanding of why this patch is pretty small, the callback stuff from updater.exe (unelevated) was already there.
Comment 5 Brian R. Bondy [:bbondy] 2011-12-12 17:00:55 PST
> so the callback is now executed from the updater.exe 
> like it sometimes was previously

It is started in the same way as the callback exe (Firefox.exe) is executed without the service (by the unelevated updater.exe).

> mostly i'm just clarifying my understanding of why this patch is pretty small, 
> the callback stuff from updater.exe (unelevated) was already there.

Ya most of the work here was done in bug 708854.  This bug is just removing the passing of the callback info.  And we used that info for determining the location of helper.exe which now we use the install dir instead.
Comment 6 Brian R. Bondy [:bbondy] 2011-12-14 23:02:05 PST
Created attachment 581885 [details] [diff] [review]
Patch v2.

Rebased
Comment 7 Robert Strong [:rstrong] (use needinfo to contact me) 2011-12-15 01:44:58 PST
Comment on attachment 581885 [details] [diff] [review]
Patch v2.

>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
>@@ -1749,17 +1749,21 @@ int NS_main(int argc, NS_tchar **argv)
>       // update.status file to make sure it succeeded, and if it did
>       // we need to manually start the PostUpdate process from the
>       // current user's session of this unelevated updater.exe the
>       // current process is running as.
>       if (useService) {
>         bool updateStatusSucceeded = false;
>         if (IsUpdateStatusSucceeded(updateStatusSucceeded) && 
>             updateStatusSucceeded) {
>-          LaunchWinPostProcess(argv[callbackIndex], gSourcePath, false, NULL);
>+          if (!LaunchWinPostProcess(argv[2], gSourcePath, false, NULL)) {
>+            fprintf(stderr, "The post update process which is only run for "
>+                    "service updates, and is run unelevated, could not be "
>+                    "launched.");
nit: which runs as the user for a service update could not be launched.

Nice!
Comment 8 Brian R. Bondy [:bbondy] 2011-12-15 08:57:26 PST
Created attachment 581993 [details] [diff] [review]
Patch v3.

Implemented review comments.
Comment 9 Brian R. Bondy [:bbondy] 2012-01-04 22:23:55 PST
https://hg.mozilla.org/mozilla-central/rev/ee4def38eede

Note You need to log in before you can comment on or make changes to this bug.