This bug is to make it explicitly clear the session ID spoofing issue identified during security review of the silent update service is considered by the secteam to be a blocker for 699700 landing on Nightly. The current proposal is to use the updater (unelevated) to handle the callback app - this is already running in the user session so there is no need for a session id and no session id spoofing vulnerability and also no need for callback path or args in the work item. I marked it tracking-firefox11: ? to attempt to indicate this needs to be fixed to land 699700 in current Nightlies.

got the wrong bug - this blocks landing 481815 in current Nightlies (the service landing, not the MAR signing)

> This bug is to make it explicitly clear the session ID spoofing issue > identified during security review of the silent update service is > considered by the secteam to be a blocker for *481815 landing on Nightly. I just wanted to mention that the wiki for the security review had a section added by me before the meeting for session ID spoofing and if it was a risk. This was identified not at the security review but before the security review. At the security review I believe the understanding was that the worst case scenario was a Firefox process would be launched in the wrong session, but it would be our code so it was not critical. It was agreed upon that making sure we are only running our own code (via authenticode cert check) was fine for this issue. The new issue identified by bsmith a couple days ago (which is a valid concern) is that even know it is our application, someone could launch http://some-bad-site-url in Firefox from a limited account into an admin account. So an authenticode cert check is not enough. I am not so much concerned with how the issue was discovered, but I want to make it clear that this is a new issue discovered and not identified in the security review of bug 481815. The requirements have changed as of a couple days ago and the description in this bug makes it sound like it was identified a long time ago. In any case I am happy to fix this and have already been working on it today.

Implementation notes: - Callback session ID is no longer in the work item file - Firefox.exe launches updater.exe unelevated whether or not it is using the service. - Updater.exe unelevated now determines if it should use the service or use the elevated updater (old way). - When updater.exe unelevated launches the service, it waits on a global named event it creates that the service sets when it is done working. - Since the service no longer knows the callback session ID the post update process for the user settings is done from within updater.exe unelevated. - The post update process from the unelevated user session is only done if the update.status file indicates a successful update. - Since there is no more launching of callbacks from the service, the service no longer does a cert check on the callback application. - callback location and argument are passed in the work item file but are not used. This will be fixed in bug 709183 Other notes: - Pushed this to elm. - Updated wiki of the silent update feature (https://wiki.mozilla.org/Windows_Service_Silent_Update) with all of the new information. - Tested locally and it was all working.

Note for Rob Strong: We talked about only using the service if it needs permissions to. This change has the side effect that this will be true. So I do not need to post a separate bug about that. I.e. only when the elevated lock file in updater.exe cannot be created in the installation dir, do we even try to use the service.

Comment on attachment 580602 [details] [diff] [review] Patch v1. Review of attachment 580602 [details] [diff] [review]: ----------------------------------------------------------------- Some nits as usual (sorry !) and a question on one piece of code. To confirm my understanding of the change - the updater creates a Windows event and then writes out the work item for the service, the service then picks up the work item and opens the event - meanwhile the updater is blocking (forever) on the event it created. When the service is done it then sets the event - allowing the unelevated updater to proceed with calling the callback and the unelevated post process. The service continues calling the elevated post process as before. Failure paths lead to falling back to asking to elevate updater.exe via a UAC prompt. please correct me if this is not accurate ! :) Feedback+ but it would be good to clear up the question around the event not existing when the service tries to open it. ::: toolkit/components/maintenanceservice/workmonitor.cpp @@ +241,5 @@ > > // Indicate that the service is busy and shouldn't be used by anyone else > + // by opening or creating a named event. Programs should check if this > + // event exists before writing a work item out. It should already be > + // created by udpater.exe so CreateEventW will lead to an open named event. typo : updater.exe @@ +248,1 @@ > if this event wasn't already created by updater.exe we could maybe get into a deadlock here if updater runs afterwards ? is there ever a valid case where the event doesn't already exist ?

> ... > please correct me if this is not accurate ! :) Your summary is exactly accurate. > Feedback+ but it would be good to clear up the question around the event not > existing when the service tries to open it. > ... > if this event wasn't already created by updater.exe we could maybe get into > a deadlock here if updater runs afterwards ? is there ever a valid case > where the event doesn't already exist ? In that case it is no harm, the service will create the event and set it. No one will use that event though. If a new updater.exe tries to run a new update while the event already exists, then the service will not be used for that update.

replaced udpater.exe reference with updater.exe.

Comment on attachment 581159 [details] [diff] [review] Patch v2. This appears to be the patch for making sure the updater is the same as the one in the instal dir and not for fixing session ID spoofing.

Sorry about that too many bugs. I attached the correct v2 now.

