Add "scrypt" to third_party for secure password hashes

Add "scrypt" to third_party for use in generating secure hash of user password that can later be used for off-line authentication in the case when on-line authentication is not available.

The planned use for this is profile-locking where unlocking has to be possible even when not connected to a network.

BUG=


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175069

[bcwhite, 2013-01-03 18:02:58]

Ping?

[Daniel Berlin, 2013-01-03 18:44:29]

lgtm

[commit-bot: I haz the power, 2013-01-03 18:51:47]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[bcwhite, 2013-01-03 19:02:53]

Roger, would you review this for submission, please?

[bcwhite, 2013-01-03 19:33:51]

Could I get an OWNER approval for the change to
chrome/chrome_browser.gypi

Thanks!

[Lei Zhang, 2013-01-03 20:03:04]

Just remove chrome/chrome_browser.gypi from this CL and add it in the future CL
that actually use scrypt.

https://codereview.chromium.org/11637016/diff/3001/third_party/scrypt/scrypt.gyp
File third_party/scrypt/scrypt.gyp (right):

https://codereview.chromium.org/11637016/diff/3001/third_party/scrypt/scrypt....
third_party/scrypt/scrypt.gyp:11: 'lib/crypto/sha256.c',
No tabs please.

[bcwhite, 2013-01-03 20:23:10]

https://codereview.chromium.org/11637016/diff/3001/third_party/scrypt/scrypt.gyp
File third_party/scrypt/scrypt.gyp (right):

https://codereview.chromium.org/11637016/diff/3001/third_party/scrypt/scrypt....
third_party/scrypt/scrypt.gyp:11: 'lib/crypto/sha256.c',
On 2013/01/03 20:03:04, Lei Zhang wrote:
> No tabs please.

Done.

[Lei Zhang, 2013-01-03 20:45:36]

lgtm

Please make sure it passes tools/checklicenses/checklicenses.py.

[bcwhite1, 2013-01-03 21:06:36]

I've run it before.  That tool spits out *a lot* of warnings!

'third_party/scrypt/lib/util/warn.h' has non-whitelisted license 'UNKNOWN'
'third_party/scrypt/scrypt_platform.h' has non-whitelisted license 'UNKNOWN'

Is that a problem?


On Thu, Jan 3, 2013 at 3:45 PM, <thestig@chromium.org> wrote:

