Remove chromeos::CryptohomeLibrary::HashPassword

Move chromeos::CryptohomeLibrary::HashPassword to chromeos::ParallelAuthenticator

This is a part of effort to remove CrosLibrary and its belongings.

BUG=126719
TEST=None


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146055

[stevenjb, 2012-07-03 15:31:14]

LGTM, just one nit and one clarification comment request.

http://codereview.chromium.org/10701075/diff/3001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/parallel_authenticator.cc (right):

http://codereview.chromium.org/10701075/diff/3001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/parallel_authenticator.cc:171: const int
kPassHashLen = 32;
nit: constant at top of file

http://codereview.chromium.org/10701075/diff/3001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/parallel_authenticator.cc:182:
sizeof(passhash_buf) / 2));
Why / 2? Maybe use a const or sizeof() instead of 2? (Or a comment).

[hashimoto, 2012-07-04 04:33:34]

Chris, I have a question about the '/2' trick you introduced 20 months ago.
(http://codereview.chromium.org/3442009/diff/37001/chrome/browser/chromeos/log...)

In the comment, you said "// only want top half, at least for now."
Is this trick still needed?

Thanks,

http://codereview.chromium.org/10701075/diff/3001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/parallel_authenticator.cc (right):

http://codereview.chromium.org/10701075/diff/3001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/parallel_authenticator.cc:171: const int
kPassHashLen = 32;
On 2012/07/03 15:31:14, stevenjb (chromium) wrote:
> nit: constant at top of file

Done.

http://codereview.chromium.org/10701075/diff/3001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/parallel_authenticator.cc:182:
sizeof(passhash_buf) / 2));
On 2012/07/03 15:31:14, stevenjb (chromium) wrote:
> Why / 2? Maybe use a const or sizeof() instead of 2? (Or a comment).