Comment on attachment 581619 [details] [diff] [review] The correct patch v2. >diff --git a/toolkit/xre/nsUpdateDriver.cpp b/toolkit/xre/nsUpdateDriver.cpp >--- a/toolkit/xre/nsUpdateDriver.cpp >+++ b/toolkit/xre/nsUpdateDriver.cpp >@@ -186,56 +186,33 @@ GetFile(nsIFile *dir, const nsCSubstring Simplification when possible... it is the best! >diff --git a/toolkit/xre/nsWindowsRestart.cpp b/toolkit/xre/nsWindowsRestart.cpp >--- a/toolkit/xre/nsWindowsRestart.cpp >+++ b/toolkit/xre/nsWindowsRestart.cpp >@@ -46,17 +46,16 @@ > #define nsWindowsRestart_cpp > #endif > > #include "nsUTF8Utils.h" > #include "nsWindowsHelpers.h" > > #include <shellapi.h> > #include <shlwapi.h> >-#include <shlobj.h> > #include <stdio.h> > #include <wchar.h> > #include <rpc.h> > #include <userenv.h> > > #pragma comment(lib, "shlwapi.lib") > #pragma comment(lib, "rpcrt4.lib") > #pragma comment(lib, "userenv.lib") The following from above were added in bug 481815... are all of them still needed besides shlobj.h which you removed? +#include "nsWindowsHelpers.h" +#include <shlwapi.h> +#include <shlobj.h> +#include <stdio.h> +#include <wchar.h> +#include <rpc.h> +#include <userenv.h> + +#pragma comment(lib, "shlwapi.lib") +#pragma comment(lib, "rpcrt4.lib") +#pragma comment(lib, "userenv.lib") I really like that this removes the service specific code from this file! Can more be removed?

(In reply to Robert Strong [:rstrong] (do not email) from comment #10) >... > I really like that this removes the service specific code from this file! > Can more be removed? meant to say updater specific code.

> Can more be removed? Possibly I'll add a bug for myself to investigate.

Shouldn't be difficult to figure out and I'll do so during the review.

Comment on attachment 581619 [details] [diff] [review] The correct patch v2. >diff --git a/toolkit/components/maintenanceservice/workmonitor.cpp b/toolkit/components/maintenanceservice/workmonitor.cpp >--- a/toolkit/components/maintenanceservice/workmonitor.cpp >+++ b/toolkit/components/maintenanceservice/workmonitor.cpp >@@ -58,70 +58,52 @@ > extern BOOL gServiceStopping; > > // Wait 15 minutes for an update operation to run at most. > // Updates usually take less than a minute so this seems like a > // significantly large and safe amount of time to wait. > static const int TIME_TO_WAIT_ON_UPDATER = 15 * 60 * 1000; > PRUnichar* MakeCommandLine(int argc, PRUnichar **argv); > BOOL WriteStatusFailure(LPCWSTR updateDirPath, int errorCode); >-BOOL WriteStatusPending(LPCWSTR updateDirPath); >-BOOL StartCallbackApp(int argcTmp, LPWSTR *argvTmp, DWORD callbackSessionID); > BOOL PathGetSiblingFilePath(LPWSTR destinationBuffer, LPCWSTR siblingFilePath, > LPCWSTR newFileName); > > // The error codes start from 16000 since Windows system error > // codes only go up to 15999 > const int SERVICE_UPDATER_COULD_NOT_BE_STARTED = 16000; > const int SERVICE_NOT_ENOUGH_COMMAND_LINE_ARGS = 16001; > const int SERVICE_UPDATER_SIGN_ERROR = 16002; >-const int SERVICE_CALLBACK_SIGN_ERROR = 16003; > > /** >- * Runs an update process in the specified sessionID as an elevated process. >+ * Runs an update process as the service SYSTEM account. nit: s/as the service SYSTEM account./as the service using the SYSTEM account./ > * >- * @param updaterPath The path to the update process to start. >- * @param workingDir The working directory to execute the update process in. >- * @param cmdLine The command line parameters to pass to the update >- * process. If they specify a callback application, it will be >- * executed with an associated unelevated token for the sessionID. >- * @param processStarted Returns TRUE if the process was started. >- * @param callbackSessionID >- * If 0 and Windows Vista, the callback application will >- * not be run. If non zero the callback application will >- * be injected into the user's session as a non-elevated process. >+ * @param updaterPath The path to the update process to start. >+ * @param workingDir The working directory to execute the update process in. >+ * @param cmdLine The command line parameters to pass to updater.exe >+ * @param processStarted Returns TRUE if the process was started. nit: s/Returns/Set to/ > * @return TRUE if the update process was run had a return code of 0. > */ > BOOL > StartUpdateProcess(LPCWSTR updaterPath, >@@ -203,24 +187,27 @@ StartUpdateProcess(LPCWSTR updaterPath, > if (selfHandlePostUpdate) { > MoveFileEx(updaterINITemp, updaterINI, MOVEFILE_REPLACE_EXISTING); > > // Only run the PostUpdate if the update was successful and if we have > // a callback application. This is the same thing updater.exe does. > if (updateWasSuccessful && argcTmp > 5) { > LPCWSTR callbackApplication = argvTmp[5]; > LPCWSTR updateInfoDir = argvTmp[1]; >- // Launch the PostProcess with admin access in session 0 followed >- // by user access with the user token. This is actually launching >- // the post update process but it takes in the callback app path >- // to figure out where. >- // The directory containing the update information. >+ >+ // Launch the PostProcess with admin access in session 0. This is >+ // actually launching the post update process but it takes in the >+ // callback app path to figure out where to apply to. >+ LOG(("Launching post update process as the service in session 0.")); > LaunchWinPostProcess(callbackApplication, updateInfoDir, NULL); >- nsAutoHandle userToken(UACHelper::OpenUserToken(callbackSessionID)); >- LaunchWinPostProcess(callbackApplication, updateInfoDir, userToken); >+ // The post process update with user only access will be done inside >+ // the unelevated updater.exe after the update process is complete >+ // from the service. We don't know here which session to start >+ // the user one from so that's why we let the unelevated updater.exe >+ // do it. s/do it/does it/ I think the comment is somewhat clearer before calling LaunchWinPostProcess >@@ -338,131 +322,92 @@ ProcessWorkItem(LPCWSTR monitoringBasePa >... >- // We can't start the callback in this case because there >- // are not enough command line parameters. We set an error instead of >- // directly setting status pending so that the app.update.service.errors >- // pref can be updated when the callback app restarts. nit: please add a comment regarding why the update.status file can't be updated. > if (argcTmp != 2 || > !WriteStatusFailure(argvTmp[1], > SERVICE_NOT_ENOUGH_COMMAND_LINE_ARGS)) { > LOG(("Could not write update.status service update failure." > "Last error: %d\n", GetLastError())); > }

Comment on attachment 581619 [details] [diff] [review] The correct patch v2. This is all so much nicer this way! >diff --git a/toolkit/mozapps/update/common/launchwinprocess.h b/toolkit/mozapps/update/common/launchwinprocess.h >--- a/toolkit/mozapps/update/common/launchwinprocess.h >+++ b/toolkit/mozapps/update/common/launchwinprocess.h >@@ -44,8 +44,11 @@ > * @param userToken The user token to run as, if NULL the current user > * will be used. > * @param updateInfoDir The directory where update info is stored. > */ > void LaunchWinPostProcess(const WCHAR *appExe, > const WCHAR *updateInfoDir, > HANDLE userToken); > BOOL StartServiceUpdate(int argc, LPWSTR *argv); >+BOOL GetUpdateDirectoryPath(LPWSTR path); >+BOOL WinLaunchServiceCommand(LPCWSTR exePath, int argc, WCHAR **argv); >+#define SERVICE_EVENT_NAME L"Global\\moz-5b780de9-065b-4341-a04f-ddd94b3723e5" launchwinprocess seems to be turning into updatehelper. The less specific name might be better especially as things are added in the future. >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 >... >@@ -1519,16 +1581,33 @@ int NS_main(int argc, NS_tchar **argv) > return 1; > } > > // Change current directory to the directory where we need to apply the update. > if (NS_tchdir(argv[2]) != 0) { > return 1; > } > >+ // The directory containing the update information. >+ gSourcePath = argv[1]; >+ >+#ifdef XP_WIN >+ bool useService = false; >+ // We never want the service to be used unless we build with >+ // the maintenance service. >+ #ifdef MOZ_MAINTENANCE_SERVICE >+ IsUpdateStatusPending(useService); nit: we typically don't indent #ifdef and don't elsewhere in this file >@@ -1831,19 +1961,19 @@ int NS_main(int argc, NS_tchar **argv) > if (gSucceeded) { > LaunchWinPostProcess(argv[callbackIndex], gSourcePath, NULL); > // The service update will only be executed if it is already installed. > // For first time installs of the service, the install will happen from > // the PostUpdate process. We do the service update process here > // because it's possible we are updating with updater.exe without the > // service if the service failed to apply the update even know it is > // installed. We want to update the service to a newer version in that >- // case. If we are not running through the service, then MOZ_SESSION_ID >- // will not exist. >- WCHAR *sessionIDStr = _wgetenv(L"MOZ_SESSION_ID"); >+ // case. If we are not running through the service, then >+ // MOZ_USING_SERVICE will not exist. >+ WCHAR *sessionIDStr = _wgetenv(L"MOZ_USING_SERVICE"); Change sessionIDStr is no longer the session ID

Comment on attachment 581619 [details] [diff] [review] The correct patch v2. >diff --git a/toolkit/mozapps/update/common/launchwinprocess.cpp b/toolkit/mozapps/update/common/launchwinprocess.cpp >--- a/toolkit/mozapps/update/common/launchwinprocess.cpp >+++ b/toolkit/mozapps/update/common/launchwinprocess.cpp >... >+ * Launch a service initiated action with the specified arguments. >+ * >+ * @param exePath The path of the executable to run >+ * @param argc The total number of arguments in argv >+ * @param argv >+ * An array of null terminated strings to pass to the exePath, >+ * argv[0] is ignored nit: formatting consistency * @param argv An array of null terminated strings to pass to the exePath, * argv[0] is ignored >+ * @return TRUE if successful >+ */ >+BOOL >+WinLaunchServiceCommand(LPCWSTR exePath, int argc, LPWSTR* argv) Just pointing out that a couple of these methods are only used by updater.cpp and nothing else.

(In reply to Brian R. Bondy [:bbondy] from comment #12) > > Can more be removed? > > Possibly I'll add a bug for myself to investigate. The following should be moved from nsWindowsRestart PathAppendSafe WriteStatusPending WriteStatusFailure Can you also remove the following unused cruft -#ifndef ERROR_ELEVATION_REQUIRED -#define ERROR_ELEVATION_REQUIRED 740L -#endif - -BOOL (WINAPI *pCreateProcessWithTokenW)(HANDLE, - DWORD, - LPCWSTR, - LPWSTR, - DWORD, - LPVOID, - LPCWSTR, - LPSTARTUPINFOW, - LPPROCESS_INFORMATION); - -BOOL (WINAPI *pIsUserAnAdmin)(VOID);

Implemented review comments up to and including Comment 17.

Comment on attachment 581884 [details] [diff] [review] Patch v3. >diff --git a/toolkit/mozapps/update/common/launchwinprocess.cpp b/toolkit/mozapps/update/common/updatehelper.cpp >rename from toolkit/mozapps/update/common/launchwinprocess.cpp >rename to toolkit/mozapps/update/common/updatehelper.cpp >--- a/toolkit/mozapps/update/common/launchwinprocess.cpp >+++ b/toolkit/mozapps/update/common/updatehelper.cpp Thanks! >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 >@@ -1396,47 +1396,27 @@ LaunchCallbackApp(const NS_tchar *workin > // passes the working directory as an --environ: command line argument. > if(NS_tchdir(workingDir) != 0) I commented about adding a space between the if and the ( in bug 481815 but you can fix it here to possibly avoid rebasing. >diff --git a/toolkit/xre/nsWindowsRestart.cpp b/toolkit/xre/nsWindowsRestart.cpp >--- a/toolkit/xre/nsWindowsRestart.cpp >+++ b/toolkit/xre/nsWindowsRestart.cpp >@@ -46,42 +46,25 @@ > #define nsWindowsRestart_cpp > #endif > > #include "nsUTF8Utils.h" > #include "nsWindowsHelpers.h" > > #include <shellapi.h> > #include <shlwapi.h> >-#include <shlobj.h> > #include <stdio.h> > #include <wchar.h> > #include <rpc.h> > #include <userenv.h> > > #pragma comment(lib, "shlwapi.lib") > #pragma comment(lib, "rpcrt4.lib") > #pragma comment(lib, "userenv.lib") I guess you didn't have a chance to check if these are all needed as mentioned in comment #10? I'd like that checked and cleaned up if possible.

> #include "nsWindowsHelpers.h" > > #include <shellapi.h> > #include <shlwapi.h> >-#include <shlobj.h> > #include <stdio.h> > #include <wchar.h> > #include <rpc.h> > #include <userenv.h> > > #pragma comment(lib, "shlwapi.lib") > #pragma comment(lib, "rpcrt4.lib") > #pragma comment(lib, "userenv.lib") > I guess you didn't have a chance to check if these are all needed as mentioned in > comment #10? I'd like that checked and cleaned up if possible. Sorry about that I meant to circle back on that one, submitting a patch with that fixed shortly. Only userenv .h/.lib is needed for CreateEnvironmentBlock. shlwapi.lib was moved to updatehelper.cpp

Implemented review comments.

This is marked as tracking for FF11 - are we still considering uplifting this?

No this will land at the same time as the service in FF12.