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

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.

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

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

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

Rebased

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!

Implemented review comments.