We want to allow websites to send http headers specifying public keys to pin.

HSTS extension?

Added stub interface agreed via email.

Working POC https://tbpl.mozilla.org/?tree=Try&rev=721953307ec9

Comment on attachment 8488709 [details] [diff] [review] pinning-tako-idl Review of attachment 8488709 [details] [diff] [review]: ----------------------------------------------------------------- Ok - see comments. ::: netwerk/base/public/nsISiteSecurityService.idl @@ +7,5 @@ > interface nsIURI; > interface nsIObserver; > interface nsIHttpChannel; > > [scriptable, uuid(e7da4bd1-7c38-4d73-843d-c1d6af9b3c85)] Don't forget to rev the uuid. @@ +106,5 @@ > + * @param aHostname the hosname to be queries about > + * @param aValue, on success returns the serialized value of the backend > + * storage > + */ > + [noscript] boolean getStringifiedState(in uint32_t aType, As we discussed on IRC, I have some concerns about exposing the internal state of nsSiteSecurityService in this way. However, if that's the simplest way to do it right now, then maybe it's fine (like I said, if I have concerns about the implementation, I'll mention them when I look at the implementation). Since this is just for pinning right now, why don't we specify that this will be in a format that's specific to that (e.g. why not just return an array of sha256 hashes, ala setPins). @@ +111,5 @@ > + in string aHostname, > + out ACString aValue); > + > + /** > + * Set public-key pins for a host. As you mentioned on IRC, we should note that this sets the pins for all contexts, private or not. Also, does this replace any existing preloaded pins? Or is it in addition? @@ +114,5 @@ > + /** > + * Set public-key pins for a host. > + * > + * @param aHost the hostname (punycode) that pins will apply to > + * @param aWithSubdomains whether these pins also apply to subdomains nit: since "include subdomains" is the commonly-used term, let's stick with that @@ +119,5 @@ > + * @param aMaxAge lifetime (in seconds) of this pin set > + * @param aPinCount number of keys being pinnned > + * @param aSha256Pins array of key fingerprints (SHA-256, base64) > + */ > + boolean setPins(in string aHost, in boolean aWithSubdomains, Maybe 'setKeyPins'?

Comment on attachment 8488756 [details] [diff] [review] pinning-tako-tests Review of attachment 8488756 [details] [diff] [review]: ----------------------------------------------------------------- This is based on test_sss_readstate. Do you think is worth splitting the reading into a new section?

Comment on attachment 8488709 [details] [diff] [review] pinning-tako-idl Review of attachment 8488709 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the quick review. ::: netwerk/base/public/nsISiteSecurityService.idl @@ +7,5 @@ > interface nsIURI; > interface nsIObserver; > interface nsIHttpChannel; > > [scriptable, uuid(e7da4bd1-7c38-4d73-843d-c1d6af9b3c85)] oops. thanks. @@ +106,5 @@ > + * @param aHostname the hosname to be queries about > + * @param aValue, on success returns the serialized value of the backend > + * storage > + */ > + [noscript] boolean getStringifiedState(in uint32_t aType, Seems reasonable, I will change it so that it returns an array of hashes and weather this entry includes subdomains. @@ +111,5 @@ > + in string aHostname, > + out ACString aValue); > + > + /** > + * Set public-key pins for a host. This replaces built-in pins. @@ +114,5 @@ > + /** > + * Set public-key pins for a host. > + * > + * @param aHost the hostname (punycode) that pins will apply to > + * @param aWithSubdomains whether these pins also apply to subdomains Agreed @@ +119,5 @@ > + * @param aMaxAge lifetime (in seconds) of this pin set > + * @param aPinCount number of keys being pinnned > + * @param aSha256Pins array of key fingerprints (SHA-256, base64) > + */ > + boolean setPins(in string aHost, in boolean aWithSubdomains, Agreed.

patch over idl + imp to address comment 16 (https://bugzilla.mozilla.org/show_bug.cgi?id=787133#c16)

Forgot a qrefresh.

Comment on attachment 8488871 [details] [diff] [review] pinning-tako-idl (v2) Review of attachment 8488871 [details] [diff] [review]: ----------------------------------------------------------------- ouch spelling. ::: netwerk/base/public/nsISiteSecurityService.idl @@ +117,5 @@ > + > + /** > + * Set public-key pins for a host. This call will store the data in > + * permanent storage, that is will force pins in privae and non-private > + * contexts. This pins replace already set by this mechanism or those /this/These/

Comment on attachment 8488756 [details] [diff] [review] pinning-tako-tests Review of attachment 8488756 [details] [diff] [review]: ----------------------------------------------------------------- I think this is along the right lines, but it needs some improvements. ::: security/manager/ssl/tests/unit/test_pinning_dynamic.js @@ +12,5 @@ > +let gSSService = null; > + > +let profileDir = do_get_profile(); > +const certdb = Cc["@mozilla.org/security/x509certdb;1"] > + .getService(Ci.nsIX509CertDB); nit: move this line one space to the left @@ +19,5 @@ > + let der = readFile(do_get_file("test_name_constraints/" + filename, false)); > + return certdb.constructX509(der, der.length); > +} > + > +function load_cert(cert_name, trust_string) { I still think it's a good idea to be consistent about using underscores vs. lower camel case @@ +20,5 @@ > + return certdb.constructX509(der, der.length); > +} > + > +function load_cert(cert_name, trust_string) { > + var cert_filename = cert_name + ".der"; let not var @@ +25,5 @@ > + addCertFromFile(certdb, "test_name_constraints/" + cert_filename, trust_string); > + return certFromFile(cert_filename); > +} > + > +function check_cert_err_generic(cert, expected_error, usage) { Isn't this in head_psm.js somewhere? Or do we still have multiple copies of it in various tests? (same with many of these functions) @@ +51,5 @@ > +function check_fail(x) { > + return check_cert_err(x, MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE); > +} > + > +const NON_ISSUED_FP = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="; This name isn't clear to me - is there a more descriptive name it can have? @@ +62,5 @@ > + check_ok_ca(load_cert('int-nc-perm-foo.com-ca-nc', ',,')); > + // no dirName > + check_ok(certFromFile('cn-www.foo.com-int-nc-perm-foo.com-ca-nc.der')); > + > + //fail to insert invalid pin entry nit: space after // @@ +63,5 @@ > + // no dirName > + check_ok(certFromFile('cn-www.foo.com-int-nc-perm-foo.com-ca-nc.der')); > + > + //fail to insert invalid pin entry > + gSSService.setPins("www.foo.com", true, 1000, 2, ["pico", "emacs"]); How about 'whatever.dynamic-pins.example.com' instead? Or do the certificates used by this test all have names like 'www.foo.com'? If so, let's come up with another solution. Also, it seems strange that the site security service doesn't even bother to verify that the inserted pins are valid base-64 of the same length as a sha256 hash. @@ +73,5 @@ > + NON_ISSUED_FP]); > + check_fail(certFromFile('cn-www.foo.com-int-nc-perm-foo.com-ca-nc.der')); > + > + // now a valid one again > + gSSService.setPins("www.foo.com", true, 1000, 2, ["pico", "TJDwKrk0EqCzvcWqJerEWp76HPA3RJOoKWkA/54CEB0="]) Where did this pin come from? If we ever need to change it, we'll need to update this too. Also, since it gets used later, factor it out into a const. @@ +78,5 @@ > + check_ok(certFromFile('cn-www.foo.com-int-nc-perm-foo.com-ca-nc.der')); > + > + //now check if the site is pinned > + do_check_false(gSSService.isSecureHost(Ci.nsISiteSecurityService.HEADER_HPKP, > + "nonexistent.mozilla.foo", 0)); Again, use some sort of example.com host @@ +80,5 @@ > + //now check if the site is pinned > + do_check_false(gSSService.isSecureHost(Ci.nsISiteSecurityService.HEADER_HPKP, > + "nonexistent.mozilla.foo", 0)); > + do_check_true(gSSService.isSecureHost(Ci.nsISiteSecurityService.HEADER_HPKP, > + "www.google.com", 0)); Use our test pinning domain. Also, comment on why this is expected to be pinned. @@ +98,5 @@ > + // until we create it. > + do_check_false(stateFile.exists()); > + let outputStream = FileUtils.openFileOutputStream(stateFile); > + let now = (new Date()).getTime(); > + writeLine("www.foo.com:PKP\t0\t0\t" + (now + 100000) + ",1,0,TJDwKrk0EqCzvcWqJerEWp76HPA3RJOoKWkA/54CEB0=:baz:tee:\n", outputStream); Maybe I'm misunderstanding this test, but what's the point of having this line but then calling setPins on the same host in checkStateRead before ensuring that this line is properly read? I would split this test into two tests: 1. That nsSiteSecurityService properly reads pins from a state file (and that they're subsequently enforced) 2. That nsSiteSecurityService properly writes pins to a state file when setPins is called

Comment on attachment 8488755 [details] [diff] [review] pinning-tako-imp (1.1) Review of attachment 8488755 [details] [diff] [review]: ----------------------------------------------------------------- Looks like a reasonable approach. ::: security/manager/boot/src/PublicKeyPinningService.cpp @@ +129,5 @@ > + * Given a certlist and an array of strings (interpreted as sha256keys) > + * evaluate if this will succeed (there is a pinkey intersection). > + */ > +static bool > +EvalChainWithStringArray(const CERTCertList* certList, The logic in this function seems very similar to some of the EvalChainWith... functions - can they be refactored to share code? @@ +203,5 @@ > if (!hostname || hostname[0] == 0) { > return false; > } > > + //nsMainThreadPtrHandle<nsISiteSecurityService> sssService; nit: remove commented-out code @@ +204,5 @@ > return false; > } > > + //nsMainThreadPtrHandle<nsISiteSecurityService> sssService; > + nsCOMPtr<nsISiteSecurityService> sssService = do_GetService(NS_SSSERVICE_CONTRACTID); nit: watch out for overly-long lines @@ +223,5 @@ > + bool found; > + rv = sssService->GetStringifiedState(nsISiteSecurityService::HEADER_HPKP, > + evalHost, > + storedPin, > + &found); nit: no need to have each of these on their own line @@ +228,5 @@ > + if (NS_FAILED(rv)) { > + return false; > + } > + if (found) { > + SitePKPState foundEntry(storedPin); nit: this block is 3-spaces indented @@ +229,5 @@ > + return false; > + } > + if (found) { > + SitePKPState foundEntry(storedPin); > + if (foundEntry.mState == SecurityPropertySet && !foundEntry.IsExpired()) { IsExpired should take the given time (i.e. it should be more like IsValidAtTime(time)) ::: security/manager/boot/src/nsSiteSecurityService.cpp @@ +51,5 @@ > #define SSSLOG(args) PR_LOG(GetSSSLog(), 4, args) > > //////////////////////////////////////////////////////////////////////////////// > > +SiteSTSState::SiteSTSState(nsCString& aStateString) nit: how about 'SiteHSTSState' @@ +82,2 @@ > SecurityPropertyState aHSTSState, > bool aHSTSIncludeSubdomains) nit: indentation @@ +102,5 @@ > //////////////////////////////////////////////////////////////////////////////// > +static bool > +stringIsBase64EncodingOf256bitValue(nsCString& encodedString) { > + size_t encodedLen = encodedString.Length(); > + if (encodedLen > 45) { not sure this is necessary if there's an exact length check below @@ +110,5 @@ > + nsresult rv = mozilla::Base64Decode(encodedString, binaryValue); > + if (NS_FAILED(rv)) { > + return false; > + } > + if (binaryValue.Length() < 32 ) { Check for length equality. Also, no magic numbers - use SHA256_LENGTH @@ +129,5 @@ > + , mState(SecurityPropertyUnset) > + , mIncludeSubdomains(false) > +{ > + uint32_t hstsState = 0; > + uint32_t hstsIncludeSubdomains = 0; // PR_sscanf doesn't handle bools. nit: nothing should reference hsts here @@ +135,5 @@ > + char mergedPKPins[maxMergedPKPPinSize]; > + memset(mergedPKPins, 0, maxMergedPKPPinSize); > + > + if (aStateString.Length() >= maxMergedPKPPinSize) { > + SSSLOG(("SSS: Cannot partse PKPState string, too large\n")); nit: 'parse' @@ +157,5 @@ > + const char* prev = cur; > + nsAutoCString pin; > + int stringLength = 0; > + for (; *cur!= 0 ; ++cur) { > + if ((':') == *cur) { What is the expected format this is stored in? Why bother storing it with ':' that we have to strip out later? Why not just grab chunks of the expected length? @@ +284,5 @@ > return NS_OK; > } > > nsresult > +nsSiteSecurityService::SetSTSState(uint32_t aType, nit: HSTS @@ +306,1 @@ > includeSubdomains); nit: indentation @@ +314,5 @@ > mozilla::DataStorageType storageType = isPrivate > ? mozilla::DataStorage_Private > : mozilla::DataStorage_Persistent; > + nsAutoCString storageKey(hostname); > + storageKey.AppendLiteral(":STS"); nit: HSTS Also, make this a constant, so we don't have to repeat it everywhere @@ +564,5 @@ > return nullptr; > } > > NS_IMETHODIMP > nsSiteSecurityService::IsSecureHost(uint32_t aType, const char* aHost, Why not rename this 'HostAssertsSecurityProperty' or something @@ +585,5 @@ > + ScopedCERTCertList certList(CERT_NewCertList()); > + if (!certList) { > + return NS_ERROR_FAILURE; > + } > + *aResult = !PublicKeyPinningService::ChainHasValidPins(certList, Hmmm - the way PublicKeyPinninService works, we don't really have a way to distinguish between 'an error occurred' and 'ChainHasValidPins returned false because this host does have pinning data' @@ +736,5 @@ > > +NS_IMETHODIMP > +nsSiteSecurityService::GetStringifiedState(uint32_t aType, > + const char* aHostname, > + nsACString &aValue, nit: /*out*/ nsACString& aValue @@ +737,5 @@ > +NS_IMETHODIMP > +nsSiteSecurityService::GetStringifiedState(uint32_t aType, > + const char* aHostname, > + nsACString &aValue, > + bool *_found) { nit: /*out*/ bool* found @@ +739,5 @@ > + const char* aHostname, > + nsACString &aValue, > + bool *_found) { > + NS_ENSURE_ARG_POINTER(_found); > + NS_ENSURE_ARG_POINTER(aHostname); nit: trailing space @@ +744,5 @@ > + *_found = false; > + SSSLOG(("Top of GetStringifiedState")); > + nsAutoCString storageKey(aHostname); > + switch (aType) { > + case nsISiteSecurityService::HEADER_HSTS: Let's not even implement this for HSTS. @@ +770,5 @@ > + return NS_OK; > +} > + > +NS_IMETHODIMP > +nsSiteSecurityService::SetPins(const char* aHost, bool aWithSubdomains, nit: aIncludeSubdomains @@ +772,5 @@ > + > +NS_IMETHODIMP > +nsSiteSecurityService::SetPins(const char* aHost, bool aWithSubdomains, > + uint32_t aMaxAge, uint32_t aPinCount, > + const char** aSha256Pins, bool *aResult) nit: bool* aResult @@ +780,5 @@ > + NS_ENSURE_ARG_POINTER(aSha256Pins); > + > + SSSLOG(("Top of SetPins")); > + > + // Expire time is millis from now. Since STS max-age is in seconds, and nit: but we're not talking about STS here, right? @@ +788,5 @@ > + nsTArray<nsCString> sha256keys; > + for (unsigned int i = 0; i < aPinCount; i++) { > + nsAutoCString pin(aSha256Pins[i]); > + SSSLOG(("SetPins pin=%s\n", pin.get())); > + sha256keys.AppendElement(pin); nit: validate the pins? @@ +795,5 @@ > + sha256keys); > + > + nsAutoCString storageKey(aHost); > + storageKey.AppendLiteral(":PKP"); > + // Note: setPins always stores data in the persisten storage This should be in the idl ::: security/manager/boot/src/nsSiteSecurityService.h @@ +50,5 @@ > + nsTArray<nsCString> mSHA256keys; > + > + bool IsExpired() > + { > + // If mHSTSExpireTime is 0, this entry never expires (this is the case for nit: update comment - do we even need knockout entries right now? ::: security/manager/ssl/src/nsNSSComponent.cpp @@ +1039,5 @@ > #endif > > mozilla::pkix::RegisterErrorTable(); > > + // Init the siteSecurityService nit: how about "Initialize the site security service" @@ +1042,5 @@ > > + // Init the siteSecurityService > + nsCOMPtr<nsISiteSecurityService> sssService = do_GetService(NS_SSSERVICE_CONTRACTID); > + if (!sssService) { > + PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("Cannot initialize siteSecurityService\n")); nit: either 'nsISiteSecurityService', 'nsSiteSecurityService', or 'site security service' ::: security/manager/ssl/tests/unit/test_sss_readstate_garbage.js @@ +38,5 @@ > writeLine("I'm a lumberjack and I'm okay; I work all night and I sleep all day!\n", outputStream); > writeLine("This is a totally bogus entry\t\n", outputStream); > writeLine("0\t0\t0\t0\t\n", outputStream); > writeLine("\t\t\t\t\t\t\t\n", outputStream); > writeLine("example.com\t\t\t\t\t\t\t\n", outputStream); should this hostname have :HSTS at the end?

Comment on attachment 8488871 [details] [diff] [review] pinning-tako-idl (v2) Review of attachment 8488871 [details] [diff] [review]: ----------------------------------------------------------------- This looks reasonable. I think the documentation could be improved (see comments), so I'd like another look before this gets checked in. ::: netwerk/base/public/nsISiteSecurityService.idl @@ +102,5 @@ > void clearAll(); > + > + /** > + * Returns a filled nsTArray<nsCString> of sha256 pins for the requested > + * domain on success. aInclude subdomains is also filled if there is any How about: "Returns an array of sha256-hashed key pins for the given domain. If these pins also apply to subdomains of the given domain, aIncludeSubdomains will be true." @@ +106,5 @@ > + * domain on success. aInclude subdomains is also filled if there is any > + * valid pin data. > + * > + * @param aHostname the hosname (punycode) to be queried about > + * @param aPinArray the set of sha256 for the queried domain nit: maybe 'the set of sha256-hashed key pins for the given domain' @@ +108,5 @@ > + * > + * @param aHostname the hosname (punycode) to be queried about > + * @param aPinArray the set of sha256 for the queried domain > + * @param aIncludeSubdomains returns true if found and the entry is for > + * subdomains too. maybe: "true if the pins apply to subdomains of the given domain" @@ +110,5 @@ > + * @param aPinArray the set of sha256 for the queried domain > + * @param aIncludeSubdomains returns true if found and the entry is for > + * subdomains too. > + */ > + [noscript] boolean getDomainPKPData(in string aHostname, Maybe 'getKeyPinsForHostname'? @@ +115,5 @@ > + out nsCStringTArrayRef aPinArray, > + out boolean aIncludeSubdomains); > + > + /** > + * Set public-key pins for a host. This call will store the data in how about "The resulting pins will be permanent and visible from private and non-private contexts" @@ +117,5 @@ > + > + /** > + * Set public-key pins for a host. This call will store the data in > + * permanent storage, that is will force pins in privae and non-private > + * contexts. This pins replace already set by this mechanism or those "replace any already" @@ +118,5 @@ > + /** > + * Set public-key pins for a host. This call will store the data in > + * permanent storage, that is will force pins in privae and non-private > + * contexts. This pins replace already set by this mechanism or those > + * built-in into Gecko. "built-in to Gecko" @@ +124,5 @@ > + * @param aHost the hostname (punycode) that pins will apply to > + * @param aIncludeSubdomains whether these pins also apply to subdomains > + * @param aMaxAge lifetime (in seconds) of this pin set > + * @param aPinCount number of keys being pinnned > + * @param aSha256Pins array of key fingerprints (SHA-256, base64) "array of hashed key fingerprints"

Keeler I updated the idl as you suggested, However the comment you suggested for getKeyPinsForHostname seems to suggest that it will return also the build-in pins. I was thinking on: Returns an array of sha256-hashed key pins for the given domain. The pins returned are only for the non-built-in domains. If these pins also apply to subdomains of the given domain, aIncludeSubdomains will be true.

What do you think of the proposed change in documentation suggested in comment 33?

(In reply to Camilo Viecco (:cviecco) from comment #33) > Keeler I updated the idl as you suggested, However the comment you suggested > for getKeyPinsForHostname seems to suggest that it will return also the > build-in pins. I was thinking on: > > Returns an array of sha256-hashed key pins for the given domain. The pins > returned are only for > the non-built-in domains. If these pins also apply to subdomains of the > given domain, aIncludeSubdomains will be true. Sounds good.

Added mozilla::pkix::time to getKeyPinsForHostname as a parameter to ensure we only get valid values at the time specified.

Comment on attachment 8489684 [details] [diff] [review] pinning-tako-idl (v3.3) Review of attachment 8489684 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but see in particular the second comment. ::: netwerk/base/public/nsISiteSecurityService.idl @@ +109,5 @@ > */ > void clearAll(); > + > + /** > + * Returns an array of sha256-hashed key pins for the given domain. If nit: maybe add ", if any" at the end of the first sentence @@ +115,5 @@ > + * aIncludeSubdomains will be true. Pins returned are only for non-built-in > + * pin entries. > + * > + * @param aHostname the hosname (punycode) to be queried about > + * @param the time at which the pins should be valid nit: document that a mozilla::pkix::Time value is the number of seconds since 0 AD (I think the other, possibly more natural option would be to use Unix Epoch time here and convert internally. This has the added benefit of less C++ code in this idl.)

Also, don't forget to obsolete old patches so everyone knows what the most recent versions of everything are.

feature related to trusted hosted app

Comment on attachment 8490141 [details] [diff] [review] pinning-tako-imp (v2) Review of attachment 8490141 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits addressed. ::: security/manager/boot/src/PublicKeyPinningService.cpp @@ +61,5 @@ > */ > static bool > EvalCertWithHashType(const CERTCertificate* cert, SECOidTag hashType, > + const StaticFingerprints* fingerprints, > + const nsTArray<nsCString> *aKeys) nit: 'const nsTArray<nsCString>* aKeys' Also, aKeys probably isn't the best name, since again, they're not keys, they hashes of keys. Maybe dynamicFingerprints? Additionally, I was thinking more like creating a structure similar to StaticFingerprints out of the dynamic pins, but I guess this works too. @@ +105,5 @@ > */ > static bool > EvalChainWithHashType(const CERTCertList* certList, SECOidTag hashType, > + const StaticPinset* pinset, > + const nsTArray<nsCString> *aKeys) nit: 'const nsTArray<nsCString>* aKeys' @@ +131,5 @@ > currentCert->subjectName)); > PR_LOG(gPublicKeyPinningLog, PR_LOG_DEBUG, > ("pkpin: certArray common_name: '%s'\n", > CERT_GetCommonName(&(currentCert->issuer)))); > + if (EvalCertWithHashType(currentCert, hashType, fingerprints, aKeys)) { aKeys is sha-256, right? Should they be passed in if hashType == sha1? ::: security/manager/boot/src/nsSiteSecurityService.cpp @@ +102,5 @@ > //////////////////////////////////////////////////////////////////////////////// > +static bool > +stringIsBase64EncodingOf256bitValue(nsCString& encodedString) { > + size_t encodedLen = encodedString.Length(); > + if (encodedLen > 2* SHA256_LENGTH) { This check isn't necessary. @@ +128,5 @@ > + : mExpireTime(0) > + , mState(SecurityPropertyUnset) > + , mIncludeSubdomains(false) > +{ > + uint32_t pkpState = 0; Let's be consistent: if the class is 'SiteHPKPState', let's have local variables be called 'hpkpState', etc. instead of 'pkpState'. @@ +130,5 @@ > + , mIncludeSubdomains(false) > +{ > + uint32_t pkpState = 0; > + uint32_t pkpIncludeSubdomains = 0; // PR_sscanf doesn't handle bools. > + const uint32_t maxMergedHPKPPinSize = 1024; nit: MaxMergedHPKPPinSize @@ +149,5 @@ > + (SecurityPropertyState)pkpState == SecurityPropertySet || > + (SecurityPropertyState)pkpState == SecurityPropertyKnockout)); > + > + SSSLOG(("SSS: loading SiteHPKPState matches=%d\n", matches)); > + const unsigned int SHA256Base64Len = 44; nit: let's just use uint32_t here @@ +154,5 @@ > + > + if (valid && (SecurityPropertyState)pkpState == SecurityPropertySet) { > + // try to expand the merged PKPins > + const char* cur = mergedHPKPins; > + //const char* prev = cur; remove commented-out code @@ +156,5 @@ > + // try to expand the merged PKPins > + const char* cur = mergedHPKPins; > + //const char* prev = cur; > + nsAutoCString pin; > + unsigned int collectedLen = 0; uint32_t @@ +158,5 @@ > + //const char* prev = cur; > + nsAutoCString pin; > + unsigned int collectedLen = 0; > + const size_t totalLen = strnlen(mergedHPKPins, maxMergedHPKPPinSize); > + while(collectedLen + SHA256Base64Len <= totalLen) { nit: 'while (' @@ +291,5 @@ > + case nsISiteSecurityService::HEADER_HPKP: > + storageKey.AppendLiteral(":HPKP"); > + break; > + default: > + storageKey.AppendLiteral(":INVALID"); assert with a failure here - this should never happen @@ +748,5 @@ > } > > +NS_IMETHODIMP > +nsSiteSecurityService::GetKeyPinsForHostname(const char* aHostname, > + mozilla::pkix::Time &aEvalTime, nit: 'mozilla::pkix::Time& aEvalTime' @@ +751,5 @@ > +nsSiteSecurityService::GetKeyPinsForHostname(const char* aHostname, > + mozilla::pkix::Time &aEvalTime, > + /*out*/ nsTArray<nsCString>& pinArray, > + /*out*/ bool *aIncludeSubdomains, > + /*out*/ bool *afound) { nit: 'bool* aIncludeSubdomains', 'bool* aFound' @@ +753,5 @@ > + /*out*/ nsTArray<nsCString>& pinArray, > + /*out*/ bool *aIncludeSubdomains, > + /*out*/ bool *afound) { > + NS_ENSURE_ARG_POINTER(afound); > + NS_ENSURE_ARG_POINTER(aHostname); I think you want NS_ENSURE_ARG for both of these, as well as for aHostname @@ +755,5 @@ > + /*out*/ bool *afound) { > + NS_ENSURE_ARG_POINTER(afound); > + NS_ENSURE_ARG_POINTER(aHostname); > + > + SSSLOG(("Top of GetDomainPKPData")); nit: update logging comment to match the function name @@ +788,5 @@ > + /*out*/bool* aResult) > +{ > + NS_ENSURE_ARG_POINTER(aHost); > + NS_ENSURE_ARG_POINTER(aResult); > + NS_ENSURE_ARG_POINTER(aSha256Pins); again, NS_ENSURE_ARG, I think ::: security/manager/boot/src/nsSiteSecurityService.h @@ +38,5 @@ > > /** > + * SiteHPKPState: A utility class that encodes/decodes a string describing > + * the public key pins of a site. > + * PKP state consists of: nit: HPKP @@ +42,5 @@ > + * PKP state consists of: > + * - Expiry time (PRTime (aka int64_t) in milliseconds) > + * - A state flag (SecurityPropertyState, default SecurityPropertyUnset) > + * - An include subdomains flag (bool, default false) > + * - An array of sha-256 hashed fingerprints of required keys base-64 encoded, right? @@ +57,5 @@ > + SecurityPropertyState mState; > + bool mIncludeSubdomains; > + nsTArray<nsCString> mSHA256keys; > + > + bool IsExpired() This isn't necessary. @@ +70,5 @@ > + return true; > + } > + return false; > + } > + bool IsExpired(mozilla::pkix::Time aTime) { nit: add a blank line before this one, and put the { on its own line @@ +72,5 @@ > + return false; > + } > + bool IsExpired(mozilla::pkix::Time aTime) { > + // If mExpireTime is 0, this entry never expires > + if (mExpireTime == 0) { This got cargo-culted. Is it necessary? @@ +76,5 @@ > + if (mExpireTime == 0) { > + return false; > + } > + if (aTime > mozilla::pkix::TimeFromEpochInSeconds(mExpireTime / > + PR_MSEC_PER_SEC)){ nit: ')) {' @@ +82,5 @@ > + } > + return false; > + } > + > + void ToString(nsCString &aString); nit: 'nsCString& aString'

Comment on attachment 8490327 [details] [diff] [review] pinning-tako-tests (v2.2) Review of attachment 8490327 [details] [diff] [review]: ----------------------------------------------------------------- NON_ISSUED_FP is a bad name , but could not think of anything better. This is still missing testing if the state has been written but there is some coverage with the sss_tests. ::: security/manager/ssl/tests/unit/test_pinning_dynamic.js @@ +18,5 @@ > + let der = readFile(do_get_file("test_pinning_dynamic/" + filename, false)); > + return certdb.constructX509(der, der.length); > +} > + > +function load_cert(cert_name, trust_string) { missed this one to camel

https://tbpl.mozilla.org/?tree=Try&rev=596adb3c6c48

Comment on attachment 8490327 [details] [diff] [review] pinning-tako-tests (v2.2) Review of attachment 8490327 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/tests/unit/test_pinning_dynamic.js @@ +24,5 @@ > + addCertFromFile(certdb, "test_pinning_dynamic/" + cert_filename, trust_string); > + return certFromFile(cert_filename); > +} > + > +function check_ok(x) { These didn't get camel-cased either. Also, x is an unclear parameter name - use cert. @@ +28,5 @@ > +function check_ok(x) { > + return checkCertErrorGeneric(certdb, x, 0, certificateUsageSSLServer); > +} > + > +function check_ok_ca (x) { nit: "check_ok_ca(', use more descriptive parameter name @@ +37,5 @@ > + return checkCertErrorGeneric(certdb, cert, MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE, > + certificateUsageSSLServer); > +} > + > +const NON_ISSUED_FP = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="; I think I was referring more to "FP" as an unclear abbreviation of "FINGERPRINT". Maybe "KEY_HASH" would be more descriptive? @@ +40,5 @@ > + > +const NON_ISSUED_FP = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="; > +const PINNING_ROOT_FP ="kXoHD1ZGyMuowchJwy+xgHlzh0kJFoI9KX0o0IrzTps="; > + > +function checkexpiredstate() { nit: checkExpiredState Also, it would be helpful to organize these functions in the order that they're expected to run (i.e. run_test first, then checkStateRead, then checkPinningNoSubdomains, then checkExpiredState). @@ +66,5 @@ > + do_check_false(gSSService.isSecureHost(Ci.nsISiteSecurityService.HEADER_HPKP, > + "x.a.pinning2.example.com", 0)); > +} > + > + nit: unnecessary extra blank line @@ +69,5 @@ > + > + > +function checkStateRead(aSubject, aTopic, aData) { > + do_check_eq(aData, SSS_STATE_FILE_NAME); > + do_check_true(gSSService != null); Isn't there a do_check_neq? @@ +72,5 @@ > + do_check_eq(aData, SSS_STATE_FILE_NAME); > + do_check_true(gSSService != null); > + > + // DB initialization MUST be delayed until after the test file has > + // been read Maybe elaborate more: 'Initializing the certificate DB will cause NSS-initialization, which in turn initializes the site security service. Since we're in part testing that the site security service correctly reads its state file, we have to make sure it doesn't start up before we've populated the file.' @@ +79,5 @@ > + > + load_cert("pinningroot", "CTu,CTu,CTu"); > + load_cert("badca", "CTu,CTu,CTu"); > + > + // the written entry is for a.pinning2.example.com without subdomains It would be a good idea to also have an entry that does include subdomains. @@ +82,5 @@ > + > + // the written entry is for a.pinning2.example.com without subdomains > + checkPinningNoSubdomains(); > + > + //add with subdomains nit: space after // @@ +93,5 @@ > + check_fail(certFromFile('cn-www.foo.com-alt-a.pinning2.example-badca.der')); > + check_ok(certFromFile('cn-www.foo.com-alt-a.pinning2.example-pinningroot.der')); > + > + do_check_true(gSSService.isSecureHost(Ci.nsISiteSecurityService.HEADER_HPKP, > + "a.pinning2.example.com", 0)); nit: alignment @@ +95,5 @@ > + > + do_check_true(gSSService.isSecureHost(Ci.nsISiteSecurityService.HEADER_HPKP, > + "a.pinning2.example.com", 0)); > + do_check_true(gSSService.isSecureHost(Ci.nsISiteSecurityService.HEADER_HPKP, > + "x.a.pinning2.example.com", 0)); nit: alignment @@ +104,5 @@ > + checkPinningNoSubdomains(); > + > + // failure to insert new pin entry leaves previous pin behavior > + try { > + gSSService.setKeyPins("a.pinning2.example.com", true, 1000, 2, ["vi", "emacs"]); Add a line after this: do_check_true(false); // this shouldn't run @@ +109,5 @@ > + } catch(e) { > + } > + checkPinningNoSubdomains(); > + > + //now check if the a built-in site is pinned nit: space after // Also, this comment is unclear considering the very next thing that happens is a site that isn't a built-in is tested. Maybe just "ensure that built-in pins work as expected". ::: security/manager/ssl/tests/unit/test_pinning_dynamic/generate.py @@ +26,5 @@ > + > +authority_key_ident = "authorityKeyIdentifier = keyid, issuer\n" > +subject_key_ident = "subjectKeyIdentifier = hash\n" > + > + nit: extra blank line @@ +30,5 @@ > + > +def generate_family(db_dir, dst_dir, ca_key, ca_cert, base_name): > + key_type = 'rsa' > + ee_ext_base = EE_basic_constraints + authority_key_ident; > + #cn =foo.com Is this comment necessary? @@ +52,5 @@ > + ca_cert, > + '/CN=x.a.pinning2.example.com') > + > + > + #cn = foo.com, alt= foo.org Same @@ +62,5 @@ > + 'cn-www.foo.com-alt-a.pinning2.example-'+ base_name, > + ee_ext_base + alt_name_ext, > + ca_key, > + ca_cert, > + '/CN=www.foo.com') no foo.com - use example.com @@ +63,5 @@ > + ee_ext_base + alt_name_ext, > + ca_key, > + ca_cert, > + '/CN=www.foo.com') > + nit: extra blank lines @@ +80,5 @@ > + generate_family(db, srcdir, ca_key, ca_cert, 'badca') > + ca_cert = 'pinningroot.der' > + ca_key = 'pinningroot.key' > + generate_family(db, srcdir, ca_key, ca_cert, 'pinningroot') > + nit: extra blank lines ::: security/manager/ssl/tests/unit/test_pinning_dynamic/pinning_root_generate.py @@ +1,3 @@ > +#!/usr/bin/python > + > +# after runing this file you MUST modify nsIdentityinfo.cpp to change the This comment seems out of date.

Comment on attachment 8490353 [details] [diff] [review] test-writing-hpkp Review of attachment 8490353 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. r=me with nits. ::: security/manager/ssl/tests/unit/test_sss_savestate.js @@ +11,3 @@ > let gProfileDir = null; > > +const NON_ISSUED_FP = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="; Again, FP is an unclear abbreviation - use KEY_HASH or something. @@ +111,5 @@ > SSService.processHeader(Ci.nsISiteSecurityService.HEADER_HSTS, > uris[uriIndex], maxAge + includeSubdomains, 0); > } > + // Put an HPKP entry > + SSService.setKeyPins("a.pinning2.example.com", true, 1000, 1, nit: how about just 'dynamic-pin.example.com' (each xpcshell has its own profile, so this won't affect anything else) @@ +112,5 @@ > uris[uriIndex], maxAge + includeSubdomains, 0); > } > + // Put an HPKP entry > + SSService.setKeyPins("a.pinning2.example.com", true, 1000, 1, > + [NON_ISSUED_FP]); nit: alignment

Fixing documentation nits. Keeping r+ from keeler.

Comment on attachment 8489684 [details] [diff] [review] pinning-tako-idl (v3.3) Forgot to make obsolete when uploading 3.4

Comment on attachment 8490411 [details] [diff] [review] pinning-tako-tests Review of attachment 8490411 [details] [diff] [review]: ----------------------------------------------------------------- It doesn't look like pinningroot.p12 needs to be checked in, right? See comments. Otherwise, looks good. ::: security/manager/ssl/tests/unit/test_pinning_dynamic.js @@ +101,5 @@ > + checkPinningNoSubdomains(); > + > + // failure to insert new pin entry leaves previous pin behavior > + try { > + gSSService.setKeyPins("a.pinning2.example.com", true, 1000, 2, ["vi", "emacs"]); Maybe instead of "vi"/"emacs" we could have something descriptive like "not a key hash". Also, what happens when the length of the array isn't the same as the argument for the length of the array? @@ +109,5 @@ > + checkPinningNoSubdomains(); > + > + // Ensure built-in pins work as expected > + do_check_false(gSSService.isSecureHost(Ci.nsISiteSecurityService.HEADER_HPKP, > + "nonexistent.mozilla.foo", 0)); nonexistent.mozilla.com @@ +119,5 @@ > + > + do_timeout(1250, checkExpiredState); > +} > + > +function checkPinningNoSubdomains() { This function name is misleading. I'm also not sure it's needed. Just copy inline any checks that are necessary in checkStateRead. ::: security/manager/ssl/tests/unit/test_pinning_dynamic/pinningroot.csr @@ +1,1 @@ > +-----BEGIN CERTIFICATE REQUEST----- Doesn't look like this needs to be kept/checked-in.

Keeping r+ from keeler

Folded tests. Keeping r+

try push for tako (after cleanup) https://tbpl.mozilla.org/?tree=Try&rev=375e4116d4ad

try is green (tako) https://tbpl.mozilla.org/?tree=Try&rev=14d734c15c6d

Fix test issues on osx 10.6 keeping r+ from keeler

Fix testing issue on slow devices keeping r+ from keeler

Comment on attachment 8490886 [details] [diff] [review] pinning-tako-imp (v2.2) keeping r+ from keeler

inbound push https://hg.mozilla.org/integration/mozilla-inbound/rev/495dcc54c2b2 https://hg.mozilla.org/integration/mozilla-inbound/rev/605c11a57482 https://hg.mozilla.org/integration/mozilla-inbound/rev/7b3ba487c01b

Comment on attachment 8490886 [details] [diff] [review] pinning-tako-imp (v2.2) Approval Request Comment [Feature/regressing bug #]: This is required for 1016421 [User impact if declined]: Cannot do trusted apps [Describe test coverage new/current, TBPL]: Added new xpchsell tests for all functionality. [Risks and why]: Might break HSTS, as it changes some interals of its storage. However all tests pass. [String/UUID change made/needed]: UUID update for nsSiteSecurityService, two new functions where added (one of them c++ only) (not in this part)

Comment on attachment 8490886 [details] [diff] [review] pinning-tako-imp (v2.2) oops wrong patch. sorry for the bugspam

Comment on attachment 8490426 [details] [diff] [review] pinning-tako-idl (v3.4) Approval Request Comment [Feature/regressing bug #]: This is required for 1016421 [User impact if declined]: Cannot do trusted apps [Describe test coverage new/current, TBPL]: Added new xpchsell tests for all functionality. [Risks and why]: Might break HSTS, as it changes some interals of its storage. However all tests pass. [String/UUID change made/needed]: UUID update for nsSiteSecurityService, two new functions where added (one of them c++ only)

Comment on attachment 8490899 [details] [diff] [review] pinning-tako-imp-aurora Approval Request Comment [Feature/regressing bug #]: This is required for 1016421 [User impact if declined]: Cannot do trusted apps [Describe test coverage new/current, TBPL]: Added new xpchsell tests for all functionality. [Risks and why]: Might break HSTS, as it changes some interals of its storage. However all tests pass. [String/UUID change made/needed]: In another part: UUID update for nsSiteSecurityService, two new functions where added (one of them c++ only)

Comment on attachment 8490887 [details] [diff] [review] pinning-tako-tests (v3.2) Approval Request Comment [Feature/regressing bug #]: This is required for 1016421 [User impact if declined]: Cannot do trusted apps [Describe test coverage new/current, TBPL]: Added new xpchsell tests for all functionality. [Risks and why]: Might break HSTS, as it changes some interals of its storage. However all tests pass. [String/UUID change made/needed]: In another part: UUID update for nsSiteSecurityService, two new functions where added (one of them c++ only)

https://hg.mozilla.org/mozilla-central/rev/495dcc54c2b2 https://hg.mozilla.org/mozilla-central/rev/605c11a57482 https://hg.mozilla.org/mozilla-central/rev/7b3ba487c01b

Comment on attachment 8490849 [details] [diff] [review] processing-and-storing-hpkp-headers (wip) Review of attachment 8490849 [details] [diff] [review]: ----------------------------------------------------------------- The naming conventions are ugly. Could you please comment on: 1. The new safeprocessheaders call vs current processheaders. a. remove the old b. keep both (this is my current preference) 2. function signature of the call into the publicKeyPinning service from the site security service.

Things look good in m-c. Pinging for aurora uplift request.

In order to identify what parts of this bug need uplifting (and are already landed in mc) part of this bug have been moved to bug 1069559 to facilitate tracking. This bug will remain for the implementation of HPKP.

Comment on attachment 8490426 [details] [diff] [review] pinning-tako-idl (v3.4) removed approval request on this bug and moved request Bug 1069559

Comment on attachment 8490887 [details] [diff] [review] pinning-tako-tests (v3.2) removed approval request on this bug and moved request Bug 1069559

(In reply to Camilo Viecco (:cviecco) from comment #73) > In order to identify what parts of this bug need uplifting (and are already > landed in mc) part of this bug have been moved to bug 1069559 to facilitate > tracking. This bug will remain for the implementation of HPKP. Does that mean that this bug no longer blocks THA? I.e. does this no longer block bug 1016421?

Comment on attachment 8492391 [details] [diff] [review] processing-and-storing-hpkp-headers (wip2) Review of attachment 8492391 [details] [diff] [review]: ----------------------------------------------------------------- Looks about on the right path. Needs some cleanup. In particular, I think at this point we should open a new bug for further HPKP work and transfer this patch there. In terms of the code, the main thing is to factor out as much common code as possible, rather than duplicating it. Also, needs tests. ::: modules/libpref/init/all.js @@ +1731,5 @@ > // Disable pinning checks by default. > pref("security.cert_pinning.enforcement_level", 0); > +// Do not process hpkp headers rooted by not built in roots by default > +// This is to prevent accidental pinning from MITM devices > +pref("security.cert_pinning.process_headers_from_non_builtin_roots", false); I'm not sure we need a pref for this - just don't ever add pins from MITM devices - they'll almost always be wrong. ::: netwerk/base/public/nsISiteSecurityService.idl @@ +6,5 @@ > > interface nsIURI; > interface nsIObserver; > interface nsIHttpChannel; > +interface nsSSLStatus; nsISSLStatus? @@ +57,5 @@ > [optional] out unsigned long long aMaxAge, > [optional] out boolean aIncludeSubdomains); > > + > + void safeProcessHeader(in uint32_t aType, How about changing the old one to unsafeProcessHeader? @@ +58,5 @@ > [optional] out boolean aIncludeSubdomains); > > + > + void safeProcessHeader(in uint32_t aType, > + in nsIURI aSourceURI, nit: indentation on all of these @@ +60,5 @@ > + > + void safeProcessHeader(in uint32_t aType, > + in nsIURI aSourceURI, > + in string aHeader, > + in nsISupports aSSLStatus, Why not just pass in an nsISSLStatus? ::: netwerk/protocol/http/nsHttpChannel.cpp @@ +1084,5 @@ > + > + // Get the SSLStatus > + nsCOMPtr<nsISSLStatusProvider> sslprov = do_QueryInterface(mSecurityInfo); > + NS_ENSURE_TRUE(sslprov, NS_ERROR_FAILURE); > + nsCOMPtr<nsISSLStatus> sslstat; how about 'sslStatus' @@ +1095,3 @@ > const nsHttpAtom atom = nsHttp::ResolveAtom("Strict-Transport-Security"); > nsAutoCString stsHeader; > + //nsresult srv; nit: remove commented-out code @@ +1098,4 @@ > rv = mResponseHead->GetHeader(atom, stsHeader); > + if (NS_SUCCEEDED(rv)) { > + rv = sss->SafeProcessHeader(nsISiteSecurityService::HEADER_HSTS, mURI, > + stsHeader.get(), sslstat, flags, nullptr, nullptr); nit: indentation @@ +1100,5 @@ > + rv = sss->SafeProcessHeader(nsISiteSecurityService::HEADER_HSTS, mURI, > + stsHeader.get(), sslstat, flags, nullptr, nullptr); > + if (NS_FAILED(rv)) { > + AddSecurityMessage(NS_LITERAL_STRING("InvalidSTSHeaders"), > + NS_LITERAL_STRING("Invalid HSTS Headers")); nit: indentation @@ +1101,5 @@ > + stsHeader.get(), sslstat, flags, nullptr, nullptr); > + if (NS_FAILED(rv)) { > + AddSecurityMessage(NS_LITERAL_STRING("InvalidSTSHeaders"), > + NS_LITERAL_STRING("Invalid HSTS Headers")); > + LOG(("STS: Failed to parse STS header, continuing load.\n")); We probably don't need the 'STS:' prefixes in the logs anymore @@ +1112,5 @@ > LOG(("STS: No STS header, continuing load.\n")); > + } > + > + // If there's a PKP header, process it > + const nsHttpAtom pkpatom = nsHttp::ResolveAtom("Public-Key-Pins"); This code is basically duplicated - seems like we could factor it out. @@ +1117,5 @@ > + nsAutoCString pkpHeader; > + rv = mResponseHead->GetHeader(pkpatom, pkpHeader); > + if (NS_SUCCEEDED(rv)) { > + rv = sss->SafeProcessHeader(nsISiteSecurityService::HEADER_HPKP, mURI, > + pkpHeader.get(), sslstat, flags, nullptr, nullptr); nit: indentation @@ +1119,5 @@ > + if (NS_SUCCEEDED(rv)) { > + rv = sss->SafeProcessHeader(nsISiteSecurityService::HEADER_HPKP, mURI, > + pkpHeader.get(), sslstat, flags, nullptr, nullptr); > + if (NS_FAILED(rv)) { > + AddSecurityMessage(NS_LITERAL_STRING("InvalidSTSHeaders"), "InvalidPKPHeaders", etc. @@ +1120,5 @@ > + rv = sss->SafeProcessHeader(nsISiteSecurityService::HEADER_HPKP, mURI, > + pkpHeader.get(), sslstat, flags, nullptr, nullptr); > + if (NS_FAILED(rv)) { > + AddSecurityMessage(NS_LITERAL_STRING("InvalidSTSHeaders"), > + NS_LITERAL_STRING("Invalid HSTS Headers")); nit: indentation (looks like this was already wrong, but we should fix it while we're here) ::: security/manager/boot/src/Makefile.in @@ +7,5 @@ > INCLUDES += \ > -I$(DIST)/public/nss \ > $(NULL) > > +LOCAL_INCLUDES += -I$(srcdir)/../../ssl/src \ We should just export the headers we need, rather than adding the entire directory. ::: security/manager/boot/src/PublicKeyPinningService.cpp @@ +168,5 @@ > } > > +bool > +PublicKeyPinningService > + ::EvalChainWithSHA256HashArray(const CERTCertList* certList, Maybe put 'PublicKeyPinningService::EvalChainWithSHA256HashArray(' all on one line, and then have the arguments on the next line ::: security/manager/boot/src/PublicKeyPinningService.h @@ +35,5 @@ > + * Returns true if there is any intersection between the certificate list > + * and the pins specified in the aSHA256key array. Values passed in are > + * assumed to be in base64 encoded form > + */ > + static bool EvalChainWithSHA256HashArray(const CERTCertList* certList, How about 'ChainMatchesPinset' or something ::: security/manager/boot/src/moz.build @@ +29,2 @@ > '../../../pkix/include', > + '../../ssl/src', Again, exactly which headers are necessary? Can we just export them? ::: security/manager/boot/src/nsSiteSecurityService.cpp @@ +245,5 @@ > "network.stricttransportsecurity.preloadlist", true); > mozilla::Preferences::AddStrongObserver(this, > "network.stricttransportsecurity.preloadlist"); > + mProcessPKPHeadersFromNonBuiltInRoots = mozilla::Preferences::GetBool( > + "security.cert_pinning.process_headers_from_non_builtin_roots", false); Again, I don't think this is necessary. @@ +388,5 @@ > + const char* aHeader, > + nsISupports* aSSLStatus, > + uint32_t aFlags, > + uint64_t *aMaxAge, > + bool *aIncludeSubdomains) nit: bool* @@ +406,5 @@ > nsIURI* aSourceURI, > const char* aHeader, > uint32_t aFlags, > uint64_t *aMaxAge, > bool *aIncludeSubdomains) Might as well fix this while we're here: bool* @@ +419,5 @@ > + const char* aHeader, > + nsISupports* aSSLStatus, > + uint32_t aFlags, > + uint64_t *aMaxAge, > + bool *aIncludeSubdomains) nit: bool* @@ +435,5 @@ > *aIncludeSubdomains = false; > } > > + if (aSSLStatus) { > + SSSLOG(("SSS: SafeProcessHeader Top")); This appears to be 'ProcessHeaderInternal', not SafeProcessHeader @@ +436,5 @@ > } > > + if (aSSLStatus) { > + SSSLOG(("SSS: SafeProcessHeader Top")); > + nsCOMPtr<nsISSLStatus> sslstat = do_QueryInterface(aSSLStatus); Again, if this were an nsISSLStatus from the beginning, this wouldn't be necessary. @@ +438,5 @@ > + if (aSSLStatus) { > + SSSLOG(("SSS: SafeProcessHeader Top")); > + nsCOMPtr<nsISSLStatus> sslstat = do_QueryInterface(aSSLStatus); > + NS_ENSURE_TRUE(sslstat, NS_ERROR_FAILURE); > + SSSLOG(("SSS: SafeProcessHeader Post sslprov")); This logging line seems unnecessary. @@ +455,5 @@ > + rv = sslstat->GetIsUntrusted(&trustcheck); > + NS_ENSURE_SUCCESS(rv, rv); > + tlsIsBroken = tlsIsBroken || trustcheck; > + if (tlsIsBroken) { > + SSSLOG(("SSS: SafeProcessHeader Cannot process headers connections is not error-free")); How about 'SSS: discarding headers from untrustworthy connection' @@ +492,5 @@ > + char* aHeader, > + nsISupports* aSSLStatus, > + uint32_t aFlags, > + uint64_t *aMaxAge, > + bool *aIncludeSubdomains) { nit: let's fix these while we're here: uint64_t* aMaxAge, bool* aIncludeSubdomains @@ +494,5 @@ > + uint32_t aFlags, > + uint64_t *aMaxAge, > + bool *aIncludeSubdomains) { > + SSSLOG(("SSS: processing HPKP header '%s'", aHeader)); > + NS_ENSURE_ARG(aSSLStatus); hasn't this already been checked? @@ +506,5 @@ > + > + NS_NAMED_LITERAL_CSTRING(max_age_var, "max-age"); > + NS_NAMED_LITERAL_CSTRING(include_subd_var, "includesubdomains"); > + NS_NAMED_LITERAL_CSTRING(pin_sha256_var, "pin-sha256"); > + //NS_NAMED_LITERAL_CSTRING(report_uri_var, "report-uri"); nit: remove commented-out code @@ +514,5 @@ > + if (NS_FAILED(rv)) { > + SSSLOG(("SSS: could not parse HPKP header")); > + return rv; > + } > + mozilla::LinkedList<nsSecurityHeaderDirective> *directives = parser.GetDirectives(); nit: * to the left @@ +516,5 @@ > + return rv; > + } > + mozilla::LinkedList<nsSecurityHeaderDirective> *directives = parser.GetDirectives(); > + > + for (nsSecurityHeaderDirective *directive = directives->getFirst(); same @@ +531,5 @@ > + SSSLOG(("SSS: HPKP found max-age directive")); > + foundMaxAge = true; > + > + size_t len = directive->mValue.Length(); > + for (size_t i = 0; i < len; i++) { Factor this code out @@ +544,5 @@ > + SSSLOG(("SSS: HPKP could not parse delta-seconds")); > + return NS_ERROR_FAILURE; > + } > + > + SSSLOG(("SSS: HPKP parsed delta-seconds: %lld", maxAge)); A lot of this code is similar for HSTS - can it be factored out? @@ +608,5 @@ > + mozilla::pkix::Time now(mozilla::pkix::Now()); > + ScopedCERTCertList certList; > + RefPtr<SharedCertVerifier> certVerifier(GetDefaultCertVerifier()); > + NS_ENSURE_TRUE(certVerifier, NS_ERROR_UNEXPECTED); > + if (certVerifier->VerifySSLServerCert(c, nullptr,//stapled ocsp nsscert, not c nit: trailing space also, add a space before and after the '//' @@ +612,5 @@ > + if (certVerifier->VerifySSLServerCert(c, nullptr,//stapled ocsp > + now, > + nullptr, // pinarg > + host.get(), // hostname > + false, //no store ints nit: space after '//' Also, maybe "don't store intermediates" @@ +616,5 @@ > + false, //no store ints > + CertVerifier::FLAG_LOCAL_ONLY, > + &certList) != SECSuccess) { > + return NS_ERROR_FAILURE; > + }; nit: no ';' here @@ +619,5 @@ > + return NS_ERROR_FAILURE; > + }; > + > + CERTCertListNode* rootNode = CERT_LIST_TAIL(certList); > + if (!rootNode) { if (CERT_LIST_END(rootNode)) @@ +632,5 @@ > + if (!isBuiltIn && !mProcessPKPHeadersFromNonBuiltInRoots) { > + return NS_OK; > + } > + > + // if maxAge == 0 we must delete all state, for now NO puncholes maybe 'hole-punching' @@ +633,5 @@ > + return NS_OK; > + } > + > + // if maxAge == 0 we must delete all state, for now NO puncholes > + if (!maxAge) { maxAge == 0 @@ +637,5 @@ > + if (!maxAge) { > + return RemoveState(aType, aSourceURI, aFlags); > + } > + > + if (!PublicKeyPinningService::EvalChainWithSHA256HashArray(certList, sha256keys)){ nit: )) { @@ +643,5 @@ > + SSSLOG(("SSS: Pins provided by %s are invalid no match with certList\n", host.get())); > + return NS_ERROR_FAILURE; > + } > + > + // finally we need to ensure that there is a "backup pin" This needs more explanation. How is it intended that this work? Can't we just ensure that there are at least two distinct key hashes? @@ +661,5 @@ > + } > + > + // Expire time is millis from now. Since STS max-age is in seconds, and > + // PR_Now() is in micros, must equalize the units at milliseconds. > + int64_t expireTime = (PR_Now() / PR_USEC_PER_MSEC) + Again, common code can be refactored. @@ +665,5 @@ > + int64_t expireTime = (PR_Now() / PR_USEC_PER_MSEC) + > + ((int64_t)maxAge * PR_MSEC_PER_SEC); > + SiteHPKPState dynamicEntry(expireTime, SecurityPropertySet, > + foundIncludeSubdomains, sha256keys); > + SSSLOG(("SSS: about to set pons for %s, expires=%ld now=%ld maxAge=%ld\n", s/pons/pins/ @@ +689,5 @@ > +nsSiteSecurityService::ProcessSTSHeaderMutating(nsIURI* aSourceURI, > + char* aHeader, > + uint32_t aFlags, > + uint64_t *aMaxAge, > + bool *aIncludeSubdomains) nit: * to the left for aMaxAge and aIncludeSubdomains @@ +1086,3 @@ > > +nsresult > +nsSiteSecurityService::setHPKPState(const char* aHost, SiteHPKPState& entry){ nit: { on the next line @@ +1086,4 @@ > > +nsresult > +nsSiteSecurityService::setHPKPState(const char* aHost, SiteHPKPState& entry){ > + nit: unnecessary blank line ::: security/manager/boot/src/nsSiteSecurityService.h @@ +130,5 @@ > bool includeSubdomains, uint32_t flags); > + nsresult ProcessHeaderInternal(uint32_t aType, nsIURI* aSourceURI, > + const char* aHeader, nsISupports* aSSLStatus, > + uint32_t aFlags, uint64_t *aMaxAge, > + bool *aIncludeSubdomains); Please fix all of the *

Comment on attachment 8494104 [details] [diff] [review] Tests hpkp Review of attachment 8494104 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/tests/mochitest/bugs/generatePinningCerts.py @@ +34,5 @@ > +def generate_family(db_dir, dst_dir, ca_key, ca_cert, base_name): > + key_type = 'rsa' > + ee_ext_base = EE_basic_constraints + authority_key_ident; > + > + for certname, common_name in certs.iteritems(): Nit: missed this one.

Comment on attachment 8494104 [details] [diff] [review] Tests hpkp Review of attachment 8494104 [details] [diff] [review]: ----------------------------------------------------------------- Like I said in comment 79, since much of this bug has been devoted to getting trusted hosted apps working, (and indeed since those patches have already landed) I think we should move further development of HPKP to a new bug. In any case, is there a reason this test needs to be a mochitest-chrome test? Since the bulk of the test is loading certificates and making XHRs to various domains, it seems like our xpcshell/TLSServer setup would be better suited to this. ::: build/pgo/server-locations.txt @@ +223,5 @@ > https://marketplace-dev.allizom.org:443 privileged > https://marketplace.allizom.org:443 privileged > + > + > +#dynamic pinning nit: space after # @@ +229,5 @@ > +https://good.include-subdomains.pinning-dynamic.example.com:443 privileged,cert=dynamicPinningGood > +https://exclude-subdomains.pinning-dynamic.example.com:443 privileged,cert=dynamicPinningGood > +https://good.exclude-subdomains.pinning-dynamic.example.com:443 privileged,cert=dynamicPinningGood > + > +https://nocert.include-subdomains.pinning-dynamic.example.com:443 privileged,cert=expired 'nocert' seems at odds with 'cert=expired' - what's going on here? (if this is how this is expected to work, please document it) ::: security/manager/ssl/tests/mochitest/bugs/chrome.ini @@ -1,4 @@ > [DEFAULT] > support-files = > pinning_headers.sjs > - badIncludeSubdomansPinning.der This patch appears to be based on another patch not yet checked in to the tree. ::: security/manager/ssl/tests/mochitest/bugs/generatePinningCerts.py @@ +18,5 @@ > +srcdir = os.getcwd() > +db = tempfile.mkdtemp() > + > +CA_basic_constraints = "basicConstraints = critical, CA:TRUE\n" > +EE_basic_constraints = "basicConstraints = CA:FALSE\n" We could just omit this for end-entities, right? @@ +23,5 @@ > + > +CA_full_ku = ("keyUsage = keyCertSign, cRLSign\n") > + > +authority_key_ident = "authorityKeyIdentifier = keyid, issuer\n" > +subject_key_ident = "subjectKeyIdentifier = hash\n" Are these extensions necessary? @@ +56,5 @@ > + key_type, > + 'badca', > + ca_ext) > + generate_family(db, srcdir, ca_key, ca_cert, 'badca') > + #copy the new cert to pgo dir nit: space after # ::: security/manager/ssl/tests/mochitest/bugs/pinning_headers.sjs @@ +7,5 @@ > + // avoid confusing cache behaviors > + response.setHeader("Cache-Control", "no-cache", false); > + > + response.setHeader("Content-Type", "text/plain", false); > + //response.SetHeader("Connection", "close", false); nit: remove commented-out code @@ +34,5 @@ > + response.setHeader("Public-Key-Pins", "max-age=anyvalue;" + VALIDPIN + > + INVALIDPIN2 + "includeSubdomains"); > + break; > + case "none": > + //response.SetHeader("Connection", "close", false); nit: remove commented-out code @@ +46,5 @@ > + INVALIDPIN2 + "includeSubdomains"); > + } > + > + response.write("Hello world!" + request.host); > + //response.write("Hello world! '" + request.queryString + "'"); nit: remove commented-out code ::: security/manager/ssl/tests/mochitest/bugs/sec_overrides.js @@ +1,1 @@ > +// Support for making sure we can talk to the invalid cert the server can Why is this necessary? We should just temporarily trust the appropriate root certificate(s). Also, there's a better way to add overrides: call nsICertOverrideService.rememberValidityOverride directly on the expected certificate/error bits. ::: security/manager/ssl/tests/mochitest/bugs/test_pinning_dynamic.html @@ +12,5 @@ > + <script type="application/javascript"> > + > + /** Test for Bug 787133 **/ > + > + nit: unnecessary vertical space @@ +33,5 @@ > + > +const urlPath = "/chrome/security/manager/ssl/tests/mochitest/bugs/pinning_headers.sjs?"; > +const overrideHost = "nocert.include-subdomains.pinning-dynamic.example.com"; > + > + nit: unnecessary blank line @@ +34,5 @@ > +const urlPath = "/chrome/security/manager/ssl/tests/mochitest/bugs/pinning_headers.sjs?"; > +const overrideHost = "nocert.include-subdomains.pinning-dynamic.example.com"; > + > + > +var badCert ={}; nit: space after = also, let not var (set the script type to "application/javascript;version=1.7") @@ +152,5 @@ > + // with no pins bad ca is allowed to pass > + checkAllowBadCACerts("After HPKP reset"); > + > + // Fail to add an invalid entry when not pinned yet > + xhrConnect("include-subdomains.pinning-dynamic.example.com", If all this test does is make XHRs, there's no need for it to be a mochitest-chrome test. xpcshell would be a lot better suited for this.

(In reply to David Keeler (:keeler) [use needinfo?] from comment #82) > Comment on attachment 8494104 [details] [diff] [review] > Tests hpkp > > Review of attachment 8494104 [details] [diff] [review]: > ----------------------------------------------------------------- > > Like I said in comment 79, since much of this bug has been devoted to > getting trusted hosted apps working, (and indeed since those patches have > already landed) I think we should move further development of HPKP to a new > bug. In any case, is there a reason this test needs to be a mochitest-chrome > test? Since the bulk of the test is loading certificates and making XHRs to > various domains, it seems like our xpcshell/TLSServer setup would be better > suited to this. Rbarnes asserted this bug for HPKP, the trusted apps work got its own bug (bug 1069559 see comment 73). Xpcshell/TLS server is not suited for this test as I need to control both certificates and headers sent (which is not possible with xpcshell/TLSServer at this moment, filed Bug 1072084 to track this). > > ::: build/pgo/server-locations.txt > @@ +223,5 @@ > > https://marketplace-dev.allizom.org:443 privileged > > https://marketplace.allizom.org:443 privileged > > + > > + > > +#dynamic pinning > > nit: space after # > > @@ +229,5 @@ > > +https://good.include-subdomains.pinning-dynamic.example.com:443 privileged,cert=dynamicPinningGood > > +https://exclude-subdomains.pinning-dynamic.example.com:443 privileged,cert=dynamicPinningGood > > +https://good.exclude-subdomains.pinning-dynamic.example.com:443 privileged,cert=dynamicPinningGood > > + > > +https://nocert.include-subdomains.pinning-dynamic.example.com:443 privileged,cert=expired > > 'nocert' seems at odds with 'cert=expired' - what's going on here? (if this > is how this is expected to work, please document it) This was a bad patch for review. The name is incorrect as you noticed. > > ::: security/manager/ssl/tests/mochitest/bugs/chrome.ini > @@ -1,4 @@ > > [DEFAULT] > > support-files = > > pinning_headers.sjs > > - badIncludeSubdomansPinning.der > > This patch appears to be based on another patch not yet checked in to the > tree. > > ::: security/manager/ssl/tests/mochitest/bugs/generatePinningCerts.py > @@ +18,5 @@ > > +srcdir = os.getcwd() > > +db = tempfile.mkdtemp() > > + > > +CA_basic_constraints = "basicConstraints = critical, CA:TRUE\n" > > +EE_basic_constraints = "basicConstraints = CA:FALSE\n" > > We could just omit this for end-entities, right? > > @@ +23,5 @@ > > + > > +CA_full_ku = ("keyUsage = keyCertSign, cRLSign\n") > > + > > +authority_key_ident = "authorityKeyIdentifier = keyid, issuer\n" > > +subject_key_ident = "subjectKeyIdentifier = hash\n" > > Are these extensions necessary? No. Will remove > > @@ +56,5 @@ > > + key_type, > > + 'badca', > > + ca_ext) > > + generate_family(db, srcdir, ca_key, ca_cert, 'badca') > > + #copy the new cert to pgo dir > > nit: space after # > > ::: security/manager/ssl/tests/mochitest/bugs/pinning_headers.sjs > @@ +7,5 @@ > > + // avoid confusing cache behaviors > > + response.setHeader("Cache-Control", "no-cache", false); > > + > > + response.setHeader("Content-Type", "text/plain", false); > > + //response.SetHeader("Connection", "close", false); > > nit: remove commented-out code > > @@ +34,5 @@ > > + response.setHeader("Public-Key-Pins", "max-age=anyvalue;" + VALIDPIN + > > + INVALIDPIN2 + "includeSubdomains"); > > + break; > > + case "none": > > + //response.SetHeader("Connection", "close", false); > > nit: remove commented-out code > > @@ +46,5 @@ > > + INVALIDPIN2 + "includeSubdomains"); > > + } > > + > > + response.write("Hello world!" + request.host); > > + //response.write("Hello world! '" + request.queryString + "'"); > > nit: remove commented-out code > > ::: security/manager/ssl/tests/mochitest/bugs/sec_overrides.js > @@ +1,1 @@ > > +// Support for making sure we can talk to the invalid cert the server can > > Why is this necessary? We should just temporarily trust the appropriate root > certificate(s). Also, there's a better way to add overrides: call > nsICertOverrideService.rememberValidityOverride directly on the expected > certificate/error bits. Notice that this a refactor of test_certificate_overrides. Were we do not want to add the trust (we want to test the error returned). The other motivation is that by doing it this way I do not have to syncronize the test with the values of the mochitest ssl database. As long as the certs provided have the correct properties this test will succeed. > > ::: security/manager/ssl/tests/mochitest/bugs/test_pinning_dynamic.html > @@ +12,5 @@ > > + <script type="application/javascript"> > > + > > + /** Test for Bug 787133 **/ > > + > > + > > nit: unnecessary vertical space > > @@ +33,5 @@ > > + > > +const urlPath = "/chrome/security/manager/ssl/tests/mochitest/bugs/pinning_headers.sjs?"; > > +const overrideHost = "nocert.include-subdomains.pinning-dynamic.example.com"; > > + > > + > > nit: unnecessary blank line > > @@ +34,5 @@ > > +const urlPath = "/chrome/security/manager/ssl/tests/mochitest/bugs/pinning_headers.sjs?"; > > +const overrideHost = "nocert.include-subdomains.pinning-dynamic.example.com"; > > + > > + > > +var badCert ={}; > > nit: space after = > also, let not var (set the script type to > "application/javascript;version=1.7") > > @@ +152,5 @@ > > + // with no pins bad ca is allowed to pass > > + checkAllowBadCACerts("After HPKP reset"); > > + > > + // Fail to add an invalid entry when not pinned yet > > + xhrConnect("include-subdomains.pinning-dynamic.example.com", > > If all this test does is make XHRs, there's no need for it to be a > mochitest-chrome test. xpcshell would be a lot better suited for this. I need to control both the certs (xpshell can do this) and the headers (I cannot do this with xpcshell)

(In reply to David Keeler (:keeler) [use needinfo?] from comment #79) > Comment on attachment 8492391 [details] [diff] [review] > processing-and-storing-hpkp-headers (wip2) > > Review of attachment 8492391 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks about on the right path. Needs some cleanup. In particular, I think at > this point we should open a new bug for further HPKP work and transfer this > patch there. In terms of the code, the main thing is to factor out as much > common code as possible, rather than duplicating it. Also, needs tests. Thanks for the feedback. Tests are in a different patch. > > ::: modules/libpref/init/all.js > @@ +1731,5 @@ > > // Disable pinning checks by default. > > pref("security.cert_pinning.enforcement_level", 0); > > +// Do not process hpkp headers rooted by not built in roots by default > > +// This is to prevent accidental pinning from MITM devices > > +pref("security.cert_pinning.process_headers_from_non_builtin_roots", false); > > I'm not sure we need a pref for this - just don't ever add pins from MITM > devices - they'll almost always be wrong. > > ::: netwerk/base/public/nsISiteSecurityService.idl > @@ +6,5 @@ > > > > interface nsIURI; > > interface nsIObserver; > > interface nsIHttpChannel; > > +interface nsSSLStatus; > > nsISSLStatus? > > @@ +57,5 @@ > > [optional] out unsigned long long aMaxAge, > > [optional] out boolean aIncludeSubdomains); > > > > + > > + void safeProcessHeader(in uint32_t aType, > > How about changing the old one to unsafeProcessHeader? > > @@ +58,5 @@ > > [optional] out boolean aIncludeSubdomains); > > > > + > > + void safeProcessHeader(in uint32_t aType, > > + in nsIURI aSourceURI, > > nit: indentation on all of these > > @@ +60,5 @@ > > + > > + void safeProcessHeader(in uint32_t aType, > > + in nsIURI aSourceURI, > > + in string aHeader, > > + in nsISupports aSSLStatus, > > Why not just pass in an nsISSLStatus? > > ::: netwerk/protocol/http/nsHttpChannel.cpp > @@ +1084,5 @@ > > + > > + // Get the SSLStatus > > + nsCOMPtr<nsISSLStatusProvider> sslprov = do_QueryInterface(mSecurityInfo); > > + NS_ENSURE_TRUE(sslprov, NS_ERROR_FAILURE); > > + nsCOMPtr<nsISSLStatus> sslstat; > > how about 'sslStatus' > > @@ +1095,3 @@ > > const nsHttpAtom atom = nsHttp::ResolveAtom("Strict-Transport-Security"); > > nsAutoCString stsHeader; > > + //nsresult srv; > > nit: remove commented-out code > > @@ +1098,4 @@ > > rv = mResponseHead->GetHeader(atom, stsHeader); > > + if (NS_SUCCEEDED(rv)) { > > + rv = sss->SafeProcessHeader(nsISiteSecurityService::HEADER_HSTS, mURI, > > + stsHeader.get(), sslstat, flags, nullptr, nullptr); > > nit: indentation > > @@ +1100,5 @@ > > + rv = sss->SafeProcessHeader(nsISiteSecurityService::HEADER_HSTS, mURI, > > + stsHeader.get(), sslstat, flags, nullptr, nullptr); > > + if (NS_FAILED(rv)) { > > + AddSecurityMessage(NS_LITERAL_STRING("InvalidSTSHeaders"), > > + NS_LITERAL_STRING("Invalid HSTS Headers")); > > nit: indentation > > @@ +1101,5 @@ > > + stsHeader.get(), sslstat, flags, nullptr, nullptr); > > + if (NS_FAILED(rv)) { > > + AddSecurityMessage(NS_LITERAL_STRING("InvalidSTSHeaders"), > > + NS_LITERAL_STRING("Invalid HSTS Headers")); > > + LOG(("STS: Failed to parse STS header, continuing load.\n")); > > We probably don't need the 'STS:' prefixes in the logs anymore > > @@ +1112,5 @@ > > LOG(("STS: No STS header, continuing load.\n")); > > + } > > + > > + // If there's a PKP header, process it > > + const nsHttpAtom pkpatom = nsHttp::ResolveAtom("Public-Key-Pins"); > > This code is basically duplicated - seems like we could factor it out. > > @@ +1117,5 @@ > > + nsAutoCString pkpHeader; > > + rv = mResponseHead->GetHeader(pkpatom, pkpHeader); > > + if (NS_SUCCEEDED(rv)) { > > + rv = sss->SafeProcessHeader(nsISiteSecurityService::HEADER_HPKP, mURI, > > + pkpHeader.get(), sslstat, flags, nullptr, nullptr); > > nit: indentation > > @@ +1119,5 @@ > > + if (NS_SUCCEEDED(rv)) { > > + rv = sss->SafeProcessHeader(nsISiteSecurityService::HEADER_HPKP, mURI, > > + pkpHeader.get(), sslstat, flags, nullptr, nullptr); > > + if (NS_FAILED(rv)) { > > + AddSecurityMessage(NS_LITERAL_STRING("InvalidSTSHeaders"), > > "InvalidPKPHeaders", etc. > > @@ +1120,5 @@ > > + rv = sss->SafeProcessHeader(nsISiteSecurityService::HEADER_HPKP, mURI, > > + pkpHeader.get(), sslstat, flags, nullptr, nullptr); > > + if (NS_FAILED(rv)) { > > + AddSecurityMessage(NS_LITERAL_STRING("InvalidSTSHeaders"), > > + NS_LITERAL_STRING("Invalid HSTS Headers")); > > nit: indentation (looks like this was already wrong, but we should fix it > while we're here) > > ::: security/manager/boot/src/Makefile.in > @@ +7,5 @@ > > INCLUDES += \ > > -I$(DIST)/public/nss \ > > $(NULL) > > > > +LOCAL_INCLUDES += -I$(srcdir)/../../ssl/src \ > > We should just export the headers we need, rather than adding the entire > directory. > > ::: security/manager/boot/src/PublicKeyPinningService.cpp > @@ +168,5 @@ > > } > > > > +bool > > +PublicKeyPinningService > > + ::EvalChainWithSHA256HashArray(const CERTCertList* certList, > > Maybe put 'PublicKeyPinningService::EvalChainWithSHA256HashArray(' all on > one line, and then have the arguments on the next line > > ::: security/manager/boot/src/PublicKeyPinningService.h > @@ +35,5 @@ > > + * Returns true if there is any intersection between the certificate list > > + * and the pins specified in the aSHA256key array. Values passed in are > > + * assumed to be in base64 encoded form > > + */ > > + static bool EvalChainWithSHA256HashArray(const CERTCertList* certList, > > How about 'ChainMatchesPinset' or something > > ::: security/manager/boot/src/moz.build > @@ +29,2 @@ > > '../../../pkix/include', > > + '../../ssl/src', > > Again, exactly which headers are necessary? Can we just export them? > > ::: security/manager/boot/src/nsSiteSecurityService.cpp > @@ +245,5 @@ > > "network.stricttransportsecurity.preloadlist", true); > > mozilla::Preferences::AddStrongObserver(this, > > "network.stricttransportsecurity.preloadlist"); > > + mProcessPKPHeadersFromNonBuiltInRoots = mozilla::Preferences::GetBool( > > + "security.cert_pinning.process_headers_from_non_builtin_roots", false); > > Again, I don't think this is necessary. > > @@ +388,5 @@ > > + const char* aHeader, > > + nsISupports* aSSLStatus, > > + uint32_t aFlags, > > + uint64_t *aMaxAge, > > + bool *aIncludeSubdomains) > > nit: bool* > > @@ +406,5 @@ > > nsIURI* aSourceURI, > > const char* aHeader, > > uint32_t aFlags, > > uint64_t *aMaxAge, > > bool *aIncludeSubdomains) > > Might as well fix this while we're here: bool* > > @@ +419,5 @@ > > + const char* aHeader, > > + nsISupports* aSSLStatus, > > + uint32_t aFlags, > > + uint64_t *aMaxAge, > > + bool *aIncludeSubdomains) > > nit: bool* > > @@ +435,5 @@ > > *aIncludeSubdomains = false; > > } > > > > + if (aSSLStatus) { > > + SSSLOG(("SSS: SafeProcessHeader Top")); > > This appears to be 'ProcessHeaderInternal', not SafeProcessHeader > > @@ +436,5 @@ > > } > > > > + if (aSSLStatus) { > > + SSSLOG(("SSS: SafeProcessHeader Top")); > > + nsCOMPtr<nsISSLStatus> sslstat = do_QueryInterface(aSSLStatus); > > Again, if this were an nsISSLStatus from the beginning, this wouldn't be > necessary. > > @@ +438,5 @@ > > + if (aSSLStatus) { > > + SSSLOG(("SSS: SafeProcessHeader Top")); > > + nsCOMPtr<nsISSLStatus> sslstat = do_QueryInterface(aSSLStatus); > > + NS_ENSURE_TRUE(sslstat, NS_ERROR_FAILURE); > > + SSSLOG(("SSS: SafeProcessHeader Post sslprov")); > > This logging line seems unnecessary. > > @@ +455,5 @@ > > + rv = sslstat->GetIsUntrusted(&trustcheck); > > + NS_ENSURE_SUCCESS(rv, rv); > > + tlsIsBroken = tlsIsBroken || trustcheck; > > + if (tlsIsBroken) { > > + SSSLOG(("SSS: SafeProcessHeader Cannot process headers connections is not error-free")); > > How about 'SSS: discarding headers from untrustworthy connection' > > @@ +492,5 @@ > > + char* aHeader, > > + nsISupports* aSSLStatus, > > + uint32_t aFlags, > > + uint64_t *aMaxAge, > > + bool *aIncludeSubdomains) { > > nit: let's fix these while we're here: uint64_t* aMaxAge, bool* > aIncludeSubdomains > > @@ +494,5 @@ > > + uint32_t aFlags, > > + uint64_t *aMaxAge, > > + bool *aIncludeSubdomains) { > > + SSSLOG(("SSS: processing HPKP header '%s'", aHeader)); > > + NS_ENSURE_ARG(aSSLStatus); > > hasn't this already been checked? > > @@ +506,5 @@ > > + > > + NS_NAMED_LITERAL_CSTRING(max_age_var, "max-age"); > > + NS_NAMED_LITERAL_CSTRING(include_subd_var, "includesubdomains"); > > + NS_NAMED_LITERAL_CSTRING(pin_sha256_var, "pin-sha256"); > > + //NS_NAMED_LITERAL_CSTRING(report_uri_var, "report-uri"); > > nit: remove commented-out code > > @@ +514,5 @@ > > + if (NS_FAILED(rv)) { > > + SSSLOG(("SSS: could not parse HPKP header")); > > + return rv; > > + } > > + mozilla::LinkedList<nsSecurityHeaderDirective> *directives = parser.GetDirectives(); > > nit: * to the left > > @@ +516,5 @@ > > + return rv; > > + } > > + mozilla::LinkedList<nsSecurityHeaderDirective> *directives = parser.GetDirectives(); > > + > > + for (nsSecurityHeaderDirective *directive = directives->getFirst(); > > same > > @@ +531,5 @@ > > + SSSLOG(("SSS: HPKP found max-age directive")); > > + foundMaxAge = true; > > + > > + size_t len = directive->mValue.Length(); > > + for (size_t i = 0; i < len; i++) { > > Factor this code out > > @@ +544,5 @@ > > + SSSLOG(("SSS: HPKP could not parse delta-seconds")); > > + return NS_ERROR_FAILURE; > > + } > > + > > + SSSLOG(("SSS: HPKP parsed delta-seconds: %lld", maxAge)); > > A lot of this code is similar for HSTS - can it be factored out? > > @@ +608,5 @@ > > + mozilla::pkix::Time now(mozilla::pkix::Now()); > > + ScopedCERTCertList certList; > > + RefPtr<SharedCertVerifier> certVerifier(GetDefaultCertVerifier()); > > + NS_ENSURE_TRUE(certVerifier, NS_ERROR_UNEXPECTED); > > + if (certVerifier->VerifySSLServerCert(c, nullptr,//stapled ocsp > > nsscert, not c > nit: trailing space > also, add a space before and after the '//' > > @@ +612,5 @@ > > + if (certVerifier->VerifySSLServerCert(c, nullptr,//stapled ocsp > > + now, > > + nullptr, // pinarg > > + host.get(), // hostname > > + false, //no store ints > > nit: space after '//' > Also, maybe "don't store intermediates" > > @@ +616,5 @@ > > + false, //no store ints > > + CertVerifier::FLAG_LOCAL_ONLY, > > + &certList) != SECSuccess) { > > + return NS_ERROR_FAILURE; > > + }; > > nit: no ';' here > > @@ +619,5 @@ > > + return NS_ERROR_FAILURE; > > + }; > > + > > + CERTCertListNode* rootNode = CERT_LIST_TAIL(certList); > > + if (!rootNode) { > > if (CERT_LIST_END(rootNode)) > > @@ +632,5 @@ > > + if (!isBuiltIn && !mProcessPKPHeadersFromNonBuiltInRoots) { > > + return NS_OK; > > + } > > + > > + // if maxAge == 0 we must delete all state, for now NO puncholes > > maybe 'hole-punching' > > @@ +633,5 @@ > > + return NS_OK; > > + } > > + > > + // if maxAge == 0 we must delete all state, for now NO puncholes > > + if (!maxAge) { > > maxAge == 0 > > @@ +637,5 @@ > > + if (!maxAge) { > > + return RemoveState(aType, aSourceURI, aFlags); > > + } > > + > > + if (!PublicKeyPinningService::EvalChainWithSHA256HashArray(certList, sha256keys)){ > > nit: )) { > > @@ +643,5 @@ > > + SSSLOG(("SSS: Pins provided by %s are invalid no match with certList\n", host.get())); > > + return NS_ERROR_FAILURE; > > + } > > + > > + // finally we need to ensure that there is a "backup pin" > > This needs more explanation. How is it intended that this work? Can't we > just ensure that there are at least two distinct key hashes? No, this is probably lack of documentation. From the spec (requirements from noting pins). o The given set of Pins contains at least one Pin that does NOT refer to an SPKI in the certificate chain. (That is, the host must set a Backup Pin; see Section 4.3.) > > @@ +661,5 @@ > > + } > > + > > + // Expire time is millis from now. Since STS max-age is in seconds, and > > + // PR_Now() is in micros, must equalize the units at milliseconds. > > + int64_t expireTime = (PR_Now() / PR_USEC_PER_MSEC) + > > Again, common code can be refactored. > > @@ +665,5 @@ > > + int64_t expireTime = (PR_Now() / PR_USEC_PER_MSEC) + > > + ((int64_t)maxAge * PR_MSEC_PER_SEC); > > + SiteHPKPState dynamicEntry(expireTime, SecurityPropertySet, > > + foundIncludeSubdomains, sha256keys); > > + SSSLOG(("SSS: about to set pons for %s, expires=%ld now=%ld maxAge=%ld\n", > > s/pons/pins/ > > @@ +689,5 @@ > > +nsSiteSecurityService::ProcessSTSHeaderMutating(nsIURI* aSourceURI, > > + char* aHeader, > > + uint32_t aFlags, > > + uint64_t *aMaxAge, > > + bool *aIncludeSubdomains) > > nit: * to the left for aMaxAge and aIncludeSubdomains > > @@ +1086,3 @@ > > > > +nsresult > > +nsSiteSecurityService::setHPKPState(const char* aHost, SiteHPKPState& entry){ > > nit: { on the next line > > @@ +1086,4 @@ > > > > +nsresult > > +nsSiteSecurityService::setHPKPState(const char* aHost, SiteHPKPState& entry){ > > + > > nit: unnecessary blank line > > ::: security/manager/boot/src/nsSiteSecurityService.h > @@ +130,5 @@ > > bool includeSubdomains, uint32_t flags); > > + nsresult ProcessHeaderInternal(uint32_t aType, nsIURI* aSourceURI, > > + const char* aHeader, nsISupports* aSSLStatus, > > + uint32_t aFlags, uint64_t *aMaxAge, > > + bool *aIncludeSubdomains); > > Please fix all of the *

Comment on attachment 8492391 [details] [diff] [review] processing-and-storing-hpkp-headers (wip2) Review of attachment 8492391 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the review. Two things worth metioning before looking at next review: ::: modules/libpref/init/all.js @@ +1731,5 @@ > // Disable pinning checks by default. > pref("security.cert_pinning.enforcement_level", 0); > +// Do not process hpkp headers rooted by not built in roots by default > +// This is to prevent accidental pinning from MITM devices > +pref("security.cert_pinning.process_headers_from_non_builtin_roots", false); We need this so we can actually do testing. ::: security/manager/boot/src/nsSiteSecurityService.cpp @@ +494,5 @@ > + uint32_t aFlags, > + uint64_t *aMaxAge, > + bool *aIncludeSubdomains) { > + SSSLOG(("SSS: processing HPKP header '%s'", aHeader)); > + NS_ENSURE_ARG(aSSLStatus); No in all code paths. When evaluating via unsafe, this code is reachable. This stops further processing.

try run https://tbpl.mozilla.org/?tree=Try&rev=c3a934521b6e

Comment on attachment 8495448 [details] [diff] [review] processing-and-storing-hpkp-headers Review of attachment 8495448 [details] [diff] [review]: ----------------------------------------------------------------- This looks great! I think it would be good to have one final look, though, so r- for now. ::: b2g/chrome/content/devtools/hud.js @@ +268,5 @@ > 'Mixed Content Blocker', > 'Mixed Content Message', > 'CSP', > 'Invalid HSTS Headers', > + 'Invalid HPKP Headers', These files probably need a quick sign-off from a b2g/devtools/dom peer or someone. ::: modules/libpref/init/all.js @@ +1742,5 @@ > > // Disable pinning checks by default. > pref("security.cert_pinning.enforcement_level", 0); > +// Do not process hpkp headers rooted by not built in roots by default > +// This is to prevent accidental pinning from MITM devices let's add a note that this is for tests ::: netwerk/base/public/nsISiteSecurityService.idl @@ +36,2 @@ > * The format of the HSTS header is defined by the HSTS specification: > * https://tools.ietf.org/html/rfc6797 nit: add a link to the HPKP spec. as it is now? @@ +64,5 @@ > + * Currently two header types are supported: HSTS (aka STS) and HPKP > + * The format of the HSTS header is defined by the HSTS specification: > + * https://tools.ietf.org/html/rfc6797 > + * and allows a host to specify that future HTTP requests should be > + * upgraded to HTTPS. We probably don't need to copy the comment verbatim here - just reference the comment for unsafeProcessHeader (or, even better, have processHeader first with the full comment, and then unsafeProcessHeader after, with a comment that just essentially says, "Same thing as processHeader but without checking that the connection was secure.") ::: netwerk/protocol/http/nsHttpChannel.cpp @@ +1041,5 @@ > return rv; > } > > /** > + * Process a single security header. Only two types are suported HSTS and HPKP. These changes probably require a quick look from a necko peer. @@ +1066,5 @@ > + consoleErrorCategory = NS_LITERAL_STRING("Invalid HPKP Headers"); > + headerName = NS_LITERAL_CSTRING("PKP"); > + break; > + default: > + return NS_ERROR_FAILURE; Add a MOZ_NOT_REACHED or equivalent here. @@ +1074,5 @@ > + nsresult rv = mResponseHead->GetHeader(atom, securityHeader); > + if (NS_SUCCEEDED(rv)) { > + nsISiteSecurityService* sss = gHttpHandler->GetSSService(); > + NS_ENSURE_TRUE(sss, NS_ERROR_OUT_OF_MEMORY); > + rv = sss->ProcessHeader(aType, mURI, securityHeader.get(), aSSLStatus, Maybe add a comment that ProcessHeader now will discard the headers itself if the channel wasn't secure (whereas before it had to be checked manually). @@ +1140,4 @@ > NS_ENSURE_SUCCESS(rv, rv); > + NS_ENSURE_TRUE(sslStatus, NS_ERROR_FAILURE); > + > + rv = ProcessSingleSecurityHeader(nsISiteSecurityService::HEADER_HSTS, Great - this is a lot cleaner. ::: security/manager/boot/src/nsSiteSecurityService.cpp @@ +6,5 @@ > > #include "mozilla/LinkedList.h" > #include "mozilla/Preferences.h" > #include "mozilla/Base64.h" > +#include "CertVerifier.h" nit: move this after base64.h (in fact, feel free to re-sort this whole block) @@ +247,5 @@ > "network.stricttransportsecurity.preloadlist"); > + mProcessPKPHeadersFromNonBuiltInRoots = mozilla::Preferences::GetBool( > + "security.cert_pinning.process_headers_from_non_builtin_roots", false); > + mozilla::Preferences::AddStrongObserver(this, > + "security.cert_pinning."); nit: "security.cert_pinning.process_headers_from_non_builtin_roots" - we don't need to watch the whole branch, right? @@ +301,5 @@ > } > > +// Expire times are in millis. Since Headers max-age is in seconds, and > +// PR_Now() is in micros, normalize the units at milliseconds. > +static int64_t expireTimeFromMaxAge(int64_t maxAge) { nit: return type on separate line, { on next line, capitalize "Expire" @@ +382,5 @@ > PRNetAddr hostAddr; > return (PR_StringToNetAddr(hostname, &hostAddr) == PR_SUCCESS); > } > > + nit: unnecessary extra blank line @@ +398,5 @@ > + NS_ERROR_NOT_IMPLEMENTED); > + > + NS_ENSURE_ARG(aSSLStatus); > + return ProcessHeaderInternal(aType, aSourceURI, aHeader, aSSLStatus, aFlags, > + aMaxAge, aIncludeSubdomains); nit: indentation @@ +410,5 @@ > + uint64_t* aMaxAge, > + bool* aIncludeSubdomains) > +{ > + return ProcessHeaderInternal(aType, aSourceURI, aHeader, nullptr, aFlags, > + aMaxAge, aIncludeSubdomains); nit: indentation @@ +414,5 @@ > + aMaxAge, aIncludeSubdomains); > +} > + > +nsresult > +nsSiteSecurityService::ProcessHeaderInternal(uint32_t aType, nit: why not just have ProcessHeaderInternal be UnsafeProcessHeader? @@ +451,5 @@ > + rv = aSSLStatus->GetIsUntrusted(&trustcheck); > + NS_ENSURE_SUCCESS(rv, rv); > + tlsIsBroken = tlsIsBroken || trustcheck; > + if (tlsIsBroken) { > + SSSLOG(("SSS: discarding headers from untrustworthy connection")); nit: 'header' @@ +456,5 @@ > + return NS_ERROR_FAILURE; > + } > + } > + > + nit: unnecessary extra blank line @@ +465,5 @@ > /* Don't process headers if a site is accessed by IP address. */ > return NS_OK; > } > > char * header = NS_strdup(aHeader); I'm fairly sure we don't need to do this anymore. @@ +477,5 @@ > + rv = ProcessPKPHeaderMutating(aSourceURI, header, aSSLStatus, aFlags, > + aMaxAge, aIncludeSubdomains); > + break; > + default: > + rv = NS_ERROR_NOT_IMPLEMENTED; I would add a MOZ_NOT_REACHED here - this really should never happen. @@ +485,5 @@ > } > > +static nsresult > +ParseSSSHeadersMutating(uint32_t aType, > + char* aHeader, I don't think we need to mutate the given header anymore, so this can be a const char*. Also, we can remove 'Mutating' in these function names. @@ +490,5 @@ > + bool& foundIncludeSubdomains, > + bool& foundMaxAge, > + bool& foundUnrecognizedDirective, > + int64_t& maxAge, > + nsTArray<nsCString>& sha256keys) { nit: { on next line @@ +551,2 @@ > directive != nullptr; directive = directive->getNext()) { > + SSSLOG(("SSS: found got directive %s\n", directive->mName.get())); nit: s/got // @@ +595,5 @@ > + } else if (aType == nsISiteSecurityService::HEADER_HPKP && > + directive->mName.Length() == pin_sha256_var.Length() && > + directive->mName.EqualsIgnoreCase(pin_sha256_var.get(), > + pin_sha256_var.Length())) { > + SSSLOG(("SSS: found pinning entry '%s' lenght=%d", nit: length @@ +596,5 @@ > + directive->mName.Length() == pin_sha256_var.Length() && > + directive->mName.EqualsIgnoreCase(pin_sha256_var.get(), > + pin_sha256_var.Length())) { > + SSSLOG(("SSS: found pinning entry '%s' lenght=%d", > + directive->mValue.get(), directive->mValue.Length())); nit: indentation @@ +605,5 @@ > + } else if (aType == nsISiteSecurityService::HEADER_HPKP && > + directive->mName.Length() == report_uri_var.Length() && > + directive->mName.EqualsIgnoreCase(report_uri_var.get(), > + report_uri_var.Length())) { > + //do nothing for now nit: space after // Also, maybe say something like "we don't support report-uri yet, but to avoid unrecognized directive warnings, we still have to handle its presence" @@ +621,5 @@ > + char* aHeader, > + nsISSLStatus* aSSLStatus, > + uint32_t aFlags, > + uint64_t* aMaxAge, > + bool* aIncludeSubdomains) { nit: { on next line @@ +653,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + nsCOMPtr<nsIX509Cert> cert; > + rv = aSSLStatus->GetServerCert(getter_AddRefs(cert)); > + NS_ENSURE_SUCCESS(rv, rv); > + NS_ENSURE_TRUE(cert, NS_ERROR_UNEXPECTED); maybe NS_ERROR_FAILURE? @@ +654,5 @@ > + nsCOMPtr<nsIX509Cert> cert; > + rv = aSSLStatus->GetServerCert(getter_AddRefs(cert)); > + NS_ENSURE_SUCCESS(rv, rv); > + NS_ENSURE_TRUE(cert, NS_ERROR_UNEXPECTED); > + ScopedCERTCertificate nssCert(cert->GetCert()); null-check nssCert @@ +663,5 @@ > + NS_ENSURE_TRUE(certVerifier, NS_ERROR_UNEXPECTED); > + if (certVerifier->VerifySSLServerCert(nssCert, nullptr, // stapled ocsp > + now, nullptr, // pinarg > + host.get(), // hostname > + false, // no store intermediates nit: "don't store intermediates" @@ +699,5 @@ > + // at least one fingerprint hash that does NOT valiate against the verified > + // chain (Section 2.5 of the spec) > + bool hasBackupPin = false; > + for (uint32_t i = 0; i < sha256keys.Length(); i++) { > + nsTArray<nsCString> singletonArray; singletonArray is an unintuitive name to me (it puts me in mind of the singleton design paradigm, which isn't what's going on here). How about something like "singlePin"? @@ +729,5 @@ > + if (aIncludeSubdomains != nullptr) { > + *aIncludeSubdomains = foundIncludeSubdomains; > + } > + > + return foundUnrecognizedDirective ? NS_SUCCESS_LOSS_OF_INSIGNIFICANT_DATA nit: maybe have '? NS_SUCCESS_LOSS_OF_INSIGNIFICANT_DATA' on the next line @@ +747,5 @@ > + bool foundMaxAge = false; > + bool foundIncludeSubdomains = false; > + bool foundUnrecognizedDirective = false; > + int64_t maxAge = 0; > + nsTArray<nsCString> sha256keys; // Unused requred for sane internal interface nit: it would be helpful to name this 'unsedSHA256Keys' or something. @@ +1066,3 @@ > > +nsresult > +nsSiteSecurityService::setHPKPState(const char* aHost, SiteHPKPState& entry, nit: capitalize 'Set' ::: security/manager/ssl/tests/mochitest/browser/browser_bug627234_perwindowpb.js @@ +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/. */ > > +var FakeSSLStatus = function(certificate) { nit: let, not var Also, looks like this version never gets a certificate, so we may as well just not take an argument here. @@ +45,5 @@ > > function doTest(aIsPrivateMode, aWindow, aCallback) { > aWindow.gBrowser.selectedBrowser.addEventListener("load", function onLoad() { > aWindow.gBrowser.selectedBrowser.removeEventListener("load", onLoad, true); > + var sslStatus = new FakeSSLStatus(); nit: let not var ::: security/manager/ssl/tests/unit/head_psm.js @@ +518,5 @@ > } > }; > } > + > +// A prototype for a fake error free sslstatus nit: "fake, error-free sslStatus" @@ +519,5 @@ > }; > } > + > +// A prototype for a fake error free sslstatus > +var FakeSSLStatus = function(certificate) { nit: let not var @@ +521,5 @@ > + > +// A prototype for a fake error free sslstatus > +var FakeSSLStatus = function(certificate) { > + if (certificate) { > + this.serverCert = certificate; This can just be set unconditionally. If it's constructed without an argument, this.serverCert will be null/undefined. ::: security/manager/ssl/tests/unit/test_ocsp_no_hsts_upgrade.js @@ +41,5 @@ > > let SSService = Cc["@mozilla.org/ssservice;1"] > .getService(Ci.nsISiteSecurityService); > let uri = Services.io.newURI("http://localhost", null, null); > + var sslStatus = new FakeSSLStatus(); nit: let not var ::: security/manager/ssl/tests/unit/test_sss_eviction.js @@ +43,5 @@ > do_check_eq(aData, SSS_STATE_FILE_NAME); > > do_check_true(gSSService.isSecureHost(Ci.nsISiteSecurityService.HEADER_HSTS, > "frequentlyused.example.com", 0)); > + var sslStatus = new FakeSSLStatus(); let not var ::: security/manager/ssl/tests/unit/test_sss_savestate.js @@ +109,5 @@ > let uriIndex = i % uris.length; > // vary max-age > let maxAge = "max-age=" + (i * 1000); > // alternate setting includeSubdomains > + var sslStatus = new FakeSSLStatus(); let not var ::: security/manager/ssl/tests/unit/test_sts_ipv4_ipv6.js @@ +1,2 @@ > function check_ip(s, v, ip) { > + var sslStatus = new FakeSSLStatus(); let @@ +21,4 @@ > parsedMaxAge, parsedIncludeSubdomains); > > + /* Test that processHeader will ignore headers for an uri, if the uri contains > + * an IP address not a hostname. probably don't need to change this comment (unless you want to fix "an uri," -> "a uri" and "IP address not a hostname" -> "IP address and not a hostname") ::: security/manager/ssl/tests/unit/test_sts_preloadlist_perwindowpb.js @@ +15,5 @@ > } > }; > > var gObserver = new Observer(); > +var sslStatus = new FakeSSLStatus(); I guess in this case it's more consistent to use var.

Comment on attachment 8496109 [details] [diff] [review] processing-and-storing-hpkp-headers (v 1.1) Review of attachment 8496109 [details] [diff] [review]: ----------------------------------------------------------------- Great - r=me with comments addressed. Remember to also get reviews for the non-PSM parts of this patch. ::: security/manager/boot/src/nsSiteSecurityService.cpp @@ +302,5 @@ > > +// Expire times are in millis. Since Headers max-age is in seconds, and > +// PR_Now() is in micros, normalize the units at milliseconds. > +static int64_t > +ExpireTimeFromMaxAge(int64_t maxAge) { nit: { on its own line @@ +304,5 @@ > +// PR_Now() is in micros, normalize the units at milliseconds. > +static int64_t > +ExpireTimeFromMaxAge(int64_t maxAge) { > + return (PR_Now() / PR_USEC_PER_MSEC) + (maxAge * PR_MSEC_PER_SEC); > +}; no trailing ';' @@ +599,5 @@ > + return NS_ERROR_FAILURE; > + } > + sha256keys.AppendElement(directive->mValue); > + } else if (aType == nsISiteSecurityService::HEADER_HPKP && > + directive->mName.Length() == report_uri_var.Length() && Just thought of something - is it an error to have more than one report-uri directive? If so, we should enforce that from the beginning (just do what maxAge/includeSubdomains do for duplicate detection). @@ +602,5 @@ > + } else if (aType == nsISiteSecurityService::HEADER_HPKP && > + directive->mName.Length() == report_uri_var.Length() && > + directive->mName.EqualsIgnoreCase(report_uri_var.get(), > + report_uri_var.Length())) { > + // We dont support the report-uri yet, but to avoid unreconized nit: "don't", "unrecognized" @@ +659,5 @@ > + > + mozilla::pkix::Time now(mozilla::pkix::Now()); > + ScopedCERTCertList certList; > + RefPtr<SharedCertVerifier> certVerifier(GetDefaultCertVerifier()); > + NS_ENSURE_TRUE(certVerifier, NS_ERROR_UNEXPECTED); nit: NS_ERROR_FAILURE ::: security/manager/ssl/tests/unit/head_psm.js @@ +519,5 @@ > }; > } > + > +// A prototype for a fake, error-free sslstatus > +let FakeSSLStatus = function(certificate) { hmmm - looks like FakeSSLStatus is never created with a certificate, so we don't need this parameter at all.

Comment on attachment 8496109 [details] [diff] [review] processing-and-storing-hpkp-headers (v 1.1) Review of attachment 8496109 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the review. ::: security/manager/boot/src/nsSiteSecurityService.cpp @@ +599,5 @@ > + return NS_ERROR_FAILURE; > + } > + sha256keys.AppendElement(directive->mValue); > + } else if (aType == nsISiteSecurityService::HEADER_HPKP && > + directive->mName.Length() == report_uri_var.Length() && Yes I missed that one will add. ::: security/manager/ssl/tests/unit/head_psm.js @@ +519,5 @@ > }; > } > + > +// A prototype for a fake, error-free sslstatus > +let FakeSSLStatus = function(certificate) { This will be used really soon now(tm) when the tests land. So I will leave them

Comment on attachment 8496109 [details] [diff] [review] processing-and-storing-hpkp-headers (v 1.1) Review of attachment 8496109 [details] [diff] [review]: ----------------------------------------------------------------- Patrick can you take a look at the changes in nsHttpChannel.(cpp|h)? jduell suggested you as a good reviewer

The webconsole bits look good to me. It'd be lovely to start adding links to MDN content explaining these (see Tanvi's work on the mixed content web console messages) but that's separate work and out of scope for this (we'd need the MDN content too).

Comment on attachment 8496273 [details] [diff] [review] new-tests-pinning Review of attachment 8496273 [details] [diff] [review]: ----------------------------------------------------------------- Feedback only! (lots of cruft). Keeler is this what you had in mind? ( browser_blockHPKP.js for the full flight + UI test) + test_pinning_header_parsing.js for the parsing only parts.

Comment on attachment 8496273 [details] [diff] [review] new-tests-pinning Review of attachment 8496273 [details] [diff] [review]: ----------------------------------------------------------------- Looks good in general. ::: browser/base/content/test/general/browser_blockHPKP.js @@ +81,5 @@ > + } > +}; > + > +// A utility to make xhr connections to the pinning domain > +function xhrConnect(message, queryString) I think it would be best to use browser navigation rather than XHR. This is an integration test, after all. ::: build/pgo/server-locations.txt @@ +225,5 @@ > + > +# Host for HPKP > +https://include-subdomains.pinning-dynamic.example.com:443 privileged,cert=dynamicPinningGood > +https://good.include-subdomains.pinning-dynamic.example.com:443 privileged,cert=dynamicPinningGood > +https://bad.include-subdomains.pinning-dynamic.example.com:443 privileged,cert=badPinningCert Rather than generating new certificates, can we use key pins from certificates that already exist in the ssltunnel certdb? ::: security/manager/ssl/tests/unit/test_pinning_header_parsing.js @@ +30,5 @@ > + gSSService.processHeader(Ci.nsISiteSecurityService.HEADER_HPKP, uri, > + pinValue, sslStatus, 0); > + do_check_true(false); // this should not run > + } catch (e) { > + do_check_true(true); This line doesn't actually do much. If for some reason the body of the catch statement doesn't run, having this line here won't notify the test harness that it hasn't happened. @@ +80,5 @@ > +const MAX_AGE_ZERO = "max-age=0;" > +const VALID_PIN1 = "pin-sha256=\""+ PINNING_ROOT_KEY_HASH + "\";"; > +const INVALID_PIN1 = "pin-sha256=\""+ NON_ISSUED_KEY_HASH1 + "\";"; > +const INVALID_PIN2 = "pin-sha256=\""+ NON_ISSUED_KEY_HASH2 + "\";"; > +const BROKEN_PIN1 = "pin-sha256=\"jdjsjsjs\";"; Doesn't look like this is tested. @@ +82,5 @@ > +const INVALID_PIN1 = "pin-sha256=\""+ NON_ISSUED_KEY_HASH1 + "\";"; > +const INVALID_PIN2 = "pin-sha256=\""+ NON_ISSUED_KEY_HASH2 + "\";"; > +const BROKEN_PIN1 = "pin-sha256=\"jdjsjsjs\";"; > +const GOOD_MAX_AGE = "max-age=69403;"; > +const INCLUDE_SUBDOMAINS = "includeSubdomains;"; Things like duplicate values, unrecognized values, report-uri would also be good to test. Also, things that are completely invalid (e.g. not in key=value form).

(In reply to Camilo Viecco (:cviecco) from comment #91) > ::: security/manager/ssl/tests/unit/head_psm.js > @@ +519,5 @@ > > }; > > } > > + > > +// A prototype for a fake, error-free sslstatus > > +let FakeSSLStatus = function(certificate) { > > This will be used really soon now(tm) when the tests land. So I will leave > them Unless you're planning to fold these patches together, I think it's important (in general) that each patch be internally consistent. That meant not including unrelated changes until they're necessary. This can easily be removed from here and added to the tests patch.

(In reply to Camilo Viecco (:cviecco) from comment #83) > (In reply to David Keeler (:keeler) [use needinfo?] from comment #82) > > Since the bulk of the test is loading certificates and making XHRs to > > various domains, it seems like our xpcshell/TLSServer setup would be better > > suited to this. > > Xpcshell/TLS server is not suited for this test as I need to control both > certificates and headers sent (which is not possible with xpcshell/TLSServer > at this moment, filed Bug 1072084 to track this). Having these tests be mochitests would be unfortunate. Right now, you can run all of the certificate tests (except for a useless and redundant cert error override mochitest) with 'mach build && mach gtest "pkix*" && mach xpcshell-test security/manager/ssl/tests/unit'. That is extremely convenient for aiding in the development of improvements to the certificate verification and TLS parts of Gecko. Adding a new requirement to run mochitests to check for regressions in new changes is quite a bit less convenient, because mochitests require the browser UI, and that interferes with attempts to run tests in the background (something which works fine today). It is very, very easy to either modify TLSServer or to create a new variant of it so that these tests can continue to be xpcshell tests, and I hope that the tests are converted to xpcshell tests. (I had thought that this was already a settled issue, from previous discussions. Definitely it is not a new idea.)

Comment on attachment 8496109 [details] [diff] [review] processing-and-storing-hpkp-headers (v 1.1) Review of attachment 8496109 [details] [diff] [review]: ----------------------------------------------------------------- a couple minor netwerk/* tweaks that I'd appreciate you trying to address before landing - not critical though. r+mcmanus I'm pretty psyched to see this landing ::: netwerk/protocol/http/nsHttpChannel.cpp @@ +1045,5 @@ > + * Process a single security header. Only two types are suported HSTS and HPKP. > + */ > +nsresult > +nsHttpChannel::ProcessSingleSecurityHeader(uint32_t aType, > + nsISSLStatus* aSSLStatus, nit - necko does the opposite of psm.. "type *var". this nit is throughout the patch. I'd prefer you fix it - but if you need to crashland it or something its ok. @@ +1052,5 @@ > + nsHttpAtom atom; > + nsAutoString consoleErrorCategory; > + nsAutoString consoleErrorTag; > + nsAutoCString headerName; > + switch (aType) { so this whole block does quite a few string ctors/dtors all for essentially use in error cases (the console* ones especially). Can you scope console* to the error path and just use atom.get() instead of creating headerName?

(In reply to Patrick McManus [:mcmanus] from comment #99) > Comment on attachment 8496109 [details] [diff] [review] > processing-and-storing-hpkp-headers (v 1.1) > > Review of attachment 8496109 [details] [diff] [review]: > ----------------------------------------------------------------- > > a couple minor netwerk/* tweaks that I'd appreciate you trying to address > before landing - not critical though. r+mcmanus > > I'm pretty psyched to see this landing > > ::: netwerk/protocol/http/nsHttpChannel.cpp > @@ +1045,5 @@ > > + * Process a single security header. Only two types are suported HSTS and HPKP. > > + */ > > +nsresult > > +nsHttpChannel::ProcessSingleSecurityHeader(uint32_t aType, > > + nsISSLStatus* aSSLStatus, > > nit - necko does the opposite of psm.. "type *var". this nit is throughout > the patch. I'd prefer you fix it - but if you need to crashland it or > something its ok. > > @@ +1052,5 @@ > > + nsHttpAtom atom; > > + nsAutoString consoleErrorCategory; > > + nsAutoString consoleErrorTag; > > + nsAutoCString headerName; > > + switch (aType) { > > so this whole block does quite a few string ctors/dtors all for essentially > use in error cases (the console* ones especially). > > Can you scope console* to the error path and just use atom.get() instead of > creating headerName? Thanks for the review. I will apply both sets of changes before landing.

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #98) > (In reply to Camilo Viecco (:cviecco) from comment #83) > > (In reply to David Keeler (:keeler) [use needinfo?] from comment #82) > > > Since the bulk of the test is loading certificates and making XHRs to > > > various domains, it seems like our xpcshell/TLSServer setup would be better > > > suited to this. > > > > Xpcshell/TLS server is not suited for this test as I need to control both > > certificates and headers sent (which is not possible with xpcshell/TLSServer > > at this moment, filed Bug 1072084 to track this). > > Having these tests be mochitests would be unfortunate. Right now, you can > run all of the certificate tests (except for a useless and redundant cert > error override mochitest) with 'mach build && mach gtest "pkix*" && mach > xpcshell-test security/manager/ssl/tests/unit'. That is extremely convenient > for aiding in the development of improvements to the certificate > verification and TLS parts of Gecko. Adding a new requirement to run > mochitests to check for regressions in new changes is quite a bit less > convenient, because mochitests require the browser UI, and that interferes > with attempts to run tests in the background (something which works fine > today). > > It is very, very easy to either modify TLSServer or to create a new variant > of it so that these tests can continue to be xpcshell tests, and I hope that > the tests are converted to xpcshell tests. (I had thought that this was > already a settled issue, from previous discussions. Definitely it is not a > new idea.) Brian, after discussing with Keeler we agreed that for now we will keep the mochitest integration tests. The moving to xpchell test only will be a follow up bug.

Comment on attachment 8496273 [details] [diff] [review] new-tests-pinning Review of attachment 8496273 [details] [diff] [review]: ----------------------------------------------------------------- Feedback only! (lots of cruft). Keeler is this what you had in mind? ( browser_blockHPKP.js for the full flight + UI test) + test_pinning_header_parsing.js for the parsing only parts. ::: build/pgo/server-locations.txt @@ +225,5 @@ > + > +# Host for HPKP > +https://include-subdomains.pinning-dynamic.example.com:443 privileged,cert=dynamicPinningGood > +https://good.include-subdomains.pinning-dynamic.example.com:443 privileged,cert=dynamicPinningGood > +https://bad.include-subdomains.pinning-dynamic.example.com:443 privileged,cert=badPinningCert I need at least the badPinningCert (and the new alternateroot.ca). I thought it was cleaner to have both. Remove the gooddynamic Pin? ::: security/manager/ssl/tests/unit/test_pinning_header_parsing.js @@ +30,5 @@ > + gSSService.processHeader(Ci.nsISiteSecurityService.HEADER_HPKP, uri, > + pinValue, sslStatus, 0); > + do_check_true(false); // this should not run > + } catch (e) { > + do_check_true(true); In this function, the expected condition is a failure during gSSService.processHeader, and thus the expected success of the test is when the catch block runs. In the success case, the content of the catch block do_check_true(true) will notify the test harness of success, and in case of failure ( gSSService.processHeader succeeding) the do_check_true(false) will notify the test harness of failure. I believe a constant number of checks makes understanding errors more easy, but removing this line is OK with me.

Can you comment on comment 102?

Keeping r+ from keeler (PSM), mcmanus (necko) and mgoodwind (webconsole)

From irc with keeler: about the certs in the certdb: (01:18:00 PM) keeler: ok, how about this: add whatever certificates you have to, but include copy/paste-able instructions in the browser_ test file on how to regenerate them if needed (01:18:15 PM) cviecco: OK will do. And the do_check_true (01:22:29 PM) keeler: let successBranchTaken = false; (01:22:30 PM) keeler: / do test (01:22:40 PM) keeler: catch { successBranchTaken = true; } (01:22:45 PM) keeler: do_check_true(successBranchTaken); (01:23:05 PM) cviecco: OK that sounds better. thank you (01:23:13 PM) keeler: no problem

Comment on attachment 8497064 [details] [diff] [review] new-tests-pinning (v2) Review of attachment 8497064 [details] [diff] [review]: ----------------------------------------------------------------- Two main issues: - How does browser_blockHPKP.js test that the error encountered is the expected error? - How does alternateroot.ca get created? Also, more documentation in the tests would be appreciated. Looks good in general, so r=me with comments addressed. ::: browser/base/content/test/general/browser.ini @@ +486,5 @@ > [browser_bug1045809.js] > skip-if = e10s > +[browser_blockHPKP.js] > +skip-if = e10s # Bug ?????? - test directly manipulates content (content.document.getElementById) > + nit: unnecessary trailing line ::: browser/base/content/test/general/browser_blockHPKP.js @@ +3,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/. */ > + > +// Test verifying that you can get a netwerror when visiting a site that has Maybe something more like "Test that visiting a site pinned with HPKP headers does not succeed when it uses a certificate with a key not in the pinset. This should result in an about:neterror page." @@ +5,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +// Test verifying that you can get a netwerror when visiting a site that has > +// been pinned via HPKP headers and has been signed by an unauthorized pin. > +// Verifying that removal of the HPKP headers succeeds (via HPKP headers) nit: Also verify that... @@ +9,5 @@ > +// Verifying that removal of the HPKP headers succeeds (via HPKP headers) > +// and that after removal the visit to the site with the previously > +// unauthorized pins succeeds. > +// > +// This test required three certs to be created: Where? build/pgo/certs, right? @@ +11,5 @@ > +// unauthorized pins succeeds. > +// > +// This test required three certs to be created: > +// 1. A new trusted root: > +// certutil -S -s "Alternate trusted authority" -s "CN=Alternate Trusted Authority" -t "C,," -x -m 1 -v 120 -n "alternateTrustedAuthority" -Z SHA256 -g 2048 -2 -d . Where does this root get trusted? Is it automatically added to the profile that gets used for mochitests? Are there any steps someone would need to take to update it? (e.g. how does alternateroot.ca get generated?) @@ +13,5 @@ > +// This test required three certs to be created: > +// 1. A new trusted root: > +// certutil -S -s "Alternate trusted authority" -s "CN=Alternate Trusted Authority" -t "C,," -x -m 1 -v 120 -n "alternateTrustedAuthority" -Z SHA256 -g 2048 -2 -d . > +// 2. A good pinning server cert (signed by the pgo root): > +// certutil -S -n "dynamicPinningGood" -s "CN=dynamic-pinning.example.com" -c "pgo temporary ca" -t "P,," -k rsa -g 2048 -Z SHA256 -m 8939454 -v 120 -8 "*.include-subdomains.pinning-dynamic.example.com,*.exclude-subdomains.pinning-dynamic.example.com,*.pinning-dynamic.example.com" -d . Well, then we need the key hash, right? How do we get that? Also, this test doesn't do anything with include/exclude subdomains - why specify in those altnames in the cert? @@ +14,5 @@ > +// 1. A new trusted root: > +// certutil -S -s "Alternate trusted authority" -s "CN=Alternate Trusted Authority" -t "C,," -x -m 1 -v 120 -n "alternateTrustedAuthority" -Z SHA256 -g 2048 -2 -d . > +// 2. A good pinning server cert (signed by the pgo root): > +// certutil -S -n "dynamicPinningGood" -s "CN=dynamic-pinning.example.com" -c "pgo temporary ca" -t "P,," -k rsa -g 2048 -Z SHA256 -m 8939454 -v 120 -8 "*.include-subdomains.pinning-dynamic.example.com,*.exclude-subdomains.pinning-dynamic.example.com,*.pinning-dynamic.example.com" -d . > +// 3. A badly pinned (but otherwise valid) server cert: Maybe "A certificate with a different key, so as to cause a key pinning violation." @@ +19,5 @@ > +// certutil -S -n "badIncludeSubdomansPinning" -s "CN=bad.include-subdomains.pinning-dynamic.example.com" -c "AlternateTrustedAuthority" -t "P,," -k rsa -g 2048 -Z SHA256 -m 893945439 -v 120 -8 "bad.include-subdomains.pinning-dynamic.example.com" -d > + > +function test() { > + waitForExplicitFinish(); > + // Enable pinning for testing both for processing and Maybe "Enable enforcing test-mode pinning and processing headers from non-builtin roots." @@ +39,5 @@ > +const ioService = Cc["@mozilla.org/network/io-service;1"] > + .getService(Ci.nsIIOService); > + > +const pinningDomain = "include-subdomains.pinning-dynamic.example.com"; > +const hpkpPinninEnablePref = "security.cert_pinning.process_headers_from_non_builtin_roots"; "hpkpPinningEnablePref" @@ +43,5 @@ > +const hpkpPinninEnablePref = "security.cert_pinning.process_headers_from_non_builtin_roots"; > +const pkpEnforcementPref = "security.cert_pinning.enforcement_level"; > +const badPinningDomain = "bad.include-subdomains.pinning-dynamic.example.com"; > + > +const urlPath = "/browser/browser/base/content/test/general/pinning_headers.sjs?"; Declare all of these consts before any code. Also, make them a bit more readily identifiable as global consts: "gPinningDomain", or "kPinningDomain", etc. @@ +61,5 @@ > + handleEvent: function() { > + gBrowser.selectedBrowser.removeEventListener("load", this, true); > + gBrowser.addProgressListener(certErrorProgressListener); > + gBrowser.selectedBrowser.loadURI("https://" + badPinningDomain); > + nit: unnecessary blank line @@ +65,5 @@ > + > + } > +}; > + > +// The browser should load about:certerror. When This happens, proceed I think it's about:neterror, actually. nit: lowercase "this" @@ +72,5 @@ > + buttonClicked: false, > + onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus) { > + if (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP) { > + let self = this; > + // Can't directly call button.click() in onStateChange We're not calling button.click() here - much of this doesn't seem necessary anymore, unless this is how we ensure that we've navigated to about:neterror? (although we should be doing that directly, and checking that the error is the one we expect) @@ +112,5 @@ > + ok(true, "load complete"); > + finish(); > + } > +}; > + nit: unnecessary trailing line ::: build/pgo/server-locations.txt @@ +224,5 @@ > https://marketplace.allizom.org:443 privileged > + > +# Host for HPKP > +https://include-subdomains.pinning-dynamic.example.com:443 privileged,cert=dynamicPinningGood > +https://good.include-subdomains.pinning-dynamic.example.com:443 privileged,cert=dynamicPinningGood this one isn't used in the mochitest @@ +225,5 @@ > + > +# Host for HPKP > +https://include-subdomains.pinning-dynamic.example.com:443 privileged,cert=dynamicPinningGood > +https://good.include-subdomains.pinning-dynamic.example.com:443 privileged,cert=dynamicPinningGood > +https://bad.include-subdomains.pinning-dynamic.example.com:443 privileged,cert=badPinningCert nit: let's have these lines be consistent in some way - either the same number of spaces between the url and the parameters, or aligning the parameters horizontally Also, maybe "dynamicPinningBad" to be consistent? @@ +226,5 @@ > +# Host for HPKP > +https://include-subdomains.pinning-dynamic.example.com:443 privileged,cert=dynamicPinningGood > +https://good.include-subdomains.pinning-dynamic.example.com:443 privileged,cert=dynamicPinningGood > +https://bad.include-subdomains.pinning-dynamic.example.com:443 privileged,cert=badPinningCert > + nit: unnecessary trailing line ::: security/manager/ssl/tests/unit/test_pinning_header_parsing.js @@ +7,5 @@ > + > +let profileDir = do_get_profile(); > +const certdb = Cc["@mozilla.org/security/x509certdb;1"] > + .getService(Ci.nsIX509CertDB); > +let gSSService = Cc["@mozilla.org/ssservice;1"] nit: unnecessary extra space after 'let' (don't forget to align .getService on the line below when you change this) @@ +35,5 @@ > + successBranchTaken = true; > + } > + do_check_true(successBranchTaken); > +} > + nit: unnecessary extra blank line @@ +37,5 @@ > + do_check_true(successBranchTaken); > +} > + > + > +function checkPassValidPin(pinValue, settingPin) { I don't understand the rationale behind this function very well. Perhaps documenting it or splitting it into multiple smaller functions would help. @@ +46,5 @@ > + let hostIsPinned; > + if (settingPin) { > + gSSService.removeState(Ci.nsISiteSecurityService.HEADER_HPKP, uri, 0); > + } > + else { nit: } else { all on one line @@ +48,5 @@ > + gSSService.removeState(Ci.nsISiteSecurityService.HEADER_HPKP, uri, 0); > + } > + else { > + // add a known valid pin! > + let validPinValue ="max-age=5000;" + VALID_PIN1 + INVALID_PIN1 ; nit: unnecessary space before final ';' @@ +82,5 @@ > +const PINNING_ROOT_KEY_HASH ="kXoHD1ZGyMuowchJwy+xgHlzh0kJFoI9KX0o0IrzTps="; > +const MAX_AGE_ZERO = "max-age=0;" > +const VALID_PIN1 = "pin-sha256=\""+ PINNING_ROOT_KEY_HASH + "\";"; > +const INVALID_PIN1 = "pin-sha256=\""+ NON_ISSUED_KEY_HASH1 + "\";"; > +const INVALID_PIN2 = "pin-sha256=\""+ NON_ISSUED_KEY_HASH2 + "\";"; INVALID_PIN is a misleading name. Maybe 'BACKUP_PIN' to use the nomenclature in the spec? This should at least be documented. @@ +112,5 @@ > + checkFailParseInvalidPin("thisisinvalidtest"); > + checkFailParseInvalidPin("invalid" + GOOD_MAX_AGE + VALID_PIN1 + INVALID_PIN1); > + > + checkPassRemovingPin("max-age=0"); > + checkPassRemovingPin(MAX_AGE_ZERO); isn't this the same as the previous test? @@ +124,5 @@ > + checkPassSettingPin(VALID_PIN1 + GOOD_MAX_AGE + INVALID_PIN2 + REPORT_URI + INCLUDE_SUBDOMAINS); > + checkPassSettingPin(INCLUDE_SUBDOMAINS + VALID_PIN1 + GOOD_MAX_AGE + INVALID_PIN2); > + checkPassSettingPin(GOOD_MAX_AGE + VALID_PIN1 + INVALID_PIN1 + UNRECOGNIZED_DIRECTIVE); > + > + do_test_finished(); Do you need do_test_finished if there isn't a do_test_pending?

Try push https://tbpl.mozilla.org/?tree=Try&rev=2d7c9456d218

pushed to inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/aeacfe204b8d https://hg.mozilla.org/integration/mozilla-inbound/rev/f86a4cd9c020

https://hg.mozilla.org/mozilla-central/rev/aeacfe204b8d https://hg.mozilla.org/mozilla-central/rev/f86a4cd9c020

Camilo, from what I can see here https://hg.mozilla.org/mozilla-central/file/f86a4cd9c020/browser/base/content/test/general/browser_blockHPKP.js#l78 you did not address the following comments from my review: (In reply to David Keeler (:keeler) [use needinfo?] from comment #107) > Comment on attachment 8497064 [details] [diff] [review] > new-tests-pinning (v2) > > Review of attachment 8497064 [details] [diff] [review]: > ----------------------------------------------------------------- > > Two main issues: > - How does browser_blockHPKP.js test that the error encountered is the > expected error? ... > ::: browser/base/content/test/general/browser_blockHPKP.js ... > @@ +72,5 @@ > > + buttonClicked: false, > > + onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus) { > > + if (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP) { > > + let self = this; > > + // Can't directly call button.click() in onStateChange > > We're not calling button.click() here - much of this doesn't seem necessary > anymore, unless this is how we ensure that we've navigated to > about:neterror? (although we should be doing that directly, and checking > that the error is the one we expect) In particular, we have no assurance that the about:neterror page encountered is a result of a key pinning failure.

I missed that on a rebase. Let me file a follow up for that (it was a hack, with a string conparisong on the text message). Sorry for the error.

follow up: Bug 1075081

Release Note Request (optional, but appreciated) [Why is this notable]: MOar security [Suggested wording]: Implemented HTTP Public Key Pinning Extension (for enhanced authentication of encrypted connections) [Links (documentation, blog post, etc)]: (blog post? older blogpost about builtins was: https://blog.mozilla.org/security/2014/09/02/public-key-pinning/)

Added to the release notes with Florian's wording (thanks). Is it enabled on mobile too?

Yes, this is enabled on Desktop, Android, and B2G.

It sounds like there's no way to clear this data. So there could potentially be a record of every https site the user ever visited stored in the browser, even if the user visited the site in private browsing mode or has their history set to clear each time they close the browser. This seems like a major privacy issue...?

(In reply to Shawn from comment #118) > It sounds like there's no way to clear this data. So there could potentially > be a record of every https site the user ever visited stored in the browser, > even if the user visited the site in private browsing mode or has their > history set to clear each time they close the browser. This seems like a > major privacy issue...? Shawn the storage for private mode is different from the storage in regular mode. When browsing in regular mode only the regular profile is used, when using private mode both storage locations are queried, so while there is no leaks from private mode into regular mode, there are leaks from regular mode into private browsing.

(In reply to David Keeler [:keeler] (use needinfo?) from comment #117) > Yes, this is enabled on Desktop, Android, and B2G. Is it? [1] is accessible on mobile (even though it shouldn't, desktop shows the correct invalid cert message,mobile doesn't). [1] https://hpkp.scotthelme.co.uk/

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #121) > (In reply to David Keeler [:keeler] (use needinfo?) from comment #117) > > Yes, this is enabled on Desktop, Android, and B2G. > > Is it? [1] is accessible on mobile (even though it shouldn't, desktop shows > the correct invalid cert message,mobile doesn't). > > [1] https://hpkp.scotthelme.co.uk/ That site is accessible on desktop and mobile for me. It looks like when the site tries to set the header, it doesn't include a pin that is valid for the current chain. To prevent sites dossing themselves, the browser does not note the site as HPKP when this happens. Bug 1124649 should help with this by giving more informative error messages in the console.

(In reply to David Keeler [:keeler] (use needinfo?) from comment #122) > (In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #121) > > (In reply to David Keeler [:keeler] (use needinfo?) from comment #117) > > > Yes, this is enabled on Desktop, Android, and B2G. > > > > Is it? [1] is accessible on mobile (even though it shouldn't, desktop shows > > the correct invalid cert message,mobile doesn't). > > > > [1] https://hpkp.scotthelme.co.uk/ > > That site is accessible on desktop and mobile for me. It looks like when the > site tries to set the header, it doesn't include a pin that is valid for the > current chain. To prevent sites dossing themselves, the browser does not > note the site as HPKP when this happens. Bug 1124649 should help with this > by giving more informative error messages in the console. ok, seems on 39 the website is accessible. But on nightly it is not (on desktop and it seems since today on Android as well), so is this a bug then? Just wondering that the behaviour changes here since it's supposed to work since 35.

Try with a new profile? Alternatively, try with tomorrow's Nightly, which should have better messaging in the console.

What would prevent a MITM proxy to rewrite these headers?