Only store leading 13 bits of password hash.

Only store leading 13 bits of password hash.

Also, take ownership for a related but un-owned histogram.

BUG=433180
TBR=sky@chromium.org

Committed: https://crrev.com/c511691cc06fd3a92f27e688795872e61efff40a
Cr-Commit-Position: refs/heads/master@{#314168}

[Mike Lerman, 2015-01-21 20:37:14]

mlerman@chromium.org changed reviewers:
+ bcwhite@chromium.org, jwd@chromium.org, jww@chromium.org

[Mike Lerman, 2015-01-21 20:37:15]

Hi reviewers,

PTAL at my CL. Thanks!

jwd - histograms
bcwhite + jwd - local_auth*

thanks!

[bcwhite, 2015-01-21 21:04:40]

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
File chrome/browser/signin/local_auth.cc (right):

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
chrome/browser/signin/local_auth.cc:36: const unsigned kHash1StoredBits = 13;
"Hash2"  This is storage method #2.  Use the same names as above (where
applicable) but change "1" to "2".

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
chrome/browser/signin/local_auth.cc:184: if (encoding == '1') {
The handling of v1 encoding should never change.  New saves need to use a new
encoding.

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
chrome/browser/signin/local_auth.cc:220: std::string TruncateStringByBits(const
std::string& str,
Rest of the file calls upward so put this function higher up, before the call to
it.

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
chrome/browser/signin/local_auth.cc:230: unsigned last_char_bitmask = pow(2,
len_bits % 8) - 1;
pow(2, len_bits % 8)  ==  (1 << (len_bits % 8))

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
File chrome/browser/signin/local_auth.h (right):

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
chrome/browser/signin/local_auth.h:37: std::string TruncateStringByBits(const
std::string& str, const size_t len_bits);
Why does this need to be public?

[jwd, 2015-01-22 21:23:45]

histograms LGTM

[jww, 2015-01-22 21:48:19]

+1 to bcwhite's comments, plus a few more.

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
File chrome/browser/signin/local_auth.cc (right):

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
chrome/browser/signin/local_auth.cc:194: password_hash =
CreateSecurePasswordHash(salt_str, password, encoding);
Given bcwhite's comment about never changing the handling of v1 encodings (which
seems correct to me as well), it might be clearer if CreateSecurePasswordHash()
uses the passed in encoding type and, for v2 encodings (that is, your new 13 bit
hash value) just calls TruncateStringByBits() itself. Then, either way, you get
the a password_hash value that you handle the same way.

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
File chrome/browser/signin/local_auth_unittest.cc (right):

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
chrome/browser/signin/local_auth_unittest.cc:61: std::string two_chars = "AB";
It seems like you might want these tests to actually be of
ValidateLocalAuthCredentials since that's the more public API (and
TruncateStringTo16Bits is used by that function exclusively anyway).

[bcwhite, 2015-01-23 01:28:38]

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
File chrome/browser/signin/local_auth.cc (right):

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
chrome/browser/signin/local_auth.cc:194: password_hash =
CreateSecurePasswordHash(salt_str, password, encoding);
I didn't pass an encoding version to "create" because I assumed that Chrome
would always be able to create only one type -- the latest.  This could be
changed if necessary but it allows ripping out code that is no longer used.

[jww, 2015-01-26 18:31:51]

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
File chrome/browser/signin/local_auth.cc (right):

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
chrome/browser/signin/local_auth.cc:194: password_hash =
CreateSecurePasswordHash(salt_str, password, encoding);
On 2015/01/23 01:28:38, bcwhite wrote:
> I didn't pass an encoding version to "create" because I assumed that Chrome
> would always be able to create only one type -- the latest.  This could be
> changed if necessary but it allows ripping out code that is no longer used.

CreateSecurePasswordHash() is used to validate the hash (as well as to store it
in the first place). And, as you pointed out before, we have to keep the v1
validation for those with old hash versions lying around anyway, so we'll need
to handle CreateSecurePasswordHash() for both v1 and v2 anyway, it's just a
matter of if that's done surrounding the calls to CreateSecurePasswordHash or
internally. It seems cleaner to me to do it inside CreateSecurePasswordHash, but
I certainly don't feel strongly about it.