> lgtm
>
> Please make sure it passes tools/checklicenses/**checklicenses.py.
>
>
https://codereview.chromium.**org/11637016/<https://codereview.chromium.org/1...
>



-- 
  Brian  (now in Montreal, Canada)
  bcwhite@google.com
-----------------------------------------------------------------------------------------
*Treat someone as they are and they will remain that way.
Treat someone as they can be and they will become that way.

*

[Lei Zhang, 2013-01-03 21:10:35]

On 2013/01/03 21:06:36, bcwhite_google.com wrote:
> I've run it before.  That tool spits out *a lot* of warnings!
> 
> 'third_party/scrypt/lib/util/warn.h' has non-whitelisted license 'UNKNOWN'
> 'third_party/scrypt/scrypt_platform.h' has non-whitelisted license 'UNKNOWN'
> 
> Is that a problem?

Yes, please fix these. You can copy the license header from one of the other
files and add it here. Include the changes in chromium.patch.

[bcwhite, 2013-01-03 21:59:52]

Okay.  Licenses added and patch updated.

[Lei Zhang, 2013-01-03 22:02:37]

lgtm

[commit-bot: I haz the power, 2013-01-03 22:06:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bcwhite@chromium.org/11637016/7003

[commit-bot: I haz the power, 2013-01-04 00:52:40]

Change committed as 175069

[Ryan Sleevi, 2013-01-04 19:35:59]

I'd like to provide some push back on this change, for now.

We're trying to get a handle on crypto code in Chromium, and reduce duplication
and exposure.
To give an idea of how bad this is:
- SHA1 implementations:
 * base
 * NSS
 * OpenSSL
 * libsrtp
 * libjingle
 * smhasher
 * WebKit
 * libxslt
 * ffmpeg
We've got nearly as many AES implementations as well.

Can you please provide a design doc or further discussion for the
src/crypto/OWNERS and for the Chrome security-team?

[jschuh, 2013-01-04 19:52:34]

Yeah, this is bloat and there's no security justification. It should just be
removed. Also, it's extremely disturbing that no one even considered approaching
the security team before moving forward with this.

[bcwhite1, 2013-01-04 22:02:45]

> I'd like to provide some push back on this change, for now.
>
> We're trying to get a handle on crypto code in Chromium, and reduce
> duplication
> and exposure.
> To give an idea of how bad this is:
> - SHA1 implementations:
>  * base
>  * NSS
>  * OpenSSL
>  * libsrtp
>  * libjingle
>  * smhasher
>  * WebKit
>  * libxslt
>  * ffmpeg
>

I agree whole-heartedly.  I was going to use OpenSSL for the secure hash
but unfortunately it is not included in all distributions.  SHA256 on its
own is not secure enough for hashed passwords and I don't know of any other
library that does all the extra work to make it so.  And I sure don't want
to be rolling my own -- that's never a good idea.  Scrypt is currently
considered the strongest password hashing library available.

I'd be happy to modify the outside scrypt code to call existing sha256 code
if you would provide some direction as to the best way to do that
(remembering that the scrypt library is C and so can't call something in a
C++ library).


Yeah, this is bloat and there's no security justification. It should just be
> removed. Also, it's extremely disturbing that no one even considered
> approaching
> the security team before moving forward with this.


scrypt is already a library approved for use in ChromeOS for user
authentication and is the strongest mechanism available for doing so; I
think that counts as some justification.  It's not *just* sha256.

https://b.corp.google.com/issue?id=7941916 is the security review ticket I
filed for the entire feature that I'm working on.

  Brian  (now in Montreal, Canada)
  bcwhite@google.com
-----------------------------------------------------------------------------------------
*Treat someone as they are and they will remain that way.
Treat someone as they can be and they will become that way.

*

[jschuh, 2013-01-04 22:16:34]

On 2013/01/04 22:02:45, bcwhite_google.com wrote:
> Yeah, this is bloat and there's no security justification. It should just be
> > removed. Also, it's extremely disturbing that no one even considered
> > approaching
> > the security team before moving forward with this.
> 
> 
> scrypt is already a library approved for use in ChromeOS for user
> authentication and is the strongest mechanism available for doing so; I
> think that counts as some justification.  It's not *just* sha256.
> 
> https://b.corp.google.com/issue?id=7941916 is the security review ticket I
> filed for the entire feature that I'm working on.

The efficacy of scrypt has nothing at all to do with the breakdown here. The
issue is why it's even being used at all. By analogy, would you start
restructuring the Chrome's UI without at least consulting with the UI leads?

[bcwhite1, 2013-01-05 00:00:06]

> Yeah, this is bloat and there's no security justification. It should just
>> be
>> > removed. Also, it's extremely disturbing that no one even considered
>> > approaching
>> > the security team before moving forward with this.
>>
>
>
>  scrypt is already a library approved for use in ChromeOS for user
>> authentication and is the strongest mechanism available for doing so; I
>> think that counts as some justification.  It's not *just* sha256.
>>
>
> 
https://b.corp.google.com/**issue?id=7941916<https://b.corp.google.com/issue?...
the security review ticket I
>> filed for the entire feature that I'm working on.
>>
>
> The efficacy of scrypt has nothing at all to do with the breakdown here.
> The
> issue is why it's even being used at all. By analogy, would you start
> restructuring the Chrome's UI without at least consulting with the UI
> leads?
>

This is part of a larger effort that is supported by the relevant Chrome
leads.  Security discussions were undertaken with GAIA and ChromeOS people.
 A security review ticket has been filed for the feature as a whole.  I
apologize if submitting a small library to the code base in advance has
offended you.

  Brian  (now in Montreal, Canada)
  bcwhite@google.com
-----------------------------------------------------------------------------------------
*Treat someone as they are and they will remain that way.
Treat someone as they can be and they will become that way.

*

[jschuh, 2013-01-05 00:28:24]

I'm sorry, but nothing in your response has addressed the concerns I've raised.
This CL introduces a large new dependency and the implied follow-on changes have
major security ramifications that up to this point are entirely unreviewed by
anyone with the appropriate expertise.

Security reviews for Chrome are not handled via buganizer. And the bug you're
referencing wasn't filed until yesterday, has had no action, and there's no
context at all for this feature in Chromium's tracker. Absent some better
explanation of what's going on here, my only option is to revert this CL.

I'm not trying to be difficult, but the way this CL landed is extremely unusual
for Chrome, and simply not the way we do things. The correct approach to this is
to file a launch-bug, ping the relevant teams (including security) with a
proposed design, and then reach some consensus before landing code. And when
landing code, make sure your reviewers are aware of the context and intent of
your CLs. Landing this dependency might be correct in the end, but the steps to
determine that haven't been taken as far as I can tell.

[bcwhite1, 2013-01-06 03:51:54]

> I'm sorry, but nothing in your response has addressed the concerns I've
> raised.
> This CL introduces a large new dependency and the implied follow-on
> changes have
> major security ramifications that up to this point are entirely unreviewed
> by
> anyone with the appropriate expertise.
>

Incorrect.  It introduces no dependency because nothing references it and
the implied follow-on changes are not part of this CL.  As it stands, it is
only a piece of "dead code".  It is also in no way "large".



> Security reviews for Chrome are not handled via buganizer. And the bug
> you're
> referencing wasn't filed until yesterday, has had no action, and there's no
> context at all for this feature in Chromium's tracker. Absent some better
> explanation of what's going on here, my only option is to revert this CL.
>

Then update the documentation because the link on the security review page
(don't have the URL as I'm not on VPN) took me to a Buganizer submission
form on which I included the link to the full security description.  Did
you even look at the link I provided?



> I'm not trying to be difficult, but the way this CL landed is extremely
> unusual
> for Chrome, and simply not the way we do things. The correct approach to
> this is
> to file a launch-bug, ping the relevant teams (including security) with a
> proposed design, and then reach some consensus before landing code. And
> when
> landing code, make sure your reviewers are aware of the context and intent
> of
> your CLs. Landing this dependency might be correct in the end, but the
> steps to
> determine that haven't been taken as far as I can tell.
>

I apologize for the unusual approach.  I'm new to Chrome and am working on
a part of the project given to me.  I assumed that some security review had
been done before it had been given to me.  I was part of security
discussions with various teams (such as GAIA and ChromeOS) but no official
"security review" had been done.  When I found out this was the case I went
through the process and filled in the information.  This CL had, however,
already been sent for review.

The purpose of the library was spelled out in the description.

  Brian  (now in Montreal, Canada)
  bcwhite@google.com
-----------------------------------------------------------------------------------------
*Treat someone as they are and they will remain that way.
Treat someone as they can be and they will become that way.

*