1125699 - Add authenticode test on real updater binary and skip the check for updater-xpcshell binary gated on the fallback key

Status: RESOLVED INVALID

(Toolkit :: Application Update, defect)

Description

[Brian R. Bondy [:bbondy]]

We're building an updater-xpcshell.exe to run updater tests now. More info here but it's not important for the description of this bug[1].

This binary gets included in the data package, but maintenance service tests are currently failing because the binary is not being signed.

Possible solutions:
i) Maybe there's an easy way to get this signed in the same way as the normal updater.exe gets signed?
The final destination of this binary is:
<tests-bundle>/xpcshell/tests/toolkit/mozapps/update/tests/data/updater.exe
The build dir is:
<objdir>/dist/bin/updater-xpcshell.exe

2) If for some reason #1 is had to do, we could build a separate maintenance service only used for tests that bypasses the authenticode check completely. We may want to do this one day anyway to simplify build stuff, but is a lot more work so would rather do this at a later date.


[1] The purpose of this is to only include a single certificate in the updater binary and not have to ship a second xpcshell certificate and rely on a registry key in a secure location to pick which one is correct.  This is also needed to land the multi platform mar work.

Comment 1

[Brian R. Bondy [:bbondy]]

Is #1 something releng can enable somehow? Or if not do you have any hints on where to look to enable this myself? Thanks!

Comment 2

[[e-mail] (:bhearsum)]

Should just be another invocation of MOZ_SIGN_CMD at the right point. I'm surprised it's not getting signed by http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/upload-files.mk#503 though, perhaps it's in SIGN_EXCLUDES, or not in SIGN_INCLUDES. I suspect that fiddling with one or both of those will do the trick (testable on try).

If that doesn't work, something like http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/windows/nsis/makensis.mk#51, but using MOZ_INTERNAL_SIGNING_FORMAT should do the trick.

Comment 3

[Brian R. Bondy [:bbondy]]

great, thanks for the info Ben

Comment 4

[Brian R. Bondy [:bbondy]]

>  I'm surprised it's not getting signed by http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/upload-files.mk#503 

This is because it's just being built in a subfolder inside toolkit/mozapps/updater and then being directly copied to the xpcshell test dir. 

So I'm thinking it would be something like this:
# Sign the updater-xpcshell binary on Windows for maintenance service tests.
ifdef MOZ_SIGN_CMD
ifeq (WINNT,$(OS_ARCH))
ifeq (x86_64,$(TARGET_CPU))
	$(MOZ_SIGN_CMD) $(foreach f,osslsigncode,-f $(f)) $(XPCSHELLTESTROOT)/data/updater$(BIN_SUFFIX)
endif
endif
endif

---

Ugh, but as I think about this more I think we shouldn't sign the updater-xpcshell because it could be used in attacks.  People could copy out this binary and then build mar files with the xpcshell cert.  Copying in this updater.exe on top of the real updater.exe just before the maintenance service executes.  It's signed by us so it would check out.

rstrong are you OK with me also building a maintenance service used only for tests that skips the authenticode checks? I think this will allow cleaning up some special setups we need to do on talos machines too like installing fake nightly certs.  I am concerned with the bootstrap first update though, we may need releng to update the maintenance service to one that doesn't do cert checking by hand.

Comment 5

[Brian R. Bondy [:bbondy]]

Actually I think the best bet is to just skip the authenticode check altogether if there's a fallback key.  This is in contrast to using the different info in the registry for it if there is a fallback key. 

This would solve the problem and not need a maintenance service new binary and no additional work for releng. We wouldn't be testing the authenticode check part of the code but I think that's ok.  Do you agree rstrong?

Comment 6

[Brian R. Bondy [:bbondy]]

Attachment: reg.diff

Implementation of what was described in the last comment.

Comment 7

[Brian R. Bondy [:bbondy]]

Attachment: Patch v2

This is similar to the last patch but it completely skips the cert attribute checks too since no cert is present on updater-xpcshell.

Not asking for a review explicitly at this time becuase I'll just have one rollup patch with all the recent work.

Comment 8

[Brian R. Bondy [:bbondy]]

With this change by the way I verified and everything passes on Oak for Windows and OSX.
Linux only has the sqlite db dynamic link library error test but we can land with that disabled. 

Summary:
We build updater-xpcshell which contains the certs for verifying MAR files (in a multi platform way via header file inclusion generated from a python file) and no longer include the xpcshell certs in the main updater binary.

We skip maintenance service checks for authenticode info when the fallback key exists (which it always exists on the talos machines).

Comment 9

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

The purpose as stated in comment #0, "[1] The purpose of this is to only include a single certificate in the updater binary and not have to ship a second xpcshell certificate and rely on a registry key in a secure location to pick which one is correct." I don't think the benefit provided by dong this is greater than testing the authenticode code path.

Could you provide details on how the binaries from the tests could be used to perform an exploit when they are signed by the xpcshell cert? I suspect there would be similar methods using these binaries that bypass the authenticode check.

