903126 - Implement a platform independent way to determine which cert to use for verifying mars

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Brian R. Bondy [:bbondy]]

This is in preparation for verifying MARs on all platforms.
RelEng: Please read over the below proposal and let me know if this sounds reasonable to you.
Note: We may need help when you have time if you agree with the proposal, to sign the test MARs as part of the build process.

updater.exe currently contains two certs: A) primary cert, and B) xpcshell cert.
The existence of a special key in the registry (which was manually added to each talos machine by releng) determines which cert will be used: A or B.
If the special registry key exists, then the xpcshell cert (B) is used for verifying MARs. 
If the special registry key does not exist, then the normal primary cert (A) is used for verifying MARs.
The MAR files that are used in xpcshell tests are signed by the private key associated with the xpcshell cert just mentioned.
The MAR files distributed for update, are verified with the primary cert, since all of the people doing updates do not have that special registry key in a secure location.

The problem with that approach is twofold:
i) It's Windows specific, we want MAR verification on all platforms now
ii) It is an annoyance when trying to run tests locally, because you need to add this special registry key manually.

Here is a proposal for how to make this platform independent and get rid of the testing annoyance:

1) We need to add a new configure option for --sign-test-mars.
This new configure option would be set in the .mozconfig of release, beta, aurora, nightly, and dep builds.
This new configure option would not be set by default for local builds.

The new configure option would sign a set of MAR files that we use for xpcshell testing, in the same way as it signs the .mar files for that build.
This would be done before packaging the tests.

2) We would remove updater.exe storing both the primary and xpcshell cert, and it would now only store a primary cert.
When the build was done with the configure option, the primary cert is the normal primary cert.
When the build is done without the configure option (local builds), we make the primary cert contain the xpcshell cert instead.

3) By doing this we would not need a check to determine whether to use the xpchsell cert or primary cert, we'd just always use the primary cert for verifying.  Note that the fallback "special registry key" will still be in use for updater.exe checks for the maintenance service, but it will no longer be used for MAR verification.

Comment 1

[Daniel Veditz [:dveditz]]

I'm all for getting rid of the xpcshell test cert if possible, but only if we can do it without having to spread the primary key to all the build machines. There may be reasons we can't use the existing signing service and in that case we may have to have relatively static test files checked into the tree with a relatively cumbersome (manual) process to get them updated.

Reasons we might not be able to use the signing service:
* We have too many builds happening all the time, load may kill it. Maybe if it was set up to run only when the testfiles changed and then check the resulting signed bits back into the tree? We probably don't want to keep hitting the service to sign the same thing over and over and over from each of the tbpl machines.
* the tinderbox machines may be on a different network than the nightly/release build machines and access to the signing server may be restricted to particular subnets or even specific machines. Those would be wise network operational security things to have done and if we're going to change that we need to get the OK from Joe Stevensen's Operational Security team.

So sec-review+ for the concept, but this is one spot where we probably have to check the implementation details.

Comment 2

[Brian R. Bondy [:bbondy]]

Needinfo request to find out if we can sign these test MAR files with release, nightly, and dep certs as part of the build process.  See Comment #1 for security consideration.

If not, is it OK to post a new bug with attachments for the MAR files that need to be signed with i) Nightly/Aurora, ii) Beta/Release, and iii) dep private keys.
We'd like RelEng to give us signed versions of those files if we can't do this in an automated way.

Another idea to allow automated signing, is for us to use our xpcshell cert instead of dep cert for MAR signing since dep builds are not updatable anyway.  The xpcshell private key is publically available in our repo by the way.  Then could we only make cases i) and ii) part of the build process without increasing load? and without increasing private key access on more machines?

Comment 3

[Brian R. Bondy [:bbondy]]

Currently:
- Release private key signs beta, release and esr update channels
- Nightly private key signs nightly, aurora, nightly-elm, nightly-profiling, nightly-oak
- Everything else gets signed with the dep private key

Comment 4

[Brian R. Bondy [:bbondy]]

Attachment: Part 1 of 2: Don't use an xpcshell cert for verification. rev1

Part 2 of 2 will depend on the details of catlee's reply.

Comment 5

[Brian R. Bondy [:bbondy]]

While you're sec-reiew'ed on this dveditz, do you think it would be OK to verify MAR files with a publically published (in our repository) NSS DB private key for dep builds for MAR file?  The reason why I think it's OK is because those builds are not updatable anyway.
I think that would reduce load a lot and would only require special handling of MAR files for nightly enabled builds.

Comment 6

[Chris AtLee [:catlee]]

If I understand correctly, you're asking if RelEng can sign some test MARs as part of our build automation that are later used in xpcshell tests?

