Add missing SHA224, SHA384, and SHA512 #63

jtmcdole · 2019-07-08T06:58:49Z

Sha384 and Sha512 share the same implementation and differ only in the
initialization vectors and the digest concatenation. The algorithm from
from rfc6234 calls for 64bit words and operations and to remain dart2js
friendly, BigInt was used.

INPUT REQUESTED
I'm not sure where the test vectors came from; but a simple comparison
to sha384 and sha512 command line methods shows it matching. If you
can point me to them, then I can write a better test.

Sha384 and Sha512 share the same implementation and differ only in the initailization vectors and the digest concatentation. The algorithm from from rfc6234 calls for 64bit words and operations and to remain dart2js friendly, BigInt was used. INPUT REQUESTED: I'm not sure where the test vectors came from; but a simple comparison to sha384 and sha512 command line methods shows it matching. If you can point me to them, then I can write a better test.

jtmcdole · 2019-07-08T07:03:04Z

Solves Issue dart-lang/core#181

kevmoo · 2019-07-08T15:50:47Z

@lrhn – thoughts here?

jtmcdole · 2019-07-08T17:05:42Z

Note: this (truncation, different initialization values) should work for sha-224 as well to fully support RFC 6234. Happy to put up a cl for that as well.

Do something less stupid; use _Sha32BitSink and _Sha64BitSink to have class heirarchies make better sense.

rakudrama · 2019-07-09T04:44:26Z

I'm worried that using BigInt will lead to performance too slow to be practical.

What throughput do you get for the various hash functions (say, 1MB digests per second) on the VM and dart2js (-O3)?

Ideally, in the browser the crypto library should be using the Web Crypto API.
I think this would be worthwhile, to access a high performance implementation that is aynchronous to avoid jank.

jtmcdole · 2019-07-09T05:50:57Z

vm, "real" 16MB buffer of random.

Instance of 'Sha224' warmup: 0:00:00.008122
Instance of 'Sha256' warmup: 0:00:00.001244
Instance of 'Sha384' warmup: 0:00:00.018599
Instance of 'Sha512' warmup: 0:00:00.005952
Instance of 'Sha224' real: 0:00:00.066492
Instance of 'Sha256' real: 0:00:00.052035
Instance of 'Sha384' real: 0:00:02.954591
Instance of 'Sha512' real: 0:00:02.935623

~5.5MB/sec I'll experiment with just using ints and addressing only the lower 32.

lrhn · 2019-07-09T10:07:12Z

I have no experience with the package:crypto API. Maybe @jonasfj will know better.

lib/src/sha512.dart

jonasfj · 2019-07-09T14:35:17Z

Ideally, in the browser the crypto library should be using the Web Crypto API.

We can't expose Web Crypto through the current synchronous APIs, as Web Crypto is asynchronous.
We would have to break package:crypto or add additional asynchronous methods.

As for the low-level implementation of primitives I'm not an expert, if publishing from the dart-lang organization might be wise to have someone with expertise review that.

Two modes of operation: if you're in a browser, you get the slower 32bit algorithm because you only have 2^53 bits. If you are on the VM / Flutter, you'll have 64bit operations wich is *much* faster: 32BIT NUMBERS: ~20MB/s hashing Removing BigInt has some good results: Instance of 'Sha224' warmup: 0:00:00.015599 Instance of 'Sha256' warmup: 0:00:00.002325 Instance of 'Sha384' warmup: 0:00:00.019082 Instance of 'Sha512' warmup: 0:00:00.010288 Instance of 'Sha224' real: 0:00:00.092928 Instance of 'Sha256' real: 0:00:00.093426 Instance of 'Sha384' real: 0:00:00.823335 Instance of 'Sha512' real: 0:00:00.807871 64BIT NUMBERS: ~236MB/s hashing On the VM, this is much faster with 64bit operations. Instance of 'Sha224' warmup: 0:00:00.013285 Instance of 'Sha256' warmup: 0:00:00.002443 Instance of 'Sha384' warmup: 0:00:00.020954 Instance of 'Sha512' warmup: 0:00:00.005616 Instance of 'Sha224' real: 0:00:00.097196 Instance of 'Sha256' real: 0:00:00.094167 Instance of 'Sha384' real: 0:00:00.067605 Instance of 'Sha512' real: 0:00:00.067564 NOTE: Compiles with dart2js - cannot reference 64bit hex as the compiler just vomits... but you can left shift by 32.

