Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11274032:
Separate http_security_headers from transport_security_state (Closed)
|
Created:
8 years, 2 months ago by unsafe Modified:
7 years, 11 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, eroman, darin-cc_chromium.org Base URL:
https://src.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionThis is the first in an intended sequence of CLs to refactor
TransportSecurityState, fix some book-keeping bugs, and hopefully add TACK.
This sequence of CLs will be derived from the original, overly-large CL
#11191005.
This CL does a few things:
- Adds a high-level API for processing HSTS/HPKP
- Move the code for handling HSTS/HPKP headers out of transport_security_state
- Move HashValue out of x509_cert_types
- Addresses several HSTS/HPKP parsing bugs identified during review of the cleanup
- Ignore unknown HSTS/HPKP directives
- Ignore unknown hash algorithms
- Handle overly-large (> int64) expirations without parsing issues
- Reject invalid pins entered by users
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175595
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : #
Total comments: 46
Patch Set 6 : #Patch Set 7 : #
Total comments: 12
Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #
Total comments: 51
Patch Set 14 : #
Total comments: 2
Patch Set 15 : #Patch Set 16 : #Patch Set 17 : #Patch Set 18 : #
Total comments: 13
Patch Set 19 : #
Total comments: 32
Patch Set 20 : #
Total comments: 21
Patch Set 21 : #Patch Set 22 : #
Total comments: 20
Patch Set 23 : #
Total comments: 10
Patch Set 24 : #
Total comments: 4
Patch Set 25 : #Patch Set 26 : #Patch Set 27 : #
Total comments: 2
Patch Set 28 : #
Total comments: 1
Patch Set 29 : #Patch Set 30 : #Patch Set 31 : #Patch Set 32 : #Messages
Total messages: 61 (0 generated)
Hi Trevor - first run through on this, highlighting the low-hanging fruits and just getting a feel for the overall code. Thanks much for splitting this up - I'm sure you can see why it'd be much harder with a larger change. Also, I didn't find an executed CLA on file, so please see http://dev.chromium.org/developers/contributing-code#TOC-Get-your-code-ready and see about submitting a CLA. This also includes updating the AUTHORS file as appropriate. I'll try to take another pass "soon", but my biggest high-level design comment would be that I'm not a big fan of putting the Base64/HashValue handing under net/base, nor the PKP parsing. The header parsing should be under net/http, and I think the Base64+tag code belongs somewhere closer to the PKP implementation. This is because the form ('hash/b64_value') is tightly coupled with the PKP spec. If TACK is going to re-use it, it should live tightly coupled to their storage mechanism, rather than general purpose on the HashValue objects. https://codereview.chromium.org/11274032/diff/7006/net/base/x509_cert_types.cc File net/base/x509_cert_types.cc (right): https://codereview.chromium.org/11274032/diff/7006/net/base/x509_cert_types.c... net/base/x509_cert_types.cc:75: if (!hashes_str.empty()) { Rather than structuring code along the if (success) { // do stuff } [else optional error handling] pattern, Chromium/Google Open Source code tries to follow the if (error) { // error handling return } // do stuff style of structure. This helps reduce indentation levels. suggested rewrite: if (hashes_str.empty()) return [something] https://codereview.chromium.org/11274032/diff/7006/net/base/x509_cert_types.c... net/base/x509_cert_types.cc:75: if (!hashes_str.empty()) { design nit: You have not documented what the pre-and-post conditions of this function in the header file, but as such, I'm not sure the current behaviours are a good API. The current API appends data to |hashes|, and returns success when there are no hashes. Typically, we try to make this very clear in the naming of the function, as the general expectation is that it'll replace |hashes|, and also try to make invalid inputs fail. hashes->clear(); if (hashes_str.empty()) return false; // Parse https://codereview.chromium.org/11274032/diff/7006/net/base/x509_cert_types.c... net/base/x509_cert_types.cc:79: for (size_t i = 0; i != type_and_b64s.size(); i++) { style nit: pre-increment (++i) http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Preinc... https://codereview.chromium.org/11274032/diff/7006/net/base/x509_cert_types.c... net/base/x509_cert_types.cc:81: RemoveChars(type_and_b64s[i], " \t\r\n", &type_and_b64); RemoveChars(type_and_b64s[i], " \t\r\n", &type_and_b64s[i]); That said, I believe the real function, based on the spec, should be net::HttpUtil::TrimLWS(), which net/base cannot use or call (layering violation) This is part of why I think this function may belong somewhere under net/http to handle header parsing. https://codereview.chromium.org/11274032/diff/7006/net/base/x509_cert_types.c... net/base/x509_cert_types.cc:202: std::string b64; naming nit: b64 is too abbreviated. Applies throughout the file (eg: Base64StringToHashes) (See http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera... ) perf: use base::StringPiece to avoid needing to make a copy of these values. You can still trim prefix/suffices as possible. https://codereview.chromium.org/11274032/diff/7006/net/base/x509_cert_types.c... net/base/x509_cert_types.cc:223: base::Base64Encode(std::string((const char*)data(), size()), &b64); design nit: Encode expects a base::StringPiece rather than an std::string. base::StringPiece lets you avoid the copy. style nit: Use C++ casts ( http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Castin... ) https://codereview.chromium.org/11274032/diff/7006/net/base/x509_cert_types.c... net/base/x509_cert_types.cc:228: return std::string("unknown/" + b64); nit: Write this as a switch statement, with no "default" case, and an error return after the switch, and the compiler will (should?) warn any time the tag is not handled. Also, NOTREACHED() here, since WriteBase64String() should never be called with an unsupported hash value (Parsing shouldn't use this, because it's user input, but outputting is a coder issue) https://codereview.chromium.org/11274032/diff/7006/net/base/x509_cert_types.h File net/base/x509_cert_types.h (right): https://codereview.chromium.org/11274032/diff/7006/net/base/x509_cert_types.h... net/base/x509_cert_types.h:81: std::string WriteBase64String() const; I feel this is not coupled to the "HashValue" concept, which is used in many more places than pinning, thus as an API, doesn't belong here. I think it is more closely aligned with some sort of pinning structure, and thus belongs in some sort of method for TransportSecurityState or equivalent. https://codereview.chromium.org/11274032/diff/7006/net/base/x509_cert_types.h... net/base/x509_cert_types.h:110: class NET_EXPORT HashValuesEqualPredicate { naming nit: HashValueEquals design nit: Is this so widely useful as to make sense in a common place? I actually had the same concern regarding HashValueLessThan (which is apparently not used anywhere and should thus be removed) https://codereview.chromium.org/11274032/diff/7006/net/base/x509_cert_types.h... net/base/x509_cert_types.h:139: HashValueVector* hashes); BUG?: You don't NET_EXPORT Base64StringToHashes, therefore it cannot be used outside of the "net" component. Broadly speaking though, "Base64String" is an encoding format for PKP pins, and thus does not seem to belong to generic net types. For that matter, it has fundamentally nothing to do with certificates (the hash value stuff has organically grown perhaps unchecked) If this is only used in one or two places, this would be better off kept to the .cc file (eg: not exposed in any header / not exported) that it's used. https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... File net/http/http_security_headers.cc (right): https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... net/http/http_security_headers.cc:18: static bool MaxAgeToInt(std::string::const_iterator begin, design: prefer placing this in an unnamed namespace rather than a static function, which is the recommended Chromium style. This would also allow you to move kMaxHSTSAgeSecs (s/.../kMaxHstsAgeSeconds) into the implementation file, as it does not seem used anywhere outside of here. Also, all of the existing static functions would be moved into this unnamed namespace at the top of the file. https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... net/http/http_security_headers.cc:23: long int i = strtol(s.data(), &endptr, 10 /* base */); BUG: Do not use strtol. Use base::StringToUint64. I do not believe "truncate correctly" is the desired approach, according to the HTTP spec. https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... net/http/http_security_headers.cc:83: while (tokenizer.GetNext()) { DESIGN: Unless I'm misreading the ABNF, this would / could be implemented better as a NameValuePairsIterator, from net/http/http_util.h https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... net/http/http_security_headers.cc:152: // BUG(156147), TODO(palmer): If max_age_candidate == 0, we should nit: TODO(palmer): http://crbug.com/156147 - if max_age_can.... https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... net/http/http_security_headers.cc:219: typedef std::pair<std::string, std::string> StringPair; nit: This would be better as pair<base::StringPiece, base::StringPiece> https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... net/http/http_security_headers.cc:268: while (!source.empty()) { again with NameValuePairsIterator https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... File net/http/http_security_headers.h (right): https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... net/http/http_security_headers.h:20: const long kMaxHSTSAgeSecs = 86400 * 365; // 1 year For seconds, we tend to use either int or use int64 values (see TimeDelta::FromSeconds) https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... net/http/http_security_headers.h:23: // returns true and populates the expiry and include_subdomains values. nit: and sets |*expiry| and and |*include_subdomains|. https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... net/http/http_security_headers.h:31: bool ParseHSTSHeader(const base::Time& now, const std::string& value, style nit: |now| and |value| belong on separate lines, as per http://dev.chromium.org/developers/coding-style#Code_Formatting nit: If you don't actually need to modify |value|, using a base::StringPiece here *may* offer a no-copy implementation of header parsing - which can be used by the HTTP header enumeration logic. https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... net/http/http_security_headers.h:33: bool* include_subdomains); // OUT Neither Google C++ Guide nor Chromium style guide require the use of this // OUT annotation. Instead, it's specified in the comments. https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... net/http/http_security_headers.h:40: // as specified in ssl_info. nit: This API behaviour should be separated out. One function to parse the header. One function to check the validity of the header, based upon |ssl_info| (although really, it should only be necessary to supply a HashValueVector I believe) The latter function belongs with/in the PKP handling logic. https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... File net/http/http_security_headers_unittest.cc (right): https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... net/http/http_security_headers_unittest.cc:27: static bool GetPublicKeyHash(const net::X509Certificate::OSCertHandle& cert, nit: see previous nit regarding unnamed namespaces https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... net/http/http_security_headers_unittest.cc:45: NOTREACHED() << "Unknown HashValueTag " << hash->tag; BUG: return false ? https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... net/http/http_security_headers_unittest.cc:57: reinterpret_cast<char*>(spki_hash.data()), spki_hash.size()), &base64); design: Why not use the existing functions you added to net/base (or wherever the pin code bits land) https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... net/http/http_security_headers_unittest.cc:71: return label + HttpUtil::Quote(base64); nit: It's not required to be quoted, is it? https://codereview.chromium.org/11274032/diff/7006/net/url_request/url_reques... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/11274032/diff/7006/net/url_request/url_reques... net/url_request/url_request_http_job.cc:672: request_->context()->transport_security_state(); style: do not use \ for continuation - not necessary style: four space indent Also line 689-690
Hi Ryan, I responded to your comments and uploaded a new version. My initial goal was that http_security_headers would contain public-key-pinning logic (parsing + IsPinListValid(), IsBackupPinPresent()), and x509_cert_types would contain the generic hashvalues stuff (reading/writing the non-HPKP-header type/base64 format), HashesIntersect(), HashValuesEqual). But it may be better to just have net/http/http_security_headers do header parsing, and move the pinning logic into a new net/base/public_key_pinning? (including IsPinListValid(), TSS:IsChainOfPublicKeysPermitted, and maybe also any of the hashvalues stuff that is judged not generally useful). Also, maybe a net/base/hash_values file should be separated out of x509_cert_types? https://codereview.chromium.org/11274032/diff/7006/net/base/x509_cert_types.cc File net/base/x509_cert_types.cc (right): https://codereview.chromium.org/11274032/diff/7006/net/base/x509_cert_types.c... net/base/x509_cert_types.cc:75: if (!hashes_str.empty()) { On 2012/10/25 01:59:09, Ryan Sleevi wrote: > design nit: > You have not documented what the pre-and-post conditions of this function in the > header file, but as such, I'm not sure the current behaviours are a good API. > > The current API appends data to |hashes|, and returns success when there are no > hashes. Fixed - documented, clears |hashes|, returns true even if none. https://codereview.chromium.org/11274032/diff/7006/net/base/x509_cert_types.c... net/base/x509_cert_types.cc:79: for (size_t i = 0; i != type_and_b64s.size(); i++) { On 2012/10/25 01:59:09, Ryan Sleevi wrote: > style nit: pre-increment (++i) > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Preinc... Done. https://codereview.chromium.org/11274032/diff/7006/net/base/x509_cert_types.c... net/base/x509_cert_types.cc:81: RemoveChars(type_and_b64s[i], " \t\r\n", &type_and_b64); On 2012/10/25 01:59:09, Ryan Sleevi wrote: > RemoveChars(type_and_b64s[i], " \t\r\n", &type_and_b64s[i]); > > That said, I believe the real function, based on the spec, should be > net::HttpUtil::TrimLWS(), which net/base cannot use or call (layering violation) This function isn't processing the HTTP header, it process net_internals input currently, and could be used for unit tests or debugging, so I think its OK at this level. https://codereview.chromium.org/11274032/diff/7006/net/base/x509_cert_types.c... net/base/x509_cert_types.cc:202: std::string b64; On 2012/10/25 01:59:09, Ryan Sleevi wrote: > naming nit: b64 is too abbreviated. Applies throughout the file (eg: > Base64StringToHashes) Changed the names. > perf: use base::StringPiece to avoid needing to make a copy of these values. It's about the same efficiency as the previous TransportSecurityState::ParsePin. I'm not familiar with base::StringPiece. This is used to read the dynamic JSON, so I suppose efficiency matters slightly, can you give me more pointers? https://codereview.chromium.org/11274032/diff/7006/net/base/x509_cert_types.c... net/base/x509_cert_types.cc:223: base::Base64Encode(std::string((const char*)data(), size()), &b64); On 2012/10/25 01:59:09, Ryan Sleevi wrote: > design nit: Encode expects a base::StringPiece rather than an std::string. > base::StringPiece lets you avoid the copy. > > style nit: Use C++ casts ( > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Castin... > ) Done. https://codereview.chromium.org/11274032/diff/7006/net/base/x509_cert_types.c... net/base/x509_cert_types.cc:228: return std::string("unknown/" + b64); On 2012/10/25 01:59:09, Ryan Sleevi wrote: > nit: Write this as a switch statement, with no "default" case, and an error > return after the switch, and the compiler will (should?) warn any time the tag > is not handled. Changed to a switch. The existing code in this file does not folow the "no default case" idiom, so I didn't. Should all the switch statements be changed? https://codereview.chromium.org/11274032/diff/7006/net/base/x509_cert_types.h File net/base/x509_cert_types.h (right): https://codereview.chromium.org/11274032/diff/7006/net/base/x509_cert_types.h... net/base/x509_cert_types.h:81: std::string WriteBase64String() const; On 2012/10/25 01:59:09, Ryan Sleevi wrote: > I think it is more closely aligned with some sort of pinning structure, and thus > belongs in some sort of method for TransportSecurityState or equivalent. Note that the type/base64 format is *not* the HPKP header format (which is pin-type="base64"). The type/base64 format is used for logging warning messages in TSS, for net-internals query/user-entry, and for dynamic JSON. I think that to the extent that HashValue and HashValueVector are generic concepts, it makes sense to have some way to read/write them as strings - for debugging/unit-testing if nothing else - so I don't feel this is overly tied to pinning.. It might be good to separate the HashValue* stuff out of x509_cert_types, though... https://codereview.chromium.org/11274032/diff/7006/net/base/x509_cert_types.h... net/base/x509_cert_types.h:110: class NET_EXPORT HashValuesEqualPredicate { Deleted HashValueLessThan. HashValueEqualsPredicate is used in this x509_cert_types.cc and http_security_headers.cc in this patch (for HashesIntersect() and IsBackupPinPresent()) but arguably all that pinning logic should be in its own file (instead of split between here and http_security_headers)... https://codereview.chromium.org/11274032/diff/7006/net/base/x509_cert_types.h... net/base/x509_cert_types.h:139: HashValueVector* hashes); On 2012/10/25 01:59:09, Ryan Sleevi wrote: > BUG?: You don't NET_EXPORT Base64StringToHashes, therefore it cannot be used Fixed. > If this is only used in one or two places, this would be better off kept to the > .cc file (eg: not exposed in any header / not exported) that it's used. HashesToBase64String is used in a couple places: net-internals for queries and TSS for warning messages. Base64StringToHashes is used for net-internals user-entry, but it probably should also be used for adding more HPKP unit tests. https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... File net/http/http_security_headers.cc (right): https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... net/http/http_security_headers.cc:18: static bool MaxAgeToInt(std::string::const_iterator begin, All of these things were as-before, just moved into a new file instead of TransportSecurityState, and with the function parameters changed slightly. kMaxHstsAgeSecs is also used in the unit tests. Anyways, can I leave these touchups for someone else to handle? https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... File net/http/http_security_headers.h (right): https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... net/http/http_security_headers.h:20: const long kMaxHSTSAgeSecs = 86400 * 365; // 1 year This was existing code; changed to int64. https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... net/http/http_security_headers.h:23: // returns true and populates the expiry and include_subdomains values. On 2012/10/25 01:59:09, Ryan Sleevi wrote: > nit: and sets |*expiry| and and |*include_subdomains|. Done. https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... net/http/http_security_headers.h:31: bool ParseHSTSHeader(const base::Time& now, const std::string& value, On 2012/10/25 01:59:09, Ryan Sleevi wrote: > style nit: |now| and |value| belong on separate lines, as per > http://dev.chromium.org/developers/coding-style#Code_Formatting What rule applies here? Couldn't figure it out! > nit: If you don't actually need to modify |value|, using a base::StringPiece > here *may* offer a no-copy implementation of header parsing - which can be used This was as-before, I don't know the conventions regarding std::string vs StringPiece? https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... net/http/http_security_headers.h:33: bool* include_subdomains); // OUT Should I remove them? https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... net/http/http_security_headers.h:40: // as specified in ssl_info. I think I agree, this was how the function was before, but it makes sense to move the parsing into net/http and keep the logic elsewhere... https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... File net/http/http_security_headers_unittest.cc (right): https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... net/http/http_security_headers_unittest.cc:27: static bool GetPublicKeyHash(const net::X509Certificate::OSCertHandle& cert, This is pre-existing code moved from TSS_unit_test with some search-and-replace for names and arguments. So, I'd rather leave that sort of change for someone else, if OK. https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... net/http/http_security_headers_unittest.cc:45: NOTREACHED() << "Unknown HashValueTag " << hash->tag; On 2012/10/25 01:59:09, Ryan Sleevi wrote: > BUG: return false ? Pre-existing issue (but fixed). https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... net/http/http_security_headers_unittest.cc:57: reinterpret_cast<char*>(spki_hash.data()), spki_hash.size()), &base64); On 2012/10/25 01:59:09, Ryan Sleevi wrote: > design: Why not use the existing functions you added to net/base (or wherever Those functions use the type/base64 format, not the pin-<type>="base64" format. https://codereview.chromium.org/11274032/diff/7006/net/http/http_security_hea... net/http/http_security_headers_unittest.cc:71: return label + HttpUtil::Quote(base64); On 2012/10/25 01:59:09, Ryan Sleevi wrote: > nit: It's not required to be quoted, is it? Not my code - the base64 is required to have quotes around it, at least. https://codereview.chromium.org/11274032/diff/7006/net/url_request/url_reques... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/11274032/diff/7006/net/url_request/url_reques... net/url_request/url_request_http_job.cc:672: request_->context()->transport_security_state(); On 2012/10/25 01:59:09, Ryan Sleevi wrote: > style: do not use \ for continuation - not necessary > style: four space indent > > Also line 689-690 Done.
Fixed a couple try errors I think (added NET_EXPORT_PRIVATE) to http_security_headers. Not sure what's going on with this one: http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...
You're failing on Android for the reasons in the unit test that I point out below. https://codereview.chromium.org/11274032/diff/16001/net/http/http_security_he... File net/http/http_security_headers_unittest.cc (right): https://codereview.chromium.org/11274032/diff/16001/net/http/http_security_he... net/http/http_security_headers_unittest.cc:27: static bool GetPublicKeyHash(const net::X509Certificate::OSCertHandle& cert, style nit: see previous comments re: unnamed namespaces vs static style nit: omit net:: from these values, since you're within net:: (line 22) https://codereview.chromium.org/11274032/diff/16001/net/http/http_security_he... net/http/http_security_headers_unittest.cc:376: scoped_ptr<CertVerifier> verifier(CertVerifier::CreateDefault()); There's no need to use a real CertVerifier - ergo, no need to use a ScopedTestRoot. Such code is *completely* orthogonal to what is being tested here. You're just testing header code - so you should not need to do anything at all with certificate validation. All of the test setup from like 382 and above is thus unrelated to this test. https://codereview.chromium.org/11274032/diff/16001/net/http/http_security_he... net/http/http_security_headers_unittest.cc:386: std::string good_pin = GetPinFromCert(ssl_info.cert, /*tag*/HASH_VALUE_SHA1); style nit: Don't do this (/*tag*/) BUG? You don't respect |tag| (line 344), thus don't end up testing SHA-256 https://codereview.chromium.org/11274032/diff/16001/net/http/http_security_he... net/http/http_security_headers_unittest.cc:392: HttpUtil::Quote("6dcfXufJLW3J6S/9rRe4vUlBj5g="); There is no need to construct this as a string with quotes - just use a const char[] kBackupPin constant https://codereview.chromium.org/11274032/diff/16001/net/http/http_security_he... net/http/http_security_headers_unittest.cc:434: std::min(kMaxHSTSAgeSecs, (int64)39408299l)); BUG: Do not create int64 constants like this. They do not work reliably cross-platform. Use GG_INT64_C (src/base/port.h) style: Even if the above was valid, the further bug would be assuming l would be sufficient for int64, and using a C-style cast rather than a C++ style cast
See below - the nits are fixed but I'm still not sure why verifier->Verify() failed on Android, as I didn't change that code, just moved it. (Should I just try to hardcode some hashes into ssl_info and rip out the CertVerifier stuff?) https://codereview.chromium.org/11274032/diff/16001/net/http/http_security_he... File net/http/http_security_headers_unittest.cc (right): https://codereview.chromium.org/11274032/diff/16001/net/http/http_security_he... net/http/http_security_headers_unittest.cc:27: static bool GetPublicKeyHash(const net::X509Certificate::OSCertHandle& cert, On 2012/10/29 09:01:24, Ryan Sleevi wrote: > style nit: see previous comments re: unnamed namespaces vs static > style nit: omit net:: from these values, since you're within net:: (line 22) Fixed (this was pre-existing code, fwiw). https://codereview.chromium.org/11274032/diff/16001/net/http/http_security_he... net/http/http_security_headers_unittest.cc:376: scoped_ptr<CertVerifier> verifier(CertVerifier::CreateDefault()); On 2012/10/29 09:01:24, Ryan Sleevi wrote: > There's no need to use a real CertVerifier - ergo, no need to use a > ScopedTestRoot. > > Such code is *completely* orthogonal to what is being tested here. You're just > testing header code - so you should not need to do anything at all with > certificate validation. > > All of the test setup from like 382 and above is thus unrelated to this test. This code is needed to setup the ssl_info, isn't it? This entire test was just moved from TSS_unittest with minimal changes to remove DomainState, all of the setup stuff is unchanged, so I don't know why 381 would fail on Android. Did TSS_unittest used to work? Is ImportCertFromFile() somehow different in this context? https://codereview.chromium.org/11274032/diff/16001/net/http/http_security_he... net/http/http_security_headers_unittest.cc:386: std::string good_pin = GetPinFromCert(ssl_info.cert, /*tag*/HASH_VALUE_SHA1); On 2012/10/29 09:01:24, Ryan Sleevi wrote: > style nit: Don't do this (/*tag*/) > BUG? You don't respect |tag| (line 344), thus don't end up testing SHA-256 Fixed (pre-existing). https://codereview.chromium.org/11274032/diff/16001/net/http/http_security_he... net/http/http_security_headers_unittest.cc:392: HttpUtil::Quote("6dcfXufJLW3J6S/9rRe4vUlBj5g="); On 2012/10/29 09:01:24, Ryan Sleevi wrote: > There is no need to construct this as a string with quotes - just use a const > char[] kBackupPin constant Fixed - changed to string literals (preexisting).. https://codereview.chromium.org/11274032/diff/16001/net/http/http_security_he... net/http/http_security_headers_unittest.cc:434: std::min(kMaxHSTSAgeSecs, (int64)39408299l)); On 2012/10/29 09:01:24, Ryan Sleevi wrote: > BUG: Do not create int64 constants like this. They do not work reliably > cross-platform. Use GG_INT64_C (src/base/port.h) > > style: Even if the above was valid, the further bug would be assuming l would be > sufficient for int64, and using a C-style cast rather than a C++ style cast Fixed.
I'll take another pass tomorrow, but I'm running out of nits, and that's a great sign :) https://codereview.chromium.org/11274032/diff/16001/net/http/http_security_he... File net/http/http_security_headers_unittest.cc (right): https://codereview.chromium.org/11274032/diff/16001/net/http/http_security_he... net/http/http_security_headers_unittest.cc:376: scoped_ptr<CertVerifier> verifier(CertVerifier::CreateDefault()); On 2012/10/29 22:30:55, unsafe wrote: > On 2012/10/29 09:01:24, Ryan Sleevi wrote: > > There's no need to use a real CertVerifier - ergo, no need to use a > > ScopedTestRoot. > > > > Such code is *completely* orthogonal to what is being tested here. You're just > > testing header code - so you should not need to do anything at all with > > certificate validation. > > > > All of the test setup from like 382 and above is thus unrelated to this test. > > This code is needed to setup the ssl_info, isn't it? This entire test was just > moved from TSS_unittest with minimal changes to remove DomainState, all of the > setup stuff is unchanged, so I don't know why 381 would fail on Android. Did > TSS_unittest used to work? Is ImportCertFromFile() somehow different in this > context? > I believe TSS_unittest is excluded from compilation on Android. I pointed out to palmer@ that we shouldn't be using the CertVerifier to do this, and should just be mocking the class. Looks like you got caught in the acquired technical debt - which is why I'm being admittedly so picky regarding new debt :-/ As to why it doesn't work on Android, in general, pinning and TACK do not and will not work on (real) android builds. This is because Android does not have any native Java API for certificate verification that can return the constructed certificate chain (on success or error). Since we use the Java APIs for cert validation on Android, this means that pinning/TACK will not be able to be implemented on Chrome for Android until Android exposes additional APIs. The best I can say at this point is the relevant bugs are filed and the appropriate teams are aware of this. Using a manually created/fake ssl_info will thus cause Android to succeed, because it's not limited by the Android cert verification. Alternatively, using a MockCertVerifier will bypass the system verification routines, and thus will also not be limited by the Android cert verification. Hence why I said to rip it all out :) https://codereview.chromium.org/11274032/diff/16001/net/http/http_security_he... net/http/http_security_headers_unittest.cc:386: std::string good_pin = GetPinFromCert(ssl_info.cert, /*tag*/HASH_VALUE_SHA1); On 2012/10/29 22:30:55, unsafe wrote: > On 2012/10/29 09:01:24, Ryan Sleevi wrote: > > style nit: Don't do this (/*tag*/) > > BUG? You don't respect |tag| (line 344), thus don't end up testing SHA-256 > > Fixed (pre-existing). Dang. On the upside, spotting all these pre-existing bugs and nits makes for a more compelling refactoring argument :)
On 2012/10/29 23:46:25, Ryan Sleevi wrote: > Hence why I said to rip it all out :) Ah, ok, took it out, simplifies things...
Recovered from IETF, back in action... Per conversation w/rsleevi did a bit more refactoring: - separated hash_value.cc/.h from x509_cert_types - renamed HashValue's serialization funcs to ToString/FromString * We had discussed moving IsPinListValid / IsBackupPinPresent from http_security_headers back to TSS, but then the http_security_headers_unittests can't do testing with different chain inputs, and there's more stuff cluttering TSS, so I changed it a little (to take a HashValueVector input instead of ssl_info), but left those checks in http_security_headers. - moved some of the more special-case hash-value stuff into local functions in the .cc files where its being used, in particular HashValuesToBase64 / Base64ToHashValues were moved into net_internals and TSS, and HashesIntersect into TSS and http_security_headers. This duplicates a bit of code. * Rsleevi may have mentioned that HPKP header processing in url_request_http_job should parse all HPKP headers, instead of just the first? I don't remember exactly, and I'm not sure if the IsPinListValid() logic should apply to all headers or their union or something, so I left this as-is...
> * Rsleevi may have mentioned that HPKP header processing in > url_request_http_job should parse all HPKP headers, instead of just the first? > I don't remember exactly, and I'm not sure if the IsPinListValid() logic should > apply to all headers or their union or something, so I left this as-is... The rule for HSTS is that *only* the first header should be parsed — we fixed that bug :) — and so perhaps we should specify that HPKP has the same behavior. I'd like for HPKP to mirror HSTS as much as possible, where doing makes sense. I think this is one such case. I'm working on the HPKP draft today so I'll add that. For now, Trevor should not have to worry about that particular issue for this CL. Whatever behavior we specify for HPKP, ensuring that we follow the spec will be my bug. I think Trevor has dealt with enough of my legacy junk in this CL already. :)
will follow up with palmer@ re: coalescing or not. You can choose to interpret that one comment re: GetNormalizedHeader as appropriate. https://codereview.chromium.org/11274032/diff/40006/chrome/browser/ui/webui/n... File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): https://codereview.chromium.org/11274032/diff/40006/chrome/browser/ui/webui/n... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1196: Base64StringToHashes(hashes_str, &state.dynamic_spki_hashes); In the original code, if a hash failed to parse, it was skipped, and the next hash used (line 1192) In the new code, if a hash fails to parse, the entire parsing stops and false is returned (line 1126) I suspect the original behaviour was more desirable - keep on parsing - especially given that you don't handle a false return here. https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.cc File net/base/hash_value.cc (right): https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.cc#ne... net/base/hash_value.cc:33: std::string base64_str; nit: (minor) You can use a base::StringPiece here, and avoid a copy https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.cc#ne... net/base/hash_value.cc:39: base64_str = value.substr(7, 44); // length of base64 string DESIGN: By taking the fixed length .substr() of a base64 string, you're discarding any potentially invalid inputs from the tail of the string. This potentially creates a place for implementation variance - where a string of 44 'A's followed by 'bababooey' would be accepted here, but perhaps rejected by another implementation. My inclination is that you should consider the entire string, and not truncate at all. https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.cc#ne... net/base/hash_value.cc:65: } nit: the case labels of switch statements get indented one space ( http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Loops_... ) https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.cc#ne... net/base/hash_value.cc:102: namespace { This should be moved to ~line 17 (unnamed namespaces = top of file) https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.h File net/base/hash_value.h (right): https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.h#new... net/base/hash_value.h:19: // Types (SHA1HashValue, SHA256HashValue, HashValue, HashValueVector) Drop this comment (and the other 'sectional' notations - eg: 72, 103) https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.h#new... net/base/hash_value.h:52: // Parse/write in this format: "sha1/Guzek9lMwR3KeIS8wwS9gBvVtIg=" nit: Can you expand on the comments for these methods a bit, based on http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... ? Things to mention: 1) what the form of the string is (eg: <hash-name>/<hash-value>" 2) Is this intended to be a stable, serialized form - or simply a logging form 3) What the state of the object is on error (FromString) 4) What the state of the output string is on error (ToString) https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.h#new... net/base/hash_value.h:53: bool FromString(const std::string& input); API: suggestion: Use (const base::StringPiece& input) instead. https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.h#new... net/base/hash_value.h:74: class NET_EXPORT SHA1HashValueLessThan { As far as NET_EXPORT goes, I don't believe any of the following classes need to be NET_EXPORTed - that is, they're not used outside of the 'net' module, ergo no need to export them. Keeping it internal to the module is more of a check to make sure the right abstractions are being exposed to those who depend on the net module. It may be better to address this in a follow-up, just to keep to a minimum changes when moving code between files (makes it easier to code spleunk the diffs to keep changes minimal when moving) https://codereview.chromium.org/11274032/diff/40006/net/base/transport_securi... File net/base/transport_security_state_static_generate.go (right): https://codereview.chromium.org/11274032/diff/40006/net/base/transport_securi... net/base/transport_security_state_static_generate.go:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. agl should double check the go code. I do think it's fine to include, and is no different than our inclusion of gperf-generated files for the PSL https://codereview.chromium.org/11274032/diff/40006/net/http/http_security_he... File net/http/http_security_headers.cc (right): https://codereview.chromium.org/11274032/diff/40006/net/http/http_security_he... net/http/http_security_headers.cc:27: return false; I'm pretty sure this whole code is a pre-existing bug waiting to happen base::StringToUInt64 best matches the 2616 MaxAge spec, AIUI. https://codereview.chromium.org/11274032/diff/40006/net/http/http_security_he... net/http/http_security_headers.cc:61: } As mentioned in person, I'm (lightly) concerned with the pessimissm of these algorithms. It'd be nice to have things pre-sorted and quickly intersected, but that may be just a TODO for the future... I'm mentioning it now mostly so I don't forget myself (and in case palmer gets motivated) https://codereview.chromium.org/11274032/diff/40006/net/http/http_security_he... net/http/http_security_headers.cc:82: } nit: // namespace https://codereview.chromium.org/11274032/diff/40006/net/http/http_security_he... net/http/http_security_headers.cc:262: } nit: All of these static functions (and the typedef) would be better in the unnamed namespace above. https://codereview.chromium.org/11274032/diff/40006/net/http/http_security_he... net/http/http_security_headers.cc:300: return false; Should these be returning false? On encountering an unrecognized algorithm, shouldn't the behaviour be to skip the pin, rather than fail pinning entirely? That is, line 300 should be a continue, not a return, the same as for unknown directives (line 306) https://codereview.chromium.org/11274032/diff/40006/net/http/http_security_he... File net/http/http_security_headers.h (right): https://codereview.chromium.org/11274032/diff/40006/net/http/http_security_he... net/http/http_security_headers.h:32: base::Time* expiry, // OUT drop the "// OUT" https://codereview.chromium.org/11274032/diff/40006/net/http/http_security_he... net/http/http_security_headers.h:40: // as specified in chain_hashes. nit: Could you expand on this comment more. In particular, what does "check" mean in this case // In order for a Public-Key-Pins header to be valid, at least one pin must be present in |chain_hashes|. (or something like that) Additionally, it would be good to clarify that |value| is the normalized header value (eg: coalesced). This is important, as Public-Key-Pins: max-age=1234; Public-Key-Pins: pin-sha1=foo; Public-Key-Pins: pin-sha256=bar Is a valid sequence of headers, and is equivalent to Public-Key-Pins: max-age=1234; pin-sha1=foo; pin-sha256=bar https://codereview.chromium.org/11274032/diff/40006/net/url_request/url_reque... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/11274032/diff/40006/net/url_request/url_reque... net/url_request/url_request_http_job.cc:708: security_state->AddHPKPHeader(request_info_.url.host(), value, ssl_info); Pretty sure that HPKP should be using HasHeader && GetNormalizedHeader, so that the headers are coalesced
https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.cc File net/base/hash_value.cc (right): https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.cc#ne... net/base/hash_value.cc:117: return NULL != bsearch(hash.data, array, arraylen, base::kSHA1Length, Perhaps it's better to use std::binary_search and <algorithm> instead of bsearch and <cstdlib>, for maximum C++ "goodness" ? https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.h File net/base/hash_value.h (right): https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.h#new... net/base/hash_value.h:103: // Misc hash-related funcs NIT: s/funcs/functions./ https://codereview.chromium.org/11274032/diff/40006/net/base/transport_securi... File net/base/transport_security_state_static_generate.go (right): https://codereview.chromium.org/11274032/diff/40006/net/base/transport_securi... net/base/transport_security_state_static_generate.go:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. This poor file. I think it got booted by policy, and that's why agl moved it to GitHub. (https://github.com/agl/transport-security-state-generate) I'd really prefer to see it here in the Chromium code base, of course, but I think we already lost that fight? Anyway, the byte array vs. base 64 string issue is not one I have a strong opinion on, but I seem to recall agl preferred the base 64. But, let's see what he says. https://codereview.chromium.org/11274032/diff/40006/net/http/http_security_he... File net/http/http_security_headers.cc (right): https://codereview.chromium.org/11274032/diff/40006/net/http/http_security_he... net/http/http_security_headers.cc:61: } By "pessimism", do you mean non-optimalness, or their generally dismal worldview? Perhaps we could use sorted arrays, sure; or even a real std::set. Maybe we should be using std::set throughout, to represent pins. ...in another CL. For now, this is OK. https://codereview.chromium.org/11274032/diff/40006/net/http/http_security_he... net/http/http_security_headers.cc:300: return false; On 2012/11/13 19:02:32, Ryan Sleevi wrote: > Should these be returning false? > > On encountering an unrecognized algorithm, shouldn't the behaviour be to skip > the pin, rather than fail pinning entirely? > > That is, line 300 should be a continue, not a return, the same as for unknown > directives (line 306) Yeah, that is probably true. https://codereview.chromium.org/11274032/diff/40006/net/http/http_security_he... File net/http/http_security_headers.h (right): https://codereview.chromium.org/11274032/diff/40006/net/http/http_security_he... net/http/http_security_headers.h:40: // as specified in chain_hashes. Well, we might decide against allowing coalescing, as we did for HSTS. I am not sure if I care either way, but consistency with HSTS seems like a good way to decide. https://codereview.chromium.org/11274032/diff/40006/net/url_request/url_reque... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/11274032/diff/40006/net/url_request/url_reque... net/url_request/url_request_http_job.cc:708: security_state->AddHPKPHeader(request_info_.url.host(), value, ssl_info); > Pretty sure that HPKP should be using HasHeader && GetNormalizedHeader, so that > the headers are coalesced As mentioned in previous comments, I tend to think we should match HSTS behavior, i.e. not coalesce. Unless we strongly feel that HSTS was wrong to specify that?
https://codereview.chromium.org/11274032/diff/40006/net/url_request/url_reque... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/11274032/diff/40006/net/url_request/url_reque... net/url_request/url_request_http_job.cc:708: security_state->AddHPKPHeader(request_info_.url.host(), value, ssl_info); On 2012/11/13 19:35:04, Chris P. wrote: > > Pretty sure that HPKP should be using HasHeader && GetNormalizedHeader, so > that > > the headers are coalesced > > As mentioned in previous comments, I tend to think we should match HSTS > behavior, i.e. not coalesce. Unless we strongly feel that HSTS was wrong to > specify that? RFC 2616 4.2 is what causes the problems "Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list [i.e., #(values)]. It MUST be possible to combine the multiple header fields into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field-value to the first, each separated by a comma. The order in which header fields with the same field-name are received is therefore significant to the interpretation of the combined field value, and thus a proxy MUST NOT change the order of these field values when a message is forwarded." So it looks like I'm wrong here, because we're using ";" as the separator rather than ",". I seem to recall Julian Reschke pointed this out on the list, and I also seem to recall Adam Barth having an opinion on the ABNF of HSTS. So yeah, it seems like only one PKP header may be present. I'm not sure what the security motivations of that are/were, but c'est le vie.
> So yeah, it seems like only one PKP header may be present. I'm not sure > what the security motivations of that are/were, but c'est le vie. The main attack I can imagine is, if a web app has a header injection vulnerability, (a) they're doomed anyway, of course; and (b) the attacker could add bad pins in additional headers. We would like for attackers to have to completely compromise the server (i.e. get root on it) to achieve bad pinning; header injection is too low a bar for such a potentially damaging attack.
OK, I think I addressed all comments. Open questions: - do you really want 1-space indents for switch cases? - Should I replace std::string with StringPiece in a couple places? - How should unrecognized HPKP hash algos be handled? https://codereview.chromium.org/11274032/diff/40006/chrome/browser/ui/webui/n... File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): https://codereview.chromium.org/11274032/diff/40006/chrome/browser/ui/webui/n... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1196: Base64StringToHashes(hashes_str, &state.dynamic_spki_hashes); On 2012/11/13 19:02:32, Ryan Sleevi wrote: > In the original code, if a hash failed to parse, it was skipped, and the next > hash used (line 1192) > > In the new code, if a hash fails to parse, the entire parsing stops and false is > returned (line 1126) > > I suspect the original behaviour was more desirable - keep on parsing - > especially given that you don't handle a false return here. Hmm, how about if it handles the false return by NOT calling EnableHost. I think that's better than silently setting an incomplete pin? https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.cc File net/base/hash_value.cc (right): https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.cc#ne... net/base/hash_value.cc:33: std::string base64_str; On 2012/11/13 19:02:32, Ryan Sleevi wrote: > nit: (minor) You can use a base::StringPiece here, and avoid a copy Leaving std::string's in-place for now as I don't have a good understanding of StringPiece. https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.cc#ne... net/base/hash_value.cc:39: base64_str = value.substr(7, 44); // length of base64 string On 2012/11/13 19:02:32, Ryan Sleevi wrote: > DESIGN: By taking the fixed length .substr() of a base64 string, you're > discarding any potentially invalid inputs from the tail of the string. This > potentially creates a place for implementation variance - where a string of 44 > 'A's followed by 'bababooey' would be accepted here, but perhaps rejected by > another implementation. > > My inclination is that you should consider the entire string, and not truncate > at all. Done. https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.cc#ne... net/base/hash_value.cc:65: } On 2012/11/13 19:02:32, Ryan Sleevi wrote: > nit: the case labels of switch statements get indented one space ( > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Loops_... > ) Hm, where does it say that? I see it saying 2-space indent? https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.cc#ne... net/base/hash_value.cc:102: namespace { On 2012/11/13 19:02:32, Ryan Sleevi wrote: > This should be moved to ~line 17 (unnamed namespaces = top of file) Done. https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.cc#ne... net/base/hash_value.cc:117: return NULL != bsearch(hash.data, array, arraylen, base::kSHA1Length, On 2012/11/13 19:35:04, Chris P. wrote: > Perhaps it's better to use std::binary_search and <algorithm> instead of bsearch > and <cstdlib>, for maximum C++ "goodness" ? Maybe - I'm leaving this as-is, if that's OK. This is only used in 2 places (cert_verify_proc_win and cert_verify_proc_mac). I wonder if it should just be duplicated into those .cc files, and removed from hash_value.h/.cc? https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.h File net/base/hash_value.h (right): https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.h#new... net/base/hash_value.h:19: // Types (SHA1HashValue, SHA256HashValue, HashValue, HashValueVector) On 2012/11/13 19:02:32, Ryan Sleevi wrote: > Drop this comment (and the other 'sectional' notations - eg: 72, 103) Done. https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.h#new... net/base/hash_value.h:52: // Parse/write in this format: "sha1/Guzek9lMwR3KeIS8wwS9gBvVtIg=" On 2012/11/13 19:02:32, Ryan Sleevi wrote: > nit: Can you expand on the comments for these methods a bit, based on > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... > ? > > Things to mention: > 1) what the form of the string is (eg: <hash-name>/<hash-value>" > 2) Is this intended to be a stable, serialized form - or simply a logging form > 3) What the state of the object is on error (FromString) > 4) What the state of the output string is on error (ToString) Done. https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.h#new... net/base/hash_value.h:53: bool FromString(const std::string& input); On 2012/11/13 19:02:32, Ryan Sleevi wrote: > API: suggestion: Use (const base::StringPiece& input) instead. I don't know what the conventions are around std::string vs StringPiece, so I'm inclined to just leave this... https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.h#new... net/base/hash_value.h:74: class NET_EXPORT SHA1HashValueLessThan { On 2012/11/13 19:02:32, Ryan Sleevi wrote: > As far as NET_EXPORT goes, I don't believe any of the following classes need to > be NET_EXPORTed I removed NET_EXPORT from this whole file, apparently nothing needs it... https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.h#new... net/base/hash_value.h:103: // Misc hash-related funcs On 2012/11/13 19:35:04, Chris P. wrote: > NIT: s/funcs/functions./ Removed entire (sectional) comment. https://codereview.chromium.org/11274032/diff/40006/net/base/transport_securi... File net/base/transport_security_state_static_generate.go (right): https://codereview.chromium.org/11274032/diff/40006/net/base/transport_securi... net/base/transport_security_state_static_generate.go:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2012/11/13 19:35:04, Chris P. wrote: > This poor file. I think it got booted by policy, and that's why agl moved it to > GitHub. (https://github.com/agl/transport-security-state-generate) I'd really > prefer to see it here in the Chromium code base, of course, but I think we > already lost that fight? > > Anyway, the byte array vs. base 64 string issue is not one I have a strong > opinion on, but I seem to recall agl preferred the base 64. But, let's see what > he says. I discussed with AGL in-person, and I think he was OK with bytearray v base64. It simplifies the Chromium code (see TSS:AddHash) and means that every lookup to a preload-pinned site doesn't need to base64-decode a bunch of hashes. https://codereview.chromium.org/11274032/diff/40006/net/http/http_security_he... File net/http/http_security_headers.cc (right): https://codereview.chromium.org/11274032/diff/40006/net/http/http_security_he... net/http/http_security_headers.cc:27: return false; On 2012/11/13 19:02:32, Ryan Sleevi wrote: > I'm pretty sure this whole code is a pre-existing bug waiting to happen > > base::StringToUInt64 best matches the 2616 MaxAge spec, AIUI. Its not me, I'm just the mover... but I can plug in StringToUInt64 if you want.. https://codereview.chromium.org/11274032/diff/40006/net/http/http_security_he... net/http/http_security_headers.cc:61: } On 2012/11/13 19:35:04, Chris P. wrote: > By "pessimism", do you mean non-optimalness, or their generally dismal > worldview? Perhaps we could use sorted arrays, sure; or even a real std::set. > Maybe we should be using std::set throughout, to represent pins. > > ...in another CL. For now, this is OK. OK, I imagine IsPinListValid could perhaps combine looking for the backup and live pin in some more efficient way, dont know if these vectors are large enough for it to be worthwhile... https://codereview.chromium.org/11274032/diff/40006/net/http/http_security_he... net/http/http_security_headers.cc:82: } On 2012/11/13 19:02:32, Ryan Sleevi wrote: > nit: // namespace Done. https://codereview.chromium.org/11274032/diff/40006/net/http/http_security_he... net/http/http_security_headers.cc:262: } On 2012/11/13 19:02:32, Ryan Sleevi wrote: > nit: All of these static functions (and the typedef) would be better in the > unnamed namespace above. Done. https://codereview.chromium.org/11274032/diff/40006/net/http/http_security_he... net/http/http_security_headers.cc:300: return false; On 2012/11/13 19:35:04, Chris P. wrote: > On 2012/11/13 19:02:32, Ryan Sleevi wrote: > > Should these be returning false? > > > > On encountering an unrecognized algorithm, shouldn't the behaviour be to skip > > the pin, rather than fail pinning entirely? I think thats a design decision for HPKP, the server might expect the header to be set atomically, and if you drop some pins you could be omitting backup pins or pins that are needed by some servers. But, you could specify that different hash algos must redundantly specify the exact same pins, just with different algos, and the client is free to accept whichever algo it chooses? https://codereview.chromium.org/11274032/diff/40006/net/http/http_security_he... File net/http/http_security_headers.h (right): https://codereview.chromium.org/11274032/diff/40006/net/http/http_security_he... net/http/http_security_headers.h:32: base::Time* expiry, // OUT On 2012/11/13 19:02:32, Ryan Sleevi wrote: > drop the "// OUT" Done. https://codereview.chromium.org/11274032/diff/40006/net/http/http_security_he... net/http/http_security_headers.h:40: // as specified in chain_hashes. On 2012/11/13 19:35:04, Chris P. wrote: > Well, we might decide against allowing coalescing, as we did for HSTS. I am not > sure if I care either way, but consistency with HSTS seems like a good way to > decide. Expanded the comment more. Did not say anything about coalescing, as I think that was resolved against doing it? https://codereview.chromium.org/11274032/diff/40006/net/url_request/url_reque... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/11274032/diff/40006/net/url_request/url_reque... net/url_request/url_request_http_job.cc:708: security_state->AddHPKPHeader(request_info_.url.host(), value, ssl_info); On 2012/11/13 19:43:15, Ryan Sleevi wrote: > but c'est le vie. I like that sentiment, if it means what I think which is: Done.
https://codereview.chromium.org/11274032/diff/40006/chrome/browser/ui/webui/n... File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): https://codereview.chromium.org/11274032/diff/40006/chrome/browser/ui/webui/n... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1196: Base64StringToHashes(hashes_str, &state.dynamic_spki_hashes); On 2012/11/13 23:20:18, unsafe wrote: > On 2012/11/13 19:02:32, Ryan Sleevi wrote: > > In the original code, if a hash failed to parse, it was skipped, and the next > > hash used (line 1192) > > > > In the new code, if a hash fails to parse, the entire parsing stops and false > is > > returned (line 1126) > > > > I suspect the original behaviour was more desirable - keep on parsing - > > especially given that you don't handle a false return here. > > Hmm, how about if it handles the false return by NOT calling EnableHost. I > think that's better than silently setting an incomplete pin? I think the old behaviour was better, for reasons expanded below. https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.cc File net/base/hash_value.cc (right): https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.cc#ne... net/base/hash_value.cc:33: std::string base64_str; On 2012/11/13 23:20:18, unsafe wrote: > On 2012/11/13 19:02:32, Ryan Sleevi wrote: > > nit: (minor) You can use a base::StringPiece here, and avoid a copy > > Leaving std::string's in-place for now as I don't have a good understanding of > StringPiece. base::StringPiece behaves like a string, except it does not copy - it just stores a char* and length. It means you can slice and dice at will without having to take a full copy. std::strings are implicitly convertable to std::StringPiece, so it shouldn't require any callsite changes along the way. https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.cc#ne... net/base/hash_value.cc:65: } On 2012/11/13 23:20:18, unsafe wrote: > On 2012/11/13 19:02:32, Ryan Sleevi wrote: > > nit: the case labels of switch statements get indented one space ( > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Loops_... > > ) > > Hm, where does it say that? I see it saying 2-space indent? Sorry, you're right - one indent, two spaces. So the inner code block is two indents - four spaces. https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.h File net/base/hash_value.h (right): https://codereview.chromium.org/11274032/diff/40006/net/base/hash_value.h#new... net/base/hash_value.h:74: class NET_EXPORT SHA1HashValueLessThan { On 2012/11/13 23:20:18, unsafe wrote: > On 2012/11/13 19:02:32, Ryan Sleevi wrote: > > As far as NET_EXPORT goes, I don't believe any of the following classes need > to > > be NET_EXPORTed > > I removed NET_EXPORT from this whole file, apparently nothing needs it... I'm not sure that's true, or you may not be building in shared-library mode. It should definitely be needed for HashValue, which is used by chrome/browser/net - not to mention HashValueVector and friends. When I looked at the dependencies, it was only from this point onwards in the file that it wasn't necessary https://codereview.chromium.org/11274032/diff/40006/net/http/http_security_he... File net/http/http_security_headers.cc (right): https://codereview.chromium.org/11274032/diff/40006/net/http/http_security_he... net/http/http_security_headers.cc:300: return false; On 2012/11/13 23:20:18, unsafe wrote: > On 2012/11/13 19:35:04, Chris P. wrote: > > On 2012/11/13 19:02:32, Ryan Sleevi wrote: > > > Should these be returning false? > > > > > > On encountering an unrecognized algorithm, shouldn't the behaviour be to > skip > > > the pin, rather than fail pinning entirely? > > I think thats a design decision for HPKP, the server might expect the header to > be set atomically, and if you drop some pins you could be omitting backup pins > or pins that are needed by some servers. > > But, you could specify that different hash algos must redundantly specify the > exact same pins, just with different algos, and the client is free to accept > whichever algo it chooses? I suspect this will be a point for discussion. Certainly, I think the old behaviour was correct - in that an unrecognized algorithm was ignored, rather than skipping pinning entirely. Yes, it's certainly possible that someone could put some necessary pin only in one algorithm, but I think it's worse to silently not pin at all. Just like unknown directives are ignored (line 306), we should be ignoring unknown algorithms. eg: should NOT return false, but should simply continue https://codereview.chromium.org/11274032/diff/43004/net/base/hash_value.cc File net/base/hash_value.cc (right): https://codereview.chromium.org/11274032/diff/43004/net/base/hash_value.cc#ne... net/base/hash_value.cc:51: return false; design nit: You can drop these two added checks (45-46, 50-51), just take .substr(7) to let the implied npos handle it, and let the handling on lines 59-60 handle decode failures. This avoids the magic number issue more or less entirely https://codereview.chromium.org/11274032/diff/43004/net/http/http_security_he... File net/http/http_security_headers.cc (right): https://codereview.chromium.org/11274032/diff/43004/net/http/http_security_he... net/http/http_security_headers.cc:83: // of ParsePinsHeader(std::string&, DomainState&). No need for this comment
On 2012/11/13 23:32:05, Ryan Sleevi wrote: > > > In the original code, if a hash failed to parse, it was skipped, and the > next > > > hash used (line 1192) > > > > > > In the new code, if a hash fails to parse, the entire parsing stops and > false > > is > > > returned (line 1126) > > > > > > I suspect the original behaviour was more desirable - keep on parsing - > > > especially given that you don't handle a false return here. > > > > Hmm, how about if it handles the false return by NOT calling EnableHost. I > > think that's better than silently setting an incomplete pin? > > I think the old behaviour was better, for reasons expanded below. My assumption - for both this and unknown-hash-algs - is that a "bad"/incomplete set of pins is potentially much worse than not having pin. For example, if your website is using different certs in different regions, a pin that omits one of those certs breaks your site. So, I would leave these things as-is. But if you're sure you want incorrectly-entered pins/unknown-hash-algs to just silently discard individual pins, I'll change it. > > > nit: (minor) You can use a base::StringPiece here, and avoid a copy Done. > > I removed NET_EXPORT from this whole file, apparently nothing needs it... > > I'm not sure that's true, or you may not be building in shared-library mode. It > should definitely be needed for HashValue, which is used by chrome/browser/net - > not to mention HashValueVector and friends. Oh, OK, I assumed compiling was a complete test.. I added NET_EXPORT back except for the later functions. > I suspect this will be a point for discussion. Certainly, I think the old > behaviour was correct - in that an unrecognized algorithm was ignored, rather > than skipping pinning entirely. Yes, it's certainly possible that someone could > put some necessary pin only in one algorithm, but I think it's worse to silently > not pin at all. Just like unknown directives are ignored (line 306), we should > be ignoring unknown algorithms. OK, let me know once you/Chris make a final call on that, I'll change it. > design nit: You can drop these two added checks (45-46, 50-51), just take > .substr(7) to let the implied npos handle it, and let the handling on lines > 59-60 handle decode failures. Done. https://codereview.chromium.org/11274032/diff/43004/net/http/http_security_he... > net/http/http_security_headers.cc:83: // of ParsePinsHeader(std::string&, > DomainState&). > No need for this comment Done.
Regarding unknown hash algos: I just changed HPKP header parsing to skip them, but left the net-internals to reject the entire pin list. To summarize: this CL changes HPKP header parsing from INTOLERANT -> SKIP for unknown hash algos, and changes net_internals from SKIP -> INTOLERANT... Seems right to me?
> My assumption - for both this and unknown-hash-algs - is that a "bad"/incomplete > set of pins is potentially much worse than not having pin. For example, if your > website is using different certs in different regions, a pin that omits one of > those certs breaks your site. Different certs in different regions is one of the motivating factors for report-only mode, and is separate from the problem of unknown hash algorithms. People should not use SHA-3 to pin some certs but SHA-256 to pin others; if people choose to use multiple hash algorithms in their pin sets, including one that the I-D does not specify, then it's up to them not shoot themselves in the foot. I think we have to be forward-compatible with future versions of the specification that may call for SHA-3. I intend to specify that behavior (ignore unknowns and keep processing) in the I-D. > So, I would leave these things as-is. But if you're sure you want > incorrectly-entered pins/unknown-hash-algs to just silently discard individual > pins, I'll change it. Yeah, please do. > Oh, OK, I assumed compiling was a complete test.. I added NET_EXPORT back except > for the later functions. The problem would appear on the component build in Windows. On Linux, for example, the problem may never bite you. I found this out the hard way, way back when.
> Regarding unknown hash algos: I just changed HPKP header parsing to skip them, > but left the net-internals to reject the entire pin list. > > To summarize: this CL changes HPKP header parsing from INTOLERANT -> SKIP for > unknown hash algos, and changes net_internals from SKIP -> INTOLERANT... > > Seems right to me? No, I think we should skip everywhere, and pin and enforce as much as we understand.
On 2012/11/14 21:03:04, Chris P. wrote: > > To summarize: this CL changes HPKP header parsing from INTOLERANT -> SKIP for > > unknown hash algos, and changes net_internals from SKIP -> INTOLERANT... > > No, I think we should skip everywhere, and pin and enforce as much as we > understand. OK, I changed net_internals input to skip unknown hash algos, but still reject the entire input if anything is malformed. So, this CL changes net_internals from "skip over all errors/unknown" and HPKP header parsing from "reject all errors/unknown", to both = "reject on any parsing error, but skip over unknown". How's that?
On Wed, Nov 14, 2012 at 1:56 PM, <unsafe@trevp.net> wrote: > So, this CL changes net_internals from "skip over all errors/unknown" and > HPKP > header parsing from "reject all errors/unknown", to both = "reject on any > parsing error, but skip over unknown". That sounds good, thanks!
On 2012/11/16 00:30:35, Chris Palmer wrote: > > That sounds good, thanks! Cool, let me know if there's anything else to improve here...
Some nits, but that's it. I think we are getting close; what do rsleevi and agl think? https://codereview.chromium.org/11274032/diff/43010/chrome/browser/ui/webui/n... File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): https://codereview.chromium.org/11274032/diff/43010/chrome/browser/ui/webui/n... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1103: std::string HashesToBase64String(const net::HashValueVector& hashes) { These utility functions (this and Base64StringToHashes) should either be in an anonymous namespace or exposed publicly in some utility file. (There is a static copy of HashesToBase64String in transport_security_state.cc, so we obviously need it in multiple places). Apologies if we have already talked about this and decided on the current course of action... https://codereview.chromium.org/11274032/diff/43010/net/base/transport_securi... File net/base/transport_security_state.cc (right): https://codereview.chromium.org/11274032/diff/43010/net/base/transport_securi... net/base/transport_security_state.cc:185: const HashValueVector& b) { NIT: Whitespace https://codereview.chromium.org/11274032/diff/43010/net/base/transport_securi... net/base/transport_security_state.cc:611: const std::string& value, NIT: Whitespace https://codereview.chromium.org/11274032/diff/43010/net/base/transport_securi... net/base/transport_security_state.cc:716: static std::string HashesToBase64String(const HashValueVector& hashes) { We've duplicated this in net_internals_ui.cc. Can we move it to some utility file where it is exposed publicly? https://codereview.chromium.org/11274032/diff/43010/net/base/transport_securi... File net/base/transport_security_state_static.h (right): https://codereview.chromium.org/11274032/diff/43010/net/base/transport_securi... net/base/transport_security_state_static.h:14: "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"; Still not super sure about this, but if rsleevi and agl are on board, OK. https://codereview.chromium.org/11274032/diff/43010/net/base/transport_securi... File net/base/transport_security_state_static_generate.go (right): https://codereview.chromium.org/11274032/diff/43010/net/base/transport_securi... net/base/transport_security_state_static_generate.go:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Putting this back in the tree will (re-)start a fight. Are we up for that? https://codereview.chromium.org/11274032/diff/43010/net/url_request/url_reque... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/11274032/diff/43010/net/url_request/url_reque... net/url_request/url_request_http_job.cc:707: if (headers->EnumerateHeader(NULL, "Public-Key-Pins", &value)) NIT: Duplicate the comment about how we MUST process only the first header.
Addressed Chris's nits, I believe. https://codereview.chromium.org/11274032/diff/43010/chrome/browser/ui/webui/n... File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): https://codereview.chromium.org/11274032/diff/43010/chrome/browser/ui/webui/n... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1103: std::string HashesToBase64String(const net::HashValueVector& hashes) { On 2012/12/06 21:20:17, Chris P. wrote: > These utility functions (this and Base64StringToHashes) should either be in an > anonymous namespace or exposed publicly in some utility file. (There is a static > copy of HashesToBase64String in transport_security_state.cc, so we obviously > need it in multiple places). Moved to anonymous namespace. I previously put these in hash_value.h/.cc, but rsleevi suggested that since HashesToBase64String() is only used twice, in somewhat disconnected places (TSS for logging; here for debugging display), he thought it was better to just duplicate it in each file, instead of putting it into a header file as an API that would have to be maintained. If it needs to get used other places, it could be moved into hash_value.h/.cc later? https://codereview.chromium.org/11274032/diff/43010/net/base/transport_securi... File net/base/transport_security_state.cc (right): https://codereview.chromium.org/11274032/diff/43010/net/base/transport_securi... net/base/transport_security_state.cc:185: const HashValueVector& b) { On 2012/12/06 21:20:17, Chris P. wrote: > NIT: Whitespace Done. https://codereview.chromium.org/11274032/diff/43010/net/base/transport_securi... net/base/transport_security_state.cc:611: const std::string& value, On 2012/12/06 21:20:17, Chris P. wrote: > NIT: Whitespace Done. https://codereview.chromium.org/11274032/diff/43010/net/base/transport_securi... net/base/transport_security_state.cc:716: static std::string HashesToBase64String(const HashValueVector& hashes) { On 2012/12/06 21:20:17, Chris P. wrote: > We've duplicated this in net_internals_ui.cc. Can we move it to some utility > file where it is exposed publicly? Moved this into an unnamed namespace. Will comment separately. https://codereview.chromium.org/11274032/diff/43010/net/base/transport_securi... File net/base/transport_security_state_static.h (right): https://codereview.chromium.org/11274032/diff/43010/net/base/transport_securi... net/base/transport_security_state_static.h:14: "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00"; On 2012/12/06 21:20:17, Chris P. wrote: > Still not super sure about this, but if rsleevi and agl are on board, OK. Here's my case for changing it to binary: On a connection, any preload pin data is loaded into a HashValueVector for checking intersection with the cert chain. If you don't have to base64 decode the preload data, the code is simpler and slightly faster, and the preloads take up less space. In particular, you don't need the TSS:ParsePin function, you can just prepare each HashValue with a memcpy, so you get a TSS diff like: -// static -bool TransportSecurityState::ParsePin(const std::string& value, - HashValue* out) { - StringPair slash = Split(Strip(value), '/'); - - if (slash.first == "sha1") - out->tag = HASH_VALUE_SHA1; - else if (slash.first == "sha256") - out->tag = HASH_VALUE_SHA256; - else - return false; - - std::string decoded; - if (!base::Base64Decode(slash.second, &decoded) || - decoded.size() != out->size()) { - return false; - } - - memcpy(out->data(), decoded.data(), out->size()); - return true; -} - HashValue hash; - - if (!TransportSecurityState::ParsePin(type_and_base64, &hash)) - return false; - + HashValue hash(HASH_VALUE_SHA1); + memcpy(hash.data(), sha1_hash, 20); https://codereview.chromium.org/11274032/diff/43010/net/url_request/url_reque... File net/url_request/url_request_http_job.cc (right): https://codereview.chromium.org/11274032/diff/43010/net/url_request/url_reque... net/url_request/url_request_http_job.cc:707: if (headers->EnumerateHeader(NULL, "Public-Key-Pins", &value)) On 2012/12/06 21:20:17, Chris P. wrote: > NIT: Duplicate the comment about how we MUST process only the first header. Done.
> Here's my case for changing it to binary: > > On a connection, any preload pin data is loaded into a HashValueVector for > checking intersection with the cert chain. > > If you don't have to base64 decode the preload data, the code is simpler and > slightly faster, and the preloads take up less space. > > In particular, you don't need the TSS:ParsePin function, you can just prepare > each HashValue with a memcpy, so you get a TSS diff like: OK. LGTM, but I am not an OWNER. sleevi, agl, any thoughts?
Thoughts will be delayed until Monday I'm afraid. Too much happening.
Sorry for the delays. I first focused on the correctness of the code in terms of the local file, but I'll need to take a larger look at the code and make sure all the pieces play together nicely. https://codereview.chromium.org/11274032/diff/50001/chrome/browser/ui/webui/n... File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): https://codereview.chromium.org/11274032/diff/50001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1165: result->SetString("static_spki_hashes", hashes_str); nit: Seems like we can just drop the temporary |hashes_str| here result->SetString( "static_spki_hashes", HashesToBase64String(...)); result->SetString( "dynamic_spki_hashes" HashesToBase64String(...)); (not sure if it needs that much wrapping, may all fit on two lines < 80, like 1161 does ) https://codereview.chromium.org/11274032/diff/50001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1200: transport_security_state->EnableHost(domain, state); I find this new structure a little confusing, as it threw my reading at first. Base64StringToHashes returns true for an empty string, which I think is different/subtle, and hides an important detail when reading the code here - that hashes_str can be empty (enabling HSTS without enabling HPKP) I would suggest removing line 122-123 from Base64... and placing them here if (!hashes_str.empty()) Base64StringToHashes(...) tss->EnableHost(...) The problem here is I don't see a good way to respect Base64StringToHashes' result, when examining the original code, and it's not clear to me whether that represents 'bug' or 'feature'. The old code ignored any bad hashes, but still enabled the HSTS. It seems like some form of feedback mechanism is needed to allow OnHSTSAdd to indicate success/failure, and then and only then check the result of Base64StringToHashes. https://codereview.chromium.org/11274032/diff/50001/net/base/hash_value.cc File net/base/hash_value.cc (right): https://codereview.chromium.org/11274032/diff/50001/net/base/hash_value.cc#ne... net/base/hash_value.cc:8: #include <cstring> style nit: Seems a bit inconsistent to mix C++ headers and C headers (eg: hash_value.h uses string.h, rather than cstring) If these are included for just one or two functions, it's typically common to explain why #include <string.h> // For memcmp Just to make sure stale headers get removed and it's clear why things are included. (That said, because string.h is included in the .h, you should be able to remove it from the .cc here if that is the reason) https://codereview.chromium.org/11274032/diff/50001/net/base/hash_value.cc#ne... net/base/hash_value.cc:70: return std::string("sha1/" + base64_str); Does this actually compile & do the right thing? I would expect it would need to be written as std::string("sha1/") + base64_str https://codereview.chromium.org/11274032/diff/50001/net/base/hash_value.cc#ne... net/base/hash_value.cc:88: // return value can be passed to memset as the length argument. If we Not sure why the comment for "might be inlined". The inlining has no bearing - it's the fact that NOTREACHED() is a no-op in release mode. I think palmer may have added the original comment, but I would just rewrite it as something like // While an invalid tag should not happen, return a non-zero length // to avoid compiler warnings when the result of size() is // used with functions like memset. That said, I think "return 0" should be valid if the HashValue is in an error state, and the caller should be responsible for checking, but that's for later clean-up :) It's the same with having ::data() return NULL (which should also be error checked) https://codereview.chromium.org/11274032/diff/50001/net/base/hash_value.h File net/base/hash_value.h (right): https://codereview.chromium.org/11274032/diff/50001/net/base/hash_value.h#new... net/base/hash_value.h:56: // - serializing public-key pins comment nit: This feels like an abstraction leak, to have a 'base' class talking about who depends on it. // Serializes/Deserializes hashes in the form of // <hash-name>"/"<base64-hash-value> // (eg: "sha1/...") // This format may be persisted to permanent storage, so // care should be taken before changing the serialization. https://codereview.chromium.org/11274032/diff/50001/net/base/hash_value.h#new... net/base/hash_value.h:61: // (but ToString() errors should not occur!) comment nit: don't indent the comment here comment nit: We should probably just say // Deserializes a HashValue from a string. On error, returns // false and does not change the contents of HashValue (which // may contain invalid data) bool FromString(...) // Serializes the HashValue to a string. If an invalid HashValue // is supplied (eg: an unknown hash tag), returns "unknown"/<base64> std::string ToString() const; Is there a reason you return <unknown> instead of an empty string? Just curious the thought behind it. https://codereview.chromium.org/11274032/diff/50001/net/base/transport_securi... File net/base/transport_security_state.cc (right): https://codereview.chromium.org/11274032/diff/50001/net/base/transport_securi... net/base/transport_security_state.cc:78: memcpy(hash.data(), sha1_hash, 20); nit: 20 -> hash.size()? https://codereview.chromium.org/11274032/diff/50001/net/base/transport_securi... net/base/transport_security_state.cc:611: &domain_state.include_subdomains)) { The wrapping here is inconsistent. Can you fit this on two lines instead of three? See also 629-632 https://codereview.chromium.org/11274032/diff/50001/net/http/http_security_he... File net/http/http_security_headers.cc (right): https://codereview.chromium.org/11274032/diff/50001/net/http/http_security_he... net/http/http_security_headers.cc:25: long int i = strtol(s.data(), &endptr, 10 /* base */); BUG: I previously advised you not to use strtol, but this has not been fixed. In particular, it's not safe to assume that long == int64, whereas an HTTP delta-seconds *may* be 64-bit https://codereview.chromium.org/11274032/diff/50001/net/http/http_security_he... net/http/http_security_headers.cc:39: i = pins.begin(); i != pins.end(); ++i) { i = pins.begin(); should fit on the previous line, as should i != pins.end(); You should only need to wrap for ++i https://codereview.chromium.org/11274032/diff/50001/net/http/http_security_he... net/http/http_security_headers.cc:56: std::find_if(b.begin(), b.end(), HashValuesEqual(*i)); style nit: four space indent here https://codereview.chromium.org/11274032/diff/50001/net/http/http_security_he... net/http/http_security_headers.cc:111: // This code has to assume that 32 bytes is SHA-256 and 20 bytes is SHA-1. comment nit: This code does not assume that (because tag is directly supplied) Just nuke 111-113 https://codereview.chromium.org/11274032/diff/50001/net/http/http_security_he... net/http/http_security_headers.cc:214: &max_age_candidate)) nit: add braces because the if condition spans multiple lines. Also, it does not seem you need to wrap like this, which if correct, means you should change the wrapping and no braces are needed. If it does need to wrap, use MaxAgeToInt(unquoted.begin(), unquoted.end(), &max_age_candidate) https://codereview.chromium.org/11274032/diff/50001/net/http/http_security_he... net/http/http_security_headers.cc:294: } nit: no braces https://codereview.chromium.org/11274032/diff/50001/net/http/http_security_he... net/http/http_security_headers.cc:298: } nit: no braces
https://codereview.chromium.org/11274032/diff/50001/chrome/browser/ui/webui/n... File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): https://codereview.chromium.org/11274032/diff/50001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1165: result->SetString("static_spki_hashes", hashes_str); On 2012/12/07 23:37:21, Ryan Sleevi wrote: > nit: Seems like we can just drop the temporary |hashes_str| here > > result->SetString( > "static_spki_hashes", > HashesToBase64String(...)); > result->SetString( > "dynamic_spki_hashes" > HashesToBase64String(...)); > > (not sure if it needs that much wrapping, may all fit on two lines < 80, like > 1161 does ) Done. https://codereview.chromium.org/11274032/diff/50001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1200: transport_security_state->EnableHost(domain, state); On 2012/12/07 23:37:21, Ryan Sleevi wrote: > I find this new structure a little confusing, as it threw my reading at first. > > Base64StringToHashes returns true for an empty string, which I think is > different/subtle, and hides an important detail when reading the code here - > that hashes_str can be empty (enabling HSTS without enabling HPKP) > > I would suggest removing line 122-123 from Base64... and placing them here > > if (!hashes_str.empty()) > Base64StringToHashes(...) > > tss->EnableHost(...) I moved lines 122-123 per your suggestion. But I think a parse failure should NOT cause HSTS or pins enablement. > The problem here is I don't see a good way to respect Base64StringToHashes' > result, when examining the original code, and it's not clear to me whether that > represents 'bug' or 'feature'. > > The old code ignored any bad hashes, but still enabled the HSTS. It seems like > some form of feedback mechanism is needed to allow OnHSTSAdd to indicate > success/failure, and then and only then check the result of > Base64StringToHashes. Feedback is provided to the user since adding a pin/HSTS immediately queries and displays the new pin/HSTS, so if a base64 error causes nothing to take effect, the user should notice it. https://codereview.chromium.org/11274032/diff/50001/net/base/hash_value.cc File net/base/hash_value.cc (right): https://codereview.chromium.org/11274032/diff/50001/net/base/hash_value.cc#ne... net/base/hash_value.cc:8: #include <cstring> On 2012/12/07 23:37:21, Ryan Sleevi wrote: > (That said, because string.h is included in the .h, you should be able to remove it from the .cc here if that is the reason) Done. https://codereview.chromium.org/11274032/diff/50001/net/base/hash_value.cc#ne... net/base/hash_value.cc:70: return std::string("sha1/" + base64_str); On 2012/12/07 23:37:21, Ryan Sleevi wrote: > Does this actually compile & do the right thing? > > I would expect it would need to be written as std::string("sha1/") + base64_str Done. https://codereview.chromium.org/11274032/diff/50001/net/base/hash_value.cc#ne... net/base/hash_value.cc:88: // return value can be passed to memset as the length argument. If we On 2012/12/07 23:37:21, Ryan Sleevi wrote: > Not sure why the comment for "might be inlined". The inlining has no bearing - > it's the fact that NOTREACHED() is a no-op in release mode. > > I think palmer may have added the original comment, but I would just rewrite it > as something like > > // While an invalid tag should not happen, return a non-zero length > // to avoid compiler warnings when the result of size() is > // used with functions like memset. > > That said, I think "return 0" should be valid if the HashValue is in an error > state, and the caller should be responsible for checking, but that's for later > clean-up :) It's the same with having ::data() return NULL (which should also be > error checked) Done. https://codereview.chromium.org/11274032/diff/50001/net/base/hash_value.h File net/base/hash_value.h (right): https://codereview.chromium.org/11274032/diff/50001/net/base/hash_value.h#new... net/base/hash_value.h:56: // - serializing public-key pins On 2012/12/07 23:37:21, Ryan Sleevi wrote: > comment nit: This feels like an abstraction leak, to have a 'base' class talking > about who depends on it. > > // Serializes/Deserializes hashes in the form of > // <hash-name>"/"<base64-hash-value> > // (eg: "sha1/...") > // This format may be persisted to permanent storage, so > // care should be taken before changing the serialization. Added your comment but left mine as I think its useful to know where this is used. I'll remove it if you prefer. https://codereview.chromium.org/11274032/diff/50001/net/base/hash_value.h#new... net/base/hash_value.h:61: // (but ToString() errors should not occur!) On 2012/12/07 23:37:21, Ryan Sleevi wrote: > comment nit: don't indent the comment here > > comment nit: We should probably just say > > // Deserializes a HashValue from a string. On error, returns > // false and does not change the contents of HashValue (which > // may contain invalid data) > bool FromString(...) > > // Serializes the HashValue to a string. If an invalid HashValue > // is supplied (eg: an unknown hash tag), returns "unknown"/<base64> > std::string ToString() const; Replaced with your comments, but your FromString() comment didn't match behavior, since the code mutates HashValue.tag prior to base64-decoding, so I tried to make that clearer. > > Is there a reason you return <unknown> instead of an empty string? Just curious > the thought behind it. If something goes wrong the error will be more traceable when you see a funky string with <unknown> in it.. Can change it if you like. https://codereview.chromium.org/11274032/diff/50001/net/base/transport_securi... File net/base/transport_security_state.cc (right): https://codereview.chromium.org/11274032/diff/50001/net/base/transport_securi... net/base/transport_security_state.cc:78: memcpy(hash.data(), sha1_hash, 20); On 2012/12/07 23:37:21, Ryan Sleevi wrote: > nit: 20 -> hash.size()? Done. https://codereview.chromium.org/11274032/diff/50001/net/base/transport_securi... net/base/transport_security_state.cc:611: &domain_state.include_subdomains)) { On 2012/12/07 23:37:21, Ryan Sleevi wrote: > The wrapping here is inconsistent. Can you fit this on two lines instead of > three? > > See also 629-632 Done. https://codereview.chromium.org/11274032/diff/50001/net/http/http_security_he... File net/http/http_security_headers.cc (right): https://codereview.chromium.org/11274032/diff/50001/net/http/http_security_he... net/http/http_security_headers.cc:25: long int i = strtol(s.data(), &endptr, 10 /* base */); On 2012/12/07 23:37:21, Ryan Sleevi wrote: > BUG: I previously advised you not to use strtol, but this has not been fixed. Not my code, but fixed. StringToUint64 doesn't seem to handle negative numbers correctly (it was failing the unit test due to returning true for negatives, with a garbage result). Also, one of the unit tests was expecting a value large enough to overflow uint64 to return true; I shortened those values in the unit test so they do not overflow uint64. https://codereview.chromium.org/11274032/diff/50001/net/http/http_security_he... net/http/http_security_headers.cc:39: i = pins.begin(); i != pins.end(); ++i) { On 2012/12/07 23:37:21, Ryan Sleevi wrote: > i = pins.begin(); should fit on the previous line, as should i != pins.end(); > > You should only need to wrap for ++i Done. https://codereview.chromium.org/11274032/diff/50001/net/http/http_security_he... net/http/http_security_headers.cc:56: std::find_if(b.begin(), b.end(), HashValuesEqual(*i)); On 2012/12/07 23:37:21, Ryan Sleevi wrote: > style nit: four space indent here Done. https://codereview.chromium.org/11274032/diff/50001/net/http/http_security_he... net/http/http_security_headers.cc:111: // This code has to assume that 32 bytes is SHA-256 and 20 bytes is SHA-1. On 2012/12/07 23:37:21, Ryan Sleevi wrote: > comment nit: This code does not assume that (because tag is directly supplied) > > Just nuke 111-113 Done. https://codereview.chromium.org/11274032/diff/50001/net/http/http_security_he... net/http/http_security_headers.cc:214: &max_age_candidate)) On 2012/12/07 23:37:21, Ryan Sleevi wrote: > nit: add braces because the if condition spans multiple lines. > > Also, it does not seem you need to wrap like this, which if correct, means you > should change the wrapping and no braces are needed. > > If it does need to wrap, use > MaxAgeToInt(unquoted.begin(), unquoted.end(), > &max_age_candidate) Done. https://codereview.chromium.org/11274032/diff/50001/net/http/http_security_he... net/http/http_security_headers.cc:294: } On 2012/12/07 23:37:21, Ryan Sleevi wrote: > nit: no braces Done. https://codereview.chromium.org/11274032/diff/50001/net/http/http_security_he... net/http/http_security_headers.cc:298: } On 2012/12/07 23:37:21, Ryan Sleevi wrote: > nit: no braces Done.
https://codereview.chromium.org/11274032/diff/61001/chrome/browser/ui/webui/n... File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): https://codereview.chromium.org/11274032/diff/61001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:130: if (hash_str.substr(0, 4) != "sha1" && hash_str.substr(0, 6) != "sha256") I fear this crashes if |hash_str| is empty (and thus index 0 is past the end of the string.) (Later: it actually doesn't with g++, but I'm not sure whether that's the C++ spec, or a G++ feature.) Additionally, the matches should be made against "sha1/" and "sha256/" to avoid matching, say, "sha256-128/". (The truncated version of SHA-256.) https://codereview.chromium.org/11274032/diff/61001/net/base/hash_value.cc File net/base/hash_value.cc (right): https://codereview.chromium.org/11274032/diff/61001/net/base/hash_value.cc#ne... net/base/hash_value.cc:41: if (value.substr(0, 5) == "sha1/") { Here's this assumption that we can index 0 of an empty string again. https://codereview.chromium.org/11274032/diff/61001/net/base/hash_value.cc#ne... net/base/hash_value.cc:117: extra blank line at end of file. https://codereview.chromium.org/11274032/diff/61001/net/base/hash_value.h File net/base/hash_value.h (right): https://codereview.chromium.org/11274032/diff/61001/net/base/hash_value.h#new... net/base/hash_value.h:22: return memcmp(data, other.data, sizeof(data)) == 0; I would be tempted to use a constant-time compare here and for SHA-256 because uses of code drift over time and the amount of extra CPU taken is negligible. Could go either way though. https://codereview.chromium.org/11274032/diff/61001/net/base/transport_securi... File net/base/transport_security_state_static_generate.go (right): https://codereview.chromium.org/11274032/diff/61001/net/base/transport_securi... net/base/transport_security_state_static_generate.go:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Changes to this code are obviously a pain to code review. Could you remove this from the CL and email out the diff? https://codereview.chromium.org/11274032/diff/61001/net/http/http_security_he... File net/http/http_security_headers.cc (right): https://codereview.chromium.org/11274032/diff/61001/net/http/http_security_he... net/http/http_security_headers.cc:33: *result = (int)i; COMPILE_ASSERT(kMaxHSTSAgeSecs < 0x80000000)? https://codereview.chromium.org/11274032/diff/61001/net/http/http_security_he... net/http/http_security_headers.cc:44: std::find_if(from_cert_chain.begin(), from_cert_chain.end(), nit: continued lines are indented four spaces. https://codereview.chromium.org/11274032/diff/61001/net/http/http_security_he... net/http/http_security_headers.cc:55: bool HashesIntersect(const HashValueVector& a, This is a copy-paste of code in transport_security_state.cc. Something for hash_value.h? https://codereview.chromium.org/11274032/diff/61001/net/http/http_security_he... net/http/http_security_headers.cc:318: blank line at end of file. (Probably in every file.) https://codereview.chromium.org/11274032/diff/61001/net/http/http_security_he... File net/http/http_security_headers.h (right): https://codereview.chromium.org/11274032/diff/61001/net/http/http_security_he... net/http/http_security_headers.h:59: extra blank line. https://codereview.chromium.org/11274032/diff/61001/net/http/http_security_he... net/http/http_security_headers.h:62: extra blank line at end of file.
I'll send out Go diff separately; removed the extra file-ending LF from new files, but now Lint is complaining about these files? (which is why I added it....) https://codereview.chromium.org/11274032/diff/61001/chrome/browser/ui/webui/n... File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): https://codereview.chromium.org/11274032/diff/61001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:130: if (hash_str.substr(0, 4) != "sha1" && hash_str.substr(0, 6) != "sha256") On 2012/12/10 17:13:44, agl wrote: > I fear this crashes if |hash_str| is empty (and thus index 0 is past the end of > the string.) > > (Later: it actually doesn't with g++, but I'm not sure whether that's the C++ > spec, or a G++ feature.) > > Additionally, the matches should be made against "sha1/" and "sha256/" to avoid > matching, say, "sha256-128/". (The truncated version of SHA-256.) Done. https://codereview.chromium.org/11274032/diff/61001/net/base/hash_value.cc File net/base/hash_value.cc (right): https://codereview.chromium.org/11274032/diff/61001/net/base/hash_value.cc#ne... net/base/hash_value.cc:41: if (value.substr(0, 5) == "sha1/") { On 2012/12/10 17:13:44, agl wrote: > Here's this assumption that we can index 0 of an empty string again. Done. https://codereview.chromium.org/11274032/diff/61001/net/base/hash_value.cc#ne... net/base/hash_value.cc:117: On 2012/12/10 17:13:44, agl wrote: > extra blank line at end of file. Done. https://codereview.chromium.org/11274032/diff/61001/net/base/hash_value.h File net/base/hash_value.h (right): https://codereview.chromium.org/11274032/diff/61001/net/base/hash_value.h#new... net/base/hash_value.h:22: return memcmp(data, other.data, sizeof(data)) == 0; On 2012/12/10 17:13:44, agl wrote: > I would be tempted to use a constant-time compare here and for SHA-256 because > uses of code drift over time and the amount of extra CPU taken is negligible. Done - also moved implementation to C file instead of inline. https://codereview.chromium.org/11274032/diff/61001/net/http/http_security_he... File net/http/http_security_headers.cc (right): https://codereview.chromium.org/11274032/diff/61001/net/http/http_security_he... net/http/http_security_headers.cc:33: *result = (int)i; On 2012/12/10 17:13:44, agl wrote: > COMPILE_ASSERT(kMaxHSTSAgeSecs < 0x80000000)? Done, also changed result to a uint32 so this code is the same on all platforms: COMPILE_ASSERT(kMaxHSTSAgeSecs <= kuint32max, kMaxHSTSAgeSecsTooLarge); https://codereview.chromium.org/11274032/diff/61001/net/http/http_security_he... net/http/http_security_headers.cc:44: std::find_if(from_cert_chain.begin(), from_cert_chain.end(), On 2012/12/10 17:13:44, agl wrote: > nit: continued lines are indented four spaces. Done. https://codereview.chromium.org/11274032/diff/61001/net/http/http_security_he... net/http/http_security_headers.cc:55: bool HashesIntersect(const HashValueVector& a, On 2012/12/10 17:13:44, agl wrote: > This is a copy-paste of code in transport_security_state.cc. Something for > hash_value.h? Possibly. Ryan didn't want to put too much into there, which is why the HashesToBase64String / Base64StringToHashes were removed from hash_value.h, but maybe I went too far removing HashesIntersect. Ryan, should I add HashesIntersect back to hash_value.h? https://codereview.chromium.org/11274032/diff/61001/net/http/http_security_he... net/http/http_security_headers.cc:318: On 2012/12/10 17:13:44, agl wrote: > blank line at end of file. (Probably in every file.) Done. https://codereview.chromium.org/11274032/diff/61001/net/http/http_security_he... File net/http/http_security_headers.h (right): https://codereview.chromium.org/11274032/diff/61001/net/http/http_security_he... net/http/http_security_headers.h:59: On 2012/12/10 17:13:44, agl wrote: > extra blank line. Done. https://codereview.chromium.org/11274032/diff/61001/net/http/http_security_he... net/http/http_security_headers.h:62: On 2012/12/10 17:13:44, agl wrote: > extra blank line at end of file. Done.
On Mon, Dec 10, 2012 at 3:59 PM, <unsafe@trevp.net> wrote: > I'll send out Go diff separately; removed the extra file-ending LF from new > files, but now Lint is complaining about these files? (which is why I added > it....) Oh don't sweat it then. Maybe lint and I disagree and it's probably more persistent than I am on this matter. Cheers AGL
Diff for transport_security_state_static_generate.go:
func writeCertsOutput(out *bufio.Writer, pins []pin) {
out.WriteString(`// These are SubjectPublicKeyInfo hashes for public key
pinning.
-// hashes are base64 encoded, SHA1 digests.
+// hashes are SHA1 digests.
`)
for _, pin := range pins {
fmt.Fprintf(out, "static const char kSPKIHash_%s[] =\n",
pin.name)
- fmt.Fprintf(out, " \"%s/%s\";\n\n", pin.spkiHashFunc,
base64.StdEncodin
+ var s string
+ for _,c := range pin.spkiHash {
+ s += fmt.Sprintf("\\x%02x", c)
+ }
+ fmt.Fprintf(out, " \"%s\";\n\n", s)
}
}
On 2012/12/10 21:06:18, unsafe wrote:
> Diff for transport_security_state_static_generate.go:
Oops, copy/paste error, this is correct:
func writeCertsOutput(out *bufio.Writer, pins []pin) {
out.WriteString(`// These are SubjectPublicKeyInfo hashes for public key
pinning. The
-// hashes are base64 encoded, SHA1 digests.
+// hashes are SHA1 digests.
`)
for _, pin := range pins {
fmt.Fprintf(out, "static const char kSPKIHash_%s[] =\n",
pin.name)
- fmt.Fprintf(out, " \"%s/%s\";\n\n", pin.spkiHashFunc,
base64.StdEncoding.EncodeToString(pin.spkiHash))
+ var s string
+ for _,c := range pin.spkiHash {
+ s += fmt.Sprintf("\\x%02x", c)
+ }
+ fmt.Fprintf(out, " \"%s\";\n\n", s)
}
}
go change LGTM. (Note, that code lives here now: https://github.com/agl/transport-security-state-generate) Cheers AGL
I updated the CL description to a better synopsis. Also, made one last fix: On 2012/12/08 09:22:42, unsafe wrote: https://codereview.chromium.org/11274032/diff/50001/net/http/http_security_he... > net/http/http_security_headers.cc:25: long int i = strtol(s.data(), &endptr, 10 > /* base */); > On 2012/12/07 23:37:21, Ryan Sleevi wrote: > > BUG: I previously advised you not to use strtol, but this has not been fixed. > > Not my code, but fixed. StringToUint64 doesn't seem to handle negative numbers > correctly (it was failing the unit test due to returning true for negatives, > with a garbage result). Also, one of the unit tests was expecting a value large > enough to overflow uint64 to return true; I shortened those values in the unit > test so they do not overflow uint64. Ignore that last sentence - I reread the StringToInt64 description more carefully, fixed the code to handle overflows, and reverted the unit test. So, this code should be correct now, and I believe this change does fix a latent issue, as Ryan suggested. But, please review! Trevor
Sorry for the delays! I have a few nits, mostly around the comments then the actual implementation. In files that make heavy use of C++ comment style of //, please use // . /* */ comments are generally rare in Chromium code, and when they are used, they tend to be used consistently throughout the file. I will take a second pass at the unit tests, I just focused on the implementation. The only thing in here that makes me a little nervous is the Base64Decode/Encode calls of user/network data. I filed http://crbug.com/167193 to clean up the interface for this, but I think adding the checks in this code as well are perhaps 'more' correct. https://codereview.chromium.org/11274032/diff/80001/chrome/browser/ui/webui/n... File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): https://codereview.chromium.org/11274032/diff/80001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:131: if (hash_str.empty()) /* pair of commas with no content between them */ nit: Drop this end-of-line comment. https://codereview.chromium.org/11274032/diff/80001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:133: /* hash_str.substr() will not throw, as hash_str is non-empty */ nit: Drop this comment. Because you're using a |0| index, it's by definition never going to through - empty or not. https://codereview.chromium.org/11274032/diff/80001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:134: if (hash_str.substr(0, 5) != "sha1/" && hash_str.substr(0, 7) != "sha256/") (pedantic) nit: if (hash_str.compare(0, 5, "sha1/") != 0 || hash_str.compare(0, 7, "sha256") != 0) { continue; } (Avoids temporaries that can't be optimized away) https://codereview.chromium.org/11274032/diff/80001/net/base/hash_value.cc File net/base/hash_value.cc (right): https://codereview.chromium.org/11274032/diff/80001/net/base/hash_value.cc#ne... net/base/hash_value.cc:32: return crypto::SecureMemEqual(data, other.data, sizeof(data)); I missed this change from your moving data. I would rather use memcmp() here and create a new SecureEquals if we believe timing to be a concern. This is because we index a number of things on their SHA1HashValue or the like, and so Equals should keep the optimized path. https://codereview.chromium.org/11274032/diff/80001/net/base/hash_value.cc#ne... net/base/hash_value.cc:52: so check for adequate string length first */ Nit: This comment is unnecessary. https://codereview.chromium.org/11274032/diff/80001/net/base/hash_value.cc#ne... net/base/hash_value.cc:53: if (value.size() > 5 && value.substr(0, 5) == "sha1/") { if (value.compare(0, 5, "sha1/") == 0) { ... } else if (value.compare(0, 7, "sha256/") == 0 { ... } if (base64_str.empty()) return false; C++03 (21.3.6.8.1) guarantees that for str.compare(), if str.size() < n, it will return < 0. See C++03 21.3.1.5 for the further guarantee that it will take min(size(), n). C++03 21.3.6.7.3 guarantees that str2 = str1.substr(str1.size()) is safe and will return an empty string. https://codereview.chromium.org/11274032/diff/80001/net/base/hash_value.cc#ne... net/base/hash_value.cc:65: decoded.size() != size()) { style nit: Does this fit on one line? If so, do that, and drop the brackets. https://codereview.chromium.org/11274032/diff/80001/net/http/http_security_he... File net/http/http_security_headers.cc (right): https://codereview.chromium.org/11274032/diff/80001/net/http/http_security_he... net/http/http_security_headers.cc:29: if (!base::StringToInt64(s, &i) && i != kint64max) If overflow happens, StringToInt64 will return false (by virtue of IteratorRangeToNumber::Invoke returning false, by virtue of IteratorRangeToNumber::Base::Invoke returning false, by virtue of IteratorRangeToNumber::Positive::CheckBounds returning false. Perhaps you meant by the comment "values greater than kMaxHSTSAgeSecs" When you do update the comment, please follow the style of the rest of the file of using C++ // comments rather than /* */, for consistency reasons. https://codereview.chromium.org/11274032/diff/80001/net/http/http_security_he... net/http/http_security_headers.cc:33: COMPILE_ASSERT(kMaxHSTSAgeSecs <= kuint32max, kMaxHSTSAgeSecsTooLarge); nit: You can move this line 33 to line 16 - the location of the COMPILE_ASSERT in code doesn't matter, and it makes it easier to separate out the logic from the pre-condition guarantees. https://codereview.chromium.org/11274032/diff/80001/net/http/http_security_he... net/http/http_security_headers.cc:117: if (!base::Base64Decode(unquoted, &decoded)) BUG: Please add a check that !unquoted.empty() before calling B64Decode
https://codereview.chromium.org/11274032/diff/80001/chrome/browser/ui/webui/n... File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): https://codereview.chromium.org/11274032/diff/80001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:131: if (hash_str.empty()) /* pair of commas with no content between them */ On 2012/12/20 23:44:13, Ryan Sleevi wrote: > nit: Drop this end-of-line comment. Done. https://codereview.chromium.org/11274032/diff/80001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:133: /* hash_str.substr() will not throw, as hash_str is non-empty */ On 2012/12/20 23:44:13, Ryan Sleevi wrote: > nit: Drop this comment. Because you're using a |0| index, it's by definition > never going to through - empty or not. Done. https://codereview.chromium.org/11274032/diff/80001/chrome/browser/ui/webui/n... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:134: if (hash_str.substr(0, 5) != "sha1/" && hash_str.substr(0, 7) != "sha256/") On 2012/12/20 23:44:13, Ryan Sleevi wrote: > (pedantic) nit: > > if (hash_str.compare(0, 5, "sha1/") != 0 || > hash_str.compare(0, 7, "sha256") != 0) { > continue; > } > > (Avoids temporaries that can't be optimized away) Done. https://codereview.chromium.org/11274032/diff/80001/net/base/hash_value.cc File net/base/hash_value.cc (right): https://codereview.chromium.org/11274032/diff/80001/net/base/hash_value.cc#ne... net/base/hash_value.cc:32: return crypto::SecureMemEqual(data, other.data, sizeof(data)); On 2012/12/20 23:44:13, Ryan Sleevi wrote: > I missed this change from your moving data. > > I would rather use memcmp() here and create a new SecureEquals if we believe > timing to be a concern. This was a new change in last patch set, based on AGL feedback. But timing leaks are not a concern in existing code, so I rolled back to memset, and added a comment explaining the potential risk. https://codereview.chromium.org/11274032/diff/80001/net/base/hash_value.cc#ne... net/base/hash_value.cc:52: so check for adequate string length first */ On 2012/12/20 23:44:13, Ryan Sleevi wrote: > Nit: This comment is unnecessary. Done. https://codereview.chromium.org/11274032/diff/80001/net/base/hash_value.cc#ne... net/base/hash_value.cc:53: if (value.size() > 5 && value.substr(0, 5) == "sha1/") { On 2012/12/20 23:44:13, Ryan Sleevi wrote: > if (value.compare(0, 5, "sha1/") == 0) { > ... > } else if (value.compare(0, 7, "sha256/") == 0 { > ... > } > if (base64_str.empty()) > return false; > > C++03 (21.3.6.8.1) guarantees that for str.compare(), if str.size() < n, it will > return < 0. See C++03 21.3.1.5 for the further guarantee that it will take > min(size(), n). > > C++03 21.3.6.7.3 guarantees that str2 = str1.substr(str1.size()) is safe and > will return an empty string. StringPiece doesn't have the 3-arg compare() but starts_with seems like a good substitute. (thanks for clarifying substr() as well - it looks like we can remove it here and elsewhere, but me (and maybe Adam?) were thinking str1.substr(str1.size()) would throw.) https://codereview.chromium.org/11274032/diff/80001/net/base/hash_value.cc#ne... net/base/hash_value.cc:65: decoded.size() != size()) { On 2012/12/20 23:44:13, Ryan Sleevi wrote: > style nit: Does this fit on one line? If so, do that, and drop the brackets. Done. https://codereview.chromium.org/11274032/diff/80001/net/http/http_security_he... File net/http/http_security_headers.cc (right): https://codereview.chromium.org/11274032/diff/80001/net/http/http_security_he... net/http/http_security_headers.cc:29: if (!base::StringToInt64(s, &i) && i != kint64max) On 2012/12/20 23:44:13, Ryan Sleevi wrote: > If overflow happens, StringToInt64 will return false (by virtue of > IteratorRangeToNumber::Invoke returning false, by virtue of > IteratorRangeToNumber::Base::Invoke returning false, by virtue of > IteratorRangeToNumber::Positive::CheckBounds returning false. > > Perhaps you meant by the comment "values greater than kMaxHSTSAgeSecs" Clarified the comment. https://codereview.chromium.org/11274032/diff/80001/net/http/http_security_he... net/http/http_security_headers.cc:33: COMPILE_ASSERT(kMaxHSTSAgeSecs <= kuint32max, kMaxHSTSAgeSecsTooLarge); On 2012/12/20 23:44:13, Ryan Sleevi wrote: > nit: You can move this line 33 to line 16 - the location of the COMPILE_ASSERT > in code doesn't matter, and it makes it easier to separate out the logic from > the pre-condition guarantees. Done. https://codereview.chromium.org/11274032/diff/80001/net/http/http_security_he... net/http/http_security_headers.cc:117: if (!base::Base64Decode(unquoted, &decoded)) On 2012/12/20 23:44:13, Ryan Sleevi wrote: > BUG: Please add a check that !unquoted.empty() before calling B64Decode Done.
Still a few nits, but we're in the home stretch. Note the comments about the failure more for StringTo[U]Int64. I'm not sure if that really represents a bug or not (I think we fail in the HTTP case too), but something to be aware of. https://codereview.chromium.org/11274032/diff/82003/chrome/browser/ui/webui/n... File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): https://codereview.chromium.org/11274032/diff/82003/chrome/browser/ui/webui/n... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:134: hash_str.compare(0, 7, "sha256/") != 0) nit: braces here { } https://codereview.chromium.org/11274032/diff/82003/net/http/http_security_he... File net/http/http_security_headers.cc (right): https://codereview.chromium.org/11274032/diff/82003/net/http/http_security_he... net/http/http_security_headers.cc:16: COMPILE_ASSERT(kMaxHSTSAgeSecs <= kuint32max, kMaxHSTSAgeSecsTooLarge); nit: Add a line break here https://codereview.chromium.org/11274032/diff/82003/net/http/http_security_he... net/http/http_security_headers.cc:23: // clip to kMaxHSTSAgeSecs, rather than returning false. So looking over the comment changes: 1) StringToInt64 -> StringToUint64 (problem solved :-p) 2) The reason to use an [u]int64 has nothing to do with the sizing here, and has everything to do with the notion of "Delta Seconds" in RFC 2616, which does not restrict the prevision. HTTP clients 'typically' (always) treat delta-seconds using maximum width integers "MaxAgeToInt converts a string representation of a number of seconds ('delta-seconds', as defined in RFC 2616) into a suitable maximum age. The string may contain an arbitrarily large number, which we clip to kMaxHSTSAgeSecs." https://codereview.chromium.org/11274032/diff/82003/net/http/http_security_he... net/http/http_security_headers.cc:24: You can nuke this extra whitespace. https://codereview.chromium.org/11274032/diff/82003/net/http/http_security_he... net/http/http_security_headers.cc:36: // result value will not be overflowed. You can just nuke this comment, since I think it duplicates the function level comment. Again, you are NOT handling int64 overflow here - when int64 is overflowed, StringTo[U]Int64 returns false ( see https://code.google.com/searchframe#OAMlx_jo-ck/src/base/string_number_conver... ), so it will not be handled.
https://codereview.chromium.org/11274032/diff/82003/chrome/browser/ui/webui/n... File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): https://codereview.chromium.org/11274032/diff/82003/chrome/browser/ui/webui/n... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:134: hash_str.compare(0, 7, "sha256/") != 0) On 2012/12/22 00:56:30, Ryan Sleevi wrote: > nit: braces here { } Done. https://codereview.chromium.org/11274032/diff/82003/net/http/http_security_he... File net/http/http_security_headers.cc (right): https://codereview.chromium.org/11274032/diff/82003/net/http/http_security_he... net/http/http_security_headers.cc:16: COMPILE_ASSERT(kMaxHSTSAgeSecs <= kuint32max, kMaxHSTSAgeSecsTooLarge); On 2012/12/22 00:56:30, Ryan Sleevi wrote: > nit: Add a line break here Done. https://codereview.chromium.org/11274032/diff/82003/net/http/http_security_he... net/http/http_security_headers.cc:23: // clip to kMaxHSTSAgeSecs, rather than returning false. On 2012/12/22 00:56:30, Ryan Sleevi wrote: > So looking over the comment changes: > > 1) StringToInt64 -> StringToUint64 (problem solved :-p) What do you mean? That the code should use StringToUint64? StringToUint64 doesn't return false on negative numbers! (which I found surprising, what's it good for?!) > 2) The reason to use an [u]int64 has nothing to do with the sizing here, and has > everything to do with the notion of "Delta Seconds" in RFC 2616, which does not > restrict the prevision. HTTP clients 'typically' (always) treat delta-seconds > using maximum width integers I'm using it because it's large enough to to contain kuint32max, if there was a StringToUint32 that did the obvious thing (returned false on negatives), I'd use that instead, but there isn't (there's a StringToUint / StringToInt, but are those guaranteed to be 32-bit types?) > "MaxAgeToInt converts a string representation of a number of seconds > ('delta-seconds', as defined in RFC 2616) into a suitable maximum age. The > string may contain an arbitrarily large number, which we clip to > kMaxHSTSAgeSecs. OK, I did something similar to this. https://codereview.chromium.org/11274032/diff/82003/net/http/http_security_he... net/http/http_security_headers.cc:24: On 2012/12/22 00:56:30, Ryan Sleevi wrote: > You can nuke this extra whitespace. Done. https://codereview.chromium.org/11274032/diff/82003/net/http/http_security_he... net/http/http_security_headers.cc:36: // result value will not be overflowed. On 2012/12/22 00:56:30, Ryan Sleevi wrote: > You can just nuke this comment, since I think it duplicates the function level > comment. Reworked the comments to be less redundant, but I think there's value in a comment describing what the function does, as well as an implementation comment describing why/how we're using StringToInt64, because there's a couple non-obvious points (handling of negative inputs, and inputs > kint64max). > Again, you are NOT handling int64 overflow here - when int64 is overflowed, > StringTo[U]Int64 returns false Yes, but in that case StringToInt64 simultaneously sets the output value to kint64max. MaxAgeToInt detects this case and does not treat it as a parse error (see following line). So, the code "handles" arbitrarily large values in the sense that an input greater than kMaxHSTSAgeSecs is clipped to kMaxHSTSAgeSecs, whether that input is also greater than kint64max or not (and the unit test checks both cases).
Oof. I feel your pain for long reviews - especially after I had vowed not to be responsible for them after receiving a 1 year long review. That said, I think this LGTM, with a few nits below. I've submitted tryjobs based on Patch Set 24 to a number of bots. Provided they go green, you should be able to use the Commit Queue, which will automatically land this CL (assuming nothing breaks in the mean time). https://codereview.chromium.org/11274032/diff/97001/net/http/http_security_he... File net/http/http_security_headers.cc (right): https://codereview.chromium.org/11274032/diff/97001/net/http/http_security_he... net/http/http_security_headers.cc:23: // is returned on any parse error. OCD nit: single spaces after period grammar nit: There's been a general comment on chromium-dev about avoiding "we" in comments. This could be reworded as: "The string may contain an arbitrarily large number, which will be clipped to kMaxHSTSAgeSecs and which is guaranteed to fit within a 32-bit unsigned integer. False is returned on any parse error." https://codereview.chromium.org/11274032/diff/97001/net/http/http_security_he... net/http/http_security_headers.cc:36: // through so that i gets clipped to kMaxHSTSAgeSecs. Same comments here as above re: rewording "Return false on any StringToInt64 parse errors *except* for int64 overflow. StringToInt64 is used, rather than StringToUint64, in order to properly handle and reject negative numbers. For values too large to be stored in an int64, StringToInt64 will return kint64max, which is handled explicitly by clipping to kMaxHSTSAgeSecs."
On 2013/01/03 01:11:32, Ryan Sleevi wrote: > I've submitted tryjobs based on Patch Set 24 to a number of bots. They failed, looks like it was due to the patch not applying cleanly, so I sync'd my source tree and submitted patchset #25. patchset #26 has fixes for your nits. Want to try again?
https://codereview.chromium.org/11274032/diff/97001/net/http/http_security_he... File net/http/http_security_headers.cc (right): https://codereview.chromium.org/11274032/diff/97001/net/http/http_security_he... net/http/http_security_headers.cc:23: // is returned on any parse error. On 2013/01/03 01:11:32, Ryan Sleevi wrote: > OCD nit: single spaces after period > grammar nit: There's been a general comment on chromium-dev about avoiding "we" > in comments. This could be reworded as: > > "The string may contain an arbitrarily large number, which will be clipped to > kMaxHSTSAgeSecs and which is guaranteed to fit within a 32-bit unsigned integer. > False is returned on any parse error." Done. https://codereview.chromium.org/11274032/diff/97001/net/http/http_security_he... net/http/http_security_headers.cc:36: // through so that i gets clipped to kMaxHSTSAgeSecs. On 2013/01/03 01:11:32, Ryan Sleevi wrote: > Same comments here as above re: rewording > > "Return false on any StringToInt64 parse errors *except* for int64 overflow. > StringToInt64 is used, rather than StringToUint64, in order to properly handle > and reject negative numbers. For values too large to be stored in an int64, > StringToInt64 will return kint64max, which is handled explicitly by clipping to > kMaxHSTSAgeSecs." Done but edited your wording to be more specific.
https://codereview.chromium.org/11274032/diff/121001/net/http/http_security_h... File net/http/http_security_headers_unittest.cc (right): https://codereview.chromium.org/11274032/diff/121001/net/http/http_security_h... net/http/http_security_headers_unittest.cc:290: std::min(kMaxHSTSAgeSecs, (int64)GG_INT64_C(39408299))); NACK: We don't use C-style casts It seems the ambiguity is in finding the right type-promoting overload, in which case, this should be std::min(static_cast<int64>(kMaxHSTSAgeSecs), GG_INT64_C(...)); (I think the following may also work std::min<int64>(kMaxHSTSAgeSecs, GG_INT64_C(...))
Fix for try failures, hopefully (flying bind, I can't reproduce and don't really understand issue...) https://codereview.chromium.org/11274032/diff/121001/net/http/http_security_h... File net/http/http_security_headers_unittest.cc (right): https://codereview.chromium.org/11274032/diff/121001/net/http/http_security_h... net/http/http_security_headers_unittest.cc:290: std::min(kMaxHSTSAgeSecs, (int64)GG_INT64_C(39408299))); On 2013/01/04 19:46:37, Ryan Sleevi wrote: > NACK: We don't use C-style casts > > It seems the ambiguity is in finding the right type-promoting overload, in which > case, this should be > > std::min(static_cast<int64>(kMaxHSTSAgeSecs), GG_INT64_C(...)); Done (though I think Ryan meant below instead, as kMaxHSTSAgeSecs is already an int64): std::min(kMaxHSTSAgeSecs, static_cast<int64>(GG_INT64_C(...)));
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/unsafe@trevp.net/11274032/136001
Presubmit check for 11274032-136001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** If this change has an associated bug, add BUG=[bug number]. ** Presubmit Warnings ** Found lines longer than 80 characters (first 5 shown). net/base/transport_security_state_static.h, line 14, 87 chars \ net/base/transport_security_state_static.h, line 17, 87 chars \ net/base/transport_security_state_static.h, line 20, 87 chars \ net/base/transport_security_state_static.h, line 23, 87 chars \ net/base/transport_security_state_static.h, line 26, 87 chars unsafe@trevp.net is not in AUTHORS file. If you are a new contributor, please visit http://www.chromium.org/developers/contributing-code and read the "Legal" section If you are a chromite, verify the contributor signed the CLA. ** Presubmit ERRORS ** Missing LGTM from an OWNER for files in these directories: chrome/browser/ui/webui/net_internals Presubmit checks took 2.7s to calculate.
Missed that the long lines need wrapping. Luckily C strings can be continued without special characters. I'm guessing that requires a tweak to the .go file as well Additionally, make sure to edit src/AUTHORS to add yourself. I see you on the CLA, but you'll also need to just add yourself to the bottom.
LGTM for net_internals/* https://chromiumcodereview.appspot.com/11274032/diff/136001/chrome/browser/ui... File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right): https://chromiumcodereview.appspot.com/11274032/diff/136001/chrome/browser/ui... chrome/browser/ui/webui/net_internals/net_internals_ui.cc:137: if (hash_str.compare(0, 5, "sha1/") != 0 && See also StartsWithASCII()
On 2013/01/07 23:55:59, Ryan Sleevi wrote: > Missed that the long lines need wrapping. Luckily C strings can be continued > without special characters. I'm guessing that requires a tweak to the .go file > as well Fixed!, also updated go and sent a pull request to AGL: https://github.com/agl/transport-security-state-generate/pull/1 > Additionally, make sure to edit src/AUTHORS to add yourself. I see you on the > CLA, but you'll also need to just add yourself to the bottom. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/unsafe@trevp.net/11274032/134003
Presubmit check for 11274032-134003 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** If this change has an associated bug, add BUG=[bug number]. ** Presubmit Warnings ** unsafe@trevp.net is not in AUTHORS file. If you are a new contributor, please visit http://www.chromium.org/developers/contributing-code and read the "Legal" section If you are a chromite, verify the contributor signed the CLA. Presubmit checks took 3.1s to calculate.
On 2013/01/08 02:03:06, I haz the power (commit-bot) wrote: > mailto:unsafe@trevp.net is not in AUTHORS file. If you are a new contributor, please Gah, sorry, forgot to add the AUTHORS file, done.
Editing the CL description to be clearer Original description as follows: This is the first in an intended sequence of CLs to refactor TransportSecurityState, fix some book-keeping bugs, and hopefully add TACK. This sequence of CLs will be derived from the original, overly-large CL #11191005. This CL does a few things: - Adds a high-level API for processing HSTS/HPKP headers, eliminating the idea of having "DomainState" objects process the headers, which I think was a bad approach (see below). - Moves the utility code for parsing HSTS/HPKP headers from "transport_security_state" into new "http_security_headers" files. - Moves the utility code for hash values from "x509_cert_types" into new "hash_value" files. - Fixes minor pre-existing issues that surfaced during the code movement / code review: Details: transport_security_state - Factored HPKP/HSTS parsing into http_security_headers - The old API for parsing HPKP/HSTS headers via DomainState objects was not great, since the "DomainState" for a domain is (or at least should be) an aggregate of data from multiple sources (static/dynamic data; superdomains w/includeSubdomains). So it doesn't make sense to have this object process the HTTP headers. Instead, new high-level APIs are added to TransportSecurityState: AddHSTSHeader() and AddHPKPHeader(). url_request_http_job: - Uses new API for processing headers. http_security_headers (new files) - Split off from transport_security_state - In http_security_headers, strtol was replaced with base::StringToInt64. I think this fixes a latent bug which would appear if kMaxHSTSAgeSecs was greater than MAXINT, if longs are a larger type than ints, and if a value was parsed that was greater than MAXINT. - Changed HPKP header parsing to ignore unknown directives, even if they start with "pin-" - Split off http_security_headers_unittests.cc, simplified tests to use fake hash values instead of relying on CertVerifier etc. (which was causing Android problems). transport_security_state_static.h: - Stores static pins as binary instead of base64, saves small amount of time / space / code to base64-decode the relevant pin list on each connection. hash_value (new files) - Split off from x509_cert_types - Added HashValue::FromString() and ToString() which are used in multiple places (net_internals, logging, serializing) transport_security_persister.cc - Uses new HashValue::FromString() and ToString() net_internals_ui - Uses new HashValue::FromString and ToString() - Has a copy of HashesToBase64String() and Base64StringToHashes(); these functions could be pulled into hash_value if they prove useful; currently transport_security_state has its own copy of HashesToBase64String. - Changed parsing of user-input pins to skip over pins with unrecognized hash algorithms, but otherwise reject the entire input on other parsing failures (previously, it skipped over all parsing failures, and tried to process the next pin). x509_cert_types - Factored HashValue / HashValueVector into hash_value Miscellany: - Moving statics to unnamed namespaces, replacing std::string with StringPiece, reformatting lines, etc.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/unsafe@trevp.net/11274032/147002
Failed to apply patch for AUTHORS: While running patch -p0 --forward --force --no-backup-if-mismatch; patching file AUTHORS Hunk #1 FAILED at 214. 1 out of 1 hunk FAILED -- saving rejects to file AUTHORS.rej Patch: AUTHORS Index: AUTHORS =================================================================== --- AUTHORS (revision 175018) +++ AUTHORS (working copy) @@ -214,3 +214,4 @@ Sungguk Lim <limasdf@gmail.com> Martin Bednorz <m.s.bednorz@gmail.com> Kamil Jiwa <kamil.jiwa@gmail.com> +Trevor Perrin <unsafe@trevp.net>
On 2013/01/08 05:36:00, I haz the power (commit-bot) wrote: > Failed to apply patch for AUTHORS: > While running patch -p0 --forward --force --no-backup-if-mismatch; > patching file AUTHORS > Hunk #1 FAILED at 214. > 1 out of 1 hunk FAILED -- saving rejects to file AUTHORS.rej > > Patch: AUTHORS > Index: AUTHORS > =================================================================== > --- AUTHORS (revision 175018) > +++ AUTHORS (working copy) > @@ -214,3 +214,4 @@ > Sungguk Lim <mailto:limasdf@gmail.com> > Martin Bednorz <mailto:m.s.bednorz@gmail.com> > Kamil Jiwa <mailto:kamil.jiwa@gmail.com> > +Trevor Perrin <mailto:unsafe@trevp.net> Heh, you just can't win. Update your checkout (git rebase or svn up, as appropriate)
On 2013/01/08 05:41:33, Ryan Sleevi wrote: > > +Trevor Perrin <mailto:unsafe@trevp.net> > > Heh, you just can't win. Update your checkout (git rebase or svn up, as > appropriate) OK, power-of-2 patchsets, that's gotta be lucky..
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/unsafe@trevp.net/11274032/133003
Retried try job too often on win_aura for step(s) content_browsertests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/unsafe@trevp.net/11274032/133003
Message was sent while issue was closed.
Change committed as 175595 |