If so, I don't see a problem. We already sign files for every single build that happens in the infrastructure. These test mar files would be small (?), so the additional load wouldn't be too much. NB that only the build machines are allowed to contact the signing service, the test machines are not permitted to request for files to be signed.

An alternative would be to have copies of mars pre-created and signed with the appropriate keys already, and just pull those down at build or test time. They would need re-creating whenever we change signing keys, but I imagine we'd need to update public keys in the tests in that case as well.

Comment 7

[Brian R. Bondy [:bbondy]]

(In reply to Chris AtLee [:catlee] from comment #6)
> If I understand correctly, you're asking if RelEng can sign some test MARs
> as part of our build automation that are later used in xpcshell tests?
> 
> If so, I don't see a problem. We already sign files for every single build
> that happens in the infrastructure. These test mar files would be small (?),
> so the additional load wouldn't be too much. NB that only the build machines
> are allowed to contact the signing service, the test machines are not
> permitted to request for files to be signed.
> 
> An alternative would be to have copies of mars pre-created and signed with
> the appropriate keys already, and just pull those down at build or test
> time. They would need re-creating whenever we change signing keys, but I
> imagine we'd need to update public keys in the tests in that case as well.

Thanks, we're going to go ahead with signing the dep builds with the publicly known xpcshell private keys.  
If we run into some hurtle then we'll fall back to trying to sign them on each build.  The test MAR files would be very small yes.

Comment 8

[Brian R. Bondy [:bbondy]]

Attachment: Part 2 of 2 - Remove unused DER certs and replace with xpcshell cert as new primary and secondary. rev1

It's my understanding that the only time the DER certs were used in the past were for misconfigured machines running the MAR tests (i.e. those without a fallback key).

This makes it so for any dep build we use the xpcshell cert always whether or not a fallback key is used.  I.e. the fallback key check will no longer be related in any way to MAR verification.

The fallback key is still used to modify the code path in the maintenance service for authenticode checks.

MAR files should not need to be re-generated because they are already signed with the xpcshell cert (who's private key is publicly known).

We talked about needing an extra new flag in the mozconfigs to avoid any problems with newly introduced channels, but I think the complexity that it would add is more cost then it's worth.

Comment 9

[Brian R. Bondy [:bbondy]]

(In reply to Brian R. Bondy [:bbondy] from comment #8)
> Created attachment 808673 [details] [diff] [review]
> Part 2 of 2 - Remove unused DER certs and replace with xpcshell cert as new
> primary and secondary. rev1
> 
> It's my understanding that the only time the DER certs were used in the past
> were for misconfigured machines running the MAR tests (i.e. those without a
> fallback key).
> 
> This makes it so for any dep build we use the xpcshell cert always whether
> or not a fallback key is used.  I.e. the fallback key check will no longer
> be related in any way to MAR verification.
> 
> The fallback key is still used to modify the code path in the maintenance
> service for authenticode checks.
> 
> MAR files should not need to be re-generated because they are already signed
> with the xpcshell cert (who's private key is publicly known).
> 
> We talked about needing an extra new flag in the mozconfigs to avoid any
> problems with newly introduced channels, but I think the complexity that it
> would add is more cost then it's worth.

Actually one other case the xpcshell cert would be used is on branch channels that have updates enabled other than these (if any):

> ifneq (,$(filter beta release esr,$(MOZ_UPDATE_CHANNEL)))
>		PRIMARY_CERT = release_primary.der
>		SECONDARY_CERT = release_secondary.der
> else ifneq (,$(filter nightly aurora nightly-elm nightly-profiling nightly-oak,$(MOZ_UPDATE_CHANNEL)))

Since these are not official nor supported I don't think it's a concern for mar attacks.

Comment 10

[Brian R. Bondy [:bbondy]]

Attachment: Part 1 of 2: Don't use an xpcshell cert for verification. rev1'

part 1 of 2 same as before just added a commit message.

Comment 11

[Robert Strong (they/them - no direct email)]

Comment on attachment 808673 [details] [diff] [review]
Part 2 of 2 - Remove unused DER certs and replace with xpcshell cert as new primary and secondary. rev1

Any side affects from having the same cert for the primary and secondary?

Comment 12

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (do not email) from comment #11)
> Comment on attachment 808673 [details] [diff] [review]
> Part 2 of 2 - Remove unused DER certs and replace with xpcshell cert as new
> primary and secondary. rev1
> 
> Any side affects from having the same cert for the primary and secondary?

Only a tiny bit of extra processing time when a failed cert check happens, which should be never unless there's an attack or server error.  It tries the primary one, then tries the secondary one if the first fails.

I do need to update some non-windows test MARs by the way to be signed.  That'll come in another patch.

Comment 13

[Stephen A Pohl [:spohl]]

Attachment: Part 1 of 2: Don't use an xpcshell cert for verification. rev1'

Updated for current trunk. Carrying over r+.

Comment 14

[Brian R. Bondy [:bbondy]]

Attachment: Part 1 of 2: Don't use an xpcshell cert for verification. rev2

Rebased

Comment 15

[Brian R. Bondy [:bbondy]]

Attachment: Part 2 of 2 - Remove unused DER certs and replace with xpcshell cert as new primary and secondary. rev2

Rebased

Comment 16

[Brian R. Bondy [:bbondy]]

Script for regenerating test MAR files:

./objdir-ff-debug/dist/bin/signmar -r ./toolkit/mozapps/update/tests/data/complete.mar ./toolkit/mozapps/update/tests/data/complete.mar2
./objdir-ff-debug/dist/bin/signmar -r ./toolkit/mozapps/update/tests/data/complete_mac.mar ./toolkit/mozapps/update/tests/data/complete_mac.mar2
./objdir-ff-debug/dist/bin/signmar -r ./toolkit/mozapps/update/tests/data/old_version.mar ./toolkit/mozapps/update/tests/data/old_version.mar2
./objdir-ff-debug/dist/bin/signmar -r ./toolkit/mozapps/update/tests/data/partial.mar ./toolkit/mozapps/update/tests/data/partial.mar2
./objdir-ff-debug/dist/bin/signmar -r ./toolkit/mozapps/update/tests/data/partial_mac.mar ./toolkit/mozapps/update/tests/data/partial_mac.mar2
./objdir-ff-debug/dist/bin/signmar -r ./toolkit/mozapps/update/tests/data/simple.mar ./toolkit/mozapps/update/tests/data/simple.mar2
./objdir-ff-debug/dist/bin/signmar -r ./toolkit/mozapps/update/tests/data/wrong_product_channel.mar ./toolkit/mozapps/update/tests/data/wrong_product_channel.mar2

rm ./toolkit/mozapps/update/tests/data/complete.mar
rm ./toolkit/mozapps/update/tests/data/complete_mac.mar
rm ./toolkit/mozapps/update/tests/data/old_version.mar
rm ./toolkit/mozapps/update/tests/data/partial.mar
rm ./toolkit/mozapps/update/tests/data/partial_mac.mar
rm ./toolkit/mozapps/update/tests/data/simple.mar
rm ./toolkit/mozapps/update/tests/data/wrong_product_channel.mar

./objdir-ff-debug/dist/bin/signmar -d modules/libmar/tests/unit/data -n mycert -s ./toolkit/mozapps/update/tests/data/complete.mar2 ./toolkit/mozapps/update/tests/data/complete.mar
./objdir-ff-debug/dist/bin/signmar -d modules/libmar/tests/unit/data -n mycert -s ./toolkit/mozapps/update/tests/data/complete_mac.mar2 ./toolkit/mozapps/update/tests/data/complete_mac.mar
./objdir-ff-debug/dist/bin/signmar -d modules/libmar/tests/unit/data -n mycert -s ./toolkit/mozapps/update/tests/data/old_version.mar2 ./toolkit/mozapps/update/tests/data/old_version.mar
./objdir-ff-debug/dist/bin/signmar -d modules/libmar/tests/unit/data -n mycert -s ./toolkit/mozapps/update/tests/data/partial.mar2 ./toolkit/mozapps/update/tests/data/partial.mar
./objdir-ff-debug/dist/bin/signmar -d modules/libmar/tests/unit/data -n mycert -s ./toolkit/mozapps/update/tests/data/partial_mac.mar2 ./toolkit/mozapps/update/tests/data/partial_mac.mar
./objdir-ff-debug/dist/bin/signmar -d modules/libmar/tests/unit/data -n mycert -s ./toolkit/mozapps/update/tests/data/simple.mar2 ./toolkit/mozapps/update/tests/data/simple.mar
./objdir-ff-debug/dist/bin/signmar -d modules/libmar/tests/unit/data -n mycert -s ./toolkit/mozapps/update/tests/data/wrong_product_channel.mar2 ./toolkit/mozapps/update/tests/data/wrong_product_channel.mar

rm ./toolkit/mozapps/update/tests/data/complete.mar2
rm ./toolkit/mozapps/update/tests/data/complete_mac.mar2
rm ./toolkit/mozapps/update/tests/data/old_version.mar2
rm ./toolkit/mozapps/update/tests/data/partial.mar2
rm ./toolkit/mozapps/update/tests/data/partial_mac.mar2
rm ./toolkit/mozapps/update/tests/data/simple.mar2
rm ./toolkit/mozapps/update/tests/data/wrong_product_channel.mar2

Comment 17

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=883e17fc475f