jtmcdole · 2019-07-10T05:38:15Z

As requested; removal of BigInt() - except doing pure 32bit math was still underwhelming. This switches between 32bit and 64bit depending on where you're running... and you get a 10x speed up on the VM.

jtmcdole · 2019-07-10T05:43:13Z

I'm worried that using BigInt will lead to performance too slow to be practical.

What throughput do you get for the various hash functions (say, 1MB digests per second) on the VM and dart2js (-O3)?

dart2js -O3:
Instance of 'minified:aE' warmup: 0:00:00.012441
js_primitives.dart:47 Instance of 'minified:aF' warmup: 0:00:00.010440
js_primitives.dart:47 Instance of 'minified:X' warmup: 0:00:00.046915
js_primitives.dart:47 Instance of 'minified:aG' warmup: 0:00:00.028890
js_primitives.dart:47 Instance of 'minified:aE' real: 0:00:00.074291
js_primitives.dart:47 Instance of 'minified:aF' real: 0:00:00.063830
js_primitives.dart:47 Instance of 'minified:X' real: 0:00:02.195310
js_primitives.dart:47 Instance of 'minified:aG' real: 0:00:02.224940

Ideally, in the browser the crypto library should be using the Web Crypto API.
I think this would be worthwhile, to access a high performance implementation that is aynchronous to avoid jank.

dart:crypto has never been async. if you are doing any sha over a large sum of data, you should use a webworker.

test/sha512_test.dart

lib/src/sha512.dart

lrhn · 2019-07-10T09:31:13Z

lib/src/sha256.dart

+    var e = _digest[4];
+    var f = _digest[5];
+    var g = _digest[6];
+    var h = _digest[7];

    for (var i = 0; i < 64; i++) {
      var temp1 = add32(add32(h, _bsig1(e)),


On a 64-bit JS VM, it's likely that signed 32-bit numbers are efficient. Unsigned are not (but for addition and multiplication, you can use signed values without changing the outcome mod 2^32).
On a 32-bit JS VM, even 32 bit integers may be heap allocated, and only signed 31-bit integers get the full performance from "Smi" optimization.

Consider using configurable imports to switch between different implementations. It might reduce code size unless the compiler can efficiently tree-shake the unused branch.

I've added the conditional import for vm/js implementations. Are you suggesting fixednum for the js version?

I think fixednum might be more efficient, but it depends a lot on details. Maybe three smi multiplications are more efficient than two heap-number multiplications. Maybe they're not. Maybe it's sufficient to use Int32 values instead of Uint32 values for JS on 64-bit platforms.

Maybe just commit something that works, and then we can optimize it later :)

Maybe just commit something that works, and then we can optimize it later :)

Indeed, make sure the tests are right, and that we have plenty of test vectors, I would rather have a few too many than later be optimizing an incorrect implementation :) hehe

lib/src/sha512.dart

Speed is still meh in dart2js: 384/512 numbers: Instance of 'minified:a2' real: 0:00:02.203820 Instance of 'minified:aO' real: 0:00:02.192515

RSP values came from http://csrc.nist.gov/groups/STM/cavp/documents/shs/shabytetestvectors.zip

jtmcdole · 2019-07-11T06:04:04Z

$ dart test/sha_monte_test.dart
00:00 +0: Monte Vectors sha224
00:00 +1: Monte Vectors sha256
00:00 +2: Monte Vectors sha384
00:00 +3: Monte Vectors sha512
00:00 +4: All tests passed!

No clue about Travis...

jtmcdole · 2019-07-11T06:17:42Z

No clue about Travis...

Downloaded dart 2.0.0 for windows 64bit - was able to reproduce the travis errors. It's only the monte tests, so I'll try to figure out what is wrong with those...

jtmcdole · 2019-07-12T02:37:21Z

I'm going on vacation, so I might not get much traction on this. I feel its pretty important to figure out what is wrong - and I suspect its 64bit bit shifting differences (but I have no evidence).

jtmcdole · 2019-07-12T02:45:16Z

well, my evidence is: slowsinks works on 2.0 :)

jtmcdole · 2019-07-12T03:46:31Z

Odd evidence: while doing a binary search to figure out where the corruption lies... I noticed the VM flipped from giving good answers to giving bad ones at random on the same file.

inputs: dartsdk.zip (windows 64bit version)

