1246972 - (CVE-2016-5294) Arbitrary target directory for result files of update process

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect)

Description

[Holger Fuhrmannek]

Attachment: invalid.loc.poc.zip

It's possible to choose an arbitrary target directory for result files of an update process. As in Bug 1177861 this leads to the possibility to change arbitrary files because for example extracted *.patch files from the .mar file are also placed in this directory.

I have tested the poc with win7 and ff 47 nightly installed to C:\Program Files (x86)\Nightly\:
To reproduce
- download poc and extract the files
- download http://releases.mozilla.org/pub/firefox/nightly/latest-mozilla-central/firefox-47.0a1.en-US.win32.complete.mar and place the file to <pocdir>/updatework/update.mar 
- execute poc.rb
- a firefox directory will appear next to the poc.rb
As seen in Bug 1177861 a more advance poc can change arbitrary files through manipulating *.patch files and the use of junctions.

Comment 1

[Al Billings [:abillings - ex-MoCo]]

Rob, can you take a look at this and maybe suggest a security rating?

Comment 2

[Robert Strong (they/them - no direct email)]

This is similar to bug 1171518 and should receive the same rating.

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 46+ since this is sec-high and seems to affect all branches.

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

We are heading into beta 10 on Monday, we could potentially still take a patch for 46 beta. We might want to coordinate extra testing with QE for when we do land a fix.

Comment 5

[Robert Strong (they/them - no direct email)]

The fix for this is likely going to be too complex to try to push for 46

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, thanks Robert.

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Any progress?  Wontfix this for beta 47 at this point.

Comment 8

[Robert Strong (they/them - no direct email)]

No progress at this time.

Matt, could you take a look at this?

Comment 9

[Molly Howell (she/her) (mostly inactive)]

The only way I can see of fixing this bug directly seems risky, so I'll offer that and another solution. rstrong, I'd like to get your feedback on these:
1) Fix the bug directly by only allowing updates to be applied to known application installation paths. To avoid spoofing, the list of known paths would have to be stored in a high-privilege location, such as in the registry at machine-level or a file in the maintenance service installation directory. Each new installation of an application would have to update this list, or it could not be updated using the service.
This seems high-risk to me, but it's the most direct fix to this bug.
2) Prevent this bug from being exploited with very much control by preventing editing of the update manifest, either by locking the file or by extracting it to RAM instead of a file. This way an attacker cannot control the specific operations the updater executes, but can still cause normal update actions to be taken in arbitrary locations, which could still be dangerous in theory.

Comment 10

[Robert Strong (they/them - no direct email)]

For 1 are you referring to just maintenance service updates? If so, we already have this information in the registry under HKLM. Why do you think it is high risk?

Comment 11

