Bug 1246972 (CVE-2016-5294)

Arbitrary target directory for result files of update process

VERIFIED FIXED in Firefox -esr45

Status

()

defect
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: hofusec, Assigned: mhowell)

Tracking

({csectype-priv-escalation, sec-high})

unspecified
mozilla51
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +
in-testsuite +

Firefox Tracking Flags

(firefox46+ wontfix, firefox47+ wontfix, firefox48+ wontfix, firefox49+ wontfix, firefox-esr4550+ verified, firefox50+ verified, firefox51 verified)

Details

(Whiteboard: [post-critsmash-triage][adv-main50+][adv-esr45.5+])

Attachments

(3 attachments, 14 obsolete attachments)

854 bytes, application/zip
Details
55.01 KB, patch
mhowell
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
54.82 KB, patch
Details | Diff | Splinter Review
Reporter

Description

3 years ago
Posted file 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.
Rob, can you take a look at this and maybe suggest a security rating?
Flags: needinfo?(robert.strong.bugs)
This is similar to bug 1171518 and should receive the same rating.
Flags: needinfo?(robert.strong.bugs)
Assignee: nobody → robert.strong.bugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: sec-high
Tracking for 46+ since this is sec-high and seems to affect all branches.
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.
The fix for this is likely going to be too complex to try to push for 46
Any progress?  Wontfix this for beta 47 at this point.
Flags: needinfo?(robert.strong.bugs)
No progress at this time.

