Enable new security checks only for the service (Temporarily)

RESOLVED FIXED in Firefox 12

Status

()

Toolkit
Application Update
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bbondy, Assigned: bbondy)

Tracking

unspecified
mozilla12
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11 unaffected, firefox12 fixed, firefox13 fixed, firefox-esr10 unaffected)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Created attachment 598284 [details] [diff] [review]
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.
Attachment #598284 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 1

6 years ago
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.
(Assignee)

Updated

6 years ago
Attachment #598284 - Flags: feedback?(imelven)
(Assignee)

Updated

6 years ago
Summary: Enable new security checks only for the service → Enable new security checks only for the service (Temporarily)

Comment 2

6 years ago
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+
(Assignee)

Comment 3

6 years ago
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+
(Assignee)

Comment 5

6 years ago
Created attachment 598569 [details] [diff] [review]
Patch v2

Fixed nits from imelven and rstrong.
Attachment #598284 - Attachment is obsolete: true
Attachment #598569 - Flags: review+
(Assignee)

Updated

6 years ago
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]
(Assignee)

Comment 7

6 years ago
> 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.
(Assignee)

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 8

6 years ago
http://hg.mozilla.org/mozilla-central/rev/0bde78f1b76a
What is the bug # tracking the reverting of this change?
(Assignee)

Comment 10

6 years ago
I'll be posting one with a patch a bit later today and will CC you and link it up.
(Assignee)

Updated

6 years ago
Blocks: 730792
(Assignee)

Updated

6 years ago
Blocks: 730821
(Assignee)

Comment 11

6 years ago
http://hg.mozilla.org/releases/mozilla-aurora/rev/18840d884c97
status-firefox11: --- → unaffected
status-firefox12: --- → fixed
status-firefox13: --- → fixed
Target Milestone: mozilla13 → mozilla12
status-firefox-esr10: --- → unaffected
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.