Import Bruce Schneier's Skein hashing algorithm
ClosedPublic

Authored by allanjude on Apr 30 2016, 9:34 PM.

Details

Summary

Connects it up to username (libmd, libcrypt, sbin/md5), and the kernel (crypto.ko).
Will be connected to ZFS in a separate commit.

Test Plan

Verified against the test files

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
allanjude retitled this revision from to Import Bruce Schneier's Skein hashing algorithm.Apr 30 2016, 9:34 PM
allanjude updated this object.
allanjude edited the test plan for this revision. (Show Details)
allanjude updated this revision to Diff 15779.Apr 30 2016, 9:37 PM

Don't link sbin/md5 against both libmd and libcrypt, this was just a test to make sure it didn't break

My intention was for the copy of skein in sys/crypto to only show the differences from the unmodified import, but it didn't actually work that way.

allanjude updated this revision to Diff 15781.May 1 2016, 3:20 AM

Remove some future changes to sbin/md5 that slipped in

bcr added a subscriber: bcr.May 2 2016, 6:33 PM

Found a small nit in the man page.

lib/libmd/skein.3
94 ↗(On Diff #15781)

Does this single "a" have to be on an line of its own?

cem added a reviewer: cem.May 26 2016, 6:36 PM
cem added a comment.May 26 2016, 6:58 PM

I don't think it's useful to drop the skein reference files in both sys/contrib and sys/crypto. Leave them in contrib and add sys/contrib/skein to $PATH in the sys/crypto Makefile.

Does anything in the kernel use Skein? Do you intend to add anything using Skein? Skein didn't win the SHA3 contest and I don't know how much real world use it gets or will get. If we don't need it for anything, I don't think we need to import it.

lib/libmd/Makefile
96–98 ↗(On Diff #15781)

Does this need to be conditional on actually compiling assembly? Does it hurt to always add these to the assembler flags?

lib/libmd/skein.3
3–6 ↗(On Diff #15781)

No he didn't. Correct the copyright header. :)

90–92 ↗(On Diff #15781)

Not true for short inputs or inputs where a dictionary attack is feasible, e.g. 64-bit values, first names, etc. I would just drop that sentence.

93–96 ↗(On Diff #15781)

I would drop this sentence too. It's not quite a magic bullet.

sys/contrib/skein/asm/skein_block_x64.asm
1 ↗(On Diff #15781)

I thought this was supposed to go in machine/ per the Makefiles earlier.

sys/contrib/skein/asm/skein_block_x64.s
1 ↗(On Diff #15781)

Is this identical to skein_block_x64.asm? Why have both?

sys/contrib/skein/asm/skein_block_xmm32.s
1 ↗(On Diff #15781)

Identical to skein_block_xmm32.asm?

sys/crypto/skein/amd64/skein_block_asm.s
1–3 ↗(On Diff #15781)

Another copy of this file?!

sys/crypto/skein/brg_endian.h
2–3 ↗(On Diff #15781)

I really don't think we need a second copy of this file. We have endian defines already from sys/endian.h. For the non-contrib/ directory, IMO simplify to just what is needed for FreeBSD.

sys/crypto/skein/brg_types.h
2–4 ↗(On Diff #15781)

Same comment as for brg_endian.

sys/crypto/skein/skein.h
103–105 ↗(On Diff #15781)

I'd prefer array-style output, e.g.,

int Skein_256_Final(Skein_256_Ctxt_t *, u08b_t[static SKEIN256_DIGEST_LENGTH]);
sys/crypto/skein/skein_freebsd.h
28–30 ↗(On Diff #15781)

Drop space between * and ctx.

32–34 ↗(On Diff #15781)

Use static, drop excess space between * and ctx.

I think the contrib stuff should probably go in the vendor tree instead, and then just have the one copy in sys/crypto

lib/libmd/Makefile
96–98 ↗(On Diff #15781)

I am not sure, I just went with what was already there.

lib/libmd/skein.3
93–96 ↗(On Diff #15781)

Should change these in the md5,sha* pages too then

sys/contrib/skein/asm/skein_block_xmm32.s
1 ↗(On Diff #15781)

different assembly dialect. Probably don't need the unused ones I guess. This was just everything from the directories that made sense in the original distribution package.

sys/crypto/skein/brg_endian.h
2–3 ↗(On Diff #15781)

ok

allanjude updated this revision to Diff 17039.May 28 2016, 6:04 PM
allanjude marked 6 inline comments as done.

Refactor the skein import via ^/vendor-sys/skein

Much cleaner, and easier to see my local changes

allanjude updated this revision to Diff 17040.May 28 2016, 6:21 PM

Fix missing manual pages for skein{256,512,1024}(1)

bump HEX_DIGEST_LENGTH

allanjude updated this revision to Diff 17041.May 28 2016, 6:28 PM

Fix a mismerge

cem added a comment.May 28 2016, 8:21 PM

There are a bunch of seemingly arbitrary changes to vendor/contrib code in this revision, and I don't know why.

lib/libcrypt/Makefile
43–44 ↗(On Diff #17040)

Is this just a rebase, or are you including this in the Skein commit? I'd keep them separate.

lib/libmd/Makefile
12–16 ↗(On Diff #17040)

same

49–52 ↗(On Diff #17040)

same

lib/libmd/skein.3
93–96 ↗(On Diff #15781)

Yes.

sbin/md5/md5.c
305–306 ↗(On Diff #17040)

Why'd you revert this?

allanjude updated this revision to Diff 17045.May 28 2016, 10:33 PM
allanjude marked 5 inline comments as done.

Reduce vendor diff

lib/libcrypt/Makefile
43–44 ↗(On Diff #17040)

it is a rebase, the sha512t256 commit went in earlier today, which had some merge conflicts with this patch since it adds to the same makefiles.

sbin/md5/Makefile
22 ↗(On Diff #17040)

mismerge

sbin/md5/md5.c
305–306 ↗(On Diff #17040)

I didn't know if it was a good idea to change the output of all of the hashing commands.
I guess this specific bit, the benchmark, likely isn't depended upon by many scripts.

Do you think it is ok to change it to the more readable format?

cem added inline comments.May 28 2016, 10:36 PM
sbin/md5/md5.c
305–306 ↗(On Diff #17040)

I don't think anything should depend on it... but now's the time to "break" it anyway, before 11 freezes.

allanjude updated this revision to Diff 17049.May 29 2016, 12:17 AM

restore md5(1) improvements

Improve the skein(3) man page

cem added a subscriber: wblock.May 29 2016, 12:23 AM
cem added inline comments.
lib/libmd/skein.3
109 ↗(On Diff #17049)

This should be a dash, not a hyphen. Does troff have a good way to format dash? Paging @wblock :).

110 ↗(On Diff #17049)

This one too.

111 ↗(On Diff #17049)

This one is (correctly) a hyphen!

215 ↗(On Diff #17049)

Should have a newline at end of file.

allanjude updated this revision to Diff 17050.May 29 2016, 12:34 AM

Switch the skein(3) man page to use em-dashes

allanjude marked an inline comment as done.May 29 2016, 12:35 AM
allanjude marked 3 inline comments as done.
allanjude added inline comments.
lib/libmd/skein.3
109 ↗(On Diff #17049)

switched to using the proper mandoc em-dash markup

cem accepted this revision.May 29 2016, 12:54 AM

Flawless ;-).

This revision is now accepted and ready to land.May 29 2016, 12:54 AM
This revision was automatically updated to reflect the committed changes.
allanjude marked an inline comment as done.