Matt, could you take a look at this?
Assignee: robert.strong.bugs → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(robert.strong.bugs) → needinfo?(mhowell)
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.
Flags: needinfo?(mhowell) → needinfo?(robert.strong.bugs)
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?
Flags: needinfo?(robert.strong.bugs)
(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.
Flags: needinfo?(mhowell) → needinfo?(robert.strong.bugs)
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?
Flags: needinfo?(robert.strong.bugs)
(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.
Assignee: nobody → mhowell
Flags: needinfo?(mhowell)
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.
(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.
MozReview-Commit-ID: FR44f4b5KbI
Assignee

Updated

3 years ago
Attachment #8753043 - Flags: review?(robert.strong.bugs)
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.
Attachment #8753043 - Flags: review?(robert.strong.bugs) → review-
MozReview-Commit-ID: FR44f4b5KbI
Assignee

Updated

3 years ago
Attachment #8753043 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Attachment #8753902 - Flags: review?(robert.strong.bugs)
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/
Attachment #8753902 - Flags: review?(robert.strong.bugs)
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.
Attachment #8753902 - Attachment is obsolete: true
Flags: needinfo?(robert.strong.bugs)
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.
Flags: needinfo?(robert.strong.bugs)
This patch could probably use some polish, but it's ready to throw at try.
Assignee

Updated

3 years ago
Attachment #8756101 - Attachment is obsolete: true
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.
Attachment #8756466 - Attachment is obsolete: true
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.
Attachment #8756599 - Flags: review?(robert.strong.bugs)
The Mac mochitest failures are succeeding on the build system without your patch... right?
Flags: needinfo?(mhowell)
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.
Attachment #8756599 - Flags: review?(robert.strong.bugs)
Talked with Mat over irc regarding the failures he is seeing locally.
Flags: needinfo?(mhowell)
Assignee

Updated

3 years ago
Attachment #8756599 - Attachment description: Patch - Always require the update working directory to be within the installation path on Windowsbug1246972.diff → 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.
Attachment #8756599 - Attachment is obsolete: true
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.
Attachment #8757479 - Flags: review?(robert.strong.bugs)
https://hg.mozilla.org/projects/oak/rev/f38999684d8c02dba1befd26a8d9f0d6056ef928
Bug 1246972 - Always require the update working directory to be within the installation path; r?rstrong
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 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;
Attachment #8757479 - Flags: review?(robert.strong.bugs) → review-
Posted patch Patch (obsolete) — Splinter Review
(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.
Attachment #8757479 - Attachment is obsolete: true
Attachment #8761845 - Flags: review?(robert.strong.bugs)
Posted patch Patch (obsolete) — Splinter Review
Updated patch; removed creation of extra working directory within the apply-to path.
Attachment #8761845 - Attachment is obsolete: true
Attachment #8761845 - Flags: review?(robert.strong.bugs)
Attachment #8763991 - Flags: review?(robert.strong.bugs)
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.
Attachment #8763991 - Flags: review?(robert.strong.bugs) → review-
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #8763991 - Attachment is obsolete: true
Attachment #8772624 - Flags: review?(robert.strong.bugs)
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.
Posted patch Patch (obsolete) — Splinter Review
Okay, let's try this one.
Attachment #8772624 - Attachment is obsolete: true
Attachment #8773858 - Flags: review?(robert.strong.bugs)
Matt, just in case please run try for asan, static analysis, etc. just in case they catch something.
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.
Flags: needinfo?(mhowell)
Looks like that run didn't turn up any issues.
Flags: needinfo?(robert.strong.bugs)
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;
   }
Flags: needinfo?(robert.strong.bugs)
Attachment #8773858 - Flags: review?(robert.strong.bugs) → review-
BTW: it looks like some Windows style newlines might have snuck into your patch
(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.
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #8773858 - Attachment is obsolete: true
Attachment #8777584 - Flags: review?(robert.strong.bugs)
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 on attachment 8777584 [details] [diff] [review]
Patch

Clearing review until comment #45 is answered
Flags: needinfo?(mhowell)
Attachment #8777584 - Flags: review?(robert.strong.bugs)
(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.
Flags: needinfo?(mhowell)
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 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.
Attachment #8777584 - Flags: review-
(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.
Flags: needinfo?(robert.strong.bugs)
It depends on how difficult it is to differentiate it in telemetry. I suspect it won't be that easy.
Flags: needinfo?(robert.strong.bugs)
Posted patch Patch (obsolete) — Splinter Review
Added new error code and test.
Attachment #8777584 - Attachment is obsolete: true
Attachment #8778458 - Flags: review?(robert.strong.bugs)
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.
Attachment #8778458 - Flags: review?(robert.strong.bugs) → review+
(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.
I'll do some manual update testing on oak after it is landed as well. Thanks!
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.
Posted patch Patch (obsolete) — Splinter Review
Rebased version of attachment 8778458 [details] [diff] [review]; r+ carried over.
Attachment #8778458 - Attachment is obsolete: true
Attachment #8784879 - Flags: sec-approval?
Attachment #8784879 - Flags: review+
Assignee

Updated

3 years ago
Attachment #8784879 - Flags: sec-approval?
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.
Attachment #8784879 - Flags: sec-approval?
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.
Attachment #8784879 - Flags: sec-approval? → sec-approval+
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 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
Attachment #8784879 - Flags: approval-mozilla-aurora?
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
Attachment #8784879 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Note that patch was backed out from inbound for asan failures, so don't actually push that one to aurora. Attempted fix incoming.
Flags: needinfo?(mhowell)
Posted patch Fixed PatchSplinter Review
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.
Attachment #8784879 - Attachment is obsolete: true
Attachment #8785392 - Flags: sec-approval?
Attachment #8785392 - Flags: review+
Attachment #8785392 - Flags: sec-approval? → sec-approval+
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.
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/c908e9677a9b
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Flags: sec-bounty?
Is this ready to request uplift? We're running out of time if this is going to make 49.
Flags: needinfo?(mhowell)
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.
Flags: needinfo?(mhowell)
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.
Group: toolkit-core-security → core-security-release
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.
Attachment #8785392 - Flags: approval-mozilla-aurora?
We need to fix this in the ESR release that matches the Firefox release that fixes it (looking like 50/45.5)
Flags: sec-bounty? → sec-bounty+
Comment on attachment 8785392 [details] [diff] [review]
Fixed Patch

Approving for Aurora. We should take this on ESR45 as well.
Attachment #8785392 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Can you make an ESR45 patch and nominate it, Matt?
Flags: needinfo?(mhowell)
Posted patch ESR45 Patch (obsolete) — Splinter Review
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
Flags: needinfo?(mhowell)
Attachment #8790451 - Flags: approval-mozilla-esr45?
Comment on attachment 8790451 [details] [diff] [review]
ESR45 Patch

sec-high, taking it on the esr branch
Attachment #8790451 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
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.
Attachment #8790451 - Attachment is obsolete: true
Attachment #8801906 - Flags: approval-mozilla-esr45?
Attachment #8801906 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Attachment #8790451 - Flags: approval-mozilla-esr45+ → approval-mozilla-esr45-
Flags: qe-verify+
QA Contact: kjozwiak
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main50+][adv-esr45.5+]
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Alias: CVE-2016-5294
Group: core-security-release
QA Contact: kjozwiak
You need to log in before you can comment on or make changes to this bug.