Comment 10

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #9)
> The purpose as stated in comment #0, "[1] The purpose of this is to only
> include a single certificate in the updater binary and not have to ship a
> second xpcshell certificate and rely on a registry key in a secure location
> to pick which one is correct." I don't think the benefit provided by dong
> this is greater than testing the authenticode code path.
> 
> Could you provide details on how the binaries from the tests could be used
> to perform an exploit when they are signed by the xpcshell cert? 


Yes sure, if we sign it, then someone could download the updater.exe binary from our talos test bundle.  It would be signed with our cert.  They now have an updater.exe binary signed by us as a good binary but it contains the xpcshell certs inside.  They could build a timing attack to copy the updater.exe into the appdata directory at the right time as well as copy in a mar file that they construct using the xpcshell cert.

> I suspect
> there would be similar methods using these binaries that bypass the
> authenticode check.

No I don't think there is any exploit with the new way I'm suggesting in the patch.
This only bypasses the authenticode check in any way, shape or form, when an HKLM entry exists.  HKLM requires elevated access to write to and we already rely on it for various security checks.

Comment 11

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

Let's go over the process and go from there. I think things have morphed somewhat since we last discussed this.

Comment 12

[Brian R. Bondy [:bbondy]]

Update here:
We discussed plan via a 1:1 and the plan is to do like the patch above, but in addition add a new test that tests the real binary for the cert check.  This is possibly with a c++ test.

Comment 13

[Brian R. Bondy [:bbondy]]

Attachment: bin signing check and test

This adds a new command line arg to the maintenance service to check if a binary cert is valid based on its updater base dir and it's filename. This is somewhat useful for troubleshooting so I didn't feel too badly like I was introducing test only code.

There's a new flag introduced which makes it work the old way (before the feedback+'ed patch) or the new way (as of the feedback+'ed patch).  Both are equivalent in terms of security because both are gated on the HKLM key write access.  We need the old way to test the real authenticode path and cert attributes with the updater.exe which is signed.  We need the new way because updater-xpcshell isn't signed.


Only requesting feedback for now because this will be rolled up with a bunch of other changes on Oak.

Comment 14

[Brian R. Bondy [:bbondy]]

Attachment: bin signing check and test'

Removed a stale comment.

Comment 15

[Brian R. Bondy [:bbondy]]

Attachment: bin signing check and test''

Getting late, forgot to hg add the test.

Comment 16

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

Comment on attachment 8556946 [details] [diff] [review]
bin signing check and test''

>diff --git a/toolkit/components/maintenanceservice/registrycertificates.cpp b/toolkit/components/maintenanceservice/registrycertificates.cpp
>--- a/toolkit/components/maintenanceservice/registrycertificates.cpp
>+++ b/toolkit/components/maintenanceservice/registrycertificates.cpp
>@@ -12,20 +12,26 @@
> #include "servicebase.h"
> #include "updatehelper.h"
> #define MAX_KEY_LENGTH 255
> 
> /**
>  * Verifies if the file path matches any certificate stored in the registry.
>  *
>  * @param  filePath The file path of the application to check if allowed.
>+ * @param  allowFallbackKeySkip TRUE if the Fallback key can be used to
>+ *   skip the cert check.  Since it is stored in a secured location, and cannot
>+ *   be created / tampered with by a low integrity process, this is the default.
>+ *   The maintenance service binary can be used to do checks directly or can be
>+ *   used by tests.
nit: state what is skipped in the comment. "this is the default" implies this is the way it typically happens and it doesn't state what the non-default is.

>  * @return TRUE if the binary matches any of the allowed certificates.
>  */
> BOOL
>-DoesBinaryMatchAllowedCertificates(LPCWSTR basePathForUpdate, LPCWSTR filePath)
>+DoesBinaryMatchAllowedCertificates(LPCWSTR basePathForUpdate, LPCWSTR filePath,
>+                                   BOOL allowFallbackKeySkip)
> { 
>   WCHAR maintenanceServiceKey[MAX_PATH + 1];
>   if (!CalculateRegistryPathFromFilePath(basePathForUpdate, 
>                                          maintenanceServiceKey)) {
>     return FALSE;
>   }
> 
>   // We use KEY_WOW64_64KEY to always force 64-bit view.
>@@ -44,17 +50,17 @@ DoesBinaryMatchAllowedCertificates(LPCWS
>     // We use this registry key on our test slaves to store the 
>     // allowed name/issuers.
>     retCode = RegOpenKeyExW(HKEY_LOCAL_MACHINE, 
>                             TEST_ONLY_FALLBACK_KEY_PATH, 0,
>                             KEY_READ | KEY_WOW64_64KEY, &baseKeyRaw);
>     if (retCode != ERROR_SUCCESS) {
>       LOG_WARN(("Could not open fallback key.  (%d)", retCode));
>       return FALSE;
>-    } else {
>+    } else if (allowFallbackKeySkip) {
>       LOG_WARN(("Fallback key present, skipping authenticode "
>                 "check and cert check."));
authenticode is a cert check so how about something along the lines of binary is signed and the certificate is valid?

Comment 17

[Brian R. Bondy [:bbondy]]

Will be landing as part of bug 973933.