cp dartsdk.zip test.file ; truncate.exe -s 2k test.file; sync
# run multiple times verifying it doesn't flip the answer...
sha512sum.exe test.file ; ./dart-sdk/bin/dart tmp.dart

jtmcdole · 2019-07-12T03:50:25Z

E.g.:

MINGW64 ~/dart/crypto (master)
$ cp dartsdk.zip test.file ; truncate.exe -s 6500 test.file; sync

MINGW64 ~/dart/crypto (master)
$ sha512sum.exe test.file; for i in {1..3}; do ./dart-sdk/bin/dart tmp.dart; done
4eeba4ca2b0bcb6ad2be03253e30120acaba265d43568e144dc1fb51e1027f0be30c4b453887f328f29626b89945193acf4802c43daf45bcade277f1abbe9e35 *test.file
088842479ddee4017315efdfaab7430112134a3806d04f1fee93a1d5f5deaaced3a4682107a3be002d02d4fd067f72599e153d4b5b0f00434baade1cd5a5e2dd
4eeba4ca2b0bcb6ad2be03253e30120acaba265d43568e144dc1fb51e1027f0be30c4b453887f328f29626b89945193acf4802c43daf45bcade277f1abbe9e35
4eeba4ca2b0bcb6ad2be03253e30120acaba265d43568e144dc1fb51e1027f0be30c4b453887f328f29626b89945193acf4802c43daf45bcade277f1abbe9e35

jtmcdole · 2019-07-12T04:15:43Z

Running the monte test and hashing the dart-sdk with other official releases found here: https://dart.dev/tools/sdk/archive shows no faults. I'm not going to debug this further: there's clearly something wrong with dartvm 2.0.0 and zero change that'll get patched. Suggested the minimum dart-sdk to be ^2.1.0.

2.0.0: fails monte; odd corruption between runs.
2.1.0: no problems
2.1.1: no problems
2.2.0: no problems
2.3.0: no problems
2.3.1: no problems
2.3.2: no problems
2.4.0: no problems
2.5.0-dev.1.0: no problems

command line:

 VERSION=2.4.0 ; curl https://storage.googleapis.com/dart-archive/channels/stable/release/$VERSION/sdk/dartsdk-windows-x64-release.zip > dart-sdk-$VERSION.zip; unzip dart-sdk-$VERSION.zip; mv dart-sdk dart-sdk-$VERSION; FILE=dartsdk.zip ; sha512sum.exe $FILE; for i in {1..3}; do ./dart-sdk-$VERSION/bin/dart tmp.dart $FILE; done; ./dart-sdk-$VERSION/bin/dart test/sha_monte_test.dart; ./dart-sdk-$VERSION/bin/dart --version

jonasfj

This looks sane to me :)

test/sha_monte_test.dart

jonasfj · 2019-07-12T11:42:50Z

I don't really own this package, or have uploader rights.
But unless anyone has more concerns (@lrhn?), I suggest we merge and publish..

@kevmoo, I see you have uploader access, maybe you want to publish, if you have no further concerns :)

lrhn · 2019-07-12T12:09:14Z

LGTM if it works.

jtmcdole · 2019-07-23T21:11:35Z

Is there anything else needed to merge this?

jtmcdole · 2019-07-25T04:08:42Z

@lrhn : I think you're the only one with write access to this repo.

jtmcdole · 2019-07-30T03:48:34Z

Still looking for some love...

jonasfj · 2019-08-05T11:04:17Z

@sethladd, @sigmundch, you two seems to have uploader permissions on pub.dev/packages/crypto, would either if you like to merge and publish this PR?

sethladd · 2019-08-05T15:48:05Z

Apologies, I'm not longer working on this library.

Maybe @kevmoo can help find a way to nudge this along?

efortuna · 2019-08-05T22:43:30Z

Okay, I've looked over this, and can merge since I have TEH POWER, but I'm relying on @lrhn @rakudrama @jonasfj and @kevmoo to follow up with the questions they raised about this on performance/testing.

kevmoo · 2019-08-05T22:57:02Z

.travis.yml

-  - 2.0.0
-  - dev
+  - 2.1.0
+  - stable 


This should have stayed dev – keeps us honest about upcoming SDKs!

..... should I send a follow-up PR? It's only been up for a few min!

Is there a different between dartvm 2.1.0 stable and dartvm 2.1.0 dev?

smaifullerton-wk · 2019-08-05T23:38:30Z

@jtmcdole @efortuna 👋 This release broke all of our Dart builds:

