728301 - Enable new security checks only for the service (Temporarily)

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Brian R. Bondy [:bbondy]]

Attachment: Patch v1.

This patch makes it so:
1) the product information block check, and
2) the signature check
will only happen with service updates.  

Updates without the service installed and updates that fallback to the updater without the service will not do the security checks (as they didn't before the service landed).

This task is planned to be reverted after testing is complete and we feel confident bringing over the new security checks into the old updater method.

Comment 1

[Brian R. Bondy [:bbondy]]

The change in the middle of the block is just indented with an if() {} around it.  The diff makes it look like a bigger change than it actually is.

Comment 2

[Ian Melven :imelven]

Comment on attachment 598284 [details] [diff] [review]
Patch v1.

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

One question, otherwise code looks good.

::: toolkit/mozapps/update/updater/updater.cpp
@@ +1602,5 @@
> +                   NS_T("%supdate-settings.ini"), gDestPath);
> +      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 installatins won't even include this file.

typo: installations.

More importantly though, which sorts of installations won't include this file ? Will they be aware they won't be protected from cross channel/product updates ? I assume this is mostly around partner repacks ?

Comment 3

[Brian R. Bondy [:bbondy]]

Thanks for the pass Ian.

> More importantly though, which sorts of installations won't include this file ? 
> Will they be aware they won't be protected from cross channel/product updates
> ? I assume this is mostly around partner repacks ?

I expect that they all will, but this would be a releng decision. By the way that comment and code is related to the product information ticket and not directly to this ticket, but it's fine to talk about it here.

Will fix the typo nit after rstrong's review comments.

Comment 4

[Robert Strong (they/them - no direct email)]

Comment on attachment 598284 [details] [diff] [review]
Patch v1.

>+  if (performMARChecks) {
>+    #ifdef MOZ_VERIFY_MAR_SIGNATURE
nit: please don't indent #ifdef, #ifndef, #endif etc. since the rest of this file doesn't

Comment 5

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2

Fixed nits from imelven and rstrong.

Comment 6

[Daniel Veditz [:dveditz]]

Why is this a security bug? Surely our code and design is public somewhere. Is it an oblique security review request? Guess I'll go with "sg:nse"

Comment 7

[Brian R. Bondy [:bbondy]]

> Why is this a security bug? Surely our code and design is public somewhere. 
> Is it an oblique security review request? Guess I'll go with "sg:nse"

Pretty much all of the libmar security enhancements are only somewhat security sensitive, but while all of the other ones are marked that way...

I marked it as security sensitive since it is based on bugs that are currently marked as security sensitive.  And since it is a bug to disable those features it will involve source code that affects those features and also probably contains conversations about those security sensitive bugs.

Comment 8

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/mozilla-central/rev/0bde78f1b76a

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

What is the bug # tracking the reverting of this change?

Comment 10

[Brian R. Bondy [:bbondy]]

I'll be posting one with a patch a bit later today and will CC you and link it up.

Comment 11

[Brian R. Bondy [:bbondy]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/18840d884c97