Last Comment Bug 709158 - updater.exe is not verified to be a particular product's/channel's/version of the updater
: updater.exe is not verified to be a particular product's/channel's/version of...
Status: RESOLVED FIXED
[sg:moderate][qa-]
:
Product: Toolkit
Classification: Components
Component: Application Update (show other bugs)
: Trunk
: x86 Windows Vista
: -- normal (vote)
: mozilla12
Assigned To: Brian R. Bondy [:bbondy]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-09 09:43 PST by Ian Melven :imelven
Modified: 2012-05-18 13:26 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
unaffected


Attachments
Patch v1. (8.70 KB, patch)
2011-12-10 19:03 PST, Brian R. Bondy [:bbondy]
ian.melven: feedback+
Details | Diff | Splinter Review
Patch v2. (8.74 KB, patch)
2011-12-12 19:44 PST, Brian R. Bondy [:bbondy]
robert.strong.bugs: review+
Details | Diff | Splinter Review
Patch v3. (8.41 KB, patch)
2011-12-14 06:34 PST, Brian R. Bondy [:bbondy]
netzen: review+
Details | Diff | Splinter Review
Patch v4. (8.39 KB, patch)
2011-12-14 23:03 PST, Brian R. Bondy [:bbondy]
netzen: review+
Details | Diff | Splinter Review
Patch v5. (9.50 KB, patch)
2011-12-15 09:02 PST, Brian R. Bondy [:bbondy]
netzen: review+
Details | Diff | Splinter Review

Description Ian Melven :imelven 2011-12-09 09:43:48 PST
While the certificate of the updater.exe is checked while performing an update via the maintenance service, the updater is not checked to be of the correct version, product or channel. This can result in an older updater being run which may contain security vulnerabilities or flaws or be missing security functionality altogether. One suggestion made during discussions of the security of the silent updates feature was to check the updater being run against the one in the directory being installed to to verify they are the same. This is a more specific variant of the issue where any signed executable could run as an updater (which probably should be filed as another bug) - this is about running an updater, but the wrong one.

Marking sg:moderate, but this is subject to discussion by both the secteam and the silent updates team if there are differing opinions.
Comment 1 Ian Melven :imelven 2011-12-09 10:16:01 PST
Bug 709173 deals with the fact that only the cert is verified on updater.exe and it could be any executable signed by Mozilla.
Comment 2 Robert Strong [:rstrong] (use needinfo to contact me) 2011-12-09 11:27:59 PST
So, verifying that this is the same updater as the updater from the install that is to be updated should suffice for this bug?
Comment 3 Ian Melven :imelven 2011-12-09 11:35:08 PST
(In reply to Robert Strong [:rstrong] (do not email) from comment #2)
> So, verifying that this is the same updater as the updater from the install
> that is to be updated should suffice for this bug?

yes, i believe that verifying the updater that's going to be run (elevated) to do the update is the same actual updater in the install directory addresses this. Then we know the updater about to be run is for the right product (it's the same as the one installed with Firefox), the right version (it's the one that's in the current install dir) and the right channel (it's the one that's in the current install dir).
Comment 4 Brian R. Bondy [:bbondy] 2011-12-10 19:03:52 PST
Created attachment 580711 [details] [diff] [review]
Patch v1.

Now compare that the updater.exe we are comparing is the same as the one in the installation directory we are patching.

Pushed to elm and tested.
Comment 5 Ian Melven :imelven 2011-12-12 12:04:39 PST
Comment on attachment 580711 [details] [diff] [review]
Patch v1.

Review of attachment 580711 [details] [diff] [review]:
-----------------------------------------------------------------

looks good, a few nits.

