Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
110523006:
Fix the hash generation algorithm to be consistent with prior implementation. (Closed)
|
Created:
6 years, 11 months ago by erikwright (departed) Modified:
6 years, 11 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix the hash generation algorithm to be consistent with prior implementation.
BUG=321680
R=bauerb@chromium.org,gab@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=244512
Patch Set 1 #
Total comments: 8
Patch Set 2 : Add a test that checks values actually generated by the previous implementation. #Patch Set 3 : Back out some unrelated code cleanup. #Patch Set 4 : Correct string constants. #
Total comments: 3
Patch Set 5 : Use Verify instead of manual hash comparison. #
Total comments: 8
Patch Set 6 : Add a comment for GetMessage. #Patch Set 7 : Update comment, order, for gab. #Patch Set 8 : Validates -> Verifies. #
Total comments: 2
Patch Set 9 : Update comment. #Patch Set 10 : Use base::ListValue. #Patch Set 11 : Use base::StringValue. #Patch Set 12 : Fix compile on Android. #
Messages
Total messages: 30 (0 generated)
PTAL. I've updated the tests to protect us from changes going forwards, but I am going to go back to the previous implementation and generate some data to prove consistency with that, too.
Let's keep this change as small as possible, keeping cleanup tasks for later when the regression test coverage has improved. https://codereview.chromium.org/110523006/diff/1/chrome/browser/prefs/pref_ha... File chrome/browser/prefs/pref_hash_calculator.cc (left): https://codereview.chromium.org/110523006/diff/1/chrome/browser/prefs/pref_ha... chrome/browser/prefs/pref_hash_calculator.cc:86: const std::string& path, const base::Value* value) const { Why move this? The move doesn't appear to be related to the problem at hand. I would leave it here (although it appears the |path| arg isn't required, feel free to clean that up) at least for now. https://codereview.chromium.org/110523006/diff/1/chrome/browser/prefs/pref_ha... File chrome/browser/prefs/pref_hash_calculator.cc (right): https://codereview.chromium.org/110523006/diff/1/chrome/browser/prefs/pref_ha... chrome/browser/prefs/pref_hash_calculator.cc:22: std::vector<uint8> digest(hmac.DigestLength()); This is nicer than the previous method hardcoding the digest size, etc., but is it required for this CL? I would like to keep cleanup tasks separate and do them only once we have confirmed that we are back to a working state and are confident that we have tests making sure future changes do not break existing hashes.
https://codereview.chromium.org/110523006/diff/1/chrome/browser/prefs/pref_ha... File chrome/browser/prefs/pref_hash_calculator.cc (right): https://codereview.chromium.org/110523006/diff/1/chrome/browser/prefs/pref_ha... chrome/browser/prefs/pref_hash_calculator.cc:23: if (!hmac.Init(key) || !hmac.Sign(message, &digest[0], digest.size())) { When does this fail? Is it really necessary to handle the DCHECK failure in production code? https://codereview.chromium.org/110523006/diff/1/chrome/browser/prefs/pref_ha... File chrome/browser/prefs/pref_hash_calculator_unittest.cc (right): https://codereview.chromium.org/110523006/diff/1/chrome/browser/prefs/pref_ha... chrome/browser/prefs/pref_hash_calculator_unittest.cc:13: const char kSeed[] = "0123456789ABCDEF0123456789ABCDEF"; The lone namespace here is unnecessary; const variables automatically get internal linkage.
PTAL. https://codereview.chromium.org/110523006/diff/1/chrome/browser/prefs/pref_ha... File chrome/browser/prefs/pref_hash_calculator.cc (right): https://codereview.chromium.org/110523006/diff/1/chrome/browser/prefs/pref_ha... chrome/browser/prefs/pref_hash_calculator.cc:22: std::vector<uint8> digest(hmac.DigestLength()); On 2014/01/07 19:18:07, gab wrote: > This is nicer than the previous method hardcoding the digest size, etc., but is > it required for this CL? > > I would like to keep cleanup tasks separate and do them only once we have > confirmed that we are back to a working state and are confident that we have > tests making sure future changes do not break existing hashes. We now have two HMAC calculations. They are virtually identical and rather than have duplicate code it made sense to factor this out. It's not really a cleanup, it's the right way to add this functionality. https://codereview.chromium.org/110523006/diff/1/chrome/browser/prefs/pref_ha... chrome/browser/prefs/pref_hash_calculator.cc:23: if (!hmac.Init(key) || !hmac.Sign(message, &digest[0], digest.size())) { On 2014/01/07 19:20:45, Bernhard Bauer wrote: > When does this fail? Is it really necessary to handle the DCHECK failure in > production code? It should presumably never fail, but it's inappropriate to ignore the result of these methods. To me that's the definition of NOTREACHED. In release, NOTREACHED() is nothing. I think this is quite common in Chromium (a NOTREACHED that really will never be reached in production code). See for example: https://code.google.com/p/chromium/codesearch#chromium/src/crypto/hmac.cc&q=N...
lgtm, thanks! https://codereview.chromium.org/110523006/diff/140001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_calculator_unittest.cc (right): https://codereview.chromium.org/110523006/diff/140001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_calculator_unittest.cc:105: TEST(PrefHashCalculatorTest, TestCompatibilityWithPrefMetricsService) { optional: s/TestCompatibilityWithPrefMetricsService/TestCompatibilityWithOldPrefMetricsService
https://codereview.chromium.org/110523006/diff/1/chrome/browser/prefs/pref_ha... File chrome/browser/prefs/pref_hash_calculator.cc (right): https://codereview.chromium.org/110523006/diff/1/chrome/browser/prefs/pref_ha... chrome/browser/prefs/pref_hash_calculator.cc:22: std::vector<uint8> digest(hmac.DigestLength()); On 2014/01/08 22:11:24, erikwright wrote: > On 2014/01/07 19:18:07, gab wrote: > > This is nicer than the previous method hardcoding the digest size, etc., but > is > > it required for this CL? > > > > I would like to keep cleanup tasks separate and do them only once we have > > confirmed that we are back to a working state and are confident that we have > > tests making sure future changes do not break existing hashes. > > We now have two HMAC calculations. They are virtually identical and rather than > have duplicate code it made sense to factor this out. It's not really a cleanup, > it's the right way to add this functionality. Of course, my bad, I'd overlooked this initially.
https://codereview.chromium.org/110523006/diff/1/chrome/browser/prefs/pref_ha... File chrome/browser/prefs/pref_hash_calculator.cc (right): https://codereview.chromium.org/110523006/diff/1/chrome/browser/prefs/pref_ha... chrome/browser/prefs/pref_hash_calculator.cc:23: if (!hmac.Init(key) || !hmac.Sign(message, &digest[0], digest.size())) { On 2014/01/08 22:11:24, erikwright wrote: > On 2014/01/07 19:20:45, Bernhard Bauer wrote: > > When does this fail? Is it really necessary to handle the DCHECK failure in > > production code? > > It should presumably never fail, but it's inappropriate to ignore the result of > these methods. To me that's the definition of NOTREACHED. In release, > NOTREACHED() is nothing. > > I think this is quite common in Chromium (a NOTREACHED that really will never be > reached in production code). See for example: > > https://code.google.com/p/chromium/codesearch#chromium/src/crypto/hmac.cc&q=N... Yes, but in production the code will look like this: if (<signing fails>) { return std::string(); } Basically, you're handling failure of a condition that can never fail, which is something you're not supposed to do (see http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-). What you can do instead is put the result into a boolean variable, then DCHECK that.
https://codereview.chromium.org/110523006/diff/140001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_calculator.cc (right): https://codereview.chromium.org/110523006/diff/140001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_calculator.cc:92: if (hash == Calculate(path, value)) SECURITY BUG: Use HMAC::Verify() for this. This is an anti-pattern to compare strings like this and can lead to security issues.
On 2014/01/09 08:51:35, Bernhard Bauer wrote: > https://codereview.chromium.org/110523006/diff/1/chrome/browser/prefs/pref_ha... > File chrome/browser/prefs/pref_hash_calculator.cc (right): > > https://codereview.chromium.org/110523006/diff/1/chrome/browser/prefs/pref_ha... > chrome/browser/prefs/pref_hash_calculator.cc:23: if (!hmac.Init(key) || > !hmac.Sign(message, &digest[0], digest.size())) { > On 2014/01/08 22:11:24, erikwright wrote: > > On 2014/01/07 19:20:45, Bernhard Bauer wrote: > > > When does this fail? Is it really necessary to handle the DCHECK failure in > > > production code? > > > > It should presumably never fail, but it's inappropriate to ignore the result > of > > these methods. To me that's the definition of NOTREACHED. In release, > > NOTREACHED() is nothing. > > > > I think this is quite common in Chromium (a NOTREACHED that really will never > be > > reached in production code). See for example: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/crypto/hmac.cc&q=N... > > Yes, but in production the code will look like this: > > if (<signing fails>) { > return std::string(); > } > > Basically, you're handling failure of a condition that can never fail, which is > something you're not supposed to do (see > http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-). > What you can do instead is put the result into a boolean variable, then DCHECK > that. I want to back off on "can never fail". If I read the documentation there is little indication as to what kinds of failure conditions might exist and whether they would be persistent or transient. If I read the implementations, I can see that it is considered highly unlikely by the implementor (NOTREACHED() for most cases) but not so unlikely that they felt comfortable removing the return code. I sent a query to the OWNERS but in the meantime I think it is correct to return std::string() in this case rather than falling through and returning the hex representation of the potentially uninitialized contents of |digest|. And I would want to have it die in debug mode to catch the case where this actually starts happening regularly. If you like, I could make that a DLOG(FATAL). That would seem to be consistent with the style guideline you referenced.
On 2014/01/09 08:51:35, Bernhard Bauer wrote: > https://codereview.chromium.org/110523006/diff/1/chrome/browser/prefs/pref_ha... > File chrome/browser/prefs/pref_hash_calculator.cc (right): > > https://codereview.chromium.org/110523006/diff/1/chrome/browser/prefs/pref_ha... > chrome/browser/prefs/pref_hash_calculator.cc:23: if (!hmac.Init(key) || > !hmac.Sign(message, &digest[0], digest.size())) { > On 2014/01/08 22:11:24, erikwright wrote: > > On 2014/01/07 19:20:45, Bernhard Bauer wrote: > > > When does this fail? Is it really necessary to handle the DCHECK failure in > > > production code? > > > > It should presumably never fail, but it's inappropriate to ignore the result > of > > these methods. To me that's the definition of NOTREACHED. In release, > > NOTREACHED() is nothing. > > > > I think this is quite common in Chromium (a NOTREACHED that really will never > be > > reached in production code). See for example: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/crypto/hmac.cc&q=N... > > Yes, but in production the code will look like this: > > if (<signing fails>) { > return std::string(); > } > > Basically, you're handling failure of a condition that can never fail, which is > something you're not supposed to do (see > http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-). > What you can do instead is put the result into a boolean variable, then DCHECK > that. I disagree on the "condition that can never fail", especially if handling 'untrusted' input as the key or data.
Replaced manual hash comparison with Verify. https://codereview.chromium.org/110523006/diff/140001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_calculator.cc (right): https://codereview.chromium.org/110523006/diff/140001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_calculator.cc:92: if (hash == Calculate(path, value)) On 2014/01/09 20:51:14, Ryan Sleevi (OoO until 1-9) wrote: > SECURITY BUG: Use HMAC::Verify() for this. This is an anti-pattern to compare > strings like this and can lead to security issues. Done.
https://codereview.chromium.org/110523006/diff/240001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_calculator.h (right): https://codereview.chromium.org/110523006/diff/240001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_calculator.h:28: PrefHashCalculator(const std::string& seed, const std::string& device_id); naming nit: |seed| is usually used as the base for some derivation, but within the implementation, you're using |seed| directly as the key. Likewise, the naming (PrefHashCalculator) implies a hash, but you're actually generating a MAC/MIC - Message Authentication/Integrity Code. PrefMacCalculator, perhaps?
https://codereview.chromium.org/110523006/diff/240001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_calculator.h (right): https://codereview.chromium.org/110523006/diff/240001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_calculator.h:28: PrefHashCalculator(const std::string& seed, const std::string& device_id); On 2014/01/09 22:35:16, Ryan Sleevi (OoO until 1-9) wrote: > naming nit: |seed| is usually used as the base for some derivation, but within > the implementation, you're using |seed| directly as the key. > > Likewise, the naming (PrefHashCalculator) implies a hash, but you're actually > generating a MAC/MIC - Message Authentication/Integrity Code. > > PrefMacCalculator, perhaps? Yea, that had been bugging me too, but the term "hash" has been used for this entire feature all over the place (code, comments, docs) since it was only a simple MAC based tracker. Hence I didn't insist with changing the naming scheme when me and Erik took ownership. Perhaps we should change the naming scheme but definitely not in this CL. To be fair I don't think it matters, to me a hash is a deterministic short representation of some data which a MAC also is despite having extra characteristics.
On 2014/01/09 23:03:45, gab wrote: > https://codereview.chromium.org/110523006/diff/240001/chrome/browser/prefs/pr... > File chrome/browser/prefs/pref_hash_calculator.h (right): > > https://codereview.chromium.org/110523006/diff/240001/chrome/browser/prefs/pr... > chrome/browser/prefs/pref_hash_calculator.h:28: PrefHashCalculator(const > std::string& seed, const std::string& device_id); > On 2014/01/09 22:35:16, Ryan Sleevi (OoO until 1-9) wrote: > > naming nit: |seed| is usually used as the base for some derivation, but within > > the implementation, you're using |seed| directly as the key. > > > > Likewise, the naming (PrefHashCalculator) implies a hash, but you're actually > > generating a MAC/MIC - Message Authentication/Integrity Code. > > > > PrefMacCalculator, perhaps? > > Yea, that had been bugging me too, but the term "hash" has been used for this > entire feature all over the place (code, comments, docs) since it was only a > simple MAC based tracker. > > Hence I didn't insist with changing the naming scheme when me and Erik took > ownership. > > Perhaps we should change the naming scheme but definitely not in this CL. > > To be fair I don't think it matters, to me a hash is a deterministic short > representation of some data which a MAC also is despite having extra > characteristics. I'm in favor of consistency, clarity, and accuracy. I think there is probably some term other than Hash that is most appropriate. The same can be said for Seed. Hash ----- Our usage is not inconsistent with the word hash, according to Wikipedia, at least. http://en.wikipedia.org/wiki/Cryptographic_hash_function "A cryptographic hash function is a hash function that takes an arbitrary block of data and returns a fixed-size bit string, the cryptographic hash value, such that any (accidental or intentional) change to the data will (with very high probability) change the hash value. The data to be encoded are often called the message, and the hash value is sometimes called the message digest or simply digest." http://en.wikipedia.org/wiki/Hash_function "The values returned by a hash function are called ... or simply hashes." http://en.wikipedia.org/wiki/Message_authentication_code "A MAC algorithm [is] sometimes called a keyed (cryptographic) hash function" So my interpretation, as gab@'s, is that Hash is not wrong, but that MAC would be more descriptive. We can look at updating the terminology a bit later.
https://codereview.chromium.org/110523006/diff/240001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_calculator.cc (right): https://codereview.chromium.org/110523006/diff/240001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_calculator.cc:24: if (!hmac.Init(key) || !hmac.Sign(message, &digest[0], digest.size())) { After discussion with Ryan, I understand the following: Essentially any cryptographic operation can fail at any time, either due to invalid parameters or system configuration. Configuration problems would be persistent barring configuration changes. Invalid parameters mean that one operation could fail (repeatably with the same inputs) while a subsequent operation with different inputs might succeed. Untrusted parameters could be exploited to cause an operation to fail, in particular if they exceed size limits. Our only untrusted parameter is the preference value. If, somehow, it prevented verification of a signature we would discard the preference value anyway, so we would ideally just prevent a settings change to a value that is incompatible with our cryptographic operations. On the other hand, if we allow it to be set, it will be interpreted as an external change on next launch. If a configuration failure were to occur, however, we would neither be able to store, nor verify, any MACs. All tracked preferences would appear to have been externally changed at next launch. We cannot distinguish a system failure from an invalid hash as both would lead to a 'false' return from Verify. In the face of a failure to Verify, then, we must assume the value is changed. In the face of that, there is little harm of storing an empty MAC when an operation fails while a preference is updated. I suggest to DLOG(FATAL) for any failure from Init or Sign so that we catch programming errors on bots.
lgtm https://codereview.chromium.org/110523006/diff/240001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_calculator.cc (right): https://codereview.chromium.org/110523006/diff/240001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_calculator.cc:24: if (!hmac.Init(key) || !hmac.Sign(message, &digest[0], digest.size())) { On 2014/01/10 15:20:02, erikwright wrote: > After discussion with Ryan, I understand the following: > > Essentially any cryptographic operation can fail at any time, either due to > invalid parameters or system configuration. Configuration problems would be > persistent barring configuration changes. Invalid parameters mean that one > operation could fail (repeatably with the same inputs) while a subsequent > operation with different inputs might succeed. > > Untrusted parameters could be exploited to cause an operation to fail, in > particular if they exceed size limits. Our only untrusted parameter is the > preference value. If, somehow, it prevented verification of a signature we would > discard the preference value anyway, so we would ideally just prevent a settings > change to a value that is incompatible with our cryptographic operations. On the > other hand, if we allow it to be set, it will be interpreted as an external > change on next launch. > > If a configuration failure were to occur, however, we would neither be able to > store, nor verify, any MACs. All tracked preferences would appear to have been > externally changed at next launch. > > We cannot distinguish a system failure from an invalid hash as both would lead > to a 'false' return from Verify. > > In the face of a failure to Verify, then, we must assume the value is changed. > In the face of that, there is little harm of storing an empty MAC when an > operation fails while a preference is updated. > > I suggest to DLOG(FATAL) for any failure from Init or Sign so that we catch > programming errors on bots. NOTREACHED() is fine with me.
Ryan, PTAL.
lgtm w/ nits (and offline question) https://codereview.chromium.org/110523006/diff/240001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_calculator.cc (right): https://codereview.chromium.org/110523006/diff/240001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_calculator.cc:31: // Validates an HMAC of |message| using |key|, encoded as a hexadecimal string. Seems like you just took GetDigestString's comment and changed the verb for "Validates"; please update this comment (in particular add a section for |digest_string|). https://codereview.chromium.org/110523006/diff/240001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_calculator.cc:107: std::string PrefHashCalculator::GetMessage(const std::string& path, Put GetMessage() impl at bottom of this file to match header ordering.
PTAL. https://codereview.chromium.org/110523006/diff/240001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_calculator.cc (right): https://codereview.chromium.org/110523006/diff/240001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_calculator.cc:31: // Validates an HMAC of |message| using |key|, encoded as a hexadecimal string. On 2014/01/10 15:56:45, gab wrote: > Seems like you just took GetDigestString's comment and changed the verb for > "Validates"; please update this comment (in particular add a section for > |digest_string|). I did, indeed. This method is GetDigestString with the action of "Get" replaced by "Validate" so it makes sense that the comments would be analogously similar. But I understand that you would like the string '|digest_string|' to appear in the comments, so I have updated it. Let me know if you had something else in mind. https://codereview.chromium.org/110523006/diff/240001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_calculator.cc:107: std::string PrefHashCalculator::GetMessage(const std::string& path, On 2014/01/10 15:56:45, gab wrote: > Put GetMessage() impl at bottom of this file to match header ordering. Done.
re-lgtm w/ minor comment nit https://codereview.chromium.org/110523006/diff/440001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_calculator.h (right): https://codereview.chromium.org/110523006/diff/440001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_calculator.h:42: // Concatenates path and value to give the hash input. It also adds device_id_.
crypto/ use LGTM https://codereview.chromium.org/110523006/diff/440001/chrome/browser/prefs/pr... File chrome/browser/prefs/pref_hash_calculator.cc (right): https://codereview.chromium.org/110523006/diff/440001/chrome/browser/prefs/pr... chrome/browser/prefs/pref_hash_calculator.cc:114: if (VerifyLegacyHash(seed_, value, digest_string)) There's a slight cryptographic risk in that you're re-using |seed| as the key for both hashes, so an attacker gets two guesses for the price of one. This is a minor risk, and is why you would normally use either different keys or use |seed_| as an actual seed for a KDF to derive a version/scheme specific key. That said, this is not a show-stopper for this purpose.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/110523006/520001
Retried try job too often on linux_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/110523006/720001
Retried try job too often on mac_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/110523006/890001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/110523006/890001
Retried try job too often on android_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/erikwright@chromium.org/110523006/1100001
Message was sent while issue was closed.
Change committed as 244512 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
