Closed Bug 728301 Opened 9 years ago Closed 9 years ago

Enable new security checks only for the service (Temporarily)

Categories

(Toolkit :: Application Update, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12
Tracking Status
firefox11 --- unaffected
firefox12 --- fixed
firefox13 --- fixed
firefox-esr10 --- unaffected

People

(Reporter: bbondy, Assigned: bbondy)

References

Details

(Whiteboard: [sg:nse][qa-])

Attachments

(1 file, 1 obsolete file)

Attached patch Patch v1. (obsolete) — Splinter Review
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.
Attachment #598284 - Flags: review?(robert.bugzilla)
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.
Attachment #598284 - Flags: feedback?(imelven)
Summary: Enable new security checks only for the service → Enable new security checks only for the service (Temporarily)
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 ?
Attachment #598284 - Flags: feedback?(imelven) → feedback+
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 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
Attachment #598284 - Flags: review?(robert.bugzilla) → review+
Attached patch Patch v2Splinter Review
Fixed nits from imelven and rstrong.
Attachment #598284 - Attachment is obsolete: true
Attachment #598569 - Flags: review+
Depends on: 728935
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"
Whiteboard: [sg:nse]
> 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.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
What is the bug # tracking the reverting of this change?
I'll be posting one with a patch a bit later today and will CC you and link it up.
Blocks: 730792
Blocks: 730821
Whiteboard: [sg:nse] → [sg:nse][qa-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.