New issue
Advanced search Search tips

Issue 731993 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , All , Chrome , Mac
Pri: 2
Type: Feature



Sign in to add a comment

Add scrypt to boringssl

Project Member Reported by nparker@chromium.org, Jun 10 2017

Issue description

(Not sure the best place to file this bug).

scrypt is needed for PhishGuard and a future project as well. This was discussed on security@chromium.org.


 
Status: Started (was: Untriaged)
This works. We also have https://crbug.com/boringssl, but we're a small group and do everything pretty ad-hoc. :-)
Status is I've started importing upstream OpenSSL's implementation. It works, but I think I can make a little cleaner. Also I think I found a bug in it when N > 2^32, though that's also a completely implausible input. :-)

Anyway, it'll be in sometime next week.
Project Member

Comment 4 by bugdroid1@chromium.org, Jun 12 2017

The following revision refers to this bug:
  https://boringssl.googlesource.com/boringssl/+/b529253beab2dd65a4fdcdca27a4755256282401

commit b529253beab2dd65a4fdcdca27a4755256282401
Author: David Benjamin <davidben@google.com>
Date: Mon Jun 12 20:32:21 2017

Implement scrypt from RFC 7914.

This imports upstream's scrypt implementation, though it's been heavily
revised. I lost track of words vs. blocks vs. bigger blocks too many
times in the original code and introduced a typedef for the fixed-width
Salsa20 blocks. The downside is going from bytes to blocks is a bit
trickier, so I took advantage of our little-endian assumption.

This also adds an missing check for N < 2^32. Upstream's code is making
this assumption in Integerify. I'll send that change back upstream. I've
also removed the weird edge case where a NULL out_key parameter means to
validate N/r/p against max_mem and nothing else. That's just in there to
get a different error code out of their PKCS#12 code.

Performance-wise, the cleanup appears to be the same (up to what little
precision I was able to get here), but an optimization to use bitwise
AND rather than modulus makes us measurably faster. Though scrypt isn't
a fast operation to begin with, so hopefully it isn't anyone's
bottleneck.

This CL does not route scrypt up to the PKCS#12 code, though we could
write our own version of that if we need to later.

BUG= chromium:731993 

Change-Id: Ib2f43344017ed37b6bafd85a2c2b103d695020b8
Reviewed-on: https://boringssl-review.googlesource.com/17084
Reviewed-by: Adam Langley <agl@google.com>

[modify] https://crrev.com/b529253beab2dd65a4fdcdca27a4755256282401/crypto/err/evp.errordata
[modify] https://crrev.com/b529253beab2dd65a4fdcdca27a4755256282401/crypto/evp/CMakeLists.txt
[modify] https://crrev.com/b529253beab2dd65a4fdcdca27a4755256282401/tool/speed.cc
[add] https://crrev.com/b529253beab2dd65a4fdcdca27a4755256282401/crypto/evp/scrypt_test.cc
[modify] https://crrev.com/b529253beab2dd65a4fdcdca27a4755256282401/include/openssl/evp.h
[add] https://crrev.com/b529253beab2dd65a4fdcdca27a4755256282401/crypto/evp/scrypt.c
[modify] https://crrev.com/b529253beab2dd65a4fdcdca27a4755256282401/sources.cmake
[modify] https://crrev.com/b529253beab2dd65a4fdcdca27a4755256282401/crypto/CMakeLists.txt
[add] https://crrev.com/b529253beab2dd65a4fdcdca27a4755256282401/crypto/evp/scrypt_tests.txt

And https://chromium-review.googlesource.com/533574 will pull it into Chromium.
Project Member

Comment 6 by bugdroid1@chromium.org, Jun 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1e56a67c9439c93cda360eb2a95bee5af8b67b62

commit 1e56a67c9439c93cda360eb2a95bee5af8b67b62
Author: David Benjamin <davidben@chromium.org>
Date: Tue Jun 13 19:16:57 2017

Roll src/third_party/boringssl/src f6584e7a5..c07635f86

https://boringssl.googlesource.com/boringssl/+log/f6584e7a5232463dab271529f65f50131ae2202a..c07635f8699e99b6af06e401beccd934f6bce53b

This pulls in, among other things, the scrypt implementation.

BUG= 731993 

Change-Id: I60121c9739d12a469bf6ee4615616504aab120c2
Reviewed-on: https://chromium-review.googlesource.com/533574
Commit-Queue: David Benjamin <davidben@chromium.org>
Commit-Queue: Steven Valdez <svaldez@chromium.org>
Reviewed-by: Steven Valdez <svaldez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#479094}
[modify] https://crrev.com/1e56a67c9439c93cda360eb2a95bee5af8b67b62/DEPS
[modify] https://crrev.com/1e56a67c9439c93cda360eb2a95bee5af8b67b62/third_party/boringssl/BUILD.generated.gni
[modify] https://crrev.com/1e56a67c9439c93cda360eb2a95bee5af8b67b62/third_party/boringssl/BUILD.generated_tests.gni
[modify] https://crrev.com/1e56a67c9439c93cda360eb2a95bee5af8b67b62/third_party/boringssl/crypto_test_data.cc
[modify] https://crrev.com/1e56a67c9439c93cda360eb2a95bee5af8b67b62/third_party/boringssl/err_data.c
[modify] https://crrev.com/1e56a67c9439c93cda360eb2a95bee5af8b67b62/third_party/boringssl/ios-arm/crypto/fipsmodule/aes-armv4.S
[modify] https://crrev.com/1e56a67c9439c93cda360eb2a95bee5af8b67b62/third_party/boringssl/linux-arm/crypto/fipsmodule/aes-armv4.S

Status: Fixed (was: Started)

Sign in to add a comment