[SEVERE] build_web_compilers:ddc on package:crypto/crypto.dartdevc.module:
Error compiling dartdevc module:crypto|lib/crypto.ddc.js

[error] Target of URI doesn't exist: 'sha512_fastsinks.dart'. (package:crypto/src/sha512.dart, line 9, col 8)

Please fix all errors before compiling (warnings are okay).

Thanks for taking a look!

jtmcdole · 2019-08-05T23:46:24Z

@jtmcdole @efortuna This release broke all of our Dart builds:

[SEVERE] build_web_compilers:ddc on package:crypto/crypto.dartdevc.module:
Error compiling dartdevc module:crypto|lib/crypto.ddc.js

[error] Target of URI doesn't exist: 'sha512_fastsinks.dart'. (package:crypto/src/sha512.dart, line 9, col 8)

Please fix all errors before compiling (warnings are okay).

Earlier I had a quick hack check to decide which files to include. It was suggested I use this line instead:

import 'sha512_fastsinks.dart' if (dart.library.js) 'sha512_slowsinks.dart';

smaifullerton-wk · 2019-08-05T23:48:10Z

It appears that upgrading to build_web_compilers ^2.0.0 fixes the issue. If there's any way to make this change backwards compatible with BWC <2.0.0 that'd be awesome. We have 10s of packages that use BWC <2.0.0 at the moment.

efortuna · 2019-08-05T23:59:59Z

Hey folks -- thanks for the report of the issues! I'm discussing with Nate and we're going to submit a fix right away

efortuna · 2019-08-06T00:33:34Z

New version pushed. Apologies for the disruption! We do recommend upgrading to using build_web_compilers to 2.0.0+, but I committed a workaround that should work for everyone, regardless of whether you do that o rnot.

postmasters · 2019-08-17T01:48:42Z

lib/src/sha384_512.dart

+/// new instance.
+class Sha384 extends Hash {
+  @override
+  final int blockSize = 16 * bytesPerWord;


This looks wrong to me. According to https://en.wikipedia.org/wiki/SHA-2, SHA512 has 1024 bits internal block size, i.e. 128 bytes. 16 * 4 is only 64.

On this note, that should have failed HMAC-SHA512 test, had there been one.

You're commenting on an outdated commit. Also, this is not HMAC-512, its just Sha512.

I see what you're getting at though - I did not update the HMAC. That can be easily done (pull requests welcome ;) )

postmasters

One trivial error and a request for HMAC-SHA512 test ;-).

* Add SHA384 and SHA512 Sha384 and Sha512 share the same implementation and differ only in the initailization vectors and the digest concatentation. The algorithm from from rfc6234 calls for 64bit words and operations and to remain dart2js friendly, BigInt was used. INPUT REQUESTED: I'm not sure where the test vectors came from; but a simple comparison to sha384 and sha512 command line methods shows it matching. If you can point me to them, then I can write a better test. * Add Sha224 + Refactor Do something less stupid; use _Sha32BitSink and _Sha64BitSink to have class heirarchies make better sense. * Support 32bit and 64bit operations for SHA384/512 Two modes of operation: if you're in a browser, you get the slower 32bit algorithm because you only have 2^53 bits. If you are on the VM / Flutter, you'll have 64bit operations wich is *much* faster: 32BIT NUMBERS: ~20MB/s hashing Removing BigInt has some good results: Instance of 'Sha224' warmup: 0:00:00.015599 Instance of 'Sha256' warmup: 0:00:00.002325 Instance of 'Sha384' warmup: 0:00:00.019082 Instance of 'Sha512' warmup: 0:00:00.010288 Instance of 'Sha224' real: 0:00:00.092928 Instance of 'Sha256' real: 0:00:00.093426 Instance of 'Sha384' real: 0:00:00.823335 Instance of 'Sha512' real: 0:00:00.807871 64BIT NUMBERS: ~236MB/s hashing On the VM, this is much faster with 64bit operations. Instance of 'Sha224' warmup: 0:00:00.013285 Instance of 'Sha256' warmup: 0:00:00.002443 Instance of 'Sha384' warmup: 0:00:00.020954 Instance of 'Sha512' warmup: 0:00:00.005616 Instance of 'Sha224' real: 0:00:00.097196 Instance of 'Sha256' real: 0:00:00.094167 Instance of 'Sha384' real: 0:00:00.067605 Instance of 'Sha512' real: 0:00:00.067564 NOTE: Compiles with dart2js - cannot reference 64bit hex as the compiler just vomits... but you can left shift by 32. * Fix comment * Add conditional imports * De-listify 32bit allocations Speed is still meh in dart2js: 384/512 numbers: Instance of 'minified:a2' real: 0:00:02.203820 Instance of 'minified:aO' real: 0:00:02.192515 * Add sha monte tests for 224,256,384, and 512 RSP values came from http://csrc.nist.gov/groups/STM/cavp/documents/shs/shabytetestvectors.zip * Fix formattting of lib/crypto.dart * Bump version (adding new features) + min sdk * Bump travis (hidden file) * Rework monte test to function-only.

