Page MenuHomeFreeBSD

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 - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 4008
Build 4051: arc lint + arc unit

Event Timeline

allanjude retitled this revision from to Import Bruce Schneier's Skein hashing algorithm.
allanjude updated this object.
allanjude edited the test plan for this revision. (Show Details)
allanjude edited edge metadata.

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 edited edge metadata.

Remove some future changes to sbin/md5 that slipped in

Found a small nit in the man page.

lib/libmd/skein.3
94

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

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
98–100

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

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

90–92

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

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

Another copy of this file?!

sys/crypto/skein/brg_endian.h
1–2

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
1–3

Same comment as for brg_endian.

sys/crypto/skein/skein.h
103–105

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

Drop space between * and ctx.

32–34

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
98–100

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

lib/libmd/skein.3
93–96

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
1–2

ok

allanjude edited edge metadata.
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 edited edge metadata.

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

bump HEX_DIGEST_LENGTH

allanjude edited edge metadata.

Fix a mismerge

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

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

lib/libmd/Makefile
12–17

same

49–65

same

lib/libmd/skein.3
93–96

Yes.

sbin/md5/md5.c
305–306

Why'd you revert this?

allanjude marked 5 inline comments as done.
allanjude edited edge metadata.

Reduce vendor diff

lib/libcrypt/Makefile
43–44

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–25

mismerge

sbin/md5/md5.c
305–306

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?

sbin/md5/md5.c
305–306

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

allanjude edited edge metadata.

restore md5(1) improvements

Improve the skein(3) man page

cem added inline comments.
lib/libmd/skein.3
109

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

110

This one too.

111

This one is (correctly) a hyphen!

216

Should have a newline at end of file.

allanjude edited edge metadata.

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

allanjude marked 3 inline comments as done.
allanjude added inline comments.
lib/libmd/skein.3
109

switched to using the proper mandoc em-dash markup

cem edited edge metadata.

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.