Closed Bug 1189843 Opened 9 years ago Closed 5 years ago

Verify MAR files are signed for all platforms and if not, enable MAR signing for all platforms

Categories

(Thunderbird :: Build Config, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: robert.strong.bugs, Unassigned)

References

Details

Attachments

(2 files, 1 obsolete file)

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
Filed bug 1189845 for SeaMonkey
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
Attached patch Fix - v1 (obsolete) β€” β€” Splinter Review
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
Flags: needinfo?(philipp)
Bug 1182352 has finally landed on inbound.
Attached patch Fix - v2 β€” β€” Splinter Review
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)
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+
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)
(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)
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)
Flags: needinfo?(robert.strong.bugs) → needinfo?(philipp)
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.
Attached patch bug1370342.patch β€” β€” Splinter Review
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)
Flags: needinfo?(philipp)
Attachment #8878789 - Flags: feedback?(philipp)
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
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?)

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

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

It looks like everything here has been done in other bugs so resolving as wfm

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: