Bug 1246972 (CVE-2016-5294)

Arbitrary target directory for result files of update process

VERIFIED FIXED in Firefox 50

Status

()

Toolkit
Application Update
VERIFIED FIXED
a year ago
2 months ago

People

(Reporter: Holger Fuhrmannek, Assigned: mhowell)

Tracking

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

unspecified
mozilla51
csectype-priv-escalation, sec-high
Points:
---
Bug Flags:
sec-bounty +
in-testsuite +

Firefox Tracking Flags

(firefox46+ wontfix, firefox47+ wontfix, firefox48+ wontfix, firefox49+ wontfix, firefox50+ verified, firefox51 verified, firefox-esr4550+ 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

a year ago
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.
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.
status-firefox46: --- → affected
status-firefox47: --- → affected
status-firefox48: --- → affected
tracking-firefox46: --- → +
tracking-firefox47: --- → +
tracking-firefox48: --- → +
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
OK, thanks Robert.
status-firefox46: affected → wontfix
Any progress?  Wontfix this for beta 47 at this point.
status-firefox47: affected → wontfix
status-firefox49: --- → affected
tracking-firefox49: --- → +
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)
(Assignee)

Comment 9

a year ago
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)
Flags: needinfo?(mhowell)
(Assignee)

Comment 11

a year ago
(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)
Flags: needinfo?(mhowell)
(Assignee)

Comment 13

a year ago
(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.
(Assignee)

Comment 15

a year ago
(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.
(Assignee)

Comment 16

a year ago
Created attachment 8753043 [details] [diff] [review]
Do update install directory registry check earlier and unconditionally

MozReview-Commit-ID: FR44f4b5KbI
(Assignee)

Updated

a year ago
Attachment #8753043 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 17

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=18dd63221a07
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-
(Assignee)

Comment 19

a year ago
Created attachment 8753902 [details] [diff] [review]
Do update install directory registry check earlier and unconditionally

MozReview-Commit-ID: FR44f4b5KbI
(Assignee)

Updated

a year ago
Attachment #8753043 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8753902 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 20

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed5cdf7a1f6d
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)
(Assignee)

Comment 22

a year ago
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)
(Assignee)

Comment 24

a year ago
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.
(Assignee)

Comment 25

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=acca8834c9cc
(Assignee)

Comment 26

a year ago
Created attachment 8756466 [details] [diff] [review]
Always require the update working directory to be within the installation path

MozReview-Commit-ID: ASxfV4uVpmD
(Assignee)

Updated

a year ago
Attachment #8756101 - Attachment is obsolete: true
(Assignee)

Comment 27

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=40a236d9190c
(Assignee)

Comment 28

a year ago
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.
Attachment #8756466 - Attachment is obsolete: true
(Assignee)

Comment 29

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e94bcd63849
(Assignee)

Comment 30

a year ago
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

a year 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
(Assignee)

Comment 34

a year ago
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.
Attachment #8756599 - Attachment is obsolete: true
(Assignee)

Comment 35

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f037546a798b
(Assignee)

Comment 36

a year ago
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)
(Assignee)

Comment 37

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

Comment 38

a year ago
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.
status-firefox50: --- → affected
tracking-firefox50: --- → +
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-
(Assignee)

Comment 40

a year ago
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.
Attachment #8757479 - Attachment is obsolete: true
Attachment #8761845 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 41

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3109cb148dc
(Assignee)

Comment 42

a year ago
Created attachment 8763991 [details] [diff] [review]
Patch

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)
(Assignee)

Comment 43

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=910a54b4b9b9
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-
(Assignee)

Comment 45

11 months ago
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.
Attachment #8763991 - Attachment is obsolete: true
Attachment #8772624 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 46

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e490ed001eaf
(Assignee)

Comment 47

11 months ago
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.
Attachment #8772624 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 48

11 months ago
Created attachment 8773858 [details] [diff] [review]
Patch

Okay, let's try this one.
Attachment #8772624 - Attachment is obsolete: true
Attachment #8773858 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 49

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5db59389e6ae
status-firefox48: affected → wontfix
Matt, just in case please run try for asan, static analysis, etc. just in case they catch something.
Flags: needinfo?(mhowell)
(Assignee)

Comment 51

11 months ago
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)
(Assignee)

Comment 52

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7fcd0396b17b
(Assignee)

Comment 53

11 months ago
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.
(Assignee)

Comment 57

11 months ago
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.
Attachment #8773858 - Attachment is obsolete: true
Attachment #8777584 - Flags: review?(robert.strong.bugs)
(Assignee)

Comment 58

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=37aa231d84ab
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)
bah! I meant comment #59
(Assignee)

Comment 62

11 months ago
(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-
(Assignee)

Comment 65

11 months ago
(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)
(Assignee)

Comment 67

11 months ago
Created attachment 8778458 [details] [diff] [review]
Patch

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+
(Assignee)

Comment 69

11 months ago
(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!
(Assignee)

Comment 71

11 months ago
https://hg.mozilla.org/projects/oak/rev/4afd44e5b901
(Assignee)

Comment 72

10 months ago
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.
(Assignee)

Comment 73

10 months ago
Created attachment 8784879 [details] [diff] [review]
Patch

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

10 months ago
Attachment #8784879 - Flags: sec-approval?
(Assignee)

Comment 74

10 months ago
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+

Updated

10 months ago
status-firefox-esr45: --- → affected
(Assignee)

Comment 76

10 months ago
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
(Assignee)

Comment 77

10 months ago
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?
(Assignee)

Comment 78

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

Comment 79

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/41ee5cb3266c
Flags: needinfo?(mhowell)
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

Updated

10 months ago
Attachment #8784879 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 81

10 months ago
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)
(Assignee)

Comment 82

10 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=27cdfe581567
(Assignee)

Comment 83

10 months ago
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.
Attachment #8784879 - Attachment is obsolete: true
Attachment #8785392 - Flags: sec-approval?
Attachment #8785392 - Flags: review+

Updated

10 months ago
Attachment #8785392 - Flags: sec-approval? → sec-approval+

Updated

10 months ago
Keywords: checkin-needed
(Assignee)

Comment 84

10 months ago
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
(Assignee)

Comment 85

10 months ago
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

10 months ago
https://hg.mozilla.org/mozilla-central/rev/c908e9677a9b
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51

Updated

10 months ago
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)
(Assignee)

Comment 88

10 months ago
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.
status-firefox49: affected → wontfix

Updated

10 months ago
Group: toolkit-core-security → core-security-release
(Assignee)

Comment 90

10 months ago
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)
tracking-firefox-esr45: --- → 50+

Updated

10 months ago
Flags: sec-bounty? → sec-bounty+
Keywords: csectype-priv-escalation
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)
(Assignee)

Comment 94

10 months ago
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
Flags: needinfo?(mhowell)
Attachment #8790451 - Flags: approval-mozilla-esr45?
https://hg.mozilla.org/releases/mozilla-aurora/rev/89ba44134660
status-firefox50: affected → fixed
Flags: in-testsuite+
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+
(Assignee)

Comment 97

8 months ago
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.
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-
(Assignee)

Comment 98

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

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

Updated

8 months ago
status-firefox-esr45: affected → fixed
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
status-firefox50: fixed → verified
status-firefox51: fixed → verified
status-firefox-esr45: fixed → verified
Flags: qe-verify+
Alias: CVE-2016-5294
Group: core-security-release
Depends on: 1362267
QA Contact: kjozwiak
You need to log in before you can comment on or make changes to this bug.