[Molly Howell (she/her) (mostly inactive)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #10)
> For 1 are you referring to just maintenance service updates? If so, we
> already have this information in the registry under HKLM. Why do you think
> it is high risk?

No, sorry, I meant application updates running via the service.

Comment 12

[Robert Strong (they/them - no direct email)]

Sorry, I meant application updates via the service as well.

We already have this info under HKLM in Software\Mozilla\MaintenanceService in the form of a hash of the path to the install directory. So, when updating using the service would getting the hash for the install directory and verifying that it exists in this location be sufficient for this bug?

Comment 13

[Molly Howell (she/her) (mostly inactive)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #12)
> We already have this info under HKLM in Software\Mozilla\MaintenanceService
> in the form of a hash of the path to the install directory.

... I forgot all about those. That is exactly what we need.

> So, when updating using the service would getting the hash for the install
> directory and verifying that it exists in this location be sufficient for this bug?

Yep, I'll do that.

Comment 14

[Robert Strong (they/them - no direct email)]

FYI: I *think* we already do check but it might fallback to allowing cases where it doesn't exist or it might not happen early enough.

Comment 15

[Molly Howell (she/her) (mostly inactive)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #14)
> FYI: I *think* we already do check but it might fallback to allowing cases
> where it doesn't exist or it might not happen early enough.

You're right, but it works out that the existing check is skipped if we can successfully write a lock file into the install dir, so any exploit satisfying that condition can get past the check. I'm going to try moving it to a much earlier spot and making it unconditional.

Comment 16

[Molly Howell (she/her) (mostly inactive)]

Attachment: Do update install directory registry check earlier and unconditionally

MozReview-Commit-ID: FR44f4b5KbI

Comment 17

[Molly Howell (she/her) (mostly inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=18dd63221a07

Comment 18

[Robert Strong (they/them - no direct email)]

Comment on attachment 8753043 [details] [diff] [review]
Do update install directory registry check earlier and unconditionally

I thought this was only going to be for the case where the service is used per comment #11.

We support updating without the service as well as well as installing without admin prvis and the removal of the useService will break updating for both of those cases.

Comment 19

[Molly Howell (she/her) (mostly inactive)]

Attachment: Do update install directory registry check earlier and unconditionally

MozReview-Commit-ID: FR44f4b5KbI

Comment 20

[Molly Howell (she/her) (mostly inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed5cdf7a1f6d

Comment 21

[Robert Strong (they/them - no direct email)]

Comment on attachment 8753902 [details] [diff] [review]
Do update install directory registry check earlier and unconditionally

When the key for the directory doesn't exist it should fallback to the non service update path by setting useService to false and sets the error in update.status. With this patch the status isn't updated and it doesn't fallback to the non service update. Does this bug also affect non service updates? Could you provide me with more detail?

Please fix the typo on the last line of this patch
s/comamnd/command/

Comment 22

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8753902 [details] [diff] [review]
Do update install directory registry check earlier and unconditionally

Obsoleting this patch because I completely misinterpreted the bug.

The problem isn't the install directory, the service already has protections for that. The problem is the working directory. For staged updates, we require that it's a child of the install directory, but otherwise there are no restrictions on it at all. That's how the updater can be made to perform file operations in arbitrary locations.
rstrong, what would you think about extending the requirement for the working directory to be a child of the install directory from just staged updates to all updates? I tried it and it made some of the tests quite angry with me, but I feel like that's just because they don't know to expect it.

Comment 23

[Robert Strong (they/them - no direct email)]

I think that should work. If you have a patch in progress that you can attach I can run the tests locally to get a better idea and to see what is going on with the tests.

Comment 24

[Molly Howell (she/her) (mostly inactive)]

Attachment: Patch - Always require the update working directory to be within the installation path

This patch could probably use some polish, but it's ready to throw at try.

Comment 25

[Molly Howell (she/her) (mostly inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=acca8834c9cc

Comment 26

[Molly Howell (she/her) (mostly inactive)]

Attachment: Always require the update working directory to be within the installation path

MozReview-Commit-ID: ASxfV4uVpmD

Comment 27

[Molly Howell (she/her) (mostly inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=40a236d9190c

Comment 28

[Molly Howell (she/her) (mostly inactive)]

Attachment: Patch - Always require the update working directory to be within the installation path on Windows

Hopefully this resolves the remaining try failures, and doesn't create more. It's difficult for me to test the MOZ_VERIFY_MAR_SIGNATURE paths locally.

Comment 29

[Molly Howell (she/her) (mostly inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e94bcd63849

Comment 30

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8756599 [details] [diff] [review]
Patch - Always require the update working directory to be within the installation path on Windows

The Mac Mochitest failures in that try run appear locally with or without this patch, so I'm calling it clean and requesting review.

Comment 31

[Robert Strong (they/them - no direct email)]

The Mac mochitest failures are succeeding on the build system without your patch... right?

Comment 32

[Robert Strong (they/them - no direct email)]

Comment on attachment 8756599 [details] [diff] [review]
Patch - Always require the update working directory to be within the installation path on Windows

Cancelling review until the Mac mochitest failures are understood.

Comment 33

[Robert Strong (they/them - no direct email)]

Talked with Mat over irc regarding the failures he is seeing locally.

Comment 34

[Molly Howell (she/her) (mostly inactive)]

Attachment: Patch - Always require the update working directory to be within the installation path on Windows

Wasn't able to really track down those Mac failures, so this is kind of a stab in the dark. New try push incoming.

Comment 35

[Molly Howell (she/her) (mostly inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f037546a798b

Comment 36

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8757479 [details] [diff] [review]
Patch - Always require the update working directory to be within the installation path on Windows

Whoa it's finally green.

Comment 37

[Molly Howell (she/her) (mostly inactive)]

https://hg.mozilla.org/projects/oak/rev/f38999684d8c02dba1befd26a8d9f0d6056ef928
Bug 1246972 - Always require the update working directory to be within the installation path; r?rstrong

Comment 38

[Molly Howell (she/her) (mostly inactive)]

Using the builds from oak, I've manually tested updating from a build with this patch using all combinations of the following states: maintenance service enabled or disabled, staged updates enabled or disabled, and install location in Program Files or inside the user profile. That makes for 8 test cases, and all of them successfully updated using the expected flow.

Comment 39

[Robert Strong (they/them - no direct email)]

Comment on attachment 8757479 [details] [diff] [review]
Patch - Always require the update working directory to be within the installation path on Windows

>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
>@@ -386,31 +386,19 @@ EnvHasValue(const char *name)
>  *
>  * @param  relpath
>  *         The relative path to convert to a full path.
>  * @return valid filesystem full path or nullptr if memory allocation fails.
>  */
> static NS_tchar*
> get_full_path(const NS_tchar *relpath)
> {
>-  size_t lendestpath = NS_tstrlen(gDestPath);
>-  size_t lenrelpath = NS_tstrlen(relpath);
>-  NS_tchar *s = (NS_tchar *) malloc((lendestpath + lenrelpath + 1) * sizeof(NS_tchar));
>-  if (!s)
>-    return nullptr;
>-
>-  NS_tchar *c = s;
>-
>-  NS_tstrcpy(c, gDestPath);
>-  c += lendestpath;
>-  NS_tstrcat(c, relpath);
>-  c += lenrelpath;
>-  *c = NS_T('\0');
>-  c++;
>-  return s;
>+  NS_tchar *fullPath = (NS_tchar *)malloc((MAX_PATH + 1) * sizeof(NS_tchar));
>+  GetFullPathName(relpath, MAX_PATH, fullPath, nullptr);
>+  return fullPath;
Per our previous conversation I'd prefer not using GetFullPathName.

>@@ -2452,17 +2440,21 @@ UpdateThreadFunc(void *param)
>         NS_tchar updateSettingsPath[MAX_TEXT_LEN];
>         NS_tsnprintf(updateSettingsPath,
>                      sizeof(updateSettingsPath) / sizeof(updateSettingsPath[0]),
> #ifdef XP_MACOSX
>                      NS_T("%s/Contents/Resources/update-settings.ini"),
> #else
>                      NS_T("%s/update-settings.ini"),
> #endif
>+#ifdef XP_WIN
>+                     gInstallDirPath);
Does this really have to be Windows only? If not, I'd prefer it to be consistent across platforms.

>+#else
>                      gWorkingDirPath);
>+#endif
>         MARChannelStringTable MARStrings;
>         if (ReadMARChannelIDs(updateSettingsPath, &MARStrings) != OK) {
>           // If we can't read from update-settings.ini then we shouldn't impose
>           // a MAR restriction.  Some installations won't even include this file.
>           MARStrings.MARChannelID[0] = '\0';
>         }
> 
>         rv = gArchiveReader.VerifyProductInformation(MARStrings.MARChannelID,
>@@ -2831,34 +2823,32 @@ int NS_main(int argc, NS_tchar **argv)
>     LOG(("Performing a replace request"));
>   }
> 
>   LOG(("PATCH DIRECTORY " LOG_S, gPatchDirPath));
>   LOG(("INSTALLATION DIRECTORY " LOG_S, gInstallDirPath));
>   LOG(("WORKING DIRECTORY " LOG_S, gWorkingDirPath));
> 
> #if defined(XP_WIN)
>-  if (sReplaceRequest || sStagedUpdate) {
>-    NS_tchar stagedParent[MAX_PATH];
>-    NS_tsnprintf(stagedParent, sizeof(stagedParent)/sizeof(stagedParent[0]),
>-                 NS_T("%s"), gWorkingDirPath);
>-    if (!PathRemoveFileSpecW(stagedParent)) {
>-      WriteStatusFile(REMOVE_FILE_SPEC_ERROR);
>-      LOG(("Error calling PathRemoveFileSpecW: %d", GetLastError()));
>-      LogFinish();
>-      return 1;
>-    }
>-
>-    if (_wcsnicmp(stagedParent, gInstallDirPath, MAX_PATH) != 0) {
>-      WriteStatusFile(INVALID_STAGED_PARENT_ERROR);
>-      LOG(("Stage and Replace requests require that the working directory " \
>-           "is a sub-directory of the installation directory! Exiting"));
>-      LogFinish();
>-      return 1;
>-    }
>+  NS_tchar workingDirParent[MAX_PATH];
>+  NS_tsnprintf(workingDirParent, sizeof(workingDirParent) / sizeof(workingDirParent[0]),
>+    NS_T("%s"), gWorkingDirPath);
>+  if (!PathRemoveFileSpecW(workingDirParent)) {
>+    WriteStatusFile(REMOVE_FILE_SPEC_ERROR);
>+    LOG(("Error calling PathRemoveFileSpecW: %d", GetLastError()));
>+    LogFinish();
>+    return 1;
>+  }
>+
>+  if (_wcsnicmp(workingDirParent, gInstallDirPath, MAX_PATH) != 0) {
>+    WriteStatusFile(INVALID_STAGED_PARENT_ERROR);
Since this applies to all updates please change the name to reflect that.

>+    LOG(("The working directory must be a sub-directory "
>+         "of the installation directory! Exiting"));
>+    LogFinish();
>+    return 1;
>   }
> #endif
> 
> #ifdef MOZ_WIDGET_GONK
>   const char *prioEnv = getenv("MOZ_UPDATER_PRIO");
>   if (prioEnv) {
>     int32_t prioVal;
>     int32_t oomScoreAdj;

Comment 40

[Molly Howell (she/her) (mostly inactive)]

Attachment: Patch

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #39)
> >+  NS_tchar *fullPath = (NS_tchar *)malloc((MAX_PATH + 1) * sizeof(NS_tchar));
> >+  GetFullPathName(relpath, MAX_PATH, fullPath, nullptr);
> >+  return fullPath;
> Per our previous conversation I'd prefer not using GetFullPathName.

Restored the previous implementation, with only the necessary changes.

> >+#ifdef XP_WIN
> >+                     gInstallDirPath);
> Does this really have to be Windows only? If not, I'd prefer it to be
> consistent across platforms.

This was my fix for the Mac chrome test failures. On (much) further investigation, those turn out to actually have been caused by an implicit assumption in a bit of the test code that the application binaries and the resource files (specifically update-settings.ini) live at the same path, which is not true on Mac OS. This version of the patch removes that assumption, which allowed this Windows check to go as well.

> >+    WriteStatusFile(INVALID_STAGED_PARENT_ERROR);
> Since this applies to all updates please change the name to reflect that.

Done.

Comment 41

[Molly Howell (she/her) (mostly inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3109cb148dc

Comment 42

[Molly Howell (she/her) (mostly inactive)]

Attachment: Patch

Updated patch; removed creation of extra working directory within the apply-to path.

Comment 43

[Molly Howell (she/her) (mostly inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=910a54b4b9b9

Comment 44

[Robert Strong (they/them - no direct email)]

Comment on attachment 8763991 [details] [diff] [review]
Patch

Per our discussion during MozLondon, please consolidate all of the NS_tchdir... I don't think we need so many of them.

Comment 45

[Molly Howell (she/her) (mostly inactive)]

Attachment: Patch

I was able to remove the chdir calls that I added, as well as all but two of the existing calls; one is needed to set the working directory for the callback app, and the other is the one that switches to the system directory to avoid holding a lock on anything we might need to delete.

Making all of that work out was a little more involved than I think either of us expected. Most of the changes come from needing the Action objects to hold their own copies of the paths they act on, so that they can call get_full_path() and properly dispose of the result, while also keeping the relative paths around so that they can be used in the log; it was easier than making the tests cope with paths in the log that include the test name. The tobedeleted directory needed a similar change as well.

Comment 46

[Molly Howell (she/her) (mostly inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e490ed001eaf

Comment 47

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8772624 [details] [diff] [review]
Patch

This patch is in pretty bad shape on Mac and Linux; working on a fixed version now.

Comment 48

[Molly Howell (she/her) (mostly inactive)]

Attachment: Patch

Okay, let's try this one.

Comment 49

[Molly Howell (she/her) (mostly inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5db59389e6ae

Comment 50

[Robert Strong (they/them - no direct email)]

Matt, just in case please run try for asan, static analysis, etc. just in case they catch something.

Comment 51

[Molly Howell (she/her) (mostly inactive)]

Okay; I can't seem to add any jobs to the previous try push, I guess it's too old, so I'll make a new one.

Comment 52

[Molly Howell (she/her) (mostly inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fcd0396b17b

Comment 53

[Molly Howell (she/her) (mostly inactive)]

Looks like that run didn't turn up any issues.

Comment 54

[Robert Strong (they/them - no direct email)]

Comment on attachment 8773858 [details] [diff] [review]
Patch

>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
>@@ -375,44 +377,85 @@ mstrtok(const NS_tchar *delims, NS_tchar
> 
> static bool
> EnvHasValue(const char *name)
> {
>   const char *val = getenv(name);
>   return (val && *val);
> }
> 
>-#ifdef XP_WIN
> /**
>- * Coverts a relative update path to a full path for Windows.
>+ * Coverts a relative update path to a full path.
>  *
>  * @param  relpath
>  *         The relative path to convert to a full path.
>  * @return valid filesystem full path or nullptr if memory allocation fails.
>  */
> static NS_tchar*
> get_full_path(const NS_tchar *relpath)
> {
>-  size_t lendestpath = NS_tstrlen(gDestPath);
>+  NS_tchar *destpath = sStagedUpdate ? gWorkingDirPath : gInstallDirPath;
>+  size_t lendestpath = NS_tstrlen(destpath);
>   size_t lenrelpath = NS_tstrlen(relpath);
>-  NS_tchar *s = (NS_tchar *) malloc((lendestpath + lenrelpath + 1) * sizeof(NS_tchar));
>-  if (!s)
>+  NS_tchar *s = new NS_tchar[lendestpath + lenrelpath + 2];
>+  if (!s) {
>     return nullptr;
>+  }
> 
>   NS_tchar *c = s;
> 
>-  NS_tstrcpy(c, gDestPath);
>+  NS_tstrcpy(c, destpath);
>   c += lendestpath;
>+#ifdef XP_WIN
>+  if (*c != '\\') {
>+    NS_tstrcat(c, NS_T("\\"));
>+    c++;
>+  }
>+#else
>+  if (*c != '/') {
>+    NS_tstrcat(c, NS_T("/"));
>+    c++;
>+  }
>+#endif
Both gWorkingDirPath and gInstallDirPath have the trailing slash removed so no need to check if there is one.
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/updater.cpp#2795
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/updater.cpp#2723

I think this should just concatenate NS_T("/") for all platforms as we do in almost all other cases when working with paths. For example, all paths in the manifest use / so it should be safe to go this route. If it is actually needed please use NS_SLASH which is defined in updatedefines.h. In either case, there is no need for the ifdef or the check for a slash.

>+
>   NS_tstrcat(c, relpath);
>   c += lenrelpath;
>   *c = NS_T('\0');
>-  c++;
>   return s;
> }
>@@ -2468,17 +2558,18 @@ UpdateThreadFunc(void *param)
>         NS_tchar updateSettingsPath[MAX_TEXT_LEN];
>         NS_tsnprintf(updateSettingsPath,
>                      sizeof(updateSettingsPath) / sizeof(updateSettingsPath[0]),
> #ifdef XP_MACOSX
>                      NS_T("%s/Contents/Resources/update-settings.ini"),
> #else
>                      NS_T("%s/update-settings.ini"),
> #endif
>-                     gWorkingDirPath);
>+                     gInstallDirPath);
Pretty sure this shouldn't be changed.

Pushed this changed back to try to verify
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d7358e3b2bd

>+
>         MARChannelStringTable MARStrings;
>         if (ReadMARChannelIDs(updateSettingsPath, &MARStrings) != OK) {
>           // If we can't read from update-settings.ini then we shouldn't impose
>           // a MAR restriction.  Some installations won't even include this file.
>           MARStrings.MARChannelID[0] = '\0';
>         }
> 
>         rv = gArchiveReader.VerifyProductInformation(MARStrings.MARChannelID,
>@@ -2844,31 +2934,32 @@ int NS_main(int argc, NS_tchar **argv)
>     LOG(("Performing a replace request"));
>   }
> 
>   LOG(("PATCH DIRECTORY " LOG_S, gPatchDirPath));
>   LOG(("INSTALLATION DIRECTORY " LOG_S, gInstallDirPath));
>   LOG(("WORKING DIRECTORY " LOG_S, gWorkingDirPath));
> 
> #if defined(XP_WIN)
>-  if (sReplaceRequest || sStagedUpdate) {
>-    NS_tchar stagedParent[MAX_PATH];
>-    NS_tsnprintf(stagedParent, sizeof(stagedParent)/sizeof(stagedParent[0]),
>+  if (_wcsnicmp(gWorkingDirPath, gInstallDirPath, MAX_PATH) != 0) {
These should always be the same on Windows when performing a normal update (e.g. not a staged update or a replace request). I think what we want here is to bail with an error code when these aren't the same during a normal update.

>+    NS_tchar workingDirParent[MAX_PATH];
>+    NS_tsnprintf(workingDirParent,
>+                 sizeof(workingDirParent) / sizeof(workingDirParent[0]),
>                  NS_T("%s"), gWorkingDirPath);
>-    if (!PathRemoveFileSpecW(stagedParent)) {
>+    if (!PathRemoveFileSpecW(workingDirParent)) {
>       WriteStatusFile(REMOVE_FILE_SPEC_ERROR);
>       LOG(("Error calling PathRemoveFileSpecW: %d", GetLastError()));
>       LogFinish();
>       return 1;
>     }
> 
>-    if (_wcsnicmp(stagedParent, gInstallDirPath, MAX_PATH) != 0) {
>-      WriteStatusFile(INVALID_STAGED_PARENT_ERROR);
>-      LOG(("Stage and Replace requests require that the working directory " \
>-           "is a sub-directory of the installation directory! Exiting"));
>+    if (_wcsnicmp(workingDirParent, gInstallDirPath, MAX_PATH) != 0) {
>+      WriteStatusFile(INVALID_APPLYTO_DIR_ERROR);
>+      LOG(("The apply-to directory must be the same as or "
>+           "a child of the installation directory! Exiting."));
>       LogFinish();
>       return 1;
>     }
>   }
> #endif
> 
> #ifdef MOZ_WIDGET_GONK
>   const char *prioEnv = getenv("MOZ_UPDATER_PRIO");
>@@ -2916,25 +3007,16 @@ int NS_main(int argc, NS_tchar **argv)
>     }
>   }
> #else
>   if (pid > 0)
>     waitpid(pid, nullptr, 0);
> #endif
> 
> #ifdef XP_WIN
>-  if (sReplaceRequest) {
>-    // On Windows, the current working directory of the process should be changed
>-    // so that it's not locked.
>-    NS_tchar sysDir[MAX_PATH + 1] = { L'\0' };
>-    if (GetSystemDirectoryW(sysDir, MAX_PATH + 1)) {
>-      NS_tchdir(sysDir);
>-    }
>-  }
>-
> #ifdef MOZ_MAINTENANCE_SERVICE
>   sUsingService = EnvHasValue("MOZ_USING_SERVICE");
>   putenv(const_cast<char*>("MOZ_USING_SERVICE="));
> #endif
>   // lastFallbackError keeps track of the last error for the service not being
>   // used, in case of an error when fallback is not enabled we write the
>   // error to the update.status file.
>   // When fallback is disabled (MOZ_NO_SERVICE_FALLBACK does not exist) then
>@@ -3380,17 +3446,17 @@ int NS_main(int argc, NS_tchar **argv)
>                           argv + callbackIndex,
>                           sUsingService);
>       }
>       return 1;
>     }
> 
>     // Doing this is only necessary when we're actually applying a patch.
>     if (!sReplaceRequest) {
>-      int len = NS_tstrlen(applyDirLongPath);
>+      int len = NS_tstrlen(gInstallDirPath);
Why did you change this?

>       NS_tchar *s = callbackLongPath;
>       NS_tchar *d = gCallbackRelPath;
>       // advance to the apply to directory and advance past the trailing backslash
>       // if present.
>       s += len;
>       if (*s == NS_T('\\'))
>         ++s;
> 
>@@ -3486,18 +3552,22 @@ int NS_main(int argc, NS_tchar **argv)
>     }
>   }
> 
>   // DELETE_DIR is not required when staging an update.
Please change this comment to something along the lines of
// DELETE_DIR is not required when performing a staged update or replace
// request though it can be used during a replace request but it doesn't
// use gDeleteDirPath.

>   if (!sStagedUpdate && !sReplaceRequest) {
>     // The directory to move files that are in use to on Windows. This directory
>     // will be deleted after the update is finished or on OS reboot using
>     // MoveFileEx if it contains files that are in use.
Please change this comment as follows
// The directory to move files that are in use to on Windows. This directory
// will either be deleted after the update is finished, on OS reboot using
// MoveFileEx if it contains files that are in use, or by the post update
// process after the update finishes. On Windows when performing a normal
// update (e.g. the update is not a staged update and is not a replace
// request) gWorkingDirPath is the same as gInstallDirPath and
// gWorkingDirPath is used because it is the destination directory.

>-    if (NS_taccess(DELETE_DIR, F_OK)) {
>-      NS_tmkdir(DELETE_DIR, 0755);
>+    NS_tsnprintf(gDeleteDirPath,
>+      sizeof(gDeleteDirPath) / sizeof(gDeleteDirPath[0]),
>+      NS_T("%s/%s"), gWorkingDirPath, DELETE_DIR);
>+
>+    if (NS_taccess(gDeleteDirPath, F_OK)) {
>+      NS_tmkdir(gDeleteDirPath, 0755);
>     }
>   }
> #endif /* XP_WIN */
@@ -4062,21 +4123,26 @@ GetManifestContents(const NS_tchar *mani
 }
 
 int AddPreCompleteActions(ActionList *list)
 {
   if (sIsOSUpdate) {
     return OK;
   }
 
+  NS_tchar manifestPath[MAXPATHLEN];
 #ifdef XP_MACOSX
-  NS_tchar *rb = GetManifestContents(NS_T("Contents/Resources/precomplete"));
+  NS_tsnprintf(manifestPath, sizeof(manifestPath) / sizeof(manifestPath[0]),
+    NS_T("%s/Contents/Resources/precomplete"), gInstallDirPath);
 #else
-  NS_tchar *rb = GetManifestContents(NS_T("precomplete"));
+  NS_tsnprintf(manifestPath, sizeof(manifestPath) / sizeof(manifestPath[0]),
+    NS_T("%s/precomplete"), gInstallDirPath);
 #endif
Though either should work I think this should be gWorkingDirPath so it will use the one in the staging directory when staging.

As mhowell suggested over irc, use get_full_path.

+
+  NS_tchar *rb = GetManifestContents(manifestPath);
   if (rb == nullptr) {
     LOG(("AddPreCompleteActions: error getting contents of precomplete " \
          "manifest"));
     // Applications aren't required to have a precomplete manifest. The mar
     // generation scripts enforce the presence of a precomplete manifest.
     return OK;
   }

Comment 55

[Robert Strong (they/them - no direct email)]

BTW: it looks like some Windows style newlines might have snuck into your patch

Comment 56

[Robert Strong (they/them - no direct email)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #54)
<snip>
> >+
> >   NS_tstrcat(c, relpath);
> >   c += lenrelpath;
> >   *c = NS_T('\0');
> >-  c++;
> >   return s;
> > }
> >@@ -2468,17 +2558,18 @@ UpdateThreadFunc(void *param)
> >         NS_tchar updateSettingsPath[MAX_TEXT_LEN];
> >         NS_tsnprintf(updateSettingsPath,
> >                      sizeof(updateSettingsPath) / sizeof(updateSettingsPath[0]),
> > #ifdef XP_MACOSX
> >                      NS_T("%s/Contents/Resources/update-settings.ini"),
> > #else
> >                      NS_T("%s/update-settings.ini"),
> > #endif
> >-                     gWorkingDirPath);
> >+                     gInstallDirPath);
> Pretty sure this shouldn't be changed.
> 
> Pushed this changed back to try to verify
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d7358e3b2bd
Debug try run finished successfully on Mac and other platforms look fine too so it should be safe to revert this.

Comment 57

[Molly Howell (she/her) (mostly inactive)]

Attachment: Patch

This fixes all the review comments, including the ones from comment 55 and comment 56. 

>-      int len = NS_tstrlen(applyDirLongPath);
>+      int len = NS_tstrlen(gInstallDirPath);
>Why did you change this?

I have no idea; I reverted that as well. My guess is that it was needed on an earlier version of the patch and stopped making sense when the chdir calls were removed.

I'll do one more try push to make sure all this is still okay.

Comment 58

[Molly Howell (she/her) (mostly inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=37aa231d84ab

Comment 59

[Robert Strong (they/them - no direct email)]

It would be a good thing to have a test for the failure when the gWorkingDirPath and gInstallDirPath aren't the same for a normal update. I'm wondering if the same restriction should be cross platform as well... could you check?
https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsUpdateDriver.cpp

Comment 60

[Robert Strong (they/them - no direct email)]

Comment on attachment 8777584 [details] [diff] [review]
Patch

Clearing review until comment #45 is answered

Comment 61

[Robert Strong (they/them - no direct email)]

bah! I meant comment #59

Comment 62

[Molly Howell (she/her) (mostly inactive)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #59)
> It would be a good thing to have a test for the failure when the
> gWorkingDirPath and gInstallDirPath aren't the same for a normal update.
Okay, I can add that.

> I'm wondering if the same restriction should be cross platform as well... could you check?
> https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsUpdateDriver.cpp
It should be possible to make it cross-platform, but it broke staging when I tried it, and I'd rather not spend time fixing that as part of this bug, since the security implications aren't really there on platforms lacking the maintenance service.

Comment 63

[Robert Strong (they/them - no direct email)]

That should only apply when the following is true and hence shouldn't break staging or replace requests.
if (!sStagedUpdate && !sReplaceRequest) {

The only case that I think it might break is gonk but I'm fine with not doing this change in this bug.

Also, you didn't add a new error and write it to the status file for 
+  if (_wcsnicmp(gWorkingDirPath, gInstallDirPath, MAX_PATH) != 0) {
+    if (!sStagedUpdate && !sReplaceRequest) {
+      LOG(("Installation directory and working directory must be the same "
+           "for non-staged updates. Exiting."));

Comment 64

[Robert Strong (they/them - no direct email)]

Comment on attachment 8777584 [details] [diff] [review]
Patch

The only remaining things to be done are the new test and

Also, you didn't add a new error and write it to the status file for 
+  if (_wcsnicmp(gWorkingDirPath, gInstallDirPath, MAX_PATH) != 0) {
+    if (!sStagedUpdate && !sReplaceRequest) {
+      LOG(("Installation directory and working directory must be the same "
+           "for non-staged updates. Exiting."));

This error should also be handled in nsUpdateService.js and reported via telemetry.

Comment 65

[Molly Howell (she/her) (mostly inactive)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #64)
> Also, you didn't add a new error and write it to the status file for 
> +  if (_wcsnicmp(gWorkingDirPath, gInstallDirPath, MAX_PATH) != 0) {
> +    if (!sStagedUpdate && !sReplaceRequest) {
> +      LOG(("Installation directory and working directory must be the same "
> +           "for non-staged updates. Exiting."));
> 
> This error should also be handled in nsUpdateService.js and reported via
> telemetry.
Do you think this needs to be a whole new error code? It seems like INVALID_APPLYTO_DIR_ERROR would work fine; it's really the same error as the other place that code is returned, and the fact that it's a stage or replace run disambiguates this one from that one.

Comment 66

[Robert Strong (they/them - no direct email)]

It depends on how difficult it is to differentiate it in telemetry. I suspect it won't be that easy.

Comment 67

[Molly Howell (she/her) (mostly inactive)]

Attachment: Patch

Added new error code and test.

Comment 68

[Robert Strong (they/them - no direct email)]

Comment on attachment 8778458 [details] [diff] [review]
Patch

I thought you were going to land this on oak first... you don't need to wait on review to do so. Anyways, this looks good.

Comment 69

[Molly Howell (she/her) (mostly inactive)]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #68)
> I thought you were going to land this on oak first... you don't need to wait
> on review to do so. Anyways, this looks good.

Yeah, sorry, I just hadn't gotten around to it yet. Trying to do it today.

Thanks for the reviews.

Comment 70

[Robert Strong (they/them - no direct email)]

I'll do some manual update testing on oak after it is landed as well. Thanks!

Comment 71

[Molly Howell (she/her) (mostly inactive)]

https://hg.mozilla.org/projects/oak/rev/4afd44e5b901

Comment 72

[Molly Howell (she/her) (mostly inactive)]

Finally managed to get that testing done. I tried staged and non-staged updates, first in Program Files, and then inside the user's profile. Everything went as expected. I'll post a rebased patch (there's one file with a non-trivial merge) with a carried-over r+ and then request security approval.

Comment 73

[Molly Howell (she/her) (mostly inactive)]

Attachment: Patch

Rebased version of attachment 8778458 [details] [diff] [review]; r+ carried over.

Comment 74

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8784879 [details] [diff] [review]
Patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I suspect it wouldn't be easy; the patch is large and the actual security fix is kind of buried.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Not in my opinion.

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?
I do not; they might be non-trivial to create because of the amount of surgery I had to do to the updater. I'm not sure this patch merits backporting, though.

How likely is this patch to cause regressions; how much testing does it need?
Unlikely. Existing automated tests are fairly extensive, and I did some manual testing myself, see comment 72.

Comment 75

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8784879 [details] [diff] [review]
Patch

sec-approval+ for trunk. I think we should take it on Aurora but I don't know that we want to take a big patch late for beta for 49.

Comment 76

[Molly Howell (she/her) (mostly inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c079fb045b9d18df1a6fe80e3a46ad1e27eff03d
Bug 1246972 - Always require the update working directory to be within the installation path; r=rstrong

Comment 77

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8784879 [details] [diff] [review]
Patch

On abillings's suggestion:

Approval Request Comment
[Feature/regressing bug #]:
Probably bug 481815.

[User impact if declined]:
Security bug classified sec-high.

[Describe test coverage new/current, TreeHerder]:
Automated test coverage for the updater is good. A new test was also added to verify the new failure condition. The latest push that includes this patch is on TreeHerder at https://treeherder.mozilla.org/#/jobs?repo=oak&revision=189f55e63199. I've also done a bit of manual testing, see comment 72.

[Risks and why]: 
This patch contains pretty wide-reaching changes to the updater, so the risk is that updates start failing. Allowing the patch to sit on nightly for a few days before uplifting it would be a good way to manage that.

[String/UUID change made/needed]:
None

Comment 78

[Molly Howell (she/her) (mostly inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4576637003b6ae52477041cd6a807f482bad5211
Bug 1246972 followup - Fix eslint errors; CLOSED TREE

Comment 79

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for xpcshell failures like https://treeherder.mozilla.org/logviewer.html#?job_id=34660566&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/41ee5cb3266c

Comment 80

[Robert Strong (they/them - no direct email)]

I landed bug 1291985 on inbound today so please add the following to the xpcshell.ini for the new test

skip-if = os != 'win' || debug && (os_version == '5.1' || os_version == '5.2')
reason = Windows only test and bug 1291985

Comment 81

[Molly Howell (she/her) (mostly inactive)]

Note that patch was backed out from inbound for asan failures, so don't actually push that one to aurora. Attempted fix incoming.

Comment 82

[Molly Howell (she/her) (mostly inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=27cdfe581567

Comment 83

[Molly Howell (she/her) (mostly inactive)]

Attachment: Fixed Patch

Okay, my stab at a fix for the asan failures seems to have worked. This is the fixed patch.

Not sure if I need to request sec-approval again, since this is a minor change to the previous patch which already had it. I'll do it anyway just to be safe.

Comment 84

[Molly Howell (she/her) (mostly inactive)]

Since this patch has a chance (and in fact a history) of causing trouble, I didn't really want to push it out last thing on a Friday. ;)

I'll request uplift again if this push sticks on nightly for a few days.

Comment 85

[Molly Howell (she/her) (mostly inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c908e9677a9b93ac25b3c35e75bdb447b6e69b56
Bug 1246972 - Always require the update working directory to be within the installation path; r=rstrong

Comment 86

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c908e9677a9b

Comment 87

[Benjamin Smedberg]

Is this ready to request uplift? We're running out of time if this is going to make 49.

Comment 88

[Molly Howell (she/her) (mostly inactive)]

Yesterday would have been the first nightly that included this patch, which means today's is the first update that's using it. I'd rather wait at least a few more hours, up to a couple of days if possible, for more people to attempt that update, to see if any problems appear.

Comment 89

[Liz Henry (:lizzard) (relman/hg->git project)]

Matt: I am planning to start the 49 beta 9 build tomorrow, then the RC build on Monday. I would rather not take this in late beta though.

Comment 90

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8785392 [details] [diff] [review]
Fixed Patch

Approval Request Comment
See comment 77. We've had a couple of nightly updates using this patch now and no reports of issues, so I'm confident that the risk has been addressed.

Comment 91

[Daniel Veditz [:dveditz]]

We need to fix this in the ESR release that matches the Firefox release that fixes it (looking like 50/45.5)

Comment 92

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8785392 [details] [diff] [review]
Fixed Patch

Approving for Aurora. We should take this on ESR45 as well.

Comment 93

[Al Billings [:abillings - ex-MoCo]]

Can you make an ESR45 patch and nominate it, Matt?

Comment 94

[Molly Howell (she/her) (mostly inactive)]

Attachment: ESR45 Patch

Same patch as attachment 8785392 [details] [diff] [review] rebased for ESR45.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
N/A, this bug is sec-high

User impact if declined: 
sec-high bug allowing privilege escalation on Windows

Fix Landed on Version:
51, with uplift to 50

Risk to taking this patch (and alternatives if risky): 
Primary risk is new bugs created in the updater by this patch causing updates to fail. This risk has been managed by running this patch on Nightly for two weeks; no issues have been observed or reported.

String or UUID changes made by this patch: 
None

Comment 95

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/89ba44134660

Comment 96

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8790451 [details] [diff] [review]
ESR45 Patch

sec-high, taking it on the esr branch

Comment 97

[Molly Howell (she/her) (mostly inactive)]

Attachment: ESR45 Patch Rebased

The ESR45 patch previously posted here and just approved had bitrotted due to the landing of bug 1168743 and friends. This one should apply cleanly.

Comment 98

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8801906 [details] [diff] [review]
ESR45 Patch Rebased

Pushed:
https://hg.mozilla.org/releases/mozilla-esr45/rev/aa1593ec5ccd027070daa190e433ed588eb2fdca

Comment 99

[Bogdan Maris, Desktop Test Engineering]

OS tested:
- Windows 7 64bit.

Reproduced the initial issue using old Nightly from (2016-02-09) and poc file attached:

* Installed Firefox
https://archive.mozilla.org/pub/firefox/nightly/2016/02/2016-02-09-03-03-47-mozilla-central/firefox-47.0a1.en-US.win32.installer.exe
* Mar file
https://archive.mozilla.org/pub/firefox/nightly/2016/02/2016-02-09-03-03-47-mozilla-central/firefox-47.0a1.en-US.win32.complete.mar

Executing poc file will result in:
- 'target' folder is created in the same directory as poc.rb
- update.log:
> PATCH DIRECTORY C:\Users\bogdan.maris\Desktop\invalid.loc.poc\updatework
> INSTALLATION DIRECTORY C:\Program Files (x86)\Nightly
> WORKING DIRECTORY C:\Users\bogdan.maris\Desktop\invalid.loc.poc\target
> failed: 6
> calling QuitProgressUI
- update.status
> failed: 6

Verified that the initial issue does not reproduce in current builds:

1) Firefox 50.0-build2

* Installed Firefox
https://archive.mozilla.org/pub/firefox/candidates/50.0-candidates/build2/win32/en-US/Firefox%20Setup%2050.0.exe
* Mar file
https://archive.mozilla.org/pub/firefox/candidates/50.0-candidates/build2/update/win32/en-US/firefox-50.0.complete.mar

Executing poc file will result in:
- 'target' folder is NOT created at all
- update.log:
> PATCH DIRECTORY C:\Users\bogdan.maris\Desktop\invalid.loc.poc\updatework
> INSTALLATION DIRECTORY C:\Program Files (x86)\Nightly
> WORKING DIRECTORY C:\Users\bogdan.maris\Desktop\invalid.loc.poc\target
> Installation directory and working directory must be the same for non-staged updates. Exiting.
- update.status:
> failed: 74

2) Firefox 45.5.0esr-build1

* Installed FirefoxESR
https://archive.mozilla.org/pub/firefox/candidates/45.5.0esr-candidates/build1/win32/en-US/Firefox%20Setup%2045.5.0esr.exe
* Mar file:
https://archive.mozilla.org/pub/firefox/candidates/45.5.0esr-candidates/build1/update/win32/en-US/firefox-45.5.0esr.complete.mar

Executing poc file will result in:
- 'target' folder is NOT created at all
- update.log:
> PATCH DIRECTORY C:\Users\bogdan.maris\Desktop\invalid.loc.poc\updatework
> INSTALLATION DIRECTORY C:\Program Files (x86)\Nightly
> WORKING DIRECTORY C:\Users\bogdan.maris\Desktop\invalid.loc.poc\target
> Installation directory and working directory must be the same for non-staged updates. Exiting.
- update.status:
> failed: 74

3) Latest Firefox Developer Edition 51.0a2 (2016-11-08)

* Installed Firefox
https://archive.mozilla.org/pub/firefox/nightly/latest-mozilla-aurora/firefox-51.0a2.en-US.win32.installer.exe
* Mar file
https://archive.mozilla.org/pub/firefox/nightly/latest-mozilla-aurora/firefox-51.0a2.en-US.win32.complete.mar

Executing poc file will result in:
- 'target' folder is NOT created at all
- update.log:
> PATCH DIRECTORY C:\Users\bogdan.maris\Desktop\invalid.loc.poc\updatework
> INSTALLATION DIRECTORY C:\Program Files (x86)\Nightly
> WORKING DIRECTORY C:\Users\bogdan.maris\Desktop\invalid.loc.poc\target
> Installation directory and working directory must be the same for non-staged updates. Exiting.
- update.status:
> failed: 74

Note:
- Nightly directory was the directory where all Firefox versions were installed, this was done to avoid editing the poc file.