Last Comment Bug 1246972 - (CVE-2016-5294) Arbitrary target directory for result files of update process
(CVE-2016-5294)
: Arbitrary target directory for result files of update process
Status: VERIFIED FIXED
[post-critsmash-triage][adv-main50+][...
: csectype-priv-escalation, sec-high
Product: Toolkit
Classification: Components
Component: Application Update (show other bugs)
: unspecified
: Unspecified Unspecified
-- normal (vote)
: mozilla51
Assigned To: Matt Howell [:mhowell]
: Kamil Jozwiak [:kjozwiak]
: Robert Strong [:rstrong] (use needinfo to contact me)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2016-02-09 08:45 PST by Holger Fuhrmannek
Modified: 2017-02-09 08:03 PST (History)
12 users (show)
dveditz: sec‑bounty+
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
wontfix
+
wontfix
+
wontfix
+
wontfix
+
verified
verified
50+
verified


Attachments
invalid.loc.poc.zip (854 bytes, application/zip)
2016-02-09 08:45 PST, Holger Fuhrmannek
no flags Details
Do update install directory registry check earlier and unconditionally (3.57 KB, patch)
2016-05-16 13:58 PDT, Matt Howell [:mhowell]
robert.strong.bugs: review-
Details | Diff | Splinter Review
Do update install directory registry check earlier and unconditionally (3.63 KB, patch)
2016-05-18 08:57 PDT, Matt Howell [:mhowell]
no flags Details | Diff | Splinter Review
Patch - Always require the update working directory to be within the installation path (6.97 KB, patch)
2016-05-24 22:52 PDT, Matt Howell [:mhowell]
no flags Details | Diff | Splinter Review
Always require the update working directory to be within the installation path (7.05 KB, patch)
2016-05-25 11:20 PDT, Matt Howell [:mhowell]
no flags Details | Diff | Splinter Review
Patch - Always require the update working directory to be within the installation path on Windows (7.92 KB, patch)
2016-05-25 15:51 PDT, Matt Howell [:mhowell]
no flags Details | Diff | Splinter Review
Patch - Always require the update working directory to be within the installation path on Windows (7.94 KB, patch)
2016-05-27 13:19 PDT, Matt Howell [:mhowell]
robert.strong.bugs: review-
Details | Diff | Splinter Review
Patch (10.92 KB, patch)
2016-06-09 15:35 PDT, Matt Howell [:mhowell]
no flags Details | Diff | Splinter Review
Patch (8.10 KB, patch)
2016-06-21 11:19 PDT, Matt Howell [:mhowell]
robert.strong.bugs: review-
Details | Diff | Splinter Review
Patch (40.49 KB, patch)
2016-07-19 16:10 PDT, Matt Howell [:mhowell]
no flags Details | Diff | Splinter Review
Patch (45.77 KB, patch)
2016-07-22 10:09 PDT, Matt Howell [:mhowell]
robert.strong.bugs: review-
Details | Diff | Splinter Review
Patch (45.88 KB, patch)
2016-08-03 16:02 PDT, Matt Howell [:mhowell]
robert.strong.bugs: review-
Details | Diff | Splinter Review
Patch (52.67 KB, patch)
2016-08-05 14:08 PDT, Matt Howell [:mhowell]
robert.strong.bugs: review+
Details | Diff | Splinter Review
Patch (53.27 KB, patch)
2016-08-25 09:23 PDT, Matt Howell [:mhowell]
mhowell: review+
abillings: approval‑mozilla‑aurora+
abillings: sec‑approval+
Details | Diff | Splinter Review
Fixed Patch (55.01 KB, patch)
2016-08-26 10:31 PDT, Matt Howell [:mhowell]
mhowell: review+
abillings: approval‑mozilla‑aurora+
abillings: sec‑approval+
Details | Diff | Splinter Review
ESR45 Patch (52.45 KB, patch)
2016-09-12 14:33 PDT, Matt Howell [:mhowell]
sledru: approval‑mozilla‑esr45-
Details | Diff | Splinter Review
ESR45 Patch Rebased (54.82 KB, patch)
2016-10-17 15:14 PDT, Matt Howell [:mhowell]
sledru: approval‑mozilla‑esr45+
Details | Diff | Splinter Review

Description User image Holger Fuhrmannek 2016-02-09 08:45:20 PST
Created attachment 8717491 [details]
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 User image Al Billings [:abillings] 2016-02-09 10:06:48 PST
Rob, can you take a look at this and maybe suggest a security rating?
Comment 2 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-02-12 10:48:14 PST
This is similar to bug 1171518 and should receive the same rating.
Comment 3 User image Liz Henry (:lizzard) (needinfo? me) 2016-03-23 10:27:23 PDT
Tracking for 46+ since this is sec-high and seems to affect all branches.
Comment 4 User image Liz Henry (:lizzard) (needinfo? me) 2016-04-08 11:36:20 PDT
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 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-04-08 11:38:36 PDT
The fix for this is likely going to be too complex to try to push for 46
Comment 6 User image Liz Henry (:lizzard) (needinfo? me) 2016-04-12 18:23:36 PDT
OK, thanks Robert.
Comment 7 User image Liz Henry (:lizzard) (needinfo? me) 2016-05-11 14:13:03 PDT
Any progress?  Wontfix this for beta 47 at this point.
Comment 8 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-05-11 14:18:59 PDT
No progress at this time.

Matt, could you take a look at this?
Comment 9 User image Matt Howell [:mhowell] 2016-05-12 10:20:31 PDT
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 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-05-13 01:38:17 PDT
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 User image Matt Howell [:mhowell] 2016-05-13 07:28:55 PDT
(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 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-05-13 13:27:23 PDT
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 User image Matt Howell [:mhowell] 2016-05-13 13:47:11 PDT
(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 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-05-13 13:49:25 PDT
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 User image Matt Howell [:mhowell] 2016-05-16 13:57:15 PDT
(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 User image Matt Howell [:mhowell] 2016-05-16 13:58:06 PDT
Created attachment 8753043 [details] [diff] [review]
Do update install directory registry check earlier and unconditionally

MozReview-Commit-ID: FR44f4b5KbI
Comment 18 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-05-17 19:18:46 PDT
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 User image Matt Howell [:mhowell] 2016-05-18 08:57:53 PDT
Created attachment 8753902 [details] [diff] [review]
Do update install directory registry check earlier and unconditionally

MozReview-Commit-ID: FR44f4b5KbI
Comment 21 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-05-19 01:19:48 PDT
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 User image Matt Howell [:mhowell] 2016-05-23 22:41:14 PDT
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 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-05-23 23:06:40 PDT
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 User image Matt Howell [:mhowell] 2016-05-24 22:52:47 PDT
Created attachment 8756101 [details] [diff] [review]
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 26 User image Matt Howell [:mhowell] 2016-05-25 11:20:15 PDT
Created attachment 8756466 [details] [diff] [review]
Always require the update working directory to be within the installation path

MozReview-Commit-ID: ASxfV4uVpmD
Comment 28 User image Matt Howell [:mhowell] 2016-05-25 15:51:04 PDT
Created attachment 8756599 [details] [diff] [review]
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 30 User image Matt Howell [:mhowell] 2016-05-26 09:21:58 PDT
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 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-05-26 09:34:25 PDT
The Mac mochitest failures are succeeding on the build system without your patch... right?
Comment 32 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-05-26 10:18:57 PDT
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 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-05-26 10:19:26 PDT
Talked with Mat over irc regarding the failures he is seeing locally.
Comment 34 User image Matt Howell [:mhowell] 2016-05-27 13:19:35 PDT
Created attachment 8757479 [details] [diff] [review]
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 36 User image Matt Howell [:mhowell] 2016-05-31 09:36:41 PDT
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 User image Matt Howell [:mhowell] 2016-06-01 14:36:05 PDT
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 User image Matt Howell [:mhowell] 2016-06-03 08:34:13 PDT
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 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-06-08 23:38:30 PDT
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 User image Matt Howell [:mhowell] 2016-06-09 15:35:53 PDT
Created attachment 8761845 [details] [diff] [review]
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 42 User image Matt Howell [:mhowell] 2016-06-21 11:19:23 PDT
Created attachment 8763991 [details] [diff] [review]
Patch

Updated patch; removed creation of extra working directory within the apply-to path.
Comment 44 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-07-07 20:40:56 PDT
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 User image Matt Howell [:mhowell] 2016-07-19 16:10:56 PDT
Created attachment 8772624 [details] [diff] [review]
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 47 User image Matt Howell [:mhowell] 2016-07-20 11:39:08 PDT
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 User image Matt Howell [:mhowell] 2016-07-22 10:09:16 PDT
Created attachment 8773858 [details] [diff] [review]
Patch

Okay, let's try this one.
Comment 50 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-08-01 14:37:17 PDT
Matt, just in case please run try for asan, static analysis, etc. just in case they catch something.
Comment 51 User image Matt Howell [:mhowell] 2016-08-01 15:09:31 PDT
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 53 User image Matt Howell [:mhowell] 2016-08-02 08:21:49 PDT
Looks like that run didn't turn up any issues.
Comment 54 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-08-03 13:21:09 PDT
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 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-08-03 14:10:00 PDT
BTW: it looks like some Windows style newlines might have snuck into your patch
Comment 56 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-08-03 15:57:32 PDT
(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 User image Matt Howell [:mhowell] 2016-08-03 16:02:19 PDT
Created attachment 8777584 [details] [diff] [review]
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 59 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-08-03 17:24:24 PDT
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 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-08-04 08:08:51 PDT
Comment on attachment 8777584 [details] [diff] [review]
Patch

Clearing review until comment #45 is answered
Comment 61 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-08-04 08:09:25 PDT
bah! I meant comment #59
Comment 62 User image Matt Howell [:mhowell] 2016-08-04 15:31:40 PDT
(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 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-08-04 15:40:19 PDT
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 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-08-05 08:01:41 PDT
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 User image Matt Howell [:mhowell] 2016-08-05 08:05:56 PDT
(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 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-08-05 08:09:35 PDT
It depends on how difficult it is to differentiate it in telemetry. I suspect it won't be that easy.
Comment 67 User image Matt Howell [:mhowell] 2016-08-05 14:08:02 PDT
Created attachment 8778458 [details] [diff] [review]
Patch

Added new error code and test.
Comment 68 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-08-11 11:09:43 PDT
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 User image Matt Howell [:mhowell] 2016-08-11 12:21:28 PDT
(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 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-08-11 12:22:55 PDT
I'll do some manual update testing on oak after it is landed as well. Thanks!
Comment 71 User image Matt Howell [:mhowell] 2016-08-11 14:25:41 PDT
https://hg.mozilla.org/projects/oak/rev/4afd44e5b901
Comment 72 User image Matt Howell [:mhowell] 2016-08-25 09:19:26 PDT
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 User image Matt Howell [:mhowell] 2016-08-25 09:23:05 PDT
Created attachment 8784879 [details] [diff] [review]
Patch

Rebased version of attachment 8778458 [details] [diff] [review]; r+ carried over.
Comment 74 User image Matt Howell [:mhowell] 2016-08-25 09:31:24 PDT
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 User image Al Billings [:abillings] 2016-08-25 10:17:16 PDT
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 User image Matt Howell [:mhowell] 2016-08-25 10:22:44 PDT
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 User image Matt Howell [:mhowell] 2016-08-25 10:42:57 PDT
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 User image Matt Howell [:mhowell] 2016-08-25 11:06:53 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/4576637003b6ae52477041cd6a807f482bad5211
Bug 1246972 followup - Fix eslint errors; CLOSED TREE
Comment 80 User image Robert Strong [:rstrong] (use needinfo to contact me) 2016-08-25 13:33:41 PDT
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 User image Matt Howell [:mhowell] 2016-08-25 15:34:04 PDT
Note that patch was backed out from inbound for asan failures, so don't actually push that one to aurora. Attempted fix incoming.
Comment 83 User image Matt Howell [:mhowell] 2016-08-26 10:31:03 PDT
Created attachment 8785392 [details] [diff] [review]
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 User image Matt Howell [:mhowell] 2016-08-29 08:17:31 PDT
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 User image Matt Howell [:mhowell] 2016-08-29 08:18:35 PDT
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 User image Wes Kocher (:KWierso) 2016-08-29 17:47:06 PDT
https://hg.mozilla.org/mozilla-central/rev/c908e9677a9b
Comment 87 User image Benjamin Smedberg [:bsmedberg] 2016-08-31 06:44:37 PDT
Is this ready to request uplift? We're running out of time if this is going to make 49.
Comment 88 User image Matt Howell [:mhowell] 2016-08-31 08:07:09 PDT
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 User image Liz Henry (:lizzard) (needinfo? me) 2016-08-31 10:14:53 PDT
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 User image Matt Howell [:mhowell] 2016-09-01 12:15:27 PDT
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 User image Daniel Veditz [:dveditz] 2016-09-08 14:26:02 PDT
We need to fix this in the ESR release that matches the Firefox release that fixes it (looking like 50/45.5)
Comment 92 User image Al Billings [:abillings] 2016-09-12 10:50:05 PDT
Comment on attachment 8785392 [details] [diff] [review]
Fixed Patch

Approving for Aurora. We should take this on ESR45 as well.
Comment 93 User image Al Billings [:abillings] 2016-09-12 10:51:03 PDT
Can you make an ESR45 patch and nominate it, Matt?
Comment 94 User image Matt Howell [:mhowell] 2016-09-12 14:33:39 PDT
Created attachment 8790451 [details] [diff] [review]
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 User image Ryan VanderMeulen [:RyanVM] 2016-09-12 19:03:01 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/89ba44134660
Comment 96 User image Sylvestre Ledru [:sylvestre] 2016-10-17 09:00:32 PDT
Comment on attachment 8790451 [details] [diff] [review]
ESR45 Patch

sec-high, taking it on the esr branch
Comment 97 User image Matt Howell [:mhowell] 2016-10-17 15:14:05 PDT
Created attachment 8801906 [details] [diff] [review]
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 99 User image Bogdan Maris, QA [:bogdan_maris] 2016-11-08 06:01:11 PST
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.

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