As of bug 699700 MAR files will be signed on Windows. This bug is to include the certificates we get from RelEng for MAR checking on Windows as part of updates and installers. The code for updater.exe using the new verify function will also be done in this task. I don't think this bug needs to be marked as Security, but I am marking it that way just in case.

Where can I find the certificates and the procedures for generating the certificates/private keys? I would like to review them too.

I did my testing with the certutil commands you gave me. But RelEng will be the ones who will be providing these. I'll add catlee to the CC list.

Done: - verify call added into archivereader, and used form udpater.cpp - assumes cert is in mar file directory (this will change, see below). TODO: - Cert should be loaded from a trusted location (e.g. program files) - Need to check both main cert and backup cert - Installer needs to install the main cert and backup cert into program files. Parts of this task are waiting the main cert and backup cert from RelEng.

(In reply to Brian R. Bondy [:bbondy] from comment #3) > TODO: > - Cert should be loaded from a trusted location (e.g. program files) > - Installer needs to install the main cert and backup cert into program > files. If we are going to do this, then we should have the certs signed by a CA cert that is embedded in the updater, and verify those signatures. Is there any way we could avoid doing this (at least for right now), and just embed the certs in the updater? Otherwise, what happens when another program overwrites our certs with its own? Also, remember the recent bug where *.jar files didn't survive system restore? We must make sure that doesn't happen with these certs.

> Otherwise, what happens when another program overwrites our certs with its own? It's my understanding that if someone could overwrite Program files then they are already running as a high integrity process (elevated) and so the system is already compromised. > Is there any way we could avoid doing this (at least for right now), and just embed the certs in the updater? We could store the certs in the resources of updater.exe but this is a bit more work. I'm fine with doing it though if you feel it's best. This would be a Windows only solution and the code wouldn't be able to be used on other platforms. By the way there are Win32 APIs for updating resources on files as well, so like I mentioned earlier, if someone had permission to write inside Program Files, they could make a simple program that replaced the embedded certs in the updater.exe program.

I see, the embedded udpater.exe solution also gets a cert check, so it should fail the verify of the signature if someone called UpdateResource on it.

(In reply to Brian Smith (:bsmith) from comment #1) > Where can I find the certificates and the procedures for generating the > certificates/private keys? I would like to review them too. What's the correct process for generating these certificates? Should we include valid issuer/common name/expiry dates? My understanding this far is that they're only used as a container for public/private rsa keys and so the certificate metadata isn't that important.

I'd like to recommend that we don't have expiry dates. I don't think the metadata is important either, but I'll let bsmith chime in.

(In reply to Brian R. Bondy [:bbondy] from comment #6) > I see, the embedded udpater.exe solution also gets a cert check, so it > should fail the verify of the signature if someone called UpdateResource on > it. I am not sure if Windows actually checks the signature on each execution in the normal case. I believe you have to do something special like [1][2], which we probably should do for updater.exe and the Windows service. [1] http://blog.didierstevens.com/2011/10/27/using-dllcharacteristics-force_integrity-flag/ [2] http://social.technet.microsoft.com/wiki/contents/articles/255.aspx

Nelson, Bob, Kai: One thing I am not sure about, regarding certutil: Why does certutil require us to bash on the keyboard for key generation? Is this input used in addition to /dev/random? Assuming I don't have a HSM for whatever reason, what is the best way to generate a key using certutil? (In reply to Chris AtLee [:catlee] from comment #7) > What's the correct process for generating these certificates? Should we > include valid issuer/common name/expiry dates? My understanding this far is > that they're only used as a container for public/private rsa keys and so the > certificate metadata isn't that important. * Install NSS (the certutil utility and all of the NSS shared libraries) onto the system that will do the MAR signing. * mkdir mar-signing-database-do-not-copy-from-this-machine * cd mar-signing-database-do-not-copy-from-this-machine These steps assume that you don't have any hardware security module or smartcard or anything else like that available for you to use. The commands for using a hardware security module or smartcard are similar, but you need to configure NSS to use your HSM/smartcard. Let me know if/when you have it available. * certutil -d . -N This will prompt you for the password that will be used to protect the key database. I believe this password is not as useful as a security measure as (1) NSS does not do a sufficient number of PBKDF2 iterations, (2) you are going to write automated scripts that pass these passwords around anyway. * certutil -S -d . -s "CN=mar-nightly-1" -n mar-nightly-1 -x -t ",," -g 2048 * certutil -S -d . -s "CN=mar-nightly-2" -n mar-nightly-2 -x -t ",," -g 2048 Parameters are explained at [1]: -S : create a new certificate -d . : in the current directory -s "CN=...." : Subject DN (shown in any cert viewer) -n ... : name of the certificate (used for finding cert in the DB) this is what you pass as the name of the cert to the mar command. -x : self-signed certificate -t ",," : do not trust this certificate for anything -g : (RSA) 2048-bit key You can see the certificates in the database: certutil -L -d . You can export the certificates (NOT the private keys): certutil -L -d . -n mar-nightly-1 -r > mar-nightly-1.der certutil -L -d . -n mar-nightly-2 -r > mar-nightly-2.der The file "key3.db" contains the private keys. This is the file you need to protect the most. When you have a HSM/smartcard, then the private keys will be stored in the HSM. FWIW, cert8.db+key3.db+softokn3.so+freebl.so is basically a software emulation of an HSM. Of course, when you acquire the HSM/smartcard, you will not be able to use the old private keys and you will need to generate new certs for the keys stored in the HSM/smartcard. That's why it is really a good idea to have the HSM/smartcard to start with, if we're ever going to use one. This advice should be verified by rrelyea, Nelson, and Kai. They have a lot more experience using certutil and other parts of NSS than I do. Note that this is assuming that you will use different keys/certs to sign each channel. I think, at a minimum, you should use different keys/certs for the pre-release builds than you use for final release builds, and that you should use different (stronger) security measures to protect the keys for the production builds than what you are planning for the nightly builds. You then need to pass the path to mar-signing-database-do-not-copy-from-this-machine as the "-d" parameter to the mar signing tool, and the name of the cert as the -n parameter to the mar signing tool (e.g. "-n mar-nightly-1"). You also need to backup the mar-signing-database-do-not-copy-from-this-machine in a very secure way (secure meaning in a way that doesn't leak the contents of the files, and also in meaning that we will never lose the backups). I have no experience with doing this so I cannot offer useful advice here. [1] http://www.mozilla.org/projects/security/pki/nss/tools/certutil.html

(In reply to Brian R. Bondy [:bbondy] from comment #8) > I'd like to recommend that we don't have expiry dates. > I don't think the metadata is important either, but I'll let bsmith chime in. There is going to be an expiry date regardless. But, since we don't check the expiry date in the current implementation, we should just make sure the expiry date is always in the past. That way, we aren't surprised by any difference in behavior between expired and not-yet-expired certificates. To do this, add "-v 3" to the command lines above: certutil -S -d . -s "CN=mar-nightly-2" -n mar-nightly-2 \ -x -t ",," -g 2048 -v -3 It seems that certutil has "right now" as the notBefore time and "right now + 3 months" as the notAfter time. "-v -3" means "add -3 to 3 months == 0 months" i.e. "make notBefore and notAfter equal." After you have exported the certs, export them and open them in Firefox (I think you can just drag and drop the file onto Firefox) and in Windows (maybe you have to name the files with a "crt" or "cer" extension instead of "der" to get the double-click-to-view behavior on Windows). Then, verify the metadata is as you please. BTW, I would avoid including "Mozilla" or "Firefox" or anything official-looking in the cert metadata. And, I would include as little metadata in the certs as possible, since we aren't validating any of it.

> bbondy: > I see, the embedded udpater.exe solution also gets a cert check, so it > should fail the verify of the signature if someone called UpdateResource on > it. > bsmith: > I am not sure if Windows actually checks the signature on each execution We check it though in bug 481815.

(In reply to Brian Smith (:bsmith) from comment #11) > (In reply to Brian R. Bondy [:bbondy] from comment #8) > > I'd like to recommend that we don't have expiry dates. > > I don't think the metadata is important either, but I'll let bsmith chime in. > > There is going to be an expiry date regardless. But, since we don't check > the expiry date in the current implementation, we should just make sure the > expiry date is always in the past. That way, we aren't surprised by any > difference in behavior between expired and not-yet-expired certificates. To > do this, add "-v 3" to the command lines above: > > certutil -S -d . -s "CN=mar-nightly-2" -n mar-nightly-2 \ > -x -t ",," -g 2048 -v -3 > > It seems that certutil has "right now" as the notBefore time and "right now > + 3 months" as the notAfter time. "-v -3" means "add -3 to 3 months == 0 > months" i.e. "make notBefore and notAfter equal." What version of nss do I need to use this? I have 3.12 on this machine currently, and when I try "-v -3" it generates an error: certutil -v: incorrect validity period: "-3"

- Implemented loading primary cert and backup cert from updater.exe on Windows. - Implemented extracting those certs to disk, but having the file locked during their check from the time of writing. - Backup cert handling is only done if primary cert handling fails. - Implemented checking the certs via libmar.

Ready for review. I tested this bug, bug 699700, and bug 481815 and I have successfully done a full silent update from a signed mar that was first verified.

Comment on attachment 576800 [details] [diff] [review] Verifying signed MARs from updater.exe Review of attachment 576800 [details] [diff] [review]: ----------------------------------------------------------------- looks good to me, a couple of very minor nits ::: toolkit/mozapps/update/updater/archivereader.cpp @@ +86,5 @@ > + > +BOOL PathGetSiblingFilePath(LPWSTR destinationBuffer, > + LPCWSTR siblingFilePath, > + LPCWSTR newFileName); > + don't see this used anywhere in this patch, maybe it is in another bit of the code though ::: toolkit/mozapps/update/updater/updater.cpp @@ +1879,5 @@ > NULL, OPEN_EXISTING, 0, NULL); > if (callbackFile != INVALID_HANDLE_VALUE) > break; > > + DWORD lastErorr = GetLastError(); nit : typo

Implemented review nits, thanks for the feedback Ian.

I talked to catlee about which certs will be in which channels: beta+release will share one set of authenticode/mar certs, nightly+aurora will share another set, everything else will have a different set.

(In reply to Brian R. Bondy [:bbondy] from comment #18) > I talked to catlee about which certs will be in which channels: > > beta+release will share one set of authenticode/mar certs, nightly+aurora > will share another set, everything else will have a different set. And every cert will have its own distinct keypair, right?

Correct. Actually each of those will also accept a matching fallback key. If either the primary key or fallback key can verify the MAR then we allow the update.

This is pushed to elm but the actual verify check is disabled until I get the real certs from catlee. I will update elm as I make changes to this.

Comment on attachment 577992 [details] [diff] [review] Verifying signed MARs from updater.exe. v2 >diff --git a/toolkit/mozapps/readstrings/errors.h b/toolkit/mozapps/readstrings/errors.h >--- a/toolkit/mozapps/readstrings/errors.h >+++ b/toolkit/mozapps/readstrings/errors.h >@@ -44,10 +44,13 @@ > // #define IO_ERROR 2 // Use READ_ERROR or WRITE_ERROR instead > #define USAGE_ERROR 3 > #define CRC_ERROR 4 > #define PARSE_ERROR 5 > #define READ_ERROR 6 > #define WRITE_ERROR 7 > #define UNEXPECTED_ERROR 8 > #define ELEVATION_CANCELED 9 >+#define CERT_LOAD_ERROR 10 >+#define CERT_HANDLING_ERROR 11 >+#define CERT_VERIFY_ERROR 12 Not for this bug but I really dislike the updater overloading readstrings errors. This is an artifact of the time we needed readstrings available for the WinCE / WinMo installer. Please file a bug to separate them. >diff --git a/toolkit/mozapps/update/updater/archivereader.cpp b/toolkit/mozapps/update/updater/archivereader.cpp >--- a/toolkit/mozapps/update/updater/archivereader.cpp >+++ b/toolkit/mozapps/update/updater/archivereader.cpp >@@ -49,16 +49,77 @@ > # include <io.h> > #endif > > static int inbuf_size = 262144; > static int outbuf_size = 262144; > static char *inbuf = NULL; > static char *outbuf = NULL; > >+#ifdef XP_WIN >+#include "resource.h" >+ >+bool LoadFileInResource(int name, int type, DWORD& size, >+ const char *&data) bool LoadFileInResource(... >+{ >+ HMODULE handle = ::GetModuleHandle(NULL); >+ if (!handle) { >+ return false; >+ } >+ >+ HRSRC resourceInfoBlockHandle = ::FindResource(handle, >+ MAKEINTRESOURCE(name), >+ MAKEINTRESOURCE(type)); >+ if (!resourceInfoBlockHandle) { >+ FreeLibrary(handle); >+ return false; >+ } >+ >+ HGLOBAL resourceHandle = ::LoadResource(handle, resourceInfoBlockHandle); >+ if (!resourceHandle) { >+ FreeLibrary(handle); >+ return false; >+ } >+ >+ size = ::SizeofResource(handle, resourceInfoBlockHandle); >+ data = static_cast<const char*>(::LockResource(resourceHandle)); >+ FreeLibrary(handle); No return value >+} >+ >+int VerifyLoadedCert(const NS_tchar *pathToMAR, int name, int type) int VerifyLoadedCert(... >+{ >+ DWORD size = 0; >+ const char *data = NULL; >+ if (!LoadFileInResource(name, type, size, data) || !data || !size) { >+ return CERT_LOAD_ERROR; >+ } >+ >+ if (mar_verify_signatureW(pathToMAR, data, size, NULL, NULL)) { I'd just use mar_verify_signature since we don't bother doing this elsewhere and we only support wide chars on Windows anyways. >+ return CERT_VERIFY_ERROR; >+ } >+ >+ return OK; >+} >+#endif >+ >+ >+int >+ArchiveReader::VerifySignature(const NS_tchar *pathToMAR) >+{ >+#ifdef XP_WIN >+ int rv = VerifyLoadedCert(pathToMAR, IDR_PRIMARY_CERT, TYPE_CERT); >+ if (rv != OK) { >+ rv = VerifyLoadedCert(pathToMAR, IDR_BACKUP_CERT, TYPE_CERT); >+ } >+ return rv; >+#else >+ return 0; This should be return OK >diff --git a/toolkit/mozapps/update/updater/updater.rc b/toolkit/mozapps/update/updater/updater.rc >--- a/toolkit/mozapps/update/updater/updater.rc >+++ b/toolkit/mozapps/update/updater/updater.rc >@@ -32,16 +32,24 @@ 1 RT_MANIFEST > // > // Icon > // > > IDI_DIALOG ICON "updater.ico" > > ///////////////////////////////////////////////////////////////////////////// > // >+// Embedded certificates for allowed MARs >+// >+ >+IDR_PRIMARY_CERT TYPE_CERT "primaryCert.der" >+IDR_BACKUP_CERT TYPE_CERT "backupCert.der" note: we are going to need a way for other apps to specify the certs to use r=me with the above addressed

Will update a patch will the above addressed bu 2 things that won't be addressed in the patch are: - mar_verify_signatureW is that way because mar_verify_signature is already used in the libmar C code. I think we can eventually change that to C++ code in another bug to clenaup various things? In C code you can't overload functions. - The certs having a better way to specify the certs will be worked out with another patch in this bug once I get the needed Certs from RelEng.

I'm going to carry forward the r+ on the newly submitted patch since only nits were addressed in this patch. I also added javadoc to the new functions in this patch.

Implemented review nits + javadoc.

This cert was sent to me by catlee and is the one we should use to verify the current signing. I think this is subject to change though and there may be different certs per channel.

Rebased for changes to libmar patch. Carrying forward r+ since there are no changes otherwise.

Will wait for another review from bsmith on the dependent libmar patch and the certs from catlee before requesting another review on this patch. Just updating with latest changes in the meantime.

we'll use dep1 for try and depend builds on all branches. we'll use nightly1 for elm, mozilla-central and mozilla-aurora nightlies. dep2 and nightly2 are our backup keys.

(In reply to Brian Smith (:bsmith) from comment #10) > Nelson, Bob, Kai: One thing I am not sure about, regarding certutil: Why > does certutil require us to bash on the keyboard for key generation? Is this > input used in addition to /dev/random? Each platform has different sources of randomness, /dev/random doesn't exist on Windows. I suspect the keyboard trick was used as an easy solution that works cross platform. > Assuming I don't have a HSM for > whatever reason, what is the best way to generate a key using certutil? If you only create a certificate rarely, manually, then you should comply with certutil's request and just type these keys. Only if you are looking for a solution for test environments, or if you have a cross platform solution that can save true random data into a file, you can use the following parameter: -z noisefile Specify the noise file to be used

Added in Makefile support for determining which cert should be used based on the channel.

Same patch as before but without the binary DER files.

Updated DER file with secondary Release MAR signing certificate from RelEng.

Comment on attachment 593100 [details] [diff] [review] Verifying signed MARs from updater.exe. v6'. >diff --git a/toolkit/mozapps/update/updater/archivereader.cpp b/toolkit/mozapps/update/updater/archivereader.cpp >--- a/toolkit/mozapps/update/updater/archivereader.cpp >+++ b/toolkit/mozapps/update/updater/archivereader.cpp >@@ -49,16 +49,107 @@ >... >+/** >+ * Obtains the data of the specified resource name and type. >+ * >+ * @param name The name ID of the resource >+ * @param type The type ID of the resource >+ * @param size Out parameter which sets the size of the returned data buffer >+ * @param data Out parameter which sets the pointer to a buffer containing >+ * the needed data. >+ * @return TRUE on success >+*/ >+BOOL >+LoadFileInResource(int name, int type, DWORD& size, >+ const char *&data) nit: I typically put data before the size of the data but I'm fine if you prefer this way.

Comment on attachment 593101 [details] [diff] [review] DER files. Patch v1. rs=rs

> nit: I typically put data before the size of the data but I'm fine > if you prefer this way. Me too, not sure why I did it that way, fixed. Carrying forward r+.

Now use aurora-nightly cert for elm since elm Nightly builds use that MAR signing cert instead. Carrying forward r+.

Comment on attachment 598668 [details] [diff] [review] Verifying signed MARs from updater.exe. v8. ># HG changeset patch ># Parent 6f7d7c4c2d57976d180bf04cda0446875418200e ># User Brian R. Bondy <netzen@gmail.com> >Bug 704285 - Include certificates for MAR sign checks on Windows as part of updates and installers >diff --git a/toolkit/mozapps/update/updater/updater.rc b/toolkit/mozapps/update/updater/updater.rc >--- a/toolkit/mozapps/update/updater/updater.rc >+++ b/toolkit/mozapps/update/updater/updater.rc >@@ -33,16 +33,33 @@ 1 RT_MANIFEST > // Icon > // > > IDI_DIALOG ICON "updater.ico" > > > ///////////////////////////////////////////////////////////////////////////// > // >+// Embedded certificates for allowed MARs >+// >+ >+#if defined(MAR_SIGNING_RELEASE_BETA) >+IDR_PRIMARY_CERT TYPE_CERT "release_primary.der" >+IDR_BACKUP_CERT TYPE_CERT "release_secondary.der" >+#elif defined(MAR_SIGNING_AURORA_NIGHTLY) >+IDR_PRIMARY_CERT TYPE_CERT "nightly_aurora_level3_primary.der" >+IDR_BACKUP_CERT TYPE_CERT "nightly_aurora_level3_secondary.der" >+#else >+IDR_PRIMARY_CERT TYPE_CERT "dep1.der" >+IDR_BACKUP_CERT TYPE_CERT "dep2.der" >+#endif >+ >+ >+///////////////////////////////////////////////////////////////////////////// >+// > // Embedded an identifier to uniquely identiy this as a Mozilla updater. > // > > STRINGTABLE > { > IDS_UPDATER_IDENTITY, "moz-updater.exe-4cdccec4-5ee0-4a06-9817-4cd899a9db49" > } > Am I reading this part right, and that it would break any applications doing a windows update without actually signing mars yet? In that the else is stuffing dep1.der/etc. into the resource? And these would be better defined in confvars.sh or something like that (I admittedly only skimmed this code after seeing Bug 728935 being filed)

> Am I reading this part right, and that it would break any applications doing a > windows update without actually signing mars yet? Updater code will only do the signature check conditionally. You need to have MOZ_VERIFY_MAR_SIGNATURE=1 in your confvars.sh or --enable-verify-mar in your .mozconfig

(In reply to Brian R. Bondy [:bbondy] from comment #39) > > Am I reading this part right, and that it would break any applications doing a > > windows update without actually signing mars yet? > > Updater code will only do the signature check conditionally. > You need to have MOZ_VERIFY_MAR_SIGNATURE=1 in your confvars.sh or > --enable-verify-mar in your .mozconfig Ok, so it won't break us by default -- but then when say SeaMonkey or Thunderbird do start signing mars we'll be broken by this, since it explicitly does Firefox [MoCo] based cert verification with no way to change.

Ya once you want to start signing we'll need another bug posted with some tweaks. I can help with that when the time comes.

Updated which channels get which DER file as per bug Carrying forward r+ on the patch. Requesting feedback review from bhearsum just for the changes in: toolkit/mozapps/update/updater/Makefile.in

Comment on attachment 599361 [details] [diff] [review] Verifying signed MARs from updater.exe. v9. Review of attachment 599361 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/update/updater/Makefile.in @@ +149,5 @@ > + > +ifneq (,$(filter beta release esr,$(MOZ_UPDATE_CHANNEL))) > +RCFLAGS += -DMAR_SIGNING_RELEASE_BETA=1 > +else > +ifneq (,$(filter nightly aurora nightly-elm nightly-profiling nightly-oak,$(MOZ_UPDATE_CHANNEL))) Ugh. This is going to be such a PITA to manage.

I think using the dep cert or some new cert for all nightlies except for aurora and m-c would help that.

(In reply to Brian R. Bondy [:bbondy] from comment #44) > I think using the dep cert or some new cert for all nightlies except for > aurora and m-c would help that. Yes, but then folks on those would get UAC prompts, no?

> Yes, but then folks on those would get UAC prompts, no? Nope, the MAR signing is an updater security enhancement and is not really related to the work in the service to get rid of the UAC dialog. The patches could have been applied independent or even before the service landed.

http://hg.mozilla.org/mozilla-central/rev/94f7927299e5 http://hg.mozilla.org/mozilla-central/rev/58402b43c9e1