CL to add SHA-1 to crypto/ https://codereview.chromium.org/2080753002/
CL to add a perf test to crypto/ https://codereview.chromium.org/2081553003/

On samus my perftest CL gives the following results:

localhost home # ./crypto_perftests --gtest_filter=*SHA1*
IMPORTANT DEBUGGING NOTE: batches of tests are run inside their
own process. For debugging a test inside a debugger, use the
--gtest_filter=<your_test_name> flag along with
--single-process-tests.
Using sharding settings from environment. This is shard 0/1
Using 1 parallel jobs.
[14791:14791:0620/080946:354455864:ERROR:icu_util.cc(173)] Invalid file descriptor to ICU data received.
Note: Google Test filter = SHA1PerfTest.SingleBlock:SHA1PerfTest.MultiBlock
[==========] Running 2 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 2 tests from SHA1PerfTest
[ RUN      ] SHA1PerfTest.SingleBlock
*RESULT sha1_perfsingle: base= 1894081.0912937613 hashes/s
*RESULT sha1_perfsingle: crypto= 4199386.889514131 hashes/s
*RESULT sha1_perfsingle: improvement= 2.2171104018813255 times
[       OK ] SHA1PerfTest.SingleBlock (1532 ms)
[ RUN      ] SHA1PerfTest.MultiBlock
*RESULT sha1_perfmulti: base= 895533.7493087598 hashes/s
*RESULT sha1_perfmulti: crypto= 2158221.3666073517 hashes/s
*RESULT sha1_perfmulti: improvement= 2.409983284575516 times
[       OK ] SHA1PerfTest.MultiBlock (3160 ms)
[----------] 2 tests from SHA1PerfTest (4692 ms total)

[----------] Global test environment tear-down
[==========] 2 tests from 1 test case ran. (4692 ms total)
[  PASSED  ] 2 tests.
[1/2] SHA1PerfTest.SingleBlock (1532 ms)
[2/2] SHA1PerfTest.MultiBlock (3160 ms)
SUCCESS: all tests passed.
Tests took 4 seconds.

We should be moving users away from SHA-1. I'm not sure adding new code, especially optimized code, helps move us closer. You can see me tracking removals at Issue 618486

Further, SHA-1 being in //base exists for a complex set of reasons that I've been trying to work through, as time permits, in Issue 618486 (related to our installer and NaCl Win64-bit stubs) which may no longer be relevant, due to our switch to BoringSSL.

A better task would be figuring out why there are 4000+ calls to SHA1HashString for news.ycombinator.com - they're almost certainly "wrong".

Actually, I think by now, it's only in //base because of base/trace_event/memory_allocator_dump_guid.cc. I don't believe the NaCl Win64 stuff uses it and BoringSSL, unlike NSS, has very static-linker-friendly low-level APIs, so having those route to BoringSSL's impl seems fine. The NaCl Win64 stuff already uses BoringSSL now.

The trace_event usage seems kind of nonsense. It's still not going to guarantee no collisions (which seems to have been the goal) due to the extreme truncation. Birthday on 2^64 isn't that big. (+primiano)
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/-rXNiOoIU4A/LI4tnA46SekJ

Anyway, while I do think it's valuable to move SHA-1 from //base to //crypto and then switching to BoringSSL's implementation, it's not because of performance. Rather I think the only value here would be to lose one copy of SHA-1 from the binary. (That and having SHA-1 equally accessible as SHA-256 rather than moreso might avoid encouraging new uses.) It doesn't make sense to add a crypto/sha1.h until/unless the thing keeping it in //base is gone or we allow //base a configuration that pulls in //third_party/boringssl.

> Anyway, while I do think it's valuable [...]

(Of course, as Ryan said, that time might be better spent getting most of the SHA-1 uses off of SHA-256. Though I don't know if that can be brought down to zero. Certainly, until TLS 1.0 and TLS 1.1 are gone, we are going to be shipping a copy of SHA-1 in the binary.)

davidben: It was using by the GPU code and installer, which is why HMAC and SHA-1 were in //base. A lot of things have changed though, and haven't done the deep-dive to figure the effects, but also, like you said, //crypto is more static-linker friendly. I tried to explain that briefly, "exists for a complex set of reasons", without going into the details, because I didn't think they were intrinsically germane to this bug.

As for SHA-1, I'm not trying to drive it down to zero overall, I'm suggesting we should not be using it in Chrome code for any operations. If we want a cryptographic hash, we should be figuring out why and reasoning about the security properties, and if we just want an 'improbable to collide' hash, we should be using one of our better suited hash functions.

I'm sorry I wasn't clearer on expressing this. I'm not sure how much of the side-conversation is germane, but to be clearer:

1) Why is news.ycombinator.com making 4000 calls
  Action: We should remove those calls
  Action: We should be looking into Issue 618486 to track that
2) We shouldn't have both //base and //crypto versions
  Action: If it can be, removing the //base version should be prioritized (either moving it to //crypto or removing it entirely). If it can't be, we should fix that.

So I was able to root cause (I think) the cause of the large number of calls to base::SHA1HashString(). By printing out the desired source string I can see that all the requests look like extension IDs.

On boot I see:

localhost ~ # grep -c SHA-1 /var/log/chrome/chrome
1389

If I login and open a page I see something like:

localhost ~ # grep -c SHA-1 /home/user/eef763a43f010f7b084145c0152551763f3136d6/log/chrome
4070

If I now look at the system log (after 5 minutes of capturing the others):

localhost ~ # grep -c SHA-1 /var/log/chrome/chrome
28052

After 40 minutes of uptime (single page open, otherwise "idle")

localhost ~ # grep -c SHA-1 /var/log/chrome/chrome
45456

localhost ~ # grep SHA-1 /var/log/chrome/chrome | cut -f 3 -d " " | sort | uniq -c | sort -n | tail -n 5
   1892 pmfjbimdmchhbnneeidfognadeopoehp
   2060 fgoepimhcoialccpbmpnnblemnepkkao
   2098 pafkbggdmjlpgkdkcbjmhmfcdpncadgh
   2323 hhaomjibdihmijegdhdafkllkbggdgoj
   3929 nckgahadagoaajjgafhacjanaoiihapd

In the user log there are 6 calls to hash nckgahadagoaajjgafhacjanaoiihapd (the ID for Google Hangouts extension) every 20s.

A page load gives around 26 hash attempts (the majority again being nckgahadagoaajjgafhacjanaoiihapd) so the page load is not something to worry about specifically.

The backtrace I obtained from gdb shows these extension IDs coming in through extensions::IsIdInArray()/IsIdInList()

I opened  https://crbug.com/622035  repeating much of the above and where i'll take a stab at trying to fix it by refactoring the extension feature code.

I agree that long term there should only be one implementation in Chrome - i'd like it to be in //crypto so that we can take advantage of the SIMD benefits that BoringSSL provides.