::: toolkit/components/maintenanceservice/servicebase.cpp
@@ +68,5 @@
> +    return FALSE;
> +  }
> +
> +  if (fileSize1 != fileSize2) {
> +    // sameContent is already set to FALSE

great comment !

::: toolkit/components/maintenanceservice/servicebase.h
@@ +43,5 @@
> +
> +// 32KiB for comparing files at a time seems reasonable.
> +// The bigger the better for speed, but this will be used
> +// on the stack so I don't want it to be too big.
> +#define COMAPARE_BLOCKSIZE 32768

typo : COMPARE_BLOCKSIZE 

32 KB sounds very reasonable to me !

::: toolkit/components/maintenanceservice/workmonitor.cpp
@@ +344,5 @@
>    int argcTmp = 0;
>    LPWSTR* argvTmp = CommandLineToArgvW(cmdlineBufferWide, &argcTmp);
>  
> +  // Verify that the updater.exe that we are executing is the same
> +  // as the one in the installation directory.

perhaps add a comment here that the one we are checking is actually
the one in the dir we are installing to, which comes from the apply-update-to
param of workitem (argvTmp[2] which is actually not a command line argument to the
service.. )
Comment 6 Brian R. Bondy [:bbondy] 2011-12-12 19:44:19 PST
Created attachment 581156 [details] [diff] [review]
Patch v2.

Thanks for the pass by on the patches.

With 10 instances of COMAPRE_BLOCKSIZE I realized just how often I use copy/paste :)
Comment 7 Robert Strong [:rstrong] (use needinfo to contact me) 2011-12-14 03:29:39 PST
Comment on attachment 581156 [details] [diff] [review]
Patch v2.

>diff --git a/toolkit/components/maintenanceservice/servicebase.cpp b/toolkit/components/maintenanceservice/servicebase.cpp
>--- a/toolkit/components/maintenanceservice/servicebase.cpp
>+++ b/toolkit/components/maintenanceservice/servicebase.cpp
>@@ -34,8 +34,84 @@
>...
>+BOOL VerifySameFiles(LPCWSTR file1Path, LPCWSTR file2Path, BOOL &sameContent)
Per the style guide
BOOL
VerifySameFiles(LPCWSTR file1Path, LPCWSTR file2Path, BOOL &sameContent)

>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
>...
>@@ -332,22 +333,57 @@ ProcessWorkItem(LPCWSTR monitoringBasePa
>     LOG(("Could not read command data for command meta file: %ls\n", 
>          notifyInfo.FileName));
>     SetEvent(serviceRunningEvent);
>     return TRUE;
>   }
>   cmdlineBuffer[cmdLineBufferRead] = '\0';
>   cmdlineBuffer[cmdLineBufferRead + 1] = '\0';
>   WCHAR *cmdlineBufferWide = reinterpret_cast<WCHAR*>(cmdlineBuffer.get());
>-  LOG(("An update command was detected and is being processed for command meta "
>-       "file: %ls\n", notifyInfo.FileName));
>+  LOG(("An update command was detected and is being processed for command meta"
>+       " file: %ls\n", notifyInfo.FileName));
nit: this line appears to be 80 so this change is unnecessary.

Looks good!
Comment 8 Brian R. Bondy [:bbondy] 2011-12-14 06:34:15 PST
Created attachment 581620 [details] [diff] [review]
Patch v3.

Fixed nits.
Remarking as r+ since the last patch was r+ w/ nits.
Comment 9 Brian R. Bondy [:bbondy] 2011-12-14 23:03:35 PST
Created attachment 581886 [details] [diff] [review]
Patch v4.

Rebased.
Comment 10 Brian R. Bondy [:bbondy] 2011-12-15 09:02:26 PST
Created attachment 581998 [details] [diff] [review]
Patch v5.

Rebased.
Comment 11 Brian R. Bondy [:bbondy] 2012-01-04 22:23:35 PST
https://hg.mozilla.org/mozilla-central/rev/7fbfb0e72565
Comment 12 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-29 15:08:57 PDT
Is this something QA can verify?
Comment 13 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-04-26 14:02:41 PDT
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #12)
> Is this something QA can verify?

Assuming that's a 'no' based on lack of response.
Comment 14 Brian R. Bondy [:bbondy] 2012-04-26 14:27:40 PDT
Sorry must have missed this one, I don't know of an easy way to test this beyond what we have seen ride through the train.

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