Last Comment Bug 728301 - Enable new security checks only for the service (Temporarily)
: Enable new security checks only for the service (Temporarily)
Status: RESOLVED FIXED
[sg:nse][qa-]
:
Product: Toolkit
Classification: Components
Component: Application Update (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla12
Assigned To: Brian R. Bondy [:bbondy]
:
: Robert Strong [:rstrong] (use needinfo to contact me)
Mentors:
Depends on: 728935
Blocks: 699700 708688 708690 708697 730792 730821
  Show dependency treegraph
 
Reported: 2012-02-17 10:26 PST by Brian R. Bondy [:bbondy]
Modified: 2012-04-19 11:53 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
fixed
fixed
unaffected


Attachments
Patch v1. (3.52 KB, patch)
2012-02-17 10:26 PST, Brian R. Bondy [:bbondy]
robert.strong.bugs: review+
ian.melven: feedback+
Details | Diff | Splinter Review
Patch v2 (3.51 KB, patch)
2012-02-18 12:40 PST, Brian R. Bondy [:bbondy]
netzen: review+
Details | Diff | Splinter Review

Description Brian R. Bondy [:bbondy] 2012-02-17 10:26:58 PST
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.
Comment 1 Brian R. Bondy [:bbondy] 2012-02-17 10:27:59 PST
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 2012-02-17 12:15:07 PST
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] 2012-02-17 12:45:35 PST
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 [:rstrong] (use needinfo to contact me) 2012-02-17 14:23:33 PST
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] 2012-02-18 12:40:33 PST
Created attachment 598569 [details] [diff] [review]
Patch v2

Fixed nits from imelven and rstrong.
Comment 6 Daniel Veditz [:dveditz] 2012-02-22 16:39:56 PST
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] 2012-02-22 16:43:22 PST
> 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] 2012-02-24 13:42:05 PST
http://hg.mozilla.org/mozilla-central/rev/0bde78f1b76a
Comment 9 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-02-26 22:02:41 PST
What is the bug # tracking the reverting of this change?
Comment 10 Brian R. Bondy [:bbondy] 2012-02-27 04:40:07 PST
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] 2012-02-29 18:24:21 PST
http://hg.mozilla.org/releases/mozilla-aurora/rev/18840d884c97

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