Key Pinning is a mechanism by which site owners can specify a set of keys (actually fingerprints of the keys) such that the in the next connection to the site, the set of keys in the cerficiate chain MUST intersect with the set of keys 'pinned' in the browser. The spec is here https://tools.ietf.org/html/draft-evans-palmer-key-pinning

Camilo, I suggest that before you write a lot of code, that we should agree on the approach. I think it is probably right to use permission manager to store the permissions, especially since we use it to store HSTS information. But, keep in mind that you cannot access the permission manager service from the certificate verification threads. That means you will have to get the information out of the permission manager on the main thread very early (before you create the socket transport, I think) and make sure it gets passed along. One thing you should NOT do is try to use SyncRunnableBase or other tricks to jump to the main thread from the certificate verification threads; the crazy thread hopping we do in SSLServerCertVerification is very unfortunate and I hope to fix that when I have time. I have some ideas for doing this, because I have been thinking about this problem for a while. Let's chat about it. Also, there are quite a few places that do certificate validation in Gecko. It might be smarter to refactor the code a little bit to reduce the number of places that have to be changed, and to reduce the likelihood that we will miss one of those places. We should also review the draft and make sure that we want to implement it exactly as drafted, or whether we want to suggest changes to the proposed spec.

This contains in essence the idl definitions of the new service.

Current pathset works (on linux x86_64), but 1. requires enabling libpkix (create new boolean pref in about:config named security.use_libpkix_verification and set it to true). 2. fails to note pins on bad pinning headers. (dup headers or non standard directives) 3. Allows the backup pin to be part of the current pinning chain. 4. Uses in-code constants for built-in pins. 5. The error code displayed to users when trying to connect to a pinnned site on pin mismatch is: Unrecognized object identifier (not correct or useful).

cviecco: are you going to request review of these patches from someone? Gerv

Gerv. I moved to work on the libpkix dependency of the bug. You are welcome to give feedback.

Request for comments: Currently, the interface provided by the publicKeyPinningService is a single call, in which the pinning service returns a bool indicating if a host is pinned and a string with the concatenated pins. Now that I am looking at the set of built-in pins. This approach seems slow as the pinningservice must make a new string and cannot use any knowledge of the built-in data to speed up queries (some pinsets are large, twitter has 19 pins and its cdn 36). So I am thinking that a better interface would be of two calls: First the caller asks if it is pinned and the pin algorithms for the domain. Then it makes a second call to the pinning service where it sends the fingerprints for the chain and the pinning service returns the yes/no. In other words, the comparison of the sets is moved from the callback into the pinning service.

This patch includes static pin information for several domains, partly generated from Chromouim built-in list.

(In reply to Camilo Viecco (:cviecco) from comment #8) > Currently, the interface provided by the publicKeyPinningService is a single > call, in which the pinning service returns a bool indicating if a host is > pinned and a string with the concatenated pins. > Now that I am looking at the set of built-in pins. This approach seems slow > as the pinningservice must make a new string and cannot use any knowledge of > the built-in data to speed up queries (some pinsets are large, twitter has > 19 pins and its cdn 36). So I am thinking that a better interface would be > of two calls: First the caller asks if it is pinned and the pin algorithms > for the domain. Then it makes a second call to the pinning service where it > sends the fingerprints for the chain and the pinning service returns the > yes/no. There is a potential race condition there where the pinning information has changed between the two calls. > In other words, the comparison of the sets is moved from the callback into > the pinning service. This would mean that the pinning service would have to become thread-safe. It is OK to copy the pinsets even if there are ~50 pins. When we introduce caching of cert validation results, we will need to have make a copy of all the inputs that went into the validation process, including the pins. So, AFAICT, it is not worth trying to avoid the copying now.

Comment on attachment 637933 [details] [diff] [review] Core hookups to libssl and permission manager. Review of attachment 637933 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/public/nsIPublicKeyPinningService.idl @@ +41,5 @@ > +interface nsIObserver; > +interface nsIHttpChannel; > + > +[scriptable, uuid(872a0968-9947-11e1-ada1-180373d97f23)] > +interface nsIPublicKeyPinningService : nsISupports Later we're going to have to store more settings like "require OCSP stapling". I don't feel too strongly about it, but should we really have an STS service, a PublicKeyPinningService, an XXX service, a YYY service, etc. that all do mostly the same thing? It makes more sense to me to have a "TransportSecurityOptionsService" or something like that. @@ +60,5 @@ > + in nsISupports aSecurityInfo, > + in string aHeader); > + > + /** > + * Removes the STS state of a host, including the includeSubdomains state s/STS/pinning/ @@ +74,5 @@ > + boolean shouldIgnorePublicKeyPinningHeader(in nsISupports aSecurityInfo); > + > + /** > + * Checks whether or not the given hostname has STS state set. > + * The host is an STS host if either it has the STS permission, or one of s/STS/... ::: netwerk/protocol/http/nsHttpHandler.cpp @@ +428,5 @@ > } > > + > +nsIPublicKeyPinningService* > +nsHttpHandler::GetPublicKeyPinningService() This does not do refcounting correctly, does it? It should return an already_AddRefed<nsIPublicKeyPinningService> to make things clearer. We should not need to cache the reference to this service, so I don't think it is necessary to have this method. The places that need the public key pinning service should just do_GetService themselves. ::: security/manager/boot/src/nsBOOTModule.cpp @@ +8,5 @@ > #include "nsEntropyCollector.h" > #include "nsSecureBrowserUIImpl.h" > #include "nsSecurityWarningDialogs.h" > #include "nsStrictTransportSecurityService.h" > +#include "nsPublicKeyPinningService.h" Why did you put this in boot instead of ssl? (nsNSSModule)? ::: security/manager/boot/src/nsPublicKeyPinningService.cpp @@ +1,2 @@ > +/* ***** BEGIN LICENSE BLOCK ***** > + * Version: MPL 1.1/GPL 2.0/LGPL 2.1 Wrong license block. Use the MPL 2.0 license block. @@ +90,5 @@ > +{ > +} > + > + > +NS_IMPL_THREADSAFE_ISUPPORTS2(nsPublicKeyPinningService, This implementation claims to be thread safe, but are we actually going to make it thread-safe? @@ +156,5 @@ > + return NS_OK; > +} > + > +NS_IMETHODIMP > +nsPublicKeyPinningService::IsPublicKeyPinnedHost(char const* aHost, char** hostPins, bool* aResult){ Like I mentioned elsewhere, the presence of this method seems to just lead to race conditions. @@ +203,5 @@ > + } > + > + > + //Now some basic sanity checks > + if(NULL==aURI || NULL == aSecurityInfo || NULL == pkHttpHeader){ Gecko style is to do these checks using NS_ENSURE_ARG_POINTER, and to do them at the top of the method. ::: security/manager/ssl/src/SSLServerCertVerification.cpp @@ +118,5 @@ > #include "sslerr.h" > > +//cviecco for the PKIXCALLBACK > +//#include <pkix_pl_cert.h> > +//#include "pkix.h" apparently I cannot include not even the pkix.h header! You cannot include these headers because they are not part of the public NSS API. @@ +671,5 @@ > + > + > +/////////////// > +//This is the callback for ca pinning! > +PRBool publicKeyPinningCallBack(void *state,const CERTCertList *certList ) You need to verify that the trust anchor is a built-in root; otherwise, you will stop corporate intercepting (MiTM) proxies that the user/administrator has configured from working. @@ +837,5 @@ > + char *pinData=NULL; > + bool hostIsPinned=false; > + nsCString pinInfo; > + CERTVerifyCallBack callBackContainer; > + bool pinningEnabled=mozilla::Preferences::GetBool("security.ca_pinning.enable",true); You cannot use the preferences service here because this code isn't running on the main thread. @@ +840,5 @@ > + CERTVerifyCallBack callBackContainer; > + bool pinningEnabled=mozilla::Preferences::GetBool("security.ca_pinning.enable",true); > + > + if(pinningEnabled){ > + nsrv=publicKeyPinningService->IsPublicKeyPinnedHost(hostname,&pinData,&hostIsPinned); This seems racy. What happens when the public key pinning information changes between this call, and the access to the key pinning information below. AFAICT, we should just pass the callback in all cases.

Comment on attachment 644971 [details] [diff] [review] patch2: incusion of built-in pins (*.google.com, *.mozilla.org, et al) Review of attachment 644971 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/boot/src/static_pins.h @@ +1,4 @@ > +/*This is a Generated File*/ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ Let's not check in generated files. Please include the input file and the script that generates this file, and hook up the script into the build process.

Camilo, I'd like to suggest a way to break up this work into separate patches: 1. The back-end, that actually enforces the pinning. 2. The automated tests for the back-end. 3. The script that imports the preloaded pins, the related code, and a test for one of those pins. 4. The processing of the pinning header and tests for that. I also think it would be good if we focused on #1 and #2 because that's already quite a bit of work and it would be good to know that the back-end stuff is working well before we do the rest of the work that depends on it working well.

