709158 - updater.exe is not verified to be a particular product's/channel's/version of the updater

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Ian Melven :imelven]

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]

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 (they/them - no direct email)]

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]

(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]]

Attachment: 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]

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]]

Attachment: 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 (they/them - no direct email)]

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]]

Attachment: Patch v3.

Fixed nits.
Remarking as r+ since the last patch was r+ w/ nits.

Comment 9

[Brian R. Bondy [:bbondy]]

Attachment: Patch v4.

Rebased.

Comment 10

[Brian R. Bondy [:bbondy]]

Attachment: Patch v5.

Rebased.

Comment 11

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/mozilla-central/rev/7fbfb0e72565

Comment 12

[u279076]

Is this something QA can verify?

Comment 13

[u279076]

(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]]

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.