Closed
Bug 1189843
Opened 9 years ago
Closed 6 years ago
Verify MAR files are signed for all platforms and if not, enable MAR signing for all platforms
Categories
(Thunderbird :: Build Config, defect)
Thunderbird
Build Config
Tracking
(Not tracked)
RESOLVED
WORKSFORME
People
(Reporter: robert.strong.bugs, Unassigned)
References
Details
Attachments
(2 files, 1 obsolete file)
10.52 KB,
patch
|
robert.strong.bugs
:
review+
|
Details | Diff | Splinter Review |
957 bytes,
patch
|
Details | Diff | Splinter Review |
The custom cert check used by app update will be removed during the 43 cycle. Instead, mar signing should be used since it provides much better security. This is already implemented on Firefox.
Configs For Firefox mar signing for reference
http://mxr.mozilla.org/mozilla-central/source/browser/confvars.sh#26
http://mxr.mozilla.org/mozilla-central/search?string=ac_add_options%20--enable-verify-mar
Bug 1182352 will remove the custom cert check code from app update. If this is not completed before the code is removed then Thunderbird will not have the security mitigation provided by the cert check.
If this bug is fixed before bug 1182352 then you should set the following prefs to false. If this is done afterwards then these prefs can be removed.
app.update.cert.checkAttributes
app.update.cert.requireBuiltIn
Reporter | ||
Comment 1•9 years ago
|
||
Filed bug 1189845 for SeaMonkey
Reporter | ||
Comment 2•9 years ago
|
||
If for whatever reason mar signing is not implemented then real cert pinning should be implemented. We didn't do this for Firefox aus since it has the same problems as the custom cert check. See bug bug 1063111
Comment 3•9 years ago
|
||
I guess it would be something like this. Untested because c-c is broken atm and I don't want to update my c-c clone for a try push.
Assignee: nobody → philipp
Status: NEW → ASSIGNED
Updated•9 years ago
|
Flags: needinfo?(philipp)
Reporter | ||
Comment 4•8 years ago
|
||
Bug 1182352 has finally landed on inbound.
Comment 5•8 years ago
|
||
Looks like I forgot about this one. Here is an unbitrotted patch and a try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=66d651d030f933fa4499a978bc43b2923c96f367
The failures look similar to what we have on comm-central already, so nothing new.
Rob, would you mind checking if this is the right thing to do? Are there more prefs in app.update we can remove?
Attachment #8642250 -
Attachment is obsolete: true
Flags: needinfo?(philipp)
Attachment #8851358 -
Flags: review?(robert.strong.bugs)
Reporter | ||
Comment 6•8 years ago
|
||
Comment on attachment 8851358 [details] [diff] [review]
Fix - v2
>diff --git a/mail/app/profile/all-thunderbird.js b/mail/app/profile/all-thunderbird.js
>--- a/mail/app/profile/all-thunderbird.js
>+++ b/mail/app/profile/all-thunderbird.js
>@@ -49,22 +49,16 @@ pref("app.update.timerFirstInterval", 30
> // App-specific update preferences
>
> // The interval to check for updates (app.update.interval) is defined in
> // the branding files.
>
> // Enables some extra Application Update Logging (can reduce performance)
> pref("app.update.log", false);
>
>-// When |app.update.cert.checkAttributes| is true or not specified the
>-// certificate attributes specified in the |app.update.certs.| preference branch
>-// are checked against the certificate for the url specified by the
>-// |app.update.url| preference.
>-pref("app.update.cert.checkAttributes", true);
>-
> // The number of certificate attribute check failures to allow for background
> // update checks before notifying the user of the failure. User initiated update
> // checks always notify the user of the certificate attribute check failure.
> pref("app.update.cert.maxErrors", 5);
This should also be removed.
The mozconfig changes appear correct. It has been awhile since I had to enable mar signing so there may be other things that need to be done but I can't think of anything else off the top of my head. With that in mind this would be a good thing to verify before releasing to users.
Attachment #8851358 -
Flags: review?(robert.strong.bugs) → review+
Comment 7•8 years ago
|
||
Thanks for the review! Two questions left:
1) What about app.update.certs.*.issuerName and app.update.certs.*.commonName ? I don't see these in mozilla-central anywhere except for a mention in a comment at https://dxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js#5457 but I'd rather ask twice given certificate information is included.
2) How would we verify that everything has worked correctly on a release build?
Flags: needinfo?(robert.strong.bugs)
Reporter | ||
Comment 8•8 years ago
|
||
(In reply to Philipp Kewisch [:Fallen] from comment #7)
> Thanks for the review! Two questions left:
>
> 1) What about app.update.certs.*.issuerName and
> app.update.certs.*.commonName ? I don't see these in mozilla-central
> anywhere except for a mention in a comment at
> https://dxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.
> js#5457 but I'd rather ask twice given certificate information is included.
Just remove them... they aren't in use as of bug 1182352 landing. Specifically, "remove app.update.certs.* preferences".
> 2) How would we verify that everything has worked correctly on a release
> build?
Typically these types of significant changes don't go immediately to release so hopefully it will be verified on a nightly. Verify by manually testing that updating was successful and it would be a good thing to verify that the next nightly was also successful.
Flags: needinfo?(robert.strong.bugs)
Reporter | ||
Comment 9•7 years ago
|
||
I am planning on removing the hash checks in the near future in bug 1373267. It would be a good thing for Thunderbird to get this bug fixed.
Flags: needinfo?(robert.strong.bugs)
Reporter | ||
Updated•7 years ago
|
Flags: needinfo?(robert.strong.bugs) → needinfo?(philipp)
Reporter | ||
Comment 11•7 years ago
|
||
Note: releng would like to have a watershed for the removal of the hash checks on the client and 56 will be a watershed for other reasons so the hash checks removal will also be landed for 56.
Comment 12•7 years ago
|
||
Philipp, I created this patch for bug 1370342. What do you think, would you use it together with your patch?
Attachment #8878789 -
Flags: feedback?(philipp)
Updated•6 years ago
|
Flags: needinfo?(philipp)
Attachment #8878789 -
Flags: feedback?(philipp)
Comment 13•6 years ago
|
||
I'm not going to pursue this bug for the time being. If this is something we need to do, maybe rjl can prioritize.
Assignee: philipp → nobody
Status: ASSIGNED → NEW
Comment 14•6 years ago
|
||
Not sure what's left here.
app.update.cert.requireBuiltIn is already gone (except for in suite/) and we have MOZ_ENABLE_SIGNMAR=1 set.
https://searchfox.org/comm-central/source/mail/confvars.sh#10 - instead of MOZ_VERIFY_MAR_SIGNATURE=1
... firefox seems to have the per-platform enable-verify-mar options set (for all?)
Reporter | ||
Comment 15•6 years ago
|
||
I removed the remaining prefs for Thunderbird in bug 1549242.
I think the last step would be to check that the MAR files are signed for all platforms.
Summary: Enable mar signing for all platforms and remove app.update.certs.* preferences → Verify MAR files are signed for all platforms and if not, enable MAR signing for all platforms
Comment 16•6 years ago
|
||
MAR signing is the "ms" group of jobs that show up in Treeherder. I downloaded a couple of them from 67.0b3 and verified the signature is present:
$ mar -T thunderbird-67.0b3.complete.mar
Compression type: xz
Signature type: sha384
Signature block found with 1 signature
- Signature 2 size 512
1 additional block found:
- Product Information Block:
- MAR channel name: thunderbird-comm-beta
- Product version: 67.0
Reporter | ||
Comment 17•6 years ago
|
||
It looks like everything here has been done in other bugs so resolving as wfm
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•