googlebot added the cla: yes label Jul 8, 2019

Add Sha224 + Refactor

b6c564f

Do something less stupid; use _Sha32BitSink and _Sha64BitSink to have class heirarchies make better sense.

jtmcdole changed the title ~~Add SHA384 and SHA512~~ Add missing SHA224, SHA384, and SHA512 Jul 9, 2019

jonasfj reviewed Jul 9, 2019

View reviewed changes

lib/src/sha512.dart Outdated Show resolved Hide resolved

jtmcdole closed this Jul 10, 2019

jtmcdole reopened this Jul 10, 2019

Fix comment

d629f9d

jonasfj reviewed Jul 10, 2019

View reviewed changes

test/sha512_test.dart Show resolved Hide resolved

lib/src/sha512.dart Outdated Show resolved Hide resolved

lrhn reviewed Jul 10, 2019

View reviewed changes

lib/src/sha512.dart Outdated Show resolved Hide resolved

jtmcdole and others added 4 commits July 10, 2019 07:35

Add conditional imports

197f2c9

De-listify 32bit allocations

a7f4098

Speed is still meh in dart2js: 384/512 numbers: Instance of 'minified:a2' real: 0:00:02.203820 Instance of 'minified:aO' real: 0:00:02.192515

Add sha monte tests for 224,256,384, and 512

b9b8207

RSP values came from http://csrc.nist.gov/groups/STM/cavp/documents/shs/shabytetestvectors.zip

Fix formattting of lib/crypto.dart

ef8617f

jtmcdole added 2 commits July 11, 2019 21:17

Bump version (adding new features) + min sdk

724aae2

Bump travis (hidden file)

c02ba89

jonasfj approved these changes Jul 12, 2019

View reviewed changes

test/sha_monte_test.dart Outdated Show resolved Hide resolved

Rework monte test to function-only.

bb4cef8

efortuna merged commit a66bf90 into dart-archive:master Aug 5, 2019

kevmoo reviewed Aug 5, 2019

View reviewed changes

postmasters reviewed Aug 17, 2019

View reviewed changes

Add missing SHA224, SHA384, and SHA512 #63

Add missing SHA224, SHA384, and SHA512 #63

Conversation

jtmcdole commented Jul 8, 2019

jtmcdole commented Jul 8, 2019

kevmoo commented Jul 8, 2019

jtmcdole commented Jul 8, 2019

rakudrama commented Jul 9, 2019

jtmcdole commented Jul 9, 2019

lrhn commented Jul 9, 2019

jonasfj commented Jul 9, 2019

jtmcdole commented Jul 10, 2019

jtmcdole commented Jul 10, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtmcdole commented Jul 11, 2019

jtmcdole commented Jul 11, 2019

jtmcdole commented Jul 12, 2019

jtmcdole commented Jul 12, 2019

jtmcdole commented Jul 12, 2019

jtmcdole commented Jul 12, 2019

jtmcdole commented Jul 12, 2019

jonasfj left a comment

Choose a reason for hiding this comment

jonasfj commented Jul 12, 2019

lrhn commented Jul 12, 2019

jtmcdole commented Jul 23, 2019

jtmcdole commented Jul 25, 2019

jtmcdole commented Jul 30, 2019

jonasfj commented Aug 5, 2019

sethladd commented Aug 5, 2019

efortuna commented Aug 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smaifullerton-wk commented Aug 5, 2019

jtmcdole commented Aug 5, 2019

smaifullerton-wk commented Aug 5, 2019

efortuna commented Aug 5, 2019

efortuna commented Aug 6, 2019

postmasters Aug 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

postmasters left a comment

Choose a reason for hiding this comment

jtmcdole commented Jul 10, 2019 •

edited

Loading

postmasters Aug 17, 2019 •

edited

Loading