Looks like this '/2' trick has been there for 20 months
(http://codereview.chromium.org/3442009/diff/37001/chrome/browser/chromeos/log...)
Adding cmasone@ who introduced this trick as a reviewer.

[Chris Masone, 2012-07-04 04:54:10]

I have no context for this question.  What is this CL for?  What
functionality is being changed or replaced?  What is a 'trick' about this
code?  I'm very lost, and neither the CL description nor the bug have any
context.

On Tue, Jul 3, 2012 at 9:33 PM, <hashimoto@chromium.org> wrote:

> Chris, I have a question about the '/2' trick you introduced 20 months ago.
> (http://codereview.chromium.**org/3442009/diff/37001/chrome/**
>
browser/chromeos/login/**parallel_authenticator.cc#**newcode601<http://codereview.chromium.org/3442009/diff/37001/chrome/browser/chromeos/login/parallel_authenticator.cc#newcode601>
> )
>
> In the comment, you said "// only want top half, at least for now."
> Is this trick still needed?
>
> Thanks,
>
>
> http://codereview.chromium.**org/10701075/diff/3001/chrome/**
>
browser/chromeos/login/**parallel_authenticator.cc<http://codereview.chromium.org/10701075/diff/3001/chrome/browser/chromeos/login/parallel_authenticator.cc>
> File chrome/browser/chromeos/login/**parallel_authenticator.cc (right):
>
> http://codereview.chromium.**org/10701075/diff/3001/chrome/**
>
browser/chromeos/login/**parallel_authenticator.cc#**newcode171<http://codereview.chromium.org/10701075/diff/3001/chrome/browser/chromeos/login/parallel_authenticator.cc#newcode171>
> chrome/browser/chromeos/login/**parallel_authenticator.cc:171: const int
> kPassHashLen = 32;
> On 2012/07/03 15:31:14, stevenjb (chromium) wrote:
>
>> nit: constant at top of file
>>
>
> Done.
>
> http://codereview.chromium.**org/10701075/diff/3001/chrome/**
>
browser/chromeos/login/**parallel_authenticator.cc#**newcode182<http://codereview.chromium.org/10701075/diff/3001/chrome/browser/chromeos/login/parallel_authenticator.cc#newcode182>
> chrome/browser/chromeos/login/**parallel_authenticator.cc:182:
> sizeof(passhash_buf) / 2));
> On 2012/07/03 15:31:14, stevenjb (chromium) wrote:
>
>> Why / 2? Maybe use a const or sizeof() instead of 2? (Or a comment).
>>
>
> Looks like this '/2' trick has been there for 20 months
> (http://codereview.chromium.**org/3442009/diff/37001/chrome/**
>
browser/chromeos/login/**parallel_authenticator.cc#**newcode601<http://codereview.chromium.org/3442009/diff/37001/chrome/browser/chromeos/login/parallel_authenticator.cc#newcode601>
> )
> Adding cmasone@ who introduced this trick as a reviewer.
>
>
http://codereview.chromium.**org/10701075/<http://codereview.chromium.org/107...
>

[hashimoto, 2012-07-04 05:04:20]

On 2012/07/04 04:54:10, Chris Masone wrote:
> I have no context for this question.  What is this CL for?  What
> functionality is being changed or replaced?  What is a 'trick' about this
> code?  I'm very lost, and neither the CL description nor the bug have any
> context.
Sorry,

I'm asking about HashPassword() function you added to Chromium OS's
ParallelAuthenticator 20 months ago.
In that function, only the first half of password hash generated with SHA-256 is
hex-encoded and returned.
Why are we passing only a half instead of the whole hash?

> 
> On Tue, Jul 3, 2012 at 9:33 PM, <mailto:hashimoto@chromium.org> wrote:
> 
> > Chris, I have a question about the '/2' trick you introduced 20 months ago.
> > (http://codereview.chromium.**org/3442009/diff/37001/chrome/**
> >
>
browser/chromeos/login/**parallel_authenticator.cc#**newcode601<http://codereview.chromium.org/3442009/diff/37001/chrome/browser/chromeos/login/parallel_authenticator.cc#newcode601>
> > )
> >
> > In the comment, you said "// only want top half, at least for now."
> > Is this trick still needed?
> >
> > Thanks,
> >
> >
> > http://codereview.chromium.**org/10701075/diff/3001/chrome/**
> >
>
browser/chromeos/login/**parallel_authenticator.cc<http://codereview.chromium.org/10701075/diff/3001/chrome/browser/chromeos/login/parallel_authenticator.cc>
> > File chrome/browser/chromeos/login/**parallel_authenticator.cc (right):
> >
> > http://codereview.chromium.**org/10701075/diff/3001/chrome/**
> >
>
browser/chromeos/login/**parallel_authenticator.cc#**newcode171<http://codereview.chromium.org/10701075/diff/3001/chrome/browser/chromeos/login/parallel_authenticator.cc#newcode171>
> > chrome/browser/chromeos/login/**parallel_authenticator.cc:171: const int
> > kPassHashLen = 32;
> > On 2012/07/03 15:31:14, stevenjb (chromium) wrote:
> >
> >> nit: constant at top of file
> >>
> >
> > Done.
> >
> > http://codereview.chromium.**org/10701075/diff/3001/chrome/**
> >
>
browser/chromeos/login/**parallel_authenticator.cc#**newcode182<http://codereview.chromium.org/10701075/diff/3001/chrome/browser/chromeos/login/parallel_authenticator.cc#newcode182>
> > chrome/browser/chromeos/login/**parallel_authenticator.cc:182:
> > sizeof(passhash_buf) / 2));
> > On 2012/07/03 15:31:14, stevenjb (chromium) wrote:
> >
> >> Why / 2? Maybe use a const or sizeof() instead of 2? (Or a comment).
> >>
> >
> > Looks like this '/2' trick has been there for 20 months
> > (http://codereview.chromium.**org/3442009/diff/37001/chrome/**
> >
>
browser/chromeos/login/**parallel_authenticator.cc#**newcode601<http://codereview.chromium.org/3442009/diff/37001/chrome/browser/chromeos/login/parallel_authenticator.cc#newcode601>
> > )
> > Adding cmasone@ who introduced this trick as a reviewer.
> >
> >
>
http://codereview.chromium.**org/10701075/%3Chttp://codereview.chromium.org/1...>
> >

[Chris Masone, 2012-07-09 16:05:03]

On Tue, Jul 3, 2012 at 10:04 PM, <hashimoto@chromium.org> wrote:

> On 2012/07/04 04:54:10, Chris Masone wrote:
>
>> I have no context for this question.  What is this CL for?  What
>> functionality is being changed or replaced?  What is a 'trick' about this
>> code?  I'm very lost, and neither the CL description nor the bug have any
>> context.
>>
> Sorry,
>
> I'm asking about HashPassword() function you added to Chromium OS's
> ParallelAuthenticator 20 months ago.
> In that function, only the first half of password hash generated with
> SHA-256 is
> hex-encoded and returned.
> Why are we passing only a half instead of the whole hash?
>
>
According to the CL description you're removing HashPassword(), which
doesn't seem to be true based on the code.  Can you fix the description to
accurately explain what you're doing in this CL, and also provide some
motivation for the change?

Now that I've had time to recall what's going on in the code, I can tell
you that changing the behavior would make every chrome OS user unable to
mount their cryptohome.  The output of HashPassword() is what we actually
use to wrap the key used to encrypt/decrypt the user's cryptohome.

The reason we do this 'weak' hashing is so that, even if an attacker
discovers and reverses the hash, it will map to many different strings and
not immediately expose the user's passphrase.


>
>
>  On Tue, Jul 3, 2012 at 9:33 PM, <mailto:hashimoto@chromium.org**> wrote:
>>
>
>  > Chris, I have a question about the '/2' trick you introduced 20 months
>> ago.
>> > (http://codereview.chromium.****org/3442009/diff/37001/chrome/****
>> >
>>
>
> browser/chromeos/login/****parallel_authenticator.cc#****newcode601<
> http://codereview.**chromium.org/3442009/diff/**
> 37001/chrome/browser/chromeos/**login/parallel_authenticator.**
>
cc#newcode601<http://codereview.chromium.org/3442009/diff/37001/chrome/browser/chromeos/login/parallel_authenticator.cc#newcode601>
> >
>
>> > )
>> >
>> > In the comment, you said "// only want top half, at least for now."
>> > Is this trick still needed?
>> >
>> > Thanks,
>> >
>> >
>> > http://codereview.chromium.****org/10701075/diff/3001/chrome/****
>> >
>>
>
> browser/chromeos/login/****parallel_authenticator.cc<http**
> ://codereview.chromium.org/**10701075/diff/3001/chrome/**
>
browser/chromeos/login/**parallel_authenticator.cc<http://codereview.chromium.org/10701075/diff/3001/chrome/browser/chromeos/login/parallel_authenticator.cc>
> >
>
>> > File chrome/browser/chromeos/login/****parallel_authenticator.cc
>> (right):
>> >
>> > http://codereview.chromium.****org/10701075/diff/3001/chrome/****
>> >
>>
>
> browser/chromeos/login/****parallel_authenticator.cc#****newcode171<
> http://codereview.**chromium.org/10701075/diff/**
> 3001/chrome/browser/chromeos/**login/parallel_authenticator.**
>
cc#newcode171<http://codereview.chromium.org/10701075/diff/3001/chrome/browser/chromeos/login/parallel_authenticator.cc#newcode171>
> >
>
>> > chrome/browser/chromeos/login/****parallel_authenticator.cc:**171:
>> const int
>>
>> > kPassHashLen = 32;
>> > On 2012/07/03 15:31:14, stevenjb (chromium) wrote:
>> >
>> >> nit: constant at top of file
>> >>
>> >
>> > Done.
>> >
>> > http://codereview.chromium.****org/10701075/diff/3001/chrome/****
>> >
>>
>
> browser/chromeos/login/****parallel_authenticator.cc#****newcode182<
> http://codereview.**chromium.org/10701075/diff/**
> 3001/chrome/browser/chromeos/**login/parallel_authenticator.**
>
cc#newcode182<http://codereview.chromium.org/10701075/diff/3001/chrome/browser/chromeos/login/parallel_authenticator.cc#newcode182>
> >
>
>> > chrome/browser/chromeos/login/****parallel_authenticator.cc:**182:
>>
>> > sizeof(passhash_buf) / 2));
>> > On 2012/07/03 15:31:14, stevenjb (chromium) wrote:
>> >
>> >> Why / 2? Maybe use a const or sizeof() instead of 2? (Or a comment).
>> >>
>> >
>> > Looks like this '/2' trick has been there for 20 months
>> > (http://codereview.chromium.****org/3442009/diff/37001/chrome/****
>> >
>>
>
> browser/chromeos/login/****parallel_authenticator.cc#****newcode601<
> http://codereview.**chromium.org/3442009/diff/**
> 37001/chrome/browser/chromeos/**login/parallel_authenticator.**
>
cc#newcode601<http://codereview.chromium.org/3442009/diff/37001/chrome/browser/chromeos/login/parallel_authenticator.cc#newcode601>
> >
>
>  > )
>> > Adding cmasone@ who introduced this trick as a reviewer.
>> >
>> >
>>
>
> http://codereview.chromium.****org/10701075/%3Chttp://coderev**
> iew.chromium.org/10701075/ <http://codereview.chromium.org/10701075/>>
>
>> >
>>
>
>
>
http://codereview.chromium.**org/10701075/<http://codereview.chromium.org/107...
>

[stevenjb, 2012-07-09 16:21:58]

On 2012/07/09 16:05:03, Chris Masone wrote:
> On Tue, Jul 3, 2012 at 10:04 PM, <mailto:hashimoto@chromium.org> wrote:
> 
> > On 2012/07/04 04:54:10, Chris Masone wrote:
> >
> >> I have no context for this question.  What is this CL for?  What
> >> functionality is being changed or replaced?  What is a 'trick' about this
> >> code?  I'm very lost, and neither the CL description nor the bug have any
> >> context.
> >>
> > Sorry,
> >
> > I'm asking about HashPassword() function you added to Chromium OS's
> > ParallelAuthenticator 20 months ago.
> > In that function, only the first half of password hash generated with
> > SHA-256 is
> > hex-encoded and returned.
> > Why are we passing only a half instead of the whole hash?
> >
> >
> According to the CL description you're removing HashPassword(), which
> doesn't seem to be true based on the code.  Can you fix the description to
> accurately explain what you're doing in this CL, and also provide some
> motivation for the change?
> 
> Now that I've had time to recall what's going on in the code, I can tell
> you that changing the behavior would make every chrome OS user unable to
> mount their cryptohome.  The output of HashPassword() is what we actually
> use to wrap the key used to encrypt/decrypt the user's cryptohome.
> 
> The reason we do this 'weak' hashing is so that, even if an attacker
> discovers and reverses the hash, it will map to many different strings and
> not immediately expose the user's passphrase.
> 

There is no intention to change the code, it is just getting moved closer to
where it is actually used. Agree that the CL description should be "move"
instead of "remove". 

I found the 'sizeof(passhash_buf) / 2' a bit mysterious and requested that we
understand and better document the code while moving it. Appreciate the
explanation.

[hashimoto, 2012-07-10 10:02:45]

Chris, thank you for your explanation.
Now the CL description and the comment in HashPassword() is updated.

On 2012/07/09 16:21:58, stevenjb (chromium) wrote:
> On 2012/07/09 16:05:03, Chris Masone wrote:
> > On Tue, Jul 3, 2012 at 10:04 PM, <mailto:hashimoto@chromium.org> wrote:
> > 
> > > On 2012/07/04 04:54:10, Chris Masone wrote:
> > >
> > >> I have no context for this question.  What is this CL for?  What
> > >> functionality is being changed or replaced?  What is a 'trick' about this
> > >> code?  I'm very lost, and neither the CL description nor the bug have any
> > >> context.
> > >>
> > > Sorry,
> > >
> > > I'm asking about HashPassword() function you added to Chromium OS's
> > > ParallelAuthenticator 20 months ago.
> > > In that function, only the first half of password hash generated with
> > > SHA-256 is
> > > hex-encoded and returned.
> > > Why are we passing only a half instead of the whole hash?
> > >
> > >
> > According to the CL description you're removing HashPassword(), which
> > doesn't seem to be true based on the code.  Can you fix the description to
> > accurately explain what you're doing in this CL, and also provide some
> > motivation for the change?
> > 
> > Now that I've had time to recall what's going on in the code, I can tell
> > you that changing the behavior would make every chrome OS user unable to
> > mount their cryptohome.  The output of HashPassword() is what we actually
> > use to wrap the key used to encrypt/decrypt the user's cryptohome.
> > 
> > The reason we do this 'weak' hashing is so that, even if an attacker
> > discovers and reverses the hash, it will map to many different strings and
> > not immediately expose the user's passphrase.
> > 
> 
> There is no intention to change the code, it is just getting moved closer to
> where it is actually used. Agree that the CL description should be "move"
> instead of "remove". 
> 
> I found the 'sizeof(passhash_buf) / 2' a bit mysterious and requested that we
> understand and better document the code while moving it. Appreciate the
> explanation.

[Chris Masone, 2012-07-10 15:27:16]

lgtm

[commit-bot: I haz the power, 2012-07-11 03:36:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/10701075/10007

[commit-bot: I haz the power, 2012-07-11 04:48:40]

Change committed as 146055