(In reply to Brian Smith (:bsmith) from comment #13) > Camilo, I'd like to suggest a way to break up this work into separate > patches: > > 1. The back-end, that actually enforces the pinning. > 2. The automated tests for the back-end. > 3. The script that imports the preloaded pins, the related code, and a test > for one of those pins. > 4. The processing of the pinning header and tests for that. > > I also think it would be good if we focused on #1 and #2 because that's > already quite a bit of work and it would be good to know that the back-end > stuff is working well before we do the rest of the work that depends on it > working well. Agreed.

(In reply to Brian Smith (:bsmith) from comment #12) > Comment on attachment 644971 [details] [diff] [review] > patch2: incusion of built-in pins (*.google.com, *.mozilla.org, et al) > > Review of attachment 644971 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: security/manager/boot/src/static_pins.h > @@ +1,4 @@ > > +/*This is a Generated File*/ > > +/* This Source Code Form is subject to the terms of the Mozilla Public > > + * License, v. 2.0. If a copy of the MPL was not distributed with this > > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > Let's not check in generated files. Please include the input file and the > script that generates this file, and hook up the script into the build > process. Since this file affects security so much, I would prefer to include both a well known file and the generator into the source, so that the generation of the dependency does not require internet connectivity at build time. The generator uses two files from the chromium three and one file from the mozilla three.

This is the base idl definition. First thing we need to agree before the rest of the review. Superreview for Kai as it is an IDL and he is the owner of PSM.

Requires applying the patch pending in https://bugzilla.mozilla.org/attachment.cgi?id=638394 . that has been pending review for 5 weeks!

Yay tests. Include a new CA that will be assumed to be a built-in CA, thus has changes in the automation cert section.

Comment on attachment 653504 [details] [diff] [review] Tests The only thing that we talked about and it not implemented are the tests for checking on the intermediate.

A convenience patch. Not needed if libpkix is enabled on the profile.

Comment on attachment 653501 [details] [diff] [review] Idl for new service It's unlikely that I'll find time to be involved in the design phase of this project. Please ensure that you are at a final stage with this work, then I'm OK to have a final pragmatic look at the IDL, if desired. (You should use the new MPL2 license headers)

Comment on attachment 653503 [details] [diff] [review] The actualt implementaiton with interfaces to PSM/NSS, includes built in pins for mozilla, and google (et al) Review of attachment 653503 [details] [diff] [review]: ----------------------------------------------------------------- Needs to revise do to changes in 764973. Do not review.

Comment on attachment 655786 [details] [diff] [review] version 2 of the implementation (after some renaming) Review of attachment 655786 [details] [diff] [review]: ----------------------------------------------------------------- Here's some initial review comments, in addition to the ones given over IRC. It seems like there's a lot of changes that will be made so perhaps it would be better to have these changes made first and then I can re-review the patches. ::: docshell/base/nsDocShell.cpp @@ +4147,5 @@ > + rv = sslstat->GetIsPublicKeyPinningFailure(&pkPinningFailure); > + NS_ENSURE_SUCCESS(rv, rv); > + } > + if(pkPinningFailure){ > + //show badsts for now! comment is wrong, right? ::: modules/libpref/src/init/all.js @@ +1332,5 @@ > pref("security.csp.debug", false); > > +////ca-pinning > +//pref("security.ca_pinning.process_http_headers", true); > +pref("security.ca_pinning.enforcement_level", 1); Where are the various levels documented? ::: netwerk/protocol/http/nsHttpHandler.h @@ +28,5 @@ > #include "nsIIDNService.h" > #include "nsITimer.h" > #include "nsIStrictTransportSecurityService.h" > #include "nsISpeculativeConnect.h" > +#include "nsIPublicKeyPinningService.h" You don't need to #include this. You can just forward declare the class. ::: security/manager/boot/src/Makefile.in @@ +22,5 @@ > nsSecureBrowserUIImpl.cpp \ > nsBOOTModule.cpp \ > nsSecurityWarningDialogs.cpp \ > nsStrictTransportSecurityService.cpp \ > + nsPublicKeyPinningService.cpp \ security/manager/ssl seems like a better place for SSL-related services. ::: security/manager/boot/src/nsPublicKeyPinningService.cpp @@ +1,2 @@ > +/* ***** BEGIN LICENSE BLOCK ***** > + * Version: MPL 2.0/GPL 2.0/LGPL 2.1 Wrong license block. @@ +35,5 @@ > + * > + * ***** END LICENSE BLOCK ***** */ > + > + > +#include "plstr.h" Usually #includes are ordered in the following order (roughly): #include <XPCOM interface headers> #include <stuff from this module> #include <Other Gecko stuff> #include <NSS stuff> #include <NSPR stuff> I suggest that you follow this convention, roughly. This ensures, in particular, that your module-specific headers are not missing forward declarations and #includes. @@ +61,5 @@ > +//The static pin file is autogenerated > +#include "static_pins.h" > + > +#if defined(PR_LOGGING) > +PRLogModuleInfo *gPublicKeyPinningLog = PR_NewLogModule("nsPublicKeyPinningservice"); IMO, we should not create a new logging module for the pinning service. It's slightly more overhead at runtime and it's less convenient for logging SSL-related things. Let's use the pipnss module instead. @@ +78,5 @@ > + > +nsDataHashtableMT<nsCStringHashKey, nsCString> nsPublicKeyPinningService::sSessionPinData; > +bool nsPublicKeyPinningService::sInitialized=false; > +bool nsPublicKeyPinningService::sInitStarted=false; > +bool nsPublicKeyPinningService::sTerminating=false; Will sSessionPinData cause us to execute a static initializer? Static initializers are not allowed because they are bad for startup performance. I don't understand why these are not non-static members of nsPublicKeyPinningService. Nit: please put these static things in the unnamed namespace: (namespace { }) instead of using "static". @@ +88,5 @@ > +} > + > +nsPublicKeyPinningService::~nsPublicKeyPinningService() > +{ > + if(this==sSingleton){ This condition should always @@ +97,5 @@ > +} > + > + > +/* > + These are for the static is fingerprint built in root I do not understand what "static is fingerprint" means. s/built in/built-in/. Also, wrap the lines at 80 characters. @@ +127,5 @@ > +} > + > + > + > +NS_IMPL_THREADSAFE_ISUPPORTS2(nsPublicKeyPinningService, Why @@ +211,5 @@ > +PRBool > +nsPublicKeyPinningService::addSessionEntry(const char *aHost, const char *aPin){ > + const nsCAutoString inHost(aHost); > + const nsCString inPin(aPin); > + //put is void, so there is really no way to check for errors here Is this because the data structure uses infallable malloc? ::: security/manager/ssl/src/SSLServerCertVerification.cpp @@ +470,5 @@ > verify_log, NULL); > } > else { > + nsCERTValInParamWrapper pkixInParams; > + nsCERTValInParamWrapper *pkixParamsToUse=survivingParams; This block of code looks very similar to the block of code in PSM_SSL_PKIX_AuthCertificate. Should these two blocks be factored into a function that gets called? Also, you need to ensure that this function is using *exactly* the same set of pins that PSM_SSL_PKIX_AuthCertificate used, because another thread might have modified the set of pins for this domain between the two calls. Since you're constructing a new CERTValInParamWrapper, you might as well just stuff the pin values into that object and pass the CERTValInParamWrapper instance around. @@ +509,5 @@ > + if(hostIsPinned ){ > + PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("pipnss: OMG! we have PK pins for host '%s'\n",hostname)); > + pinInfo=pinData; > + callBackState.pinInfo=&pinInfo; > + callBackState.enforcementLevel=mozilla::Preferences::GetInt("security.ca_pinning.enforcement_level", 1); You must not access preferences off the main thread. You need to save the value of the pref in the nsNSSSocketInfo instance on the main thread, and then pass that value into this code. @@ +541,3 @@ > cvout, static_cast<void*>(infoObject)); > + > + if(hostIsPinned && (false==callBackState.pinningValue )){ Do not use the "x == true" or "x != false" to compare against "false" or "true". Just use "x" or "!x". Also, please comply with the Mozilla coding style guidelines and/or the MFBT coding style guidelines. @@ +719,5 @@ > +/** > + Parses the PK pin string, assumes the input string is well formed. > +*/ > + > +extern "C" { Why extern "C" when this takes C++ types as parameters? AFAICT, you do not need extern "C" for the callback either because the callback isn't being looked up by name.

Comment on attachment 653501 [details] [diff] [review] Idl for new service Review of attachment 653501 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/public/nsIPublicKeyPinningService.idl @@ +66,5 @@ > + * Checks if the given security info is for an PublicKey host with a broken > + * transport layer (certificate errors like invalid CN). > + * Currently not implemented > + */ > + boolean shouldIgnorePublicKeyPinningHeader(in nsISupports aSecurityInfo); I think that we can just call processPublicKeyPinningHeader passing the security info, and have it do this check itself. @@ +87,5 @@ > + * hashenum = 3|4. 3 is sha1, 4 is sha 256 > + * from security/nss/lib/freebl/hasht. > + * > + */ > + boolean isPublicKeyPinnedHost(in string aHost, out string hostPins); First, PINS are per-origin, not per-host, right? What should we do when isPublicKeyPinnedHost returns true but it returns an empty hostPins? Why not: string getPublicKeyPinsForHost(in string scheme, in string aHost, in long port); Where an empty string means no pins. @@ +93,5 @@ > + /** > + * Adds a entry to the current pin set. This entry will only exist > + * for the current browsing session. These entries are queried > + * before the built in list, so this function MUST be only available > + * from privileged (XUL) js. All XPCOM interfaces are restricted to chrome by default. @@ +101,5 @@ > + * format as isPublicKeyPinnedHost. Must be > + * sorted or the insertaion would fail. > + * @return PR_TRUE if the entry was succesfully inserted. > + */ > + void insertSessionPinEntry(in string aHost, in string aHostPins); Why do we need this, if we have processPublicKeyPinningHeader? Is this just for writing tests? It seems dangerous to require the caller to sort the pins instead of just accepting unsorted pins. Since the number of pins for a given host is going to be extremely small (less than 100) except in DoS type situations, why bother with binary search or whatever other reason we'd require sorted pins? Instead, we can just do a linear search and avoid the complexity and avoid the opportunity for things to go wrong. @@ +110,5 @@ > +%{C++ > +#define NS_PUBLICKEYPINNINGSERVICE_CONTRACTID "@mozilla.org/publickeypinningservice;1" > +#define NS_PUBLICKEYPINNINGSERVICE_CLASSNAME "publickeypinningservice" > + > +#define PUBLICKEYPINNINGPERMISSION "publicKeyPin/use" is this still needed?

Comment on attachment 653501 [details] [diff] [review] Idl for new service Review of attachment 653501 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/public/nsIPublicKeyPinningService.idl @@ +66,5 @@ > + * Checks if the given security info is for an PublicKey host with a broken > + * transport layer (certificate errors like invalid CN). > + * Currently not implemented > + */ > + boolean shouldIgnorePublicKeyPinningHeader(in nsISupports aSecurityInfo); Agreed. This should be removed @@ +87,5 @@ > + * hashenum = 3|4. 3 is sha1, 4 is sha 256 > + * from security/nss/lib/freebl/hasht. > + * > + */ > + boolean isPublicKeyPinnedHost(in string aHost, out string hostPins); First: The spec makes them per host not per origin. It seems strange, but it actually makes some sense as as and org you should trust only a limited set of hosts. Second: the change of interface from bool return to string return I think is not good (and maybe here I am doing premature optimization). The advantage of a bool/int is that we do not have to do an allocation on failure. But if that is of no concern then I think your interface is better. @@ +93,5 @@ > + /** > + * Adds a entry to the current pin set. This entry will only exist > + * for the current browsing session. These entries are queried > + * before the built in list, so this function MUST be only available > + * from privileged (XUL) js. OK. Will remove the comment. @@ +101,5 @@ > + * format as isPublicKeyPinnedHost. Must be > + * sorted or the insertaion would fail. > + * @return PR_TRUE if the entry was succesfully inserted. > + */ > + void insertSessionPinEntry(in string aHost, in string aHostPins); My reson for using faster searches is that we expect most lookups to fail (remember we do it for every part of the chain). I have no numbers to determine that it is faster/slower. But I agree that the interface should handle them in any order (ie, if we want ordering we do it internally). I will remove the requirement to make the insertions sorted (but keep internal data sorted ) instead I will sort the data as inserted. @@ +110,5 @@ > +%{C++ > +#define NS_PUBLICKEYPINNINGSERVICE_CONTRACTID "@mozilla.org/publickeypinningservice;1" > +#define NS_PUBLICKEYPINNINGSERVICE_CLASSNAME "publickeypinningservice" > + > +#define PUBLICKEYPINNINGPERMISSION "publicKeyPin/use" No it is not needed anymore. Agreed It will go away.

With the exception of the comments for the 'isthisHostPinned' this could be the final idl.

Should if we agree on the isHostPinned. This should be the final idl.

Just for backup, to compilion+running with the version 3 of the idl. Comments for the version2 of the implementation not yet addressed fully

Test for v3 of idl. (more comprehensive)

On bug 770594 you mentioned issues on https proxies. By that you mean the ssl proxy specified in Preferences->Advanced->Network -> Connection Settings?

Not forgoten. https://tbpl.mozilla.org/?tree=Try&rev=94ac7b11afde

+1 - this is very important for security See bug 471779 for Firefox updater.

(In reply to Ben Bucksch (:BenB) from comment #37) > +1 - this is very important for security > > See bug 471779 for Firefox updater. This does not actually help much with Firefox updates because we need to be able to update Firefox through (corporate and/or local anti-virus) MitM proxies. If anything, as far as the updater is concerned, we need to extend the MAR signing mechanism used on Windows to the other platforms. (In fact, I would like to avoid using SSL at all as part of updates, to reduce the number of moving parts in the update process; e.g. to avoid bugs in our SSL implementation affecting the ability of users to download Firefox.) IMO, Products that are not Firefox should implement the MAR signing mechanism that Firefox uses, instead of relying on this mechanism, for similar reasons. All that said, we are doing a lot of work to resolve the prerequisites for this feature so that it can land.

> This does not actually help much with Firefox updates because we need to be able to update > Firefox through (corporate and/or local anti-virus) MitM proxies. 1) They are already broken. We currently check for a specific CA, and these CAs don't give out MITM certs, from what I know. 2) The admin can reconfigure the clients which cert they check for. The admin can just configure their own cert (or update server, even) 3) We (as a project) consider MITM SSL proxies bad, even in a corporate setup.

Anyway, Firefox updater is a side issue not relevant here. I mentioned it just as a reference to another bug. Let's discuss there. > we are doing a lot of work to resolve the prerequisites for this feature so that it can land. Thank you.

Via insanity::pkix https://tbpl.mozilla.org/?tree=Try&rev=29aaf4b71a18

Comment on attachment 8343423 [details] [diff] [review] pinning-idl Review of attachment 8343423 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to start. Just a few comments. ::: netwerk/base/public/nsIPublicKeyPinningService.idl @@ +9,5 @@ > + > + > +[scriptable, uuid(872a0968-9947-11e1-ada1-180373d97f23)] > + > +interface nsIPublicKeyPinningService : nsISupports How about a function that takes a host and the verified chain and returns true if either the host isn't pinned or the pin is found in the chain? Maybe something like "isPinSatisfiedForHostAndChain"? (except that's a bit of a cumbersome name...) @@ +33,5 @@ > + * If hostPins are returned these will be sorted > + * alphabetically by their base64 representation. > + * > + */ > + boolean isPublicKeyPinnedHost(in string aHost, out string hostPins); Maybe just "isPinnedHost"? @@ +45,5 @@ > + * @param aHostPins The string represinging the pins, in the same > + * format as isPublicKeyPinnedHost. > + * @return PR_TRUE if the entry was succesfully inserted. > + */ > + void insertSessionPinEntry(in string aHost, in string aHostPins); We're only pinning amo to begin with, and it'll be using the built-in list, right? I don't think we should even worry about adding new, dynamic entries until we're ready to move on to the next phase.

> I don't think we should even worry about adding new, dynamic entries > until we're ready to move on to the next phase. This feature would be highly useful for security-conscious users. Thus, please do go as far as you can with this. Esp. the IDL should be prepared for that, to avoid changing interfaces later.

Part of rewrite with built-in pins only

Comment on attachment 8363215 [details] [diff] [review] certverify-add-insertErrorIntoVerifyLog Review of attachment 8363215 [details] [diff] [review]: ----------------------------------------------------------------- This is needed for pinning an other stuff (such as key sizes). Move it to another bug?

Comment on attachment 8363217 [details] [diff] [review] always-compute-chain-in-cerverify Review of attachment 8363217 [details] [diff] [review]: ----------------------------------------------------------------- Need to always compute the certchain unconditionally for usage ssl-server so that we can put pinning errors inside the verifylog (this does it for the classic side).

Comment on attachment 8363218 [details] [diff] [review] always-compute-chain-libpkix Review of attachment 8363218 [details] [diff] [review]: ----------------------------------------------------------------- Libpkix provides a callback at path verification time, (I dont need to compute a new chain as this callback is called just before the final checks on the chain are done).

Comment on attachment 8363223 [details] [diff] [review] add-hostname-to-certverify0 Review of attachment 8363223 [details] [diff] [review]: ----------------------------------------------------------------- Cerverifier needs a new paramater (hostname) in order to evaluate the pinning info. Unfortunatelly this is not available in all places where the certverifier is called.

Comment on attachment 8363227 [details] [diff] [review] pinning-service-imp Review of attachment 8363227 [details] [diff] [review]: ----------------------------------------------------------------- This looks as a hack because it comments out the dynamic stuff. Just take a look at nsPublicKeyPinningService::chainHasValidPins and ignore the fact that I am using sha1 fingerprints (they should be moved to sha256, but that does not affect the feedback). Also, I am combining google's pins and mozilla's which would not happen in the first iteration (ignore make-fingerprints.pl).

Comment on attachment 8363229 [details] [diff] [review] pinning-interface Review of attachment 8363229 [details] [diff] [review]: ----------------------------------------------------------------- Adds pin checks to the 'classic' path (notice these are evaluated also in the case of cert error to ensure the error is not overridable).

Comment on attachment 8363215 [details] [diff] [review] certverify-add-insertErrorIntoVerifyLog Review of attachment 8363215 [details] [diff] [review]: ----------------------------------------------------------------- Yes, this probably should be a separate bug. ::: security/manager/ssl/src/CertVerifier.cpp @@ +39,5 @@ > MOZ_COUNT_DTOR(CertVerifier); > } > > +static SECStatus > +insertErrorIntoVerifyLog(CERTCertificate * cert, const PRErrorCode err, Can we add a public function that basically exports cert_AddToVerifyLog from certvfy.c? (this would be an NSS patch)

Comment on attachment 8363217 [details] [diff] [review] always-compute-chain-in-cerverify Review of attachment 8363217 [details] [diff] [review]: ----------------------------------------------------------------- It seems like we'd be doing extra work for every verification, regardless of if there is any pinning information. Perhaps we should only do this if we know there is pinning information for the given host/certificate?

Comment on attachment 8363215 [details] [diff] [review] certverify-add-insertErrorIntoVerifyLog Review of attachment 8363215 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/src/CertVerifier.cpp @@ +39,5 @@ > MOZ_COUNT_DTOR(CertVerifier); > } > > +static SECStatus > +insertErrorIntoVerifyLog(CERTCertificate * cert, const PRErrorCode err, I would prefer it to live inside PSM as: 1. It is only needed inside this file 2. Would need to go to nss review and wait for a new nss release. 3. Augmenting NSS would remove flexibility (this would become part of the ABI)

Comment on attachment 8363218 [details] [diff] [review] always-compute-chain-libpkix Review of attachment 8363218 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/src/CertVerifier.cpp @@ +296,5 @@ > i = 3; > + > + CERTChainVerifyCallback callbackContainer; > + chainValidationCallbackState_t callbackState; > + if (usage == certificateUsageSSLServer) { Again, it seems like we should do this only if we know the host in question has a pin.

Comment on attachment 8363223 [details] [diff] [review] add-hostname-to-certverify0 Review of attachment 8363223 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/src/SSLServerCertVerification.cpp @@ +531,5 @@ > CERTVerifyLogContentsCleaner verify_log_cleaner(verify_log); > verify_log->arena = log_arena; > > srv = certVerifier->VerifyCert(cert, certificateUsageSSLServer, now, > + infoObject, 0, nullptr, nullptr, nullptr, verify_log); //XXX get hostname infoObject should have the hostname (it's an nsNSSSocketInfo)

(In reply to David Keeler (:keeler) from comment #61) > Comment on attachment 8363217 [details] [diff] [review] > always-compute-chain-in-cerverify > > Review of attachment 8363217 [details] [diff] [review]: > ----------------------------------------------------------------- > > It seems like we'd be doing extra work for every verification, regardless of > if there is any pinning information. Perhaps we should only do this if we > know there is pinning information for the given host/certificate? In the most performance sensitive call (successful SSLServerCertVerification) the chain is being computed anyway. This only changes the case for: places where the chain is not requested with usage SSL_server and the error cases. This will happen only in viewing the certificate which is already a slow operation. For static pins, this first check for pins could be ok, but I would have to ensure I keep a some data struct on the dynamic case to prevent TOCTOU issues.

(In reply to David Keeler (:keeler) from comment #63) > Comment on attachment 8363218 [details] [diff] [review] > always-compute-chain-libpkix > > Review of attachment 8363218 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: security/manager/ssl/src/CertVerifier.cpp > @@ +296,5 @@ > > i = 3; > > + > > + CERTChainVerifyCallback callbackContainer; > > + chainValidationCallbackState_t callbackState; > > + if (usage == certificateUsageSSLServer) { > > Again, it seems like we should do this only if we know the host in question > has a pin. There is no extra computation, libpkix already has this chain (well there is some extra allocs if the callback is present).

Comment on attachment 8363227 [details] [diff] [review] pinning-service-imp Review of attachment 8363227 [details] [diff] [review]: ----------------------------------------------------------------- This looks like a good start. ::: security/manager/boot/src/make-fingerprints.pl @@ +1,1 @@ > +#!/usr/bin/perl -w This should probably go in security/manager/tools. Also, I don't know perl. Could I convince you to write this in python or javascript (run using xpcshell)? ::: security/manager/boot/src/nsPublicKeyPinningService.cpp @@ +64,5 @@ > + return strcmp((char *)key, (char*) *actual_entry); > +} > + > +PRBool > +nsPublicKeyPinningService::isFingerprintBuiltinRoot(const char *aFingerPrint){ I think there's a way to tell if a certificate is a builtin by its token. (Or maybe I'm not understanding why you're doing this - you're checking for builtin-ness because we won't enforce pinning for non-built-in roots, right?)

Comment on attachment 8363229 [details] [diff] [review] pinning-interface Review of attachment 8363229 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.

Comment on attachment 8363227 [details] [diff] [review] pinning-service-imp Review of attachment 8363227 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/boot/src/make-fingerprints.pl @@ +1,1 @@ > +#!/usr/bin/perl -w Everything is possible (Mohamed worked a bit this summer on pythonizing it but failed after a few days). Bear with me the perl generator until after we have migrated everything to sha256.

Comment on attachment 8367641 [details] [diff] [review] always-compute-chain-usagesslserver-classic Review of attachment 8367641 [details] [diff] [review]: ----------------------------------------------------------------- Unless I'm misunderstanding how this works, I don't think this patch achieves what we want. The idea is to always get the certificate chain, even if verification fails, right? Anyway, see comments below... ::: security/certverifier/CertVerifier.cpp @@ +100,5 @@ > /*optional out*/ CERTVerifyLog* verifyLog) > { > SECStatus rv; > SECCertUsage enumUsage; > + switch(usage){ As long as you're changing this line for indentation, you might as well fix the spaces (one after "switch" and one before "{"). I guess you could do it in a different patch like we've agreed on. @@ +151,5 @@ > } else { > rv = CERT_VerifyCertificate(CERT_GetDefaultCertDB(), cert, true, > usage, time, pinArg, verifyLog, nullptr); > } > + if (rv == SECSuccess || usage == certificateUsageSSLServer) { So, this changes the code to always try and get the certificate chain (for SSL server usage), even if verification fails, right? @@ +161,2 @@ > } > + if (rv == SECSuccess && validationChain) { This rv is still the result of the verification call, right? So if verification fails, we'll actually not get the certificate chain, it seems. Also, will CERT_GetCertChainFromCert return non-null if the certificate won't verify? Additionally, if usage == certificateUsageSSLServer, should we make validationChain not optional?

Comment on attachment 8367641 [details] [diff] [review] always-compute-chain-usagesslserver-classic Review of attachment 8367641 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/certverifier/CertVerifier.cpp @@ +100,5 @@ > /*optional out*/ CERTVerifyLog* verifyLog) > { > SECStatus rv; > SECCertUsage enumUsage; > + switch(usage){ my bad (auto color does not matched 'switch') @@ +151,5 @@ > } else { > rv = CERT_VerifyCertificate(CERT_GetDefaultCertDB(), cert, true, > usage, time, pinArg, verifyLog, nullptr); > } > + if (rv == SECSuccess || usage == certificateUsageSSLServer) { Yes (as I will need to compute potential new errors based on the chain (pinning failure)) @@ +161,2 @@ > } > + if (rv == SECSuccess && validationChain) { Incorrect, on an untrusted cert, CERT_GetCertChainFromCert will return a chain with only the cert as the member of the chain (lenght 1). Also, the chain should only be returned on successful verification. less than half of the places where we call verifycert need the chain, it should be left optional. Seems like I need to better document verifycert.

(In reply to David Keeler (:keeler) from comment #76) > Unless I'm misunderstanding how this works, I don't think this patch > achieves what we want. The idea is to always get the certificate chain, even > if verification fails, right? Anyway, see comments below... We should not rely on being able to build a cert chain (even a partial one) if verification fails. For reporting pinning errors, we should send the certs that are in the SSL handshake, unprocessed. If the spec doesn't allow that then there's a problem with the spec.

Comment on attachment 8373730 [details] [diff] [review] reorder-verifycert-parameters Review of attachment 8373730 [details] [diff] [review]: ----------------------------------------------------------------- Is this urgent? My vote would be to do cleanup like this after landing insanity to avoid bitrot. Also, it should probably be a separate bug. ::: security/certverifier/CertVerifier.h @@ +30,5 @@ > const Flags flags = 0, > /*optional out*/ insanity::pkix::ScopedCERTCertList* validationChain = nullptr, > /*optional out*/ SECOidTag* evOidPolicy = nullptr , > + /*optional out*/ CERTVerifyLog* verifyLog = nullptr, > + /*optional in*/ const SECItem* stapledOCSPResponse = nullptr); I actually think this should go before all of the out parameters (so just after flags). Also, anything with a default value is implicitly an optional in parameter, so we probably don't need the comment.

Comment on attachment 8373732 [details] [diff] [review] pinning-service-imp (v2) Review of attachment 8373732 [details] [diff] [review]: ----------------------------------------------------------------- What happens in a situation like this: foo.com: pin-sha256="d6qzRu9zOECb90Uez27xWltNsj0e1Md7GkYYkVoZWmM="; includeSubdomains bar.foo.com: pin-sha256="LPJNul+wow4m6DsqxbninhsWHlwfp0JecwQzYpOLmCQ=" And no certificate in bar.foo.com's chain matches the pin it specifies, but one does match the foo.com entry? Also, maybe it would be simpler to break this into two parts: 1. pinning 2. the non-built-in root exception (also, why not just traverse the list before looking up any pins to see if it chains to a non-built-in root? Then you would know to quit early, and otherwise fail closed.) Overall, looks like a good approach. It still needs some cleanup, and I think the implementation could be simpler (particularly for checking if a certificate is a built-in root). ::: security/manager/boot/src/make-fingerprints.pl @@ +224,5 @@ > + #\xC3\xAD > + $string_alias =~ s/\\x[0-9A-Fa-f]{2}//g; > + > + $string_alias =~ s/-|\/|\(|\)/__/g; > + nit: unnecessary whitespace @@ +432,5 @@ > + > + > + print INCFILE <<"LABEL"; > +/*Domainlist*/ > +class nsTransportSecurityPreload nit: trailing whitespace @@ +434,5 @@ > + print INCFILE <<"LABEL"; > +/*Domainlist*/ > +class nsTransportSecurityPreload > +{ > + public: nit: trailing whitespace ::: security/manager/boot/src/mozilla_transport_security_state_static.json @@ +29,5 @@ > +//Geotrust Primary -> www.mozilla.org > +//Geotrust Global -> *. addons.mozilla.org > + > +//from bug 77646, mozilla uses GeoTrust, Digicert and Thawte > +//geotrust ca info: http://www.geotrust.com/resources/root-certificates/index.html nit: trailing space @@ +84,5 @@ > + "thawte Primary Root CA - G3", > + "Baltimore CyberTrust Root" > + ] > + }, > + { nit: trailing whitespace @@ +145,5 @@ > + { "name": "cdn.mozilla.net", "include_subdomains": true, "mode": "force-https", "pins": "mozilla_cdn" }, > + { "name": "cdn.mozilla.org", "include_subdomains": true, "mode": "force-https", "pins": "mozilla_cdn" }, > + { "name": "media.mozilla.com", "include_subdomains": true, "mode": "force-https", "pins": "mozilla_cdn" }, > + { "name": "getpersonas.org", "include_subdomains": true, "mode": "force-https", "pins": "mozilla_cdn" } > + int: trailing whitespace ::: security/manager/boot/src/nsPublicKeyPinningService.cpp @@ +13,5 @@ > +#include "portreg.h" > + > +#include "plstr.h" > +#include "prlog.h" > +#include "prprf.h" nit: sort all of the #includes and separate them according to https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#C.2FC.2B.2B_practices @@ +160,5 @@ > + // we still consider the chain valid if the anchor cert is not > + // a built-in root > + getbase64pkpin(anchorCert, 0, base64Out); > + trustAnchorIsBuiltInRoot = > + nsPublicKeyPinningService::isFingerprintBuiltinRoot(base64Out); See https://mxr.mozilla.org/mozilla-central/source/toolkit/modules/CertUtils.jsm#171 for an easier way to see if a certificate is a built-in root. @@ +189,5 @@ > + > +bool nsPublicKeyPinningService::chainHasValidPins(const CERTCertList *certList, > + const char *hostname, > + bool strictPinning) { > + const nsCString localHost(hostname); This is unused. Also, localHost is probably a poor variable name to use. @@ +194,5 @@ > + > + nsTransportSecurityPreload *foundEntry = nullptr; > + char *evalHost = (char *)hostname; > + char *evalPart; > + while(nullptr == foundEntry && (evalPart = strchr(evalHost, '.')) ){ !foundEntry @@ +203,5 @@ > + kPublicKeyPinningPreloadList, > + kPublicKeyPinningPreloadListLength, > + sizeof(nsTransportSecurityPreload), > + TransportSecurityPreloadCompare); > + if (nullptr != foundEntry) { !foundEntry @@ +207,5 @@ > + if (nullptr != foundEntry) { > + PR_LOG(gPublicKeyPinningLog, PR_LOG_DEBUG, > + ("pkpin: iteration found pinning for host: '%s'\n", evalHost)); > + if (evalHost != hostname) { > + if (false == foundEntry->mIncludeSubdomains){ !foundEntry->mIncludeSubdomains ::: security/manager/boot/src/nsPublicKeyPinningService.h @@ +8,5 @@ > + > +#include "cert.h" > + > +// {872a0968-9947-11e1-ada1-180373d97f23} > +#define NS_PUBLIC_KEY_PINNING_CID \ Doesn't look like this is used. Add a nsIPublicKeyPinningService.idl if you need this kind of functionality. @@ +22,5 @@ > + /** > + Given a fingeprint returns true if it is a built in root. > + The input the base64 sha256 fp > + */ > + static PRBool isFingerprintBuiltinRoot(const char *); I think use of PR types is discouraged. ::: security/manager/boot/src/static_pins.h @@ +1,1 @@ > +/*This is a Generated File*/ nit: space after and before /*, */ Also, how is this file generated? @@ +874,5 @@ > + > +const nsStaticPinset kBuiltInRootPinningFingerprint = { 155, kBuiltInRootPinningFingerprintData }; > + > +/*Domainlist*/ > +class nsTransportSecurityPreload nit: trailing whitespace @@ +876,5 @@ > + > +/*Domainlist*/ > +class nsTransportSecurityPreload > +{ > + public: nit: trailing whitespace

Comment on attachment 8371975 [details] [diff] [review] always-compute-chain-usagesslserver-classic (v1.1) Review of attachment 8371975 [details] [diff] [review]: ----------------------------------------------------------------- I'm still not understanding this patch - I'm cancelling the review until we can talk about it over vidyo, hopefully tomorrow.

Comment on attachment 8384925 [details] [diff] [review] pinning-certverify-interface Review of attachment 8384925 [details] [diff] [review]: ----------------------------------------------------------------- As you requested I folded all the interface patches together (minus the implementation of the pinning per se). I is kind of overwhelming as I am also moving some parameters of certverify but I think the move makes the call simpler.

Comment on attachment 8384925 [details] [diff] [review] pinning-certverify-interface Review of attachment 8384925 [details] [diff] [review]: ----------------------------------------------------------------- Overall, this looks good. I think the implementation is a bit more general than it needs to be - let's avoid function pointers, etc., and optimize for making the insanity::pkix implementation clear and simple. Also, keep an eye out for formatting issues. ::: security/apps/AppTrustDomain.h @@ +34,5 @@ > const CERTCertificate* cert, > /*const*/ CERTCertificate* issuerCertToDup, > PRTime time, > /*optional*/ const SECItem* stapledOCSPresponse); > + SECStatus IsChainValid(const CERTCertList * certChain) {return SECSuccess;}; nit: CERTCertList* ::: security/certverifier/CertVerifier.h @@ +76,5 @@ > #endif > const bool mOCSPDownloadEnabled; > const bool mOCSPStrict; > const bool mOCSPGETEnabled; > + const int mPinningEnforcementLevel; maybe make this an enumerated type, like implementation_config? ::: security/certverifier/NSSCertDBTrustDomain.cpp @@ +281,5 @@ > + PR_SetError(SEC_ERROR_INVALID_ARGS, 0); > + return SECFailure; > + } > + PRBool ChainOK; > + rv = (mCheckChainCallback->isChainValid)(mCheckChainCallback->isChainValidArg, Function pointers make code more difficult to read and understand. ::: security/certverifier/NSSCertDBTrustDomain.h @@ +55,5 @@ > LocalOnlyOCSPForEV = 4, > }; > NSSCertDBTrustDomain(SECTrustType certDBTrustType, OCSPFetching ocspFetching, > + void* pinArg, > + CERTChainVerifyCallback* checkChainCallback = nullptr); Since NSSCertDBTrustDomain is an implementation of TrustDomain, I don't think we need to use another level of indirection by having a pluggable CERTChainVerifyCallback. Why can't we pass it exactly the information it needs? (e.g. the hostname?) I know this might result in some code duplication, but let's optimize for making the implementation for insanity::pkix as clear and simple as possible, since that's what we're moving towards (and away from classic verification). ::: security/insanity/include/insanity/pkixtypes.h @@ +97,5 @@ > PRTime time, > /*optional*/ const SECItem* stapledOCSPresponse) = 0; > > + // Called as soon as we think we have a valid chain but before revocation > + // revocation checks are done. Called to compute chain level checks. nit: "revocation" is repeated ::: security/insanity/lib/pkixbuild.cpp @@ +222,5 @@ > + return MapSECStatus(SECFailure); > + } > + > + //Buildlist > + rv = subject.PrependNSSCertToList(certChain.get()); It's a shame we have to build an entirely new certificate list here for every path we try. I bet we could re-configure how path building works to build the chain as it goes (instead of as it unwinds), so you already have a complete chain at this point, but that's probably something to leave for a follow-up bug. @@ +223,5 @@ > + } > + > + //Buildlist > + rv = subject.PrependNSSCertToList(certChain.get()); > + if (rv != SECSuccess) { I think rv is a Result, not a SECStatus. @@ +231,5 @@ > + while (child) { > + rv = child->PrependNSSCertToList(certChain.get()); > + if (rv != SECSuccess) { > + return rv; > + }; nit: stray ";" @@ +247,5 @@ > if (!results) { > return FatalError; > } > rv = subject.PrependNSSCertToList(results.get()); > + if (rv != SECSuccess) { this isn't necessary ::: security/manager/ssl/src/SSLServerCertVerification.cpp @@ +615,5 @@ > PLArenaPool* log_arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE); > PLArenaPoolCleanerFalseParam log_arena_cleaner(log_arena); > if (!log_arena) { > NS_ERROR("PORT_NewArena failed"); > + } this shouldn't be moved ::: security/manager/ssl/src/nsCMS.cpp @@ +268,3 @@ > certificateUsageEmailSigner, > + PR_Now(), nullptr /*XXX pinarg*/, > + nullptr /*hostname*/); I'm not sure we need to specify nullptr for either pinarg or hostname here, but no big deal.

Comment on attachment 8401599 [details] [diff] [review] pinning-service-imp (v3.1) Review of attachment 8401599 [details] [diff] [review]: ----------------------------------------------------------------- The moving of make-fingerprint.pl to python is something to be done later.

I didn't get to your patches today, Camilo - I'll review them on Monday.

(In reply to David Keeler (:keeler) from comment #94) > I didn't get to your patches today, Camilo - I'll review them on Monday. No problem, we all have our schedule messed up these days.

Ok, so that estimate was too optimistic. I need more time to take a better look at these.

Comment on attachment 8401599 [details] [diff] [review] pinning-service-imp (v3.1) Review of attachment 8401599 [details] [diff] [review]: ----------------------------------------------------------------- Looks good for the most part. Watch out for style/formatting issues. I think the main effort will be to re-write the generator in javascript. ::: security/manager/boot/src/make-fingerprints.pl @@ +1,1 @@ > +#!/usr/bin/perl -w My understanding from the conversation over email is that this will be re-written in javascript (xpcshell). ::: security/manager/boot/src/mozilla_transport_security_state_static.json @@ +1,1 @@ > +// Copyright (c) 2012 The Chromium Authors. All rights reserved. Why are we checking this in to our tree? ::: security/manager/boot/src/nsPublicKeyPinningService.cpp @@ +2,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "nsPublicKeyPinningService.h" > +//The static pin file is autogenerated nit: space after // @@ +14,5 @@ > +#include "prprf.h" > +#include "seccomon.h" > +#include "sechash.h" > + > + nit: unnecessary blank line @@ +20,5 @@ > +PRLogModuleInfo *gPublicKeyPinningLog = > + PR_NewLogModule("nsPublicKeyPinningservice"); > +#endif > + > +#define PKPINLOG(args) PR_LOG(gPublicKeyPinningLog, 4, args) nit: is this used if PKPIN_PARSER_FAIL_IF isn't used? @@ +22,5 @@ > +#endif > + > +#define PKPINLOG(args) PR_LOG(gPublicKeyPinningLog, 4, args) > + > +#define PKPIN_PARSER_FAIL_IF(test, args) \ nit: is this used? @@ +28,5 @@ > + PKPINLOG(args); \ > + return NS_ERROR_FAILURE; \ > + } > + > +nsPublicKeyPinningService::nsPublicKeyPinningService() If the constructor/destructor don't need to do anything, just stick with the implicit ones and don't explicitly define them. @@ +42,5 @@ > + public pins cleaner. > +*/ > +typedef struct > +{ > + public: nit: don't indent "public:", indent everything else to only 2 spaces @@ +47,5 @@ > + HASH_HashType hashType; > + unsigned int hashLength; > + unsigned int base64Length; > + char id[2]; > +} PKPinHashInfo; this needs more documentation - what does id represent? @@ +49,5 @@ > + unsigned int base64Length; > + char id[2]; > +} PKPinHashInfo; > + > +static const PKPinHashInfo kpkPinHashInfo[]={ nit: watch out for double spaces, have spaces around '=' @@ +50,5 @@ > + char id[2]; > +} PKPinHashInfo; > + > +static const PKPinHashInfo kpkPinHashInfo[]={ > + {HASH_AlgSHA256, SHA256_LENGTH, 44, "4" }, nit: 44 and "4" seem like magic numbers - document how these values are arrived at @@ +56,5 @@ > + > +static const int kpkPinHashInfoSize = 1; > + > +static SECStatus > +getbase64pkpin(const CERTCertificate *cert, const int pkhashinfoindex, nit: GetBase64PKPin, pkHashInfoIndex @@ +64,5 @@ > + const SECItem *derPublicKey = &(cert->derPublicKey); > + > + memset(temp_dest, 0x00, HASH_LENGTH_MAX); > + srv = HASH_HashBuf(kpkPinHashInfo[pkhashinfoindex].hashType, temp_dest, > + (unsigned char *)derPublicKey->data, derPublicKey->len); nit: alignment @@ +73,5 @@ > + SECItem toConvert; > + toConvert.type = siBuffer; > + toConvert.len = kpkPinHashInfo[pkhashinfoindex].hashLength; > + toConvert.data = (unsigned char *) temp_dest; > + if (!NSSBase64_EncodeItem(NULL, base64Out, HASH_LENGTH_MAX*2, &toConvert)) { nits: nullptr, two spaces before base64Out @@ +90,5 @@ > + SECStatus srv; > + CERTCertificate *currentCert; > + char base64Out[HASH_LENGTH_MAX*2]; > + > + //now we can actually do something nit: space after // @@ +105,5 @@ > + PR_LOG(gPublicKeyPinningLog, PR_LOG_DEBUG, > + ("pkpin: certArray[%d] common_name: '%s'\n", i, > + CERT_GetCommonName(&(currentCert->issuer)))); > + > + // I only need sha256fp for builtins (so far) Since you only need sha-256 for now, let's not implement the more general case yet. We may not ever need it, and it needlessly complicates the implementation now. @@ +117,5 @@ > + ("pkpin: base_64(hash(key)='%s'\n", base64Out)); > + // Compare, linear search for now... > + int j; > + for (j = 0; j < pinSet->size; j++){ > + if ( 0 == strcmp(pinSet->data[j], base64Out)){ nit: strcmp(...) == 0 @@ +136,5 @@ > + > +/** > + Comparator for the is public key pinned host. > +*/ > + nit: unnecessary blank line @@ +137,5 @@ > +/** > + Comparator for the is public key pinned host. > +*/ > + > +static int TransportSecurityPreloadCompare(const void *key, const void *entry) nit: 'static int' on a separate line from TransportSecurityPreloadCompare @@ +144,5 @@ > + const nsTransportSecurityPreload *preloadEntry = > + (const nsTransportSecurityPreload *)entry; > + > + return strcmp(keyStr, preloadEntry->mHost); > + nit: unnecessary blank line @@ +150,5 @@ > + > +/** > + Expand all names of the EE and evaluate > +*/ > +static bool checkChainAgainstAllNames(const CERTCertList *certList) { nit: return type on separate line, opening brace on separate line @@ +154,5 @@ > +static bool checkChainAgainstAllNames(const CERTCertList *certList) { > + > + PR_LOG(gPublicKeyPinningLog, PR_LOG_DEBUG, > + ("pkpin: top of checkChainAgainstAllNames")); > + CERTCertListNode *node = CERT_LIST_HEAD(certList); CERTCertListNode* @@ +158,5 @@ > + CERTCertListNode *node = CERT_LIST_HEAD(certList); > + if (!node) { > + return false; > + } > + CERTCertificate * cert = node->cert; CERTCertificate* @@ +163,5 @@ > + if (!cert) { > + return false; > + } > + > + PLArenaPool *arena = nullptr; PLArenaPool* @@ +209,5 @@ > + > + return hasValidPins; > +} > + > +bool nsPublicKeyPinningService::chainHasValidPins(const CERTCertList *certList, nit: return type on separate line @@ +210,5 @@ > + return hasValidPins; > +} > + > +bool nsPublicKeyPinningService::chainHasValidPins(const CERTCertList *certList, > + const char *hostname) { nit: opening brace on separate line @@ +215,5 @@ > + if (!certList) { > + return false; > + } > + if (!hostname || hostname[0] == 0) { > + return checkChainAgainstAllNames(certList); What's the rationale behind this? It seems like this is trying to solve a problem that might be better solved in a different way. @@ +222,5 @@ > + const nsCString localHost(hostname); > + nsTransportSecurityPreload *foundEntry = nullptr; > + char *evalHost = (char *)hostname; > + char *evalPart; > + while(nullptr == foundEntry && (evalPart = strchr(evalHost, '.')) ){ while (!foundEntry && ... @@ +226,5 @@ > + while(nullptr == foundEntry && (evalPart = strchr(evalHost, '.')) ){ > + PR_LOG(gPublicKeyPinningLog, PR_LOG_DEBUG, > + ("pkpin: Iteration Quering for pinning for host: '%s'\n", > + evalHost)); > + foundEntry = (nsTransportSecurityPreload *)bsearch(evalHost, nsTransportSecurityPreload isn't a descriptive name - maybe KeyPinEntryPreloaded? @@ +231,5 @@ > + kPublicKeyPinningPreloadList, > + kPublicKeyPinningPreloadListLength, > + sizeof(nsTransportSecurityPreload), > + TransportSecurityPreloadCompare); > + if (nullptr != foundEntry) { if (foundEntry) @@ +236,5 @@ > + PR_LOG(gPublicKeyPinningLog, PR_LOG_DEBUG, > + ("pkpin: iteration found pinning for host: '%s'\n", evalHost)); > + if (evalHost != hostname) { > + if (false == foundEntry->mIncludeSubdomains){ > + // failure well, but this isn't failure - it's just that the pin doesn't apply to this host @@ +246,5 @@ > + //and pass the dot! > + evalHost++; > + } > + > + if ((nullptr != foundEntry) && (nullptr != foundEntry->pinset)) { if (foundEntry && foundEntry->pinset) (would an entry's pinset ever be null? That seems like a fatal error.) @@ +252,5 @@ > + } > + > + return true; //no need for pin eval. > +} > + nit: trailing blank line ::: security/manager/boot/src/nsPublicKeyPinningService.h @@ +1,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > + nit: unnecessary blank line @@ +7,5 @@ > +#define __nsPublicKeyPinningService_h__ > + > +#include "cert.h" > + > +class nsPublicKeyPinningService nit: just PublicKeyPinningService - nothing new needs to be prefixed with "ns" anymore also, maybe put it in the namespace mozilla::psm @@ +13,5 @@ > +public: > + nsPublicKeyPinningService(); > + virtual ~nsPublicKeyPinningService(); > + > + static bool chainHasValidPins(const CERTCertList *certList, nit: capitalize the first letter of the name of the function Shouldn't this not be static? Otherwise, why have a constructor? In fact, why have this be a class at all? @@ +18,5 @@ > + const char *hostname); > +}; > + > +#endif // __nsPublicKeyPinningServiceService_h__ > + nit: trailing blank line ::: security/manager/boot/src/static_pins.h @@ +115,5 @@ > +static const char kVeriSign_Universal_Root_Certification_AuthorityFingerprint[]= > + "lnsM2T/O9/J84sJFdnrpsFp3awZJ+ZZbYpCWhGloaHI="; > + > +/*Now the pinsets, each is an ordered list by the actual value of the FP*/ > +class nsStaticPinset{ nit: nothing new needs to be named "ns" anymore also, space before {

Comment on attachment 8401615 [details] [diff] [review] pinning-certverify-interface (v2) Review of attachment 8401615 [details] [diff] [review]: ----------------------------------------------------------------- Overall, this looks good. I noticed a number of style/formatting nits. While I think having a TrustDomain callback for checking additional properties on potential chains could be useful, it's unfortunate that that means allocating and populating a separate CERTCertList each time. I think we could optimize this, but that can be a follow-up. ::: security/apps/AppTrustDomain.h @@ +34,5 @@ > const CERTCertificate* cert, > /*const*/ CERTCertificate* issuerCertToDup, > PRTime time, > /*optional*/ const SECItem* stapledOCSPresponse); > + SECStatus IsChainValid(const CERTCertList* certChain) {return SECSuccess;}; nit: { return SECSuccess; } ::: security/certverifier/CertVerifier.cpp @@ +40,5 @@ > #endif > ocsp_download_config odc, > ocsp_strict_config osc, > + ocsp_get_config ogc, > + pinning_enforcement_level pinningEnforcementLevel) nit: pinning_enforcement_config to be consistent? @@ +98,5 @@ > > return SECSuccess; > } > + > +// We assume the NSS is not shutting down for the duration of this call I don't think this comment is necessary - it applies to everything in this file @@ +100,5 @@ > } > + > +// We assume the NSS is not shutting down for the duration of this call > + > +//typedef ScopedPtr<PK11SlotList, PK11_FreeSlotList> nit: remove this if it isn't necessary @@ +104,5 @@ > +//typedef ScopedPtr<PK11SlotList, PK11_FreeSlotList> > +// pkixScopedPK11SlotList; > + > +SECStatus > +isCertBuiltInRoot(CERTCertificate* cert, bool *result) { nit: bool* @@ +119,5 @@ > + PK11SlotListElement* le; > + for (le = slots->head; le; le = le->next) { > + char* token = PK11_GetTokenName(le->slot); > + PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, ("BuiltInRoot? subject=%s token=%s",cert->subjectName, token)); > + if (0 == strcmp("Builtin Object Token", token)) { nit: strcmp(...) == 0 @@ +131,5 @@ > +typedef struct { > + const char *inHostname; > + const CertVerifier::pinning_enforcement_level pinningEnforcementLevel; > + const SECCertificateUsage usage; > +}chainValidationCallbackState_t; nit: "} ChainValidationCallbackState" @@ +136,3 @@ > > SECStatus chainValidationCallback(void* state, const CERTCertList* certList, > PRBool* chainOK) why are we using PRBool? @@ +157,5 @@ > + PR_SetError(PR_INVALID_STATE_ERROR, 0); > + return SECFailure; > + } > + > + CERTCertificate *currentCert; nit: CERTCertificate* @@ +258,5 @@ > + } > + if (usage == certificateUsageSSLServer) { > + PRBool chainOK = PR_FALSE; > + SECStatus srv = chainValidationCallback(callbackState, certChain.get(), > + &chainOK); nit: indentation @@ +269,5 @@ > + insertErrorIntoVerifyLog(cert, > + SEC_ERROR_APPLICATION_CALLBACK_ERROR, > + verifyLog); > + } > + PR_SetError(SEC_ERROR_APPLICATION_CALLBACK_ERROR, 0); //same as nsspkix nit: "// same as libpkix" @@ +564,2 @@ > { > + chainValidationCallbackState_t callbackState = {inHostname, nit: "{ inHostname," @@ +564,4 @@ > { > + chainValidationCallbackState_t callbackState = {inHostname, > + mPinningEnforcementLevel, > + usage}; nit: "usage };" @@ +946,2 @@ > certificateUsageSSLServer, time, > + pinarg, hostname, 0, &validationChain, evOidPolicy, nit: is this longer than 80 characters? Also check that nothing else is. ::: security/certverifier/CertVerifier.h @@ +27,4 @@ > const SECCertificateUsage usage, > const PRTime time, > void* pinArg, > + const char *inHostname, nit: "const char*" @@ +31,5 @@ > const Flags flags = 0, > /*optional out*/ mozilla::pkix::ScopedCERTCertList* validationChain = nullptr, > /*optional out*/ SECOidTag* evOidPolicy = nullptr , > + /*optional out*/ CERTVerifyLog* verifyLog = nullptr, > + /*optional in*/ const SECItem* stapledOCSPResponse = nullptr); all in parameters should go before out parameters ::: security/certverifier/NSSCertDBTrustDomain.cpp @@ +382,5 @@ > + } > + PRBool ChainOK; > + rv = (mCheckChainCallback->isChainValid)(mCheckChainCallback->isChainValidArg, > + certChain, > + &ChainOK); nit: put this in the previous line @@ +392,5 @@ > + // we should only return success if the chain is valid > + if (ChainOK) { > + return SECSuccess; > + } > + PR_SetError( SEC_ERROR_APPLICATION_CALLBACK_ERROR, 0); nit: "PR_SetError(SEC_ERROR..." @@ +396,5 @@ > + PR_SetError( SEC_ERROR_APPLICATION_CALLBACK_ERROR, 0); > + return SECFailure; > +} > + > + nit: unnecessary blank line ::: security/certverifier/NSSCertDBTrustDomain.h @@ +78,5 @@ > /*const*/ CERTCertificate* issuerCert, > PRTime time, > /*optional*/ const SECItem* stapledOCSPResponse); > > + virtual SECStatus IsChainValid(const CERTCertList * certChain); nit: "const CERTCertList*" ::: security/manager/ssl/src/SharedCertVerifier.h @@ +27,5 @@ > #ifndef NSS_NO_LIBPKIX > ac, cdc, > #endif > + odc, osc, ogc, > + pinningEnforcementLevel) since everything else is abbreviated, might as well call this "pel" ::: security/manager/ssl/src/nsCMS.cpp @@ +268,3 @@ > certificateUsageEmailSigner, > + PR_Now(), nullptr /*XXX pinarg*/, > + nullptr /*hostname*/); nit: "/* hostname */" ::: security/manager/ssl/src/nsNSSCertificate.cpp @@ +832,3 @@ > certificateUsageSSLServer, PR_Now(), > nullptr, /*XXX fixme*/ > + nullptr, /*hostname*/ nit: "/* hostname */" etc. ::: security/pkix/include/pkix/pkixtypes.h @@ +100,5 @@ > PRTime time, > /*optional*/ const SECItem* stapledOCSPresponse) = 0; > > + // Called as soon as we think we have a valid chain but before revocation > + // checks are done. Called to compute chain level checks, no implementable nit: "not implementable". However, it's not that it's not implementable in mozilla::pkix, it's that the trust domain might want to do some additional checks. How about just "Called to compute additional chain level checks desired by the trust domain." @@ +101,5 @@ > /*optional*/ const SECItem* stapledOCSPresponse) = 0; > > + // Called as soon as we think we have a valid chain but before revocation > + // checks are done. Called to compute chain level checks, no implementable > + // wihtin insanity. nit: "within mozilla::pkix" @@ +102,5 @@ > > + // Called as soon as we think we have a valid chain but before revocation > + // checks are done. Called to compute chain level checks, no implementable > + // wihtin insanity. > + virtual SECStatus IsChainValid(const CERTCertList * certChain) = 0; nit: "const CERTCertList*" ::: security/pkix/lib/pkixbuild.cpp @@ +225,5 @@ > } > } > > if (trustLevel == TrustDomain::TrustAnchor) { > + //Call checkchain on the issuer this comment isn't necessary @@ +226,5 @@ > } > > if (trustLevel == TrustDomain::TrustAnchor) { > + //Call checkchain on the issuer > + ScopedCERTCertList certChain (CERT_NewCertList()); nit: "certChain(CERT_NewCertList());" @@ +232,5 @@ > + PR_SetError(SEC_ERROR_NO_MEMORY, 0); > + return MapSECStatus(SECFailure); > + } > + > + //Buildlist this comment isn't necessary @@ +242,5 @@ > + while (child) { > + rv = child->PrependNSSCertToList(certChain.get()); > + if (rv != Success) { > + return rv; > + }; nit: "}" @@ +248,5 @@ > + } > + > + SECStatus srv = trustDomain.IsChainValid(certChain.get()); > + if (srv != SECSuccess) { > + return MapSECStatus(srv); Does MapSECStatus handle when PR_GetError returns SEC_ERROR_APPLICATION_CALLBACK_ERROR?

Generator via js, ready for feedback

Comment on attachment 8405608 [details] [diff] [review] generator-via-js Review of attachment 8405608 [details] [diff] [review]: ----------------------------------------------------------------- Opps I clobbered one of your files

Comment on attachment 8401599 [details] [diff] [review] pinning-service-imp (v3.1) Review of attachment 8401599 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/boot/src/nsPublicKeyPinningService.h @@ +13,5 @@ > +public: > + nsPublicKeyPinningService(); > + virtual ~nsPublicKeyPinningService(); > + > + static bool chainHasValidPins(const CERTCertList *certList, why static: So that it can be called within certverifier, with this having no need to get the pinning service. Why a class? because soon after this land the I will start adding parts to this interface so that pins could be added by addons and/or websites. I am just preparing the state to when this is needed.

Comment on attachment 8405608 [details] [diff] [review] generator-via-js Review of attachment 8405608 [details] [diff] [review]: ----------------------------------------------------------------- This is a good approach. Watch out for style issues and things that are leftover from the HSTS generator that aren't necessary anymore. Also, is this not going to download the most recent pin list? Why is the .json file (i.e. MOZINPUT) checked in to the tree? ::: security/manager/boot/src/genHPKPStaticpins.js @@ +6,5 @@ > +// 1. [obtain firefox source code] > +// 2. [build/obtain firefox binaries] > +// 3. run `[path to]/run-mozilla.sh [path to]/xpcshell \ > +// [path to]/getHSTSPreloadlist.js \ > +// [absolute path to]/nsSTSPreloadlist.inc' Watch out for things like this that reference the HSTS preload list. @@ +9,5 @@ > +// [path to]/getHSTSPreloadlist.js \ > +// [absolute path to]/nsSTSPreloadlist.inc' > + > +// <https://developer.mozilla.org/en/XPConnect/xpcshell/HOWTO> > +// <https://bugzilla.mozilla.org/show_bug.cgi?id=546628> These comments are unnecessary now. @@ +32,5 @@ > +const FILE_HEADER = "/* This Source Code Form is subject to the terms of the Mozilla Public\n" + > +" * License, v. 2.0. If a copy of the MPL was not distributed with this\n" + > +" * file, You can obtain one at http://mozilla.org/MPL/2.0/. */\n" + > +"\n" + > +"/*****************************************************************************/\n" + This seems a bit long - trim to 80 chars? @@ +162,5 @@ > + try { > + let file = FileUtils.getFile("CurWorkD", [OUTPUT]); > + let errorFile = FileUtils.getFile("CurWorkD", [ERROR_OUTPUT]); > + let fos = FileUtils.openSafeFileOutputStream(file); > + let eos = FileUtils.openSafeFileOutputStream(errorFile); This isn't necessary anymore. @@ +199,5 @@ > + //print (pinsetdef); > + writeTo(pinsetdef, fos); > + > + for (let pinset of jsonPins["pinsets"].sort(compareByName)) { > + let printVal = "static const char* const kPinSet_" + pinset.name + "_Data[] = {\n"; Why not just write one line at a time, instead of building up a string and then writing it? @@ +218,5 @@ > + writeTo(printVal, fos); > + } > + > + // now the domainlist entries > +const domainHeader ="/*Domainlist*/\n" + This clutters up this code - declare it outside of the function.

Comment on attachment 8409037 [details] [diff] [review] pinning-service-imp (v4) Review of attachment 8409037 [details] [diff] [review]: ----------------------------------------------------------------- to get your feet wet

Comment on attachment 8409037 [details] [diff] [review] pinning-service-imp (v4) Review of attachment 8409037 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/boot/src/genHPKPStaticpins.js @@ +20,5 @@ > +Cu.import("resource:///modules/XPCOMUtils.jsm"); > + > +const OUTPUT = "static_pins.h"; > +const MOZINPUT = "mozilla_transport_security_state_static.json"; > +const PINNNG_MINIMUM_REQUIRED_MAX_AGE = 60 * 60 * 24 * 7 * 18; Please don't misspell var names. PINNING Also it looks like this is always needed in milliseconds, so just compute it like that and name it PINNING_MIN_REQUIRED_MAX_AGE_MS @@ +57,5 @@ > + let lf = Components.classes["@mozilla.org/file/directory_service;1"] > + .getService(Components.interfaces.nsIProperties) > + .get("CurWorkD", Components.interfaces.nsILocalFile); > + > + let bits = path.split("/"); This looks unnecessarily complicated: does the file not read properly if there are embedded ../..? @@ +104,5 @@ > + return true; > + } > + return false; > +} > + All functions like this need a comment, like // Returns a pair of maps [certNameToSKD, certSDKToName] between cert nicknames and digests. @@ +115,5 @@ > + let cert = enumerator.getNext().QueryInterface(Ci.nsIX509Cert); > + if (!isCertBuiltIn(cert)) { > + continue; > + } > + let name = cert.nickname.substr(21); hardcoded constants == not cool. Is there something special about 21? @@ +129,5 @@ > + mozJSON = JSON.parse(mozFile); > + return mozJSON; > +} > + > +function nameToAlias(certName) { This only works because "this" changes context in the scope of a function. I think it's much better to do let alias = certName.replace("..."); return alias; up top so it's clear to the reader there are no side effects to certName and understanding JS scoping rules is not required. @@ +135,5 @@ > + certName = certName.replace(/\s-\s/g, "_"); > + certName = certName.replace(/\s/g, "_"); > + certName = certName.replace(/\./g, "_dot_"); > + > + // ##finally remove all non-ascii chars s/##finally/Finally @@ +141,5 @@ > + > + return "k" + certName + "Fingerprint"; > +} > + > +function compareByName (a, b) { No need for this, JS string compare already knows lexicographical sort order. return a.name.localeCompare(b.name); @@ +152,5 @@ > + > +function genExpirationTime() { > + let now = new Date(); > + let nowMillis = now.getTime(); > + let expirationMillis = nowMillis + (PINNNG_MINIMUM_REQUIRED_MAX_AGE * 1000); PINNING @@ +173,5 @@ > + for (let certName of pinsetEntry.static_spki_hashes) { > + usedFingerPrints.push(certName); > + } > + } > + // remove duplicate entries If you want a set, just use a map, not an array let usedFingerPrints = {} usedFingerPrints[certname] = 1; for (certname in usedFingerPrints) { @@ +226,5 @@ > + FileUtils.closeSafeFileOutputStream(fos); > + > + } > + catch (e) { > + dump("ERROR: problem writing output to '" + OUTPUT + "': " + e + "\n"); In general it is much better to make try/catch blocks smaller. ::: security/manager/boot/src/mozilla_transport_security_state_static.json @@ +62,5 @@ > + "Verisign Class 2 Public Primary Certification Authority - G3", > + "Verisign Class 3 Public Primary Certification Authority - G3", > + "Verisign Class 4 Public Primary Certification Authority - G3", > + "VeriSign Class 3 Public Primary Certification Authority - G5", > + "VeriSign Universal Root Certification Authority", Please set your editor to highlight trailing whitespace: https://www.gnu.org/software/emacs/manual/html_node/emacs/Useless-Whitespace.html @@ +121,5 @@ > + ], > + > + "entries": [ > + > + // (*.)mozilla.(com|org), iff using SSL, must use an acceptable certificate. Delete unused code. ::: security/manager/boot/src/nsPublicKeyPinningService.cpp @@ +93,5 @@ > + ("pkpin: found pin base_64(hash(key)='%s'\n", base64Out)); > + return true; > + } > + } > +#if defined(PR_LOGGING) ??? Just increment @@ +98,5 @@ > + i++; > +#endif > + } > + > + //If I had not found it, we must fail! This is the kind of thing that needs to be in a function header comment. @@ +134,5 @@ > + return false; > + } > + > + PLArenaPool* arena = nullptr; > + arena = PORT_NewArena(1024); // what value here? What happens if we overflow getting the names? @@ +151,5 @@ > + currentName = nameList; > + do { > + if (currentName->type == certDNSName > + && currentName->name.other.data[0] != 0) { > + // no need to cleaup, as the arenal cleanup will do s/arenal/areanal @@ +161,5 @@ > + hostName[currentName->name.other.len] = 0; > + memcpy(hostName, currentName->name.other.data, > + currentName->name.other.len); > + if (!hostName[0]) { > + // cannot call chainHasValidPins on empty or null hostname It looks like below, ChainHasValidPins does something for empty hostnames. Oh wait, that calls this. This is why function comments are important. @@ +168,5 @@ > + hasValidPins |= nsPublicKeyPinningService:: > + ChainHasValidPins(certList, hostName, time); > + } > + currentName = CERT_GetNextGeneralName(currentName); > + } while (currentName != nameList && !hasValidPins); Weird -- so this is a circular list? @@ +189,5 @@ > + } > + if (time > kPreloadPKPinsExpirationTime) { > + return true; > + } > + if (!hostname || hostname[0] == 0) { Are you sure you want this? I think it would make more sense to force callers to be reasonable and not pass in null hostnames, and just return false. @@ +197,5 @@ > + const nsCString localHost(hostname); > + nsTransportSecurityPreload *foundEntry = nullptr; > + char *evalHost = (char *)hostname; > + char *evalPart; > + while (!foundEntry && (evalPart = strchr(evalHost, '.'))) { boo on strchr -- is this threadsafe? @@ +201,5 @@ > + while (!foundEntry && (evalPart = strchr(evalHost, '.'))) { > + PR_LOG(gPublicKeyPinningLog, PR_LOG_DEBUG, > + ("pkpin: Iteration Quering for pinning for host: '%s'\n", > + evalHost)); > + foundEntry = (nsTransportSecurityPreload *)bsearch(evalHost, static_cast<nsITransportSecurityPreload*>(bsearch... is (marginally) safer ::: security/manager/boot/src/nsPublicKeyPinningService.h @@ +8,5 @@ > + > +class nsPublicKeyPinningService > +{ > +public: > + static bool ChainHasValidPins(const CERTCertList *certList, All function headers should have comments, especially those that are for public use. ::: security/manager/boot/src/static_pins.h @@ +3,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +/*****************************************************************************/ > +/* This is an automatically generated file. If you're not */ > +/* nsPublicKeyPinningSerice.cpp, you shouldn't be #including it. */ You could enforce this with a compiler by making nsStaticPinSet private to nsPublicKeyPinningService. But now there are a lot of static globals in nsPublicKeyPinningService.cpp, and it would be a pain.

Comment on attachment 8409037 [details] [diff] [review] pinning-service-imp (v4) Review of attachment 8409037 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the feedback, I am confused by the compare issue in the js file. ::: security/manager/boot/src/genHPKPStaticpins.js @@ +57,5 @@ > + let lf = Components.classes["@mozilla.org/file/directory_service;1"] > + .getService(Components.interfaces.nsIProperties) > + .get("CurWorkD", Components.interfaces.nsILocalFile); > + > + let bits = path.split("/"); I copied this part from the xpchsell utilities (my guess is portability issues) and we want this to run on non-unix machines. @@ +115,5 @@ > + let cert = enumerator.getNext().QueryInterface(Ci.nsIX509Cert); > + if (!isCertBuiltIn(cert)) { > + continue; > + } > + let name = cert.nickname.substr(21); Agreed with change. What is special is that is the lenght of "Builtin Object Token:" which is a prefix that all built-in nicknames have and after it it comes the actual name of the cert* (as identified by out certlist). @@ +141,5 @@ > + > + return "k" + certName + "Fingerprint"; > +} > + > +function compareByName (a, b) { dont follow this to solve line 209. explain? @@ +173,5 @@ > + for (let certName of pinsetEntry.static_spki_hashes) { > + usedFingerPrints.push(certName); > + } > + } > + // remove duplicate entries because I want them sorted so that a human can read the output? (suggested solutions are welcomed) ::: security/manager/boot/src/nsPublicKeyPinningService.cpp @@ +134,5 @@ > + return false; > + } > + > + PLArenaPool* arena = nullptr; > + arena = PORT_NewArena(1024); // what value here? that is the initial arena size, it gets resized on need (but is costly) @@ +189,5 @@ > + } > + if (time > kPreloadPKPinsExpirationTime) { > + return true; > + } > + if (!hostname || hostname[0] == 0) { The problem is that we dont always have the hostname when we are trying to do a verification and in those cases the 'safe' thing to to is to evaluate for all the names. @@ +197,5 @@ > + const nsCString localHost(hostname); > + nsTransportSecurityPreload *foundEntry = nullptr; > + char *evalHost = (char *)hostname; > + char *evalPart; > + while (!foundEntry && (evalPart = strchr(evalHost, '.'))) { yes it is threadsafe (nothing but a loop on local data, you are probably thinking strtok).

> I copied this part from the xpchsell utilities (my guess is portability > issues) and we want this to run on non-unix machines. Unless you have checked that leaving in .. breaks file reads, please delete this code. Otherwise, every time someone adds something to the codebase, it just gets copied around endlessly even if it's not required. > @@ +115,5 @@ > > + let cert = enumerator.getNext().QueryInterface(Ci.nsIX509Cert); > > + if (!isCertBuiltIn(cert)) { > > + continue; > > + } > > + let name = cert.nickname.substr(21); > > Agreed with change. What is special is that is the lenght of "Builtin Object > Token:" which is a prefix that all built-in nicknames have and after it it > comes the actual name of the cert* (as identified by out certlist). Sorry, I don't understand. Are you going to move this to const CERT_NAME_LENGTH = 20? > @@ +141,5 @@ > > + > > + return "k" + certName + "Fingerprint"; > > +} > > + > > +function compareByName (a, b) { > > dont follow this to solve line 209. explain? That is a one-line replacement for your function. > @@ +173,5 @@ > > + for (let certName of pinsetEntry.static_spki_hashes) { > > + usedFingerPrints.push(certName); > > + } > > + } > > + // remove duplicate entries > > because I want them sorted so that a human can read the output? (suggested > solutions are welcomed) Object.keys(usedFingerprints).sort() > @@ +189,5 @@ > > + } > > + if (time > kPreloadPKPinsExpirationTime) { > > + return true; > > + } > > + if (!hostname || hostname[0] == 0) { > > The problem is that we dont always have the hostname when we are trying to > do a verification and in those cases the 'safe' thing to to is to evaluate > for all the names. Please put this in the documentation.

Comment on attachment 8409037 [details] [diff] [review] pinning-service-imp (v4) Review of attachment 8409037 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/boot/src/static_pins.h @@ +217,5 @@ > + { "addons.mozilla.org", true, &kPinSet_mozilla_cdn }, > + { "cdn.mozilla.net", true, &kPinSet_mozilla_cdn }, > + { "cdn.mozilla.org", true, &kPinSet_mozilla_cdn }, > + { "getpersonas.org", true, &kPinSet_mozilla_cdn }, > + { "media.mozilla.com", true, &kPinSet_mozilla_cdn }, For testing, we need to grab DERs from https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/tlsserver/generate_certs.sh and compute pinsets {"pinning.example.com", include_subdomains = true, &kPinSet_mozilla_test}, {"nosubdomains.example.com", include_subdomains = false, &kPinSet_mozilla_test} Then we can inject certificates for good.pinning.example.com and bad.pinning.example.com and also test with and without subdomains.

Comment on attachment 8413956 [details] [diff] [review] pinning-service-imp (v5) Review of attachment 8413956 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/boot/src/PublicKeyPinningService.cpp @@ +8,5 @@ > + > +#include "nsString.h" > +#include "cert.h" > +#include "nssb64.h" > +#include "plstr.h" Do you need this? @@ +11,5 @@ > +#include "nssb64.h" > +#include "plstr.h" > +#include "portreg.h" > +#include "prlog.h" > +#include "prprf.h" You aren't using this header. @@ +54,5 @@ > + return SECSuccess; > +} > + > +/** > + * Given a Pinset and certlist ensure that there is interectsion between them intersection @@ +91,5 @@ > + PR_LOG(gPublicKeyPinningLog, PR_LOG_DEBUG, > + ("pkpin: base_64(hash(key)='%s'\n", base64Out)); > + // Compare, linear search for now... > + int j; > + for (j = 0; j < pinSet->size; j++){ for (int j; @@ +126,5 @@ > +CheckAgainstOneName(const CERTCertList *certList, const char *hostname, > + const PRTime time); > + > +/** > + * Extract all the DNS names for a host (incliding CN) and evaluate the including @@ +171,5 @@ > + hostName[currentName->name.other.len] = 0; > + memcpy(hostName, currentName->name.other.data, > + currentName->name.other.len); > + if (!hostName[0]) { > + // cannot call chainHasValidPins on empty or null hostname update comment @@ +193,5 @@ > + * Check PKPins on the given certlist against the specified hostname > + */ > + > +static bool > +CheckAgainstOneName(const CERTCertList *certList, CheckPinsForHostname @@ +209,5 @@ > + char *evalHost = (char *)hostname; > + char *evalPart; > + while (!foundEntry && (evalPart = strchr(evalHost, '.'))) { > + PR_LOG(gPublicKeyPinningLog, PR_LOG_DEBUG, > + ("pkpin: Iteration Quering for pinning for host: '%s'\n", Querying ::: security/manager/boot/src/PublicKeyPinningService.h @@ +14,5 @@ > +public: > + /** > + * Check Public Pins of the given chain against the pins specified > + * by the hostname. If the hostname is null or empty evaluate > + * against all the possible names for the host (including CN). Can you specify how this list of hostnames is constructed? Also, needs documentation of all overrides (like the pref, or the fact that installing a root CA invalidates this check) ::: security/manager/boot/src/mozilla_transport_security_state_static.json @@ +126,5 @@ > + > + //{ "name": "mozilla.com", "include_subdomains": true, "pins": "mozilla" }, > + //{ "name": "mozilla.org", "include_subdomains": true, "pins": "mozilla" }, > + //{ "name": "mozilla.net", "include_subdomains": true, "pins": "mozilla" }, > + //{ "name": "marketplace.firefox.com", "include_subdomains": true, "mode": "force-https", "pins": "mozilla" }, Remove unused code.

Comment on attachment 8413956 [details] [diff] [review] pinning-service-imp (v5) Review of attachment 8413956 [details] [diff] [review]: ----------------------------------------------------------------- Looking good. I'd like a final look, so r- for now. Watch out for formatting issues. ::: security/manager/boot/src/PublicKeyPinningService.cpp @@ +2,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "PublicKeyPinningService.h" > +// The static pin file is autogenerated nit: "autogenerated by genHPKPStaticpins.js" @@ +20,5 @@ > +using namespace mozilla::psm; > + > +#if defined(PR_LOGGING) > +PRLogModuleInfo *gPublicKeyPinningLog = > + PR_NewLogModule("PublicKeyPinningservice"); nit: PublicKeyPinningService @@ +26,5 @@ > + > +/* > + Computes in the location specified by base64Out the SHA256 digest > + of the DER Encoded subject Public Key Info for the given cert > + using. Word-smith this sentence. @@ +27,5 @@ > +/* > + Computes in the location specified by base64Out the SHA256 digest > + of the DER Encoded subject Public Key Info for the given cert > + using. > + Assumes base64Out is allocated to at least HASH_LENGTH_MAX*2 bytes Make this take a reference to a char array of size HASH_LENGTH_MAX*2. @@ +30,5 @@ > + using. > + Assumes base64Out is allocated to at least HASH_LENGTH_MAX*2 bytes > +*/ > +static SECStatus > +GetBase64SHA256SPKI(const CERTCertificate *cert, const int pkhashinfoindex, Looks like pkhashinfoindex isn't being used yet, so let's remove it for now. @@ +38,5 @@ > + const SECItem *derPublicKey = &(cert->derPublicKey); > + > + memset(temp_dest, 0x00, HASH_LENGTH_MAX); > + srv = HASH_HashBuf(HASH_AlgSHA256, temp_dest, > + (unsigned char *)derPublicKey->data, derPublicKey->len); nit: indentation @@ +54,5 @@ > + return SECSuccess; > +} > + > +/** > + * Given a Pinset and certlist ensure that there is interectsion between them nit: capitalization consistency (Pinset vs certlist) @@ +56,5 @@ > + > +/** > + * Given a Pinset and certlist ensure that there is interectsion between them > + */ > + nit: no blank line here or any other similar situation (elsewhere in this file, for example) @@ +60,5 @@ > + > +static bool > +EvalPinWithPinset(const CERTCertList *certList, const nsStaticPinset *pinSet) { > +#if defined(PR_LOGGING) > + int i = 0; Is keeping track of the index that matches helpful? I think these #if defined(PR_LOGGING) sections make the code harder to read. @@ +80,5 @@ > + PR_LOG(gPublicKeyPinningLog, PR_LOG_DEBUG, > + ("pkpin: certArray[%d] common_name: '%s'\n", i, > + CERT_GetCommonName(&(currentCert->issuer)))); > + > + // I only need sha256fp for builtins (so far) nit: this comment is unnecessary @@ +92,5 @@ > + ("pkpin: base_64(hash(key)='%s'\n", base64Out)); > + // Compare, linear search for now... > + int j; > + for (j = 0; j < pinSet->size; j++){ > + if (!strcmp(pinSet->data[j], base64Out)){ nit: if (strcmp(...) == 0) @@ +102,5 @@ > +#if defined(PR_LOGGING) > + i++; > +#endif > + } > + //If I had not found it, we must fail! nit: "we", not "I" @@ +113,5 @@ > +*/ > + > +static int > +TransportSecurityPreloadCompare(const void *key, const void *entry) { > + const char *keyStr = (const char *)key; reinterpret_cast<const char*>(key) or const_cast or whatever's appropriate @@ +115,5 @@ > +static int > +TransportSecurityPreloadCompare(const void *key, const void *entry) { > + const char *keyStr = (const char *)key; > + const nsTransportSecurityPreload *preloadEntry = > + (const nsTransportSecurityPreload *)entry; reinterpret_cast<const TransportSecurityPreload*>(entry) @@ +122,5 @@ > +} > + > + > +static bool > +CheckAgainstOneName(const CERTCertList *certList, const char *hostname, Why not just move the implementation here - it's not too long. @@ +143,5 @@ > + if (!cert) { > + return false; > + } > + > + PLArenaPool* arena = nullptr; ScopedPLArenaPool @@ +144,5 @@ > + return false; > + } > + > + PLArenaPool* arena = nullptr; > + arena = PORT_NewArena(1024); // what value here? I think 1024 is common, so this is probably fine. @@ +168,5 @@ > + if (!hostName) { > + break; > + } > + hostName[currentName->name.other.len] = 0; > + memcpy(hostName, currentName->name.other.data, Why do we have to memcpy this? Can't we use currentName->name.other.data directly? @@ +174,5 @@ > + if (!hostName[0]) { > + // cannot call chainHasValidPins on empty or null hostname > + break; > + } > + hasValidPins |= CheckAgainstOneName(certList, hostName, time); The control flow involving hasValidPins is a bit more complicated than necessary, I think. How about: if (CheckAgainstOneName(...)) { break; } @@ +205,5 @@ > + } > + > + const nsCString localHost(hostname); > + nsTransportSecurityPreload *foundEntry = nullptr; > + char *evalHost = (char *)hostname; reinterpret_cast<char*>(hostname) @@ +227,5 @@ > + } > + } > + } > + evalHost = evalPart; > + //and pass the dot! nit: "// Advance past the '.'" @@ +235,5 @@ > + if (foundEntry && foundEntry->pinset) { > + return EvalPinWithPinset(certList, foundEntry->pinset); > + } > + > + return true; //no need for pin eval. nit: "// No pinning information for this hostname." @@ +240,5 @@ > +} > + > +bool > +PublicKeyPinningService::ChainHasValidPins(const CERTCertList *certList, > + const char *hostname, nit: indentation ::: security/manager/boot/src/PublicKeyPinningService.h @@ +1,3 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ nit: add a blank line here @@ +1,4 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > +#ifndef __PublicKeyPinningService_h__ nit: see the specification for #include guards in https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style @@ +12,5 @@ > +class PublicKeyPinningService > +{ > +public: > + /** > + * Check Public Pins of the given chain against the pins specified What's the expected order of the chain? EE-first or root-first? @@ +13,5 @@ > +{ > +public: > + /** > + * Check Public Pins of the given chain against the pins specified > + * by the hostname. If the hostname is null or empty evaluate In what situation would it be empty? Why is this something we need to handle specifically? @@ +14,5 @@ > +public: > + /** > + * Check Public Pins of the given chain against the pins specified > + * by the hostname. If the hostname is null or empty evaluate > + * against all the possible names for the host (including CN). Hmmm - but if it's a wildcard like *.example.com and there's an entry for foo.example.com, this won't catch it, right? @@ +23,5 @@ > +}; > + > +}} // namespace mozilla::psm > + > +#endif // __PublicKeyPinningServiceService_h__ nit: guard (see above) ::: security/manager/boot/src/genHPKPStaticpins.js @@ +1,1 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public nit: let's call this genHPKPStaticPins.js @@ +16,5 @@ > + .getService(Ci.nsIX509CertDB2); > + > +Cu.import("resource://gre/modules/Services.jsm"); > +Cu.import("resource://gre/modules/FileUtils.jsm"); > +Cu.import("resource:///modules/XPCOMUtils.jsm"); nit: let { XPCOMUtils } = Cu.import("resource:///modules/XPCOMUtils.jsm", {});, etc. @@ +18,5 @@ > +Cu.import("resource://gre/modules/Services.jsm"); > +Cu.import("resource://gre/modules/FileUtils.jsm"); > +Cu.import("resource:///modules/XPCOMUtils.jsm"); > + > +const OUTPUT = "static_pins.h"; nit: StaticHPKPins.h or something? @@ +54,5 @@ > + print("filename =" + filename) > + let path = filename; > + > + let lf = Components.classes["@mozilla.org/file/directory_service;1"] > + .getService(Components.interfaces.nsIProperties) nit: indent two spaces @@ +61,5 @@ > + let bits = path.split("/"); > + for (let i = 0; i < bits.length; i++) { > + if (bits[i]) { > + if (bits[i] == "..") > + lf = lf.parent; nit: braces around if statements @@ +66,5 @@ > + else > + lf.append(bits[i]); > + } > + } > + let file = lf; why use this alias? @@ +79,5 @@ > + if (!match) { > + data = data + line.value; > + } > + } > + // The last line of the read is a false, but content can be there this seems unfortunate - is it possible for line.value to not change if there actually isn't anything more to read, thus resulting in double-adding the last line? @@ +133,5 @@ > + return mozJSON; > +} > + > +function nameToAlias(certName) { > + //remove c tokens what's a c token? @@ +138,5 @@ > + certName = certName.replace(/\s-\s/g, "_"); > + certName = certName.replace(/\s/g, "_"); > + certName = certName.replace(/\./g, "_dot_"); > + > + // ##finally remove all non-ascii chars nit: no ## @@ +153,5 @@ > + return "const PRTime kPreloadPKPinsExpirationTime = INT64_C(" + > + expirationMicros +");\n"; > +} > + > +function writeFile(certNametoSDK, certSDKtoName, jsonPins) { nit: certNameToSDK, certSDKToName @@ +164,5 @@ > + // compute used keys > + let usedFingerPrints = {}; > + let pinset = jsonPins["pinsets"]; > + for (let pinsetEntry of pinset) { > + for (let certName of pinsetEntry.static_spki_hashes) { nit: mixing naming styles @@ +165,5 @@ > + let usedFingerPrints = {}; > + let pinset = jsonPins["pinsets"]; > + for (let pinsetEntry of pinset) { > + for (let certName of pinsetEntry.static_spki_hashes) { > + usedFingerPrints[certName] = 1; nit: = true @@ +169,5 @@ > + usedFingerPrints[certName] = 1; > + } > + } > + for (let certName of usedFingerPrints.keys().sort()) { > + let printVal = "/* "+ certName +"*/\nstatic const char " + nit: space before */ @@ +171,5 @@ > + } > + for (let certName of usedFingerPrints.keys().sort()) { > + let printVal = "/* "+ certName +"*/\nstatic const char " + > + nameToAlias(certName) + > + "[]=\n \"" + certNametoSDK[certName] + "\";\n\n"; let's output one line at a time, like this: writeTo("/* " + certName + " */\n", fos); writeTo("static const char " + nameToAlias(certName) + "[] = \n", fos); writeTo(" \"" + certNameToSDK[certName] + "\";\n", fos); writeTo("\n", fos); etc. @@ +223,5 @@ > + > +// **************************************************************************** > +// This is where the action happens: > + > +let [certNametoSKD, certSDKtoName] = loadNSSCertinfo(); nit: certNameToSKD, certSKDToName ::: security/manager/boot/src/mozilla_transport_security_state_static.json @@ +1,1 @@ > +// -*- Mode: javascript; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- nit: we don't have to follow Chrome's naming convention here - how about PreloadedHPKPins.json or something a little more descriptive. @@ +3,5 @@ > +// License, v. 2.0. If a copy of the MPL was not distributed with this > +// file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +// The top-level element is a dictionary with two keys: "pinsets" maps details > +// of certificate pinning to a name and "entries" contains the HSTS details for nit: HPKP, not HSTS @@ +27,5 @@ > +//Geotrust Primary -> www.mozilla.org > +//Geotrust Global -> *. addons.mozilla.org > + > +//from bug 77646, mozilla uses GeoTrust, Digicert and Thawte > +//geotrust ca info: http://www.geotrust.com/resources/root-certificates/index.html nit: space after // ::: security/manager/boot/src/static_pins.h @@ +139,5 @@ > + kGeoTrust_Universal_CAFingerprint, > + kGeoTrust_Primary_Certification_Authority_G3Fingerprint, > + kDigiCert_Global_Root_CAFingerprint, > + kGeoTrust_Primary_Certification_Authority_G2Fingerprint, > +}; nit: modify generator to include a blank line after every pin set. @@ +203,5 @@ > + kGeoTrust_Primary_Certification_Authority_G2Fingerprint, > +}; > +const nsStaticPinset kPinSet_mozilla_cdn = { 28, kPinSet_mozilla_cdn_Data}; > +/*Domainlist*/ > +class nsTransportSecurityPreload nit: no "ns" prefix anywhere anymore

Why doesn't this pull from Chrome's list like the current HSTS preload stuff does?

(In reply to Reed Loden [:reed] from comment #112) > Why doesn't this pull from Chrome's list like the current HSTS preload stuff > does? a). My understanding is that we've been asked to not pin Google sites yet. b). One step at a time - we'll do some integration in bug 1002696.

https://tbpl.mozilla.org/?tree=Try&rev=731670394dc6

Comment on attachment 8413956 [details] [diff] [review] pinning-service-imp (v5) Review of attachment 8413956 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/boot/src/PublicKeyPinningService.h @@ +14,5 @@ > +public: > + /** > + * Check Public Pins of the given chain against the pins specified > + * by the hostname. If the hostname is null or empty evaluate > + * against all the possible names for the host (including CN). Correct. ::: security/manager/boot/src/genHPKPStaticpins.js @@ +79,5 @@ > + if (!match) { > + data = data + line.value; > + } > + } > + // The last line of the read is a false, but content can be there Line should be empty in this case so we should be safe here. @@ +164,5 @@ > + // compute used keys > + let usedFingerPrints = {}; > + let pinset = jsonPins["pinsets"]; > + for (let pinsetEntry of pinset) { > + for (let certName of pinsetEntry.static_spki_hashes) { This comes from the JSON file. I am trying to keep google's naming convention but I am willing to change.

This looks like something we should be sure to get QA testing around once it lands.

Comment on attachment 8414543 [details] [diff] [review] pinning-service-imp (v6.1) Review of attachment 8414543 [details] [diff] [review]: ----------------------------------------------------------------- All of my concerns are basically stylistic/formatting/comments/etc., so r=me with nits addressed. To be clear, though, please make sure all pointer/reference symbols are next to their type, not the identifier (so "const Foo& foo", not "const Foo &foo"). Also watch out for lack of spaces around operators. ::: security/manager/boot/src/PreloadedHPKPins.json @@ +85,5 @@ > + ], > + > + "entries": [ > + > + // (*.)mozilla.(com|org), iff using SSL, must use an acceptable certificate. is this comment necessary? @@ +87,5 @@ > + "entries": [ > + > + // (*.)mozilla.(com|org), iff using SSL, must use an acceptable certificate. > + > + // Now mozilla cdn sites (much less strict) this comment doesn't seem necessary or accurate ::: security/manager/boot/src/PublicKeyPinningService.cpp @@ +2,5 @@ > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "PublicKeyPinningService.h" > +// autogenerated by genHPKPStaticpins.js nit: comment on the same line as the include @@ +19,5 @@ > +using namespace mozilla; > +using namespace mozilla::psm; > + > +#if defined(PR_LOGGING) > +PRLogModuleInfo *gPublicKeyPinningLog = nit: PRLogModuleInfo* @@ +24,5 @@ > + PR_NewLogModule("PublicKeyPinningService"); > +#endif > + > +/* > + Computes in the location specified by base64Out the SHA256 digest nit: update comment to match new argument names @@ +26,5 @@ > + > +/* > + Computes in the location specified by base64Out the SHA256 digest > + of the DER Encoded subject Public Key Info for the given cert > + using. nit: remove "using" @@ +29,5 @@ > + of the DER Encoded subject Public Key Info for the given cert > + using. > +*/ > +static SECStatus > +GetBase64SHA256SPKI(const CERTCertificate *cert, nit: const CERTCertificate* @@ +39,5 @@ > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return SECFailure; > + } > + rv = Base64Encode(nsDependentCSubstring( > + reinterpret_cast<const char*> (digest.get().data), nit: no space after the cast @@ +52,5 @@ > +/** > + * Given a Pinset and certlist ensure that there is intersection between them > + */ > +static bool > +EvalPinWithPinset(const CERTCertList *certList, const StaticPinset *pinSet) { nit: * next to type names here and elsewhere @@ +75,5 @@ > + // I only need sha256fp for builtins (so far) > + srv = GetBase64SHA256SPKI(currentCert, base64Out); > + if (srv != SECSuccess) { > + PR_LOG(gPublicKeyPinningLog, PR_LOG_DEBUG, > + ("pkpin: getbase64pin failed!\n")); nit: "GetBase64SHA256SPKI failed" @@ +89,5 @@ > + return true; > + } > + } > + } > + //If I had not found it, we must fail! nit: "we" not "I" here and elsewhere, and a space after "//" @@ +97,5 @@ > + > +/** > + Comparator for the is public key pinned host. > +*/ > + nit: blank line @@ +121,5 @@ > + if (!hostname || hostname[0] == 0) { > + return false; > + } > + > + const nsCString localHost(hostname); this is unused @@ +126,5 @@ > + TransportSecurityPreload *foundEntry = nullptr; > + char *evalHost = const_cast<char*>(hostname); > + char *evalPart; > + > + while (!foundEntry && (evalPart = strchr(evalHost, '.'))) { add comment about how this prevents pins for unqualified domain names (or tlds, which I guess is what we want) @@ +159,5 @@ > + > +/** > + * Extract all the DNS names for a host (including CN) and evaluate the > + * certifiate pins against all of them (Currently is an OR so we stop > + * evaluating at the first OK pin. nit: include closing ")" @@ +184,5 @@ > + CERTGeneralName* nameList; > + CERTGeneralName* currentName; > + nameList = CERT_GetConstrainedCertificateNames(cert, arena.get(), PR_TRUE); > + if (!nameList) { > + //goto done; remove this @@ +192,5 @@ > + currentName = nameList; > + do { > + if (currentName->type == certDNSName > + && currentName->name.other.data[0] != 0) { > + // no need to cleaup, as the arenal cleanup will do nit: "arena" ::: security/manager/boot/src/PublicKeyPinningService.h @@ +13,5 @@ > +class PublicKeyPinningService > +{ > +public: > + /** > + * Check Public Pins of the given chain against the pins specified Maybe "Check that there is an intersection of the keys in the given certificate chain and the pin set specified by the hostname." @@ +15,5 @@ > +public: > + /** > + * Check Public Pins of the given chain against the pins specified > + * by the hostname. If the hostname is null or empty evaluate > + * against all the possible names for the host (Common Name (CN) plus "all possible domain names for the EE cert" @@ +16,5 @@ > + /** > + * Check Public Pins of the given chain against the pins specified > + * by the hostname. If the hostname is null or empty evaluate > + * against all the possible names for the host (Common Name (CN) plus > + * all DNS Name: subject Alt Name entries). nit: include the caveat that if an alt name is a wildcard, it won't necessarily find a pinset that would otherwise be valid for it. @@ +26,5 @@ > +}; > + > +}} // namespace mozilla::psm > + > +#endif // __PublicKeyPinningServiceService_h__ nit: have this comment match the guard name ::: security/manager/boot/src/StaticHPKPins.h @@ +3,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +/*****************************************************************************/ > +/* This is an automatically generated file. If you're not */ > +/* nsPublicKeyPinningSerice.cpp, you shouldn't be #including it. */ nit: it's now PublicKeyPinningService.cpp @@ +8,5 @@ > +/*****************************************************************************/ > +#include <stdint.h> > +/* Baltimore CyberTrust Root */ > +static const char kBaltimore_CyberTrust_RootFingerprint[]= > + "Y9mvm0exBk1JoQ57f9Vm28jKo5lFm/woKcVxrYxu80o="; nit: two spaces for indentation for this whole file @@ +119,5 @@ > +static const char kthawte_Primary_Root_CA___G3Fingerprint[]= > + "GQbGEk27Q4V40A4GbVBUxsN/D6YCjAVUXgmU7drshik="; > + > +/*Now the pinsets, each is an ordered list by the actual value of the FP*/ > +class StaticPinset{ nit: "{" on a separate line @@ +120,5 @@ > + "GQbGEk27Q4V40A4GbVBUxsN/D6YCjAVUXgmU7drshik="; > + > +/*Now the pinsets, each is an ordered list by the actual value of the FP*/ > +class StaticPinset{ > + public: nit: unindent "public:" (or just have this be a struct) @@ +121,5 @@ > + > +/*Now the pinsets, each is an ordered list by the actual value of the FP*/ > +class StaticPinset{ > + public: > + const int size; nit: let's use size_t for now, and then figure out a way to template-ize this in a follow-up bug (the compiler knows how large our data is at compile-time, so this shouldn't be necessary) @@ +122,5 @@ > +/*Now the pinsets, each is an ordered list by the actual value of the FP*/ > +class StaticPinset{ > + public: > + const int size; > + const char* const* data; why not have this be an array type? @@ +177,5 @@ > + > +/*Domainlist*/ > +class TransportSecurityPreload > +{ > + public: nit: unindent "public:" or have this be a struct @@ +184,5 @@ > + const StaticPinset *pinset; > +}; > + > +static const TransportSecurityPreload kPublicKeyPinningPreloadList[] = { > + { "addons.mozilla.net", true, &kPinSet_mozilla }, nit: two spaces for indentation ::: security/manager/boot/src/genHPKPStaticPins.js @@ +15,5 @@ > +let { Services } = Cu.import("resource://gre/modules/Services.jsm", {}); > +let { XPCOMUtils } = Cu.import("resource:///modules/XPCOMUtils.jsm", {}); > + > +const certdb2 = Cc["@mozilla.org/security/x509certdb;1"] > + .getService(Ci.nsIX509CertDB2); super tiny nit: move this line one space to the left @@ +50,5 @@ > + fos.write(string, string.length); > +} > + > +function readFile(filename) { > + print("filename =" + filename) nit: trailing ";" ::: security/manager/boot/src/moz.build @@ +17,5 @@ > SOURCES += [ > 'nsSecureBrowserUIImpl.cpp', > ] > > + nit: unnecessary blank line

Keeping r+ from keeler

("leave-open" is a keyword now; it's preferred that you use that instead of the whiteboard version.)

Comment on attachment 8414798 [details] [diff] [review] pinning-certverify-interface (v3) Review of attachment 8414798 [details] [diff] [review]: ----------------------------------------------------------------- I think this is looking good. I'd like another look, though, so r- for now. ::: security/apps/AppTrustDomain.h @@ +34,5 @@ > const CERTCertificate* cert, > /*const*/ CERTCertificate* issuerCertToDup, > PRTime time, > /*optional*/ const SECItem* stapledOCSPresponse); > + SECStatus IsChainValid(const CERTCertList* certChain) {return SECSuccess;} nit: spaces after/before {/} ::: security/certverifier/CertVerifier.cpp @@ +40,5 @@ > #endif > ocsp_download_config odc, > ocsp_strict_config osc, > + ocsp_get_config ogc, > + pinning_enforcement_config pinningEnforcementLevel) abbreviate to "pel" @@ +99,5 @@ > return SECSuccess; > } > + > +SECStatus > +isCertBuiltInRoot(CERTCertificate* cert, bool* result) { nit: bool& @@ +105,5 @@ > + ScopedPtr<PK11SlotList, PK11_FreeSlotList> slots; > + slots = PK11_GetAllSlotsForCert(cert, nullptr); > + if (!slots) { > + if (PORT_GetError() == SEC_ERROR_NO_TOKEN) { > + //no list nit: space after "//" @@ +111,5 @@ > + } > + return SECFailure; > + } > + PK11SlotListElement* le; > + for (le = slots->head; le; le = le->next) { nit: "for (PK11SlotListElement* le = slots->head; ..." @@ +113,5 @@ > + } > + PK11SlotListElement* le; > + for (le = slots->head; le; le = le->next) { > + char* token = PK11_GetTokenName(le->slot); > + PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, ("BuiltInRoot? subject=%s token=%s",cert->subjectName, token)); nit: watch out for overly-long lines @@ +123,5 @@ > + return SECSuccess; > +} > + > +typedef struct { > + const char *inHostname; nit: "hostname" @@ +124,5 @@ > +} > + > +typedef struct { > + const char *inHostname; > + const CertVerifier::pinning_enforcement_config pinningEnforcementLevel; nit: maybe just "enforcementLevel" @@ +132,5 @@ > > SECStatus chainValidationCallback(void* state, const CERTCertList* certList, > PRBool* chainOK) > { > + ChainValidationCallbackState* inState = nit: maybe "callbackState" @@ +133,5 @@ > SECStatus chainValidationCallback(void* state, const CERTCertList* certList, > PRBool* chainOK) > { > + ChainValidationCallbackState* inState = > + reinterpret_cast<ChainValidationCallbackState *> (state); nit: no space after ChainValidationCallbackState or ">" @@ +152,5 @@ > + "no state! \n")); > + PR_SetError(PR_INVALID_STATE_ERROR, 0); > + return SECFailure; > + } > + return early if pinningEnforcementLevel is disabled or if the usage is SSLServer @@ +153,5 @@ > + PR_SetError(PR_INVALID_STATE_ERROR, 0); > + return SECFailure; > + } > + > + CERTCertificate* currentCert; nit: declare this where it is used @@ +161,5 @@ > + !CERT_LIST_END(node, certList); > + node = CERT_LIST_NEXT(node)) { > + currentCert = node->cert; > + if (CERT_LIST_END(CERT_LIST_NEXT(node), certList)) { > + SECStatus srv = isCertBuiltInRoot(currentCert, &isBuiltInRoot); return early if pinningEnforcementLevel is "allow MITM" @@ +170,5 @@ > + } > + } > + > + if (inState->usage != certificateUsageSSLServer || > + inState->pinningEnforcementLevel == 0) { Use the enum name for the enforcement level, not "0". Also, move this to before the built-in root check. @@ +176,5 @@ > + ("verifycert: Callback shortcut pel=%d \n", inState->pinningEnforcementLevel)); > + *chainOK = PR_TRUE; > + return SECSuccess; > + } > + if (!isBuiltInRoot && If we return early, above, we don't need this. @@ +183,5 @@ > + *chainOK = PR_TRUE; > + return SECSuccess; > + } > + > + bool hasValidPins = PublicKeyPinningService:: *chainOK = PublicKeyPinningService::ChainHasValidPins(...) @@ +205,5 @@ > { > SECStatus rv; > SECCertUsage enumUsage; > + switch (usage) { > + case certificateUsageSSLClient: nit: while we're modifying this, let's go ahead and remove the double-spaces here and on the next case line @@ +257,5 @@ > + } > + if (usage == certificateUsageSSLServer) { > + PRBool chainOK = PR_FALSE; > + SECStatus srv = chainValidationCallback(callbackState, certChain.get(), > + &chainOK); nit: indentation @@ +262,5 @@ > + if (srv != SECSuccess) { > + return srv; > + } > + if (chainOK != PR_TRUE) { > + //Set error, nit: I don't think this comment is necessary @@ +554,4 @@ > const SECCertificateUsage usage, > const PRTime time, > void* pinArg, > + const char *inHostname, nit: just "hostname" @@ +805,1 @@ > verifyLog); nit: verifyLog on the same line as validationChain ::: security/certverifier/CertVerifier.h @@ +27,4 @@ > const SECCertificateUsage usage, > const PRTime time, > void* pinArg, > + const char *inHostname, nit: "hostname" @@ +55,5 @@ > > + enum pinning_enforcement_config { > + disabled = 0, > + ignoreUserImportedCAs = 1, > + strict = 2 how about "pinningDisabled", "pinningAllowMITM", and "pinningStrict" @@ +93,5 @@ > const SECCertificateUsage usage, > const PRTime time, > void* pinArg, > const Flags flags, > + void* callbackState, We know exactly what this is, though, so let's not use void* ::: security/certverifier/NSSCertDBTrustDomain.cpp @@ +379,5 @@ > + if (!mCheckChainCallback->isChainValid) { > + PR_SetError(SEC_ERROR_INVALID_ARGS, 0); > + return SECFailure; > + } > + PRBool ChainOK; chainOK ::: security/manager/ssl/src/SharedCertVerifier.h @@ +24,5 @@ > missing_cert_download_config ac, crl_download_config cdc, > #endif > ocsp_download_config odc, ocsp_strict_config osc, > + ocsp_get_config ogc, > + pinning_enforcement_config pinningEnforcementLevel) again, "pinningEnforcementLevel" is too long of a name ::: security/manager/ssl/src/nsNSSComponent.cpp @@ +995,5 @@ > } > > + CertVerifier::pinning_enforcement_config > + pinningEnforcementLevel = CertVerifier::ignoreUserImportedCAs; > + int prefPinningEnforcementLevel = Preferences::GetInt("security.ca_pinning.enforcement_level", nit: "security.cert_pinning.enforcement_level" @@ +1002,5 @@ > + case 0 : pinningEnforcementLevel = CertVerifier::disabled; > + break; > + case 1 : pinningEnforcementLevel = CertVerifier::ignoreUserImportedCAs; > + break; > + default: pinningEnforcementLevel = CertVerifier::strict; give strict an actual value and have the default case do nothing (since the default is "allow MITM", right?) ::: security/pkix/include/pkix/pkixtypes.h @@ +101,5 @@ > /*optional*/ const SECItem* stapledOCSPresponse) = 0; > > + // Called as soon as we think we have a valid chain but before revocation > + // checks are done. Called to compute additional chain level checks, by the > + // trustdomain. nit: TrustDomain ::: security/pkix/lib/pkixbuild.cpp @@ +234,5 @@ > + rv = subject.PrependNSSCertToList(certChain.get()); > + if (rv != Success) { > + return rv; > + } > + BackCert *child = subject.childCert; nit: BackCert*

Comment on attachment 8415218 [details] [diff] [review] pinning-certverify-interface (v4) Review of attachment 8415218 [details] [diff] [review]: ----------------------------------------------------------------- This is almost ready to go, but there are still a few issues. In general, when I make a comment that (for instance) points out a style nit, I would appreciate it if you went through the patch and looked for other instances of the same issue. That way, if I miss something, it'll still get fixed (also, it means I don't have to spend time on the next review pass on it). Also, please be sure to address each comment on the next version of the patch (there were some issues from a previous review that were not addressed). If I make a comment that isn't clear, please let me know and I'll be more clear. ::: security/certverifier/CertVerifier.cpp @@ +99,5 @@ > return SECSuccess; > } > + > +SECStatus > +isCertBuiltInRoot(CERTCertificate* cert, bool &result) { nit: bool& also, function names have the first letter capitalized: IsCertBuiltInRoot @@ +122,5 @@ > + } > + return SECSuccess; > +} > + > +typedef struct _ChainValidationCallbackState { just "struct ChainValidationCallbackState" also, "{" on the next line @@ +126,5 @@ > +typedef struct _ChainValidationCallbackState { > + const char* hostname; > + const CertVerifier::pinning_enforcement_config pinningEnforcementLevel; > + const SECCertificateUsage usage; > + const PRTime time; nit: two spaces for indentation, not three @@ +127,5 @@ > + const char* hostname; > + const CertVerifier::pinning_enforcement_config pinningEnforcementLevel; > + const SECCertificateUsage usage; > + const PRTime time; > +} ChainValidationCallbackState; just "};" @@ +133,5 @@ > SECStatus chainValidationCallback(void* state, const CERTCertList* certList, > PRBool* chainOK) > { > + ChainValidationCallbackState* callbackState = > + reinterpret_cast<ChainValidationCallbackState *> (state); nit: reinterpret_cast<ChainValidationCallbackState*>(state); (this was an issue from a previous review that was no addressed) @@ +162,5 @@ > + *chainOK = PR_TRUE; > + return SECSuccess; > + } > + > + CERTCertificate* currentCert; move this to where it's used (this was an issue on a previous review that was not addressed) @@ +164,5 @@ > + } > + > + CERTCertificate* currentCert; > + CERTCertListNode* node; > + bool isBuiltInRoot = false; move this to where it's used @@ +165,5 @@ > + > + CERTCertificate* currentCert; > + CERTCertListNode* node; > + bool isBuiltInRoot = false; > + for (node = CERT_LIST_HEAD(certList); nit: "for (CERTCertListNode* node = ..." @@ +251,2 @@ > PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, ("VerifyCert: getting chain in 'classic' \n")); > + certChain = CERT_GetCertChainFromCert(cert, time, enumUsage); nit: there's no need to separate the declaration and initialization of certChain (have them on the same line if possible) @@ +265,5 @@ > + insertErrorIntoVerifyLog(cert, > + SEC_ERROR_APPLICATION_CALLBACK_ERROR, > + verifyLog); > + } > + PR_SetError(SEC_ERROR_APPLICATION_CALLBACK_ERROR, 0); //same as libpkix nit: space after "//" @@ +551,4 @@ > const SECCertificateUsage usage, > const PRTime time, > void* pinArg, > + const char *hostname, nit: const char* ::: security/certverifier/CertVerifier.h @@ +12,5 @@ > > namespace mozilla { namespace psm { > > +struct _ChainValidationCallbackState; > +typedef _ChainValidationCallbackState ChainValidationCallbackState; just "struct ChainValidationCallbackState;" @@ +30,4 @@ > const SECCertificateUsage usage, > const PRTime time, > void* pinArg, > + const char *hostname, nit: const char* ::: security/certverifier/NSSCertDBTrustDomain.cpp @@ +370,5 @@ > +NSSCertDBTrustDomain::IsChainValid(const CERTCertList* certChain) { > + SECStatus rv = SECFailure; > + > + PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, > + ("NSSCertDBTrustDomain: Top of IsChainValid mCheckCallback=%p", mCheckChainCallback)); nit: is this line >80 chars? @@ +379,5 @@ > + if (!mCheckChainCallback->isChainValid) { > + PR_SetError(SEC_ERROR_INVALID_ARGS, 0); > + return SECFailure; > + } > + PRBool ChainOK; Local variables start with a lowercase letter: chainOK (this was an issue from a previous review that was not addressed) ::: security/manager/ssl/src/nsNSSComponent.cpp @@ +998,5 @@ > + pinningEnforcementLevel = CertVerifier::pinningAllowUserCAMITM; > + int prefPinningEnforcementLevel = Preferences::GetInt("security.cert_pinning.enforcement_level", > + pinningEnforcementLevel); > + switch (prefPinningEnforcementLevel) { > + case 0 : pinningEnforcementLevel = CertVerifier::pinningDisabled; nit: no space after 0 @@ +1000,5 @@ > + pinningEnforcementLevel); > + switch (prefPinningEnforcementLevel) { > + case 0 : pinningEnforcementLevel = CertVerifier::pinningDisabled; > + break; > + case 2 : pinningEnforcementLevel = CertVerifier::pinningStrict; nit: no space after 2 @@ +1003,5 @@ > + break; > + case 2 : pinningEnforcementLevel = CertVerifier::pinningStrict; > + break; > + default: pinningEnforcementLevel = CertVerifier::pinningAllowUserCAMITM; > + } nit: let's have a blank line after this one to visually separate this from the other config values @@ +1641,5 @@ > || prefName.Equals("security.OCSP.GET.enabled") > || prefName.Equals("security.ssl.enable_ocsp_stapling") > || prefName.Equals("security.use_mozillapkix_verification") > + || prefName.Equals("security.use_libpkix_verification") > + || prefName.Equals("security.ca_pinning.enforcement_level")) { "cert_pinning" ::: security/manager/ssl/src/nsUsageArrayHelper.cpp @@ +105,5 @@ > MOZ_CRASH("unknown cert usage passed to check()"); > } > > + SECStatus rv = certVerifier->VerifyCert(mCert, aCertUsage, > + time, nullptr /*XXX:wincx*/, nullptr /*hostname*/, nit: let's fix this indentation while we're here

Comment on attachment 8415471 [details] [diff] [review] pinning-certverify-interface (v5.1) Review of attachment 8415471 [details] [diff] [review]: ----------------------------------------------------------------- Awesome - let's ship it! ::: security/certverifier/CertVerifier.cpp @@ +134,5 @@ > SECStatus chainValidationCallback(void* state, const CERTCertList* certList, > PRBool* chainOK) > { > + ChainValidationCallbackState* callbackState = > + reinterpret_cast<ChainValidationCallbackState *>(state); nit: ChainValidationCallbackState* @@ +175,5 @@ > + if (srv != SECSuccess) { > + PR_LOG(gCertVerifierLog, PR_LOG_DEBUG, ("Is BuiltInRoot failure")); > + return srv; > + } > + if (!isBuiltInRoot && We should probably add a comment explaining why we do this. (Maybe something like "If desired, the user can enable "allow user CA MITM mode", in which case key pinning is not enforced for certificates that chain to trust anchors that are not in Mozilla's root program.") @@ +177,5 @@ > + return srv; > + } > + if (!isBuiltInRoot && > + (callbackState->pinningEnforcementLevel == > + CertVerifier::pinningAllowUserCAMITM)) { nit: one more space of indentation here ::: security/manager/ssl/src/nsNSSCertificate.cpp @@ +836,2 @@ > CertVerifier::FLAG_LOCAL_ONLY, > + nullptr, /* stapledOCSPResponse*/ super tiny nit: space before */ @@ +1475,4 @@ > certificateUsageSSLServer, PR_Now(), > nullptr /* XXX pinarg */, > + nullptr /* hostname*/, > + flags, nullptr /* stapledOCSPResponse*/ , nullptr, &resultOidTag); tiny nit: space before */ ::: security/manager/ssl/src/nsNSSComponent.cpp @@ +1004,5 @@ > + break; > + case 2: > + pinningEnforcementLevel = CertVerifier::pinningStrict; > + break; > + default: super tiny nit: maybe it would be a good idea to also add "case 1:" here to be clear that there actually is a value associated with allowUserCAMITM

Keeping r+ from keeler

Looks like this is all landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/41f9684032c6 https://hg.mozilla.org/integration/mozilla-inbound/rev/affd460bc3d7

Part 2 now landed in central https://hg.mozilla.org/mozilla-central/rev/affd460bc3d7 We have pinning on! (only for addons and cdn)

Added in the 32 release notes with the title: "Allow Certificate (key) pinning for sites"