[bcwhite, 2015-01-27 18:02:25]

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
File chrome/browser/signin/local_auth.cc (right):

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
chrome/browser/signin/local_auth.cc:194: password_hash =
CreateSecurePasswordHash(salt_str, password, encoding);
I guess I always imagined that a new method would involve a significantly
different function.  No problem passing it though since this one is so similar.

[Mike Lerman, 2015-01-27 21:02:27]

Create a HashEncoding object to track versions better. Thanks for the feedback!

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
File chrome/browser/signin/local_auth.cc (right):

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
chrome/browser/signin/local_auth.cc:36: const unsigned kHash1StoredBits = 13;
On 2015/01/21 21:04:40, bcwhite wrote:
> "Hash2"  This is storage method #2.  Use the same names as above (where
> applicable) but change "1" to "2".

Done.

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
chrome/browser/signin/local_auth.cc:184: if (encoding == '1') {
On 2015/01/21 21:04:40, bcwhite wrote:
> The handling of v1 encoding should never change.  New saves need to use a new
> encoding.

Done.

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
chrome/browser/signin/local_auth.cc:194: password_hash =
CreateSecurePasswordHash(salt_str, password, encoding);
On 2015/01/27 18:02:25, bcwhite wrote:
> I guess I always imagined that a new method would involve a significantly
> different function.  No problem passing it though since this one is so
similar.

Ok, CreateSecurePasswordHash will call Truncate.

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
chrome/browser/signin/local_auth.cc:220: std::string TruncateStringByBits(const
std::string& str,
On 2015/01/21 21:04:40, bcwhite wrote:
> Rest of the file calls upward so put this function higher up, before the call
to
> it.

Done.

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
chrome/browser/signin/local_auth.cc:230: unsigned last_char_bitmask = pow(2,
len_bits % 8) - 1;
On 2015/01/21 21:04:40, bcwhite wrote:
> pow(2, len_bits % 8)  ==  (1 << (len_bits % 8))

Done.

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
File chrome/browser/signin/local_auth.h (right):

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
chrome/browser/signin/local_auth.h:37: std::string TruncateStringByBits(const
std::string& str, const size_t len_bits);
On 2015/01/21 21:04:40, bcwhite wrote:
> Why does this need to be public?

So I can unit test it.

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
File chrome/browser/signin/local_auth_unittest.cc (right):

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
chrome/browser/signin/local_auth_unittest.cc:61: std::string two_chars = "AB";
TruncateStringTo16Bit was a tricky method to write and I really want to have
unit tests that are specific to it. It would be hard to unit test it effectively
through only calling ValidateLocalAuthCredentials.

I can additionally make unit tests for ValidateLocalAuthCredentials if you see a
need, although the flow's already tested above.

[bcwhite, 2015-01-28 15:34:12]

https://codereview.chromium.org/862103002/diff/100001/chrome/browser/signin/l...
File chrome/browser/signin/local_auth.cc (right):

https://codereview.chromium.org/862103002/diff/100001/chrome/browser/signin/l...
chrome/browser/signin/local_auth.cc:25: struct HashEncoding {
I'm not sure it's worth the trouble to create an array of information like this.
 You're making assumptions about the future that everything will fit into this
model.  Plus, there will likely never be more than 2 encoding supported at any
one time: the current and the previous (for upgrade purposes).

Given the security aspects of this, there is certain benefit to clearly
delineating each encoding method such that it can easily be evaluated.

[Mike Lerman, 2015-01-28 15:38:38]

https://codereview.chromium.org/862103002/diff/100001/chrome/browser/signin/l...
File chrome/browser/signin/local_auth.cc (right):

https://codereview.chromium.org/862103002/diff/100001/chrome/browser/signin/l...
chrome/browser/signin/local_auth.cc:25: struct HashEncoding {
On 2015/01/28 15:34:12, bcwhite wrote:
> I'm not sure it's worth the trouble to create an array of information like
this.
>  You're making assumptions about the future that everything will fit into this
> model.  Plus, there will likely never be more than 2 encoding supported at any
> one time: the current and the previous (for upgrade purposes).
> 
> Given the security aspects of this, there is certain benefit to clearly
> delineating each encoding method such that it can easily be evaluated.

We'll always need to support older versions. I may lock my Chrome and not come
back to it for a while - perhaps several releases and iterations later (visiting
a relatives'... whatever). I'll always need to backward support all releases. If
things need to be added that don't fit this model, we'll create more variables,
or cross that bridge when we come to it.

Having a nice structure makes the rest of the code much easier, other than
having little if statements everywhere. This is much more readable down below.

As for evaluating each encoding method as a unit, I agree, and that's why I kept
each block of constants defined, going so far as to defined kHash2Bits = 256,
even though it's the same as kHash1Bits. That way, you can easily review Hash2
as a complete encoding entity.

[bcwhite, 2015-01-28 16:01:36]

https://codereview.chromium.org/862103002/diff/100001/chrome/browser/signin/l...
File chrome/browser/signin/local_auth.cc (right):

https://codereview.chromium.org/862103002/diff/100001/chrome/browser/signin/l...
chrome/browser/signin/local_auth.cc:25: struct HashEncoding {
Not true.  The locally saved hash is required only for offline authentication. 
In the case of not logging in over the span of months, falling back to online
authentication is not unreasonable.  In the other case of upgrading Chrome by
multiple versions then it's also reasonable.

I'd rather have a few *big* if statements so an auditor can clearly follow
exactly what Encoding-v2 does without having to constantly reference how a
structure was datafilled.  It also makes it easier to remove older encodings
that are no longer needed.

[Mike Lerman, 2015-01-28 16:45:56]

https://codereview.chromium.org/862103002/diff/100001/chrome/browser/signin/l...
File chrome/browser/signin/local_auth.cc (right):

https://codereview.chromium.org/862103002/diff/100001/chrome/browser/signin/l...
chrome/browser/signin/local_auth.cc:25: struct HashEncoding {
On 2015/01/28 16:01:35, bcwhite wrote:
> Not true.  The locally saved hash is required only for offline authentication.

> In the case of not logging in over the span of months, falling back to online
> authentication is not unreasonable.  In the other case of upgrading Chrome by
> multiple versions then it's also reasonable.
> 
> I'd rather have a few *big* if statements so an auditor can clearly follow
> exactly what Encoding-v2 does without having to constantly reference how a
> structure was datafilled.  It also makes it easier to remove older encodings
> that are no longer needed.

The way an auditor would reference each encoding's configuration is identical!
In either case, the details are stored in a big block of comments, either lines
51-54 or lines 58-63, depending on your desired encoding.

I'd still rather keep the door open for backward compatibility. We're going to
the effort to support the offline auth case, let's do it properly. There's no
gain, at this point, in drawing an arbitrary line to say we'll backward support
1 version but not 0 or 2. Especially since some channels upgrade more rapidly -
if we run into a problem with an encoding and need to modify it in a subsequent
release, our canary and dev channels could get burned. I just don't see a
technical reason to not, at least right now, build in multiple version - it
benefits our users and is, at this point, still maintainable.

An unrelated point is that having this data structure makes the rest of the code
much, much more readable and also more maintainable (future encodings won't need
any changes to the body of the code, unless they include more features).

[jww, 2015-01-29 11:44:45]

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
File chrome/browser/signin/local_auth.h (right):

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
chrome/browser/signin/local_auth.h:37: std::string TruncateStringByBits(const
std::string& str, const size_t len_bits);
On 2015/01/27 21:02:26, Mike Lerman wrote:
> On 2015/01/21 21:04:40, bcwhite wrote:
> > Why does this need to be public?
> 
> So I can unit test it. 

For unit tests, I've generally made methods private and then used the
FRIEND_TEST_* macros to make the tests friends of the class I want to test. See
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ssl...
for an example of this use.

My rule of thumb is that if it's not a public API outside of unit tests, it
should be private/protected.

https://codereview.chromium.org/862103002/diff/100001/chrome/browser/signin/l...
File chrome/browser/signin/local_auth.cc (right):

https://codereview.chromium.org/862103002/diff/100001/chrome/browser/signin/l...
chrome/browser/signin/local_auth.cc:190: DCHECK_EQ(encoding.hash_bytes,
salt_str.length());
This seems to me like an unnecessary DCHECK. This is testing a property that the
API of crypto::RandBytes promises, so if anyone is going to DCHECK this, it
should be crypto::RandBytes, not the caller.

[Mike Lerman, 2015-01-29 16:57:42]

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
File chrome/browser/signin/local_auth.h (right):

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
chrome/browser/signin/local_auth.h:37: std::string TruncateStringByBits(const
std::string& str, const size_t len_bits);
On 2015/01/29 11:44:45, jww wrote:
> On 2015/01/27 21:02:26, Mike Lerman wrote:
> > On 2015/01/21 21:04:40, bcwhite wrote:
> > > Why does this need to be public?
> > 
> > So I can unit test it. 
> 
> For unit tests, I've generally made methods private and then used the
> FRIEND_TEST_* macros to make the tests friends of the class I want to test.
See
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ssl...
> for an example of this use.
> 
> My rule of thumb is that if it's not a public API outside of unit tests, it
> should be private/protected.  

I would - if this was a class. Sadly, it's a namespace. Unless I make it a class
with static methods... I can do that.

https://codereview.chromium.org/862103002/diff/100001/chrome/browser/signin/l...
File chrome/browser/signin/local_auth.cc (right):

https://codereview.chromium.org/862103002/diff/100001/chrome/browser/signin/l...
chrome/browser/signin/local_auth.cc:190: DCHECK_EQ(encoding.hash_bytes,
salt_str.length());
On 2015/01/29 11:44:45, jww wrote:
> This seems to me like an unnecessary DCHECK. This is testing a property that
the
> API of crypto::RandBytes promises, so if anyone is going to DCHECK this, it
> should be crypto::RandBytes, not the caller.

Done. I ain't touching crypto:: anything :)

[jww, 2015-01-29 17:40:57]

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
File chrome/browser/signin/local_auth.h (right):

https://codereview.chromium.org/862103002/diff/40001/chrome/browser/signin/lo...
chrome/browser/signin/local_auth.h:37: std::string TruncateStringByBits(const
std::string& str, const size_t len_bits);
On 2015/01/29 16:57:42, Mike Lerman wrote:
> On 2015/01/29 11:44:45, jww wrote:
> > On 2015/01/27 21:02:26, Mike Lerman wrote:
> > > On 2015/01/21 21:04:40, bcwhite wrote:
> > > > Why does this need to be public?
> > > 
> > > So I can unit test it. 
> > 
> > For unit tests, I've generally made methods private and then used the
> > FRIEND_TEST_* macros to make the tests friends of the class I want to test.
> See
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ssl...
> > for an example of this use.
> > 
> > My rule of thumb is that if it's not a public API outside of unit tests, it
> > should be private/protected.  
> 
> I would - if this was a class. Sadly, it's a namespace. Unless I make it a
class
> with static methods... I can do that.

Lulz, totally missed that. Nevermind :-)

[Mike Lerman, 2015-01-29 22:26:57]

Brian, we spoke offline, and mostly agreed to disagree.

Any other comments, Brian or Joel?

[jww, 2015-01-30 09:48:15]

lgtm, modulo bcwhite's comments.

[bcwhite, 2015-02-02 15:25:47]

lgtm

[Mike Lerman, 2015-02-02 15:31:54]

mlerman@chromium.org changed reviewers:
+ sky@chromium.org

[Mike Lerman, 2015-02-02 15:31:54]

Hi Sky,

I need a rubber-stamp review in c/b/ui, I changed a namespace call to a static
class call. Thanks!

[Mike Lerman, 2015-02-02 16:19:28]

Actually Sky, even less, i'm just removing some un-needed headers from c/b/ui
files, no actual code changes beyond that. Moving you to TBR for that.

[Mike Lerman, 2015-02-02 16:19:33]

The CQ bit was checked by mlerman@chromium.org

[commit-bot: I haz the power, 2015-02-02 16:20:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/862103002/120001

[commit-bot: I haz the power, 2015-02-02 18:16:26]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-02 18:18:02]

Patchset 7 (id:??) landed as
https://crrev.com/c511691cc06fd3a92f27e688795872e61efff40a
Cr-Commit-Position: refs/heads/master@{#314168}