[tor-bugs] #17783 [Tor]: Pull in a SHA-3 implementation.

[Tor Bug Tracker & Wiki]

#17783: Pull in a SHA-3 implementation.
-----------------------------+------------------------------------
 Reporter:  yawning          |          Owner:  yawning
     Type:  enhancement      |         Status:  needs_revision
 Priority:  Medium           |      Milestone:  Tor: 0.2.8.x-final
Component:  Tor              |        Version:  Tor: unspecified
 Severity:  Normal           |     Resolution:
 Keywords:  tor-core crypto  |  Actual Points:
Parent ID:                   |         Points:
  Sponsor:                   |
-----------------------------+------------------------------------

Comment (by yawning):

 Replying to [comment:4 nickm]:
 [snip]
 >  * first commit is a pure import; that's fine.
 >    * But I don't like the code duplication between keccak-tiny-
 unrolled.c and keccak-tiny.c.  Keeping them in sync would seem to be
 annoyingly hard.  Let's only maintain one. Which is the only-slightly-
 unrolled one, and how much better is the other one's performance?

 The author recommends `keccak-tiny-unrolled.c`, I personally didn't find a
 huge difference between either, though I suspect this would be
 CPU/microarchitecture specific.  For modern Intel, I suspect that the
 unrolled version doesn't fit in the LSD cache...

 >  * second commit:
 >    * forks the api and makes the changes non-mergeable to upstream by
 introducing tor dependencies. I'm not 100% a fan of making the changes
 non-upstreamable; can we isolate our changes behind a set of #defines and
 some "keccak-tiny-private.h" file?

 I wasn't really planning on fighting to merge this upstream, but ok.

 >    * The correct use of KECCAK_{XOF,HASH}_DELIM and
 KECCAK_TARGET_TO_RATE in keccak_init, the fact that you need to init
 before you add bytes or finalize, and the fact that you need to finalize
 before you squeeze, should probably be documented. In fact, it might not
 be crazy to assert in the code that these requirements are met.

 Ok.

 >    * There should be a comment explaining that nobody should ever look
 at keccak_state's internals.

 Ok (though I do at one point in our XOF wrapper, I'll change that).

 >  * third commit (9edd9a8e2c2f283e7607e59524ffdf0b2ae51516):
 >    * The assertions about bit sizes are getting big; probably we should
 have a new inline function that takes a digest_algorithm_t and returns the
 size of its output.

 Ok.

 >    * I would like to see a more unit tests here.  In particular, I would
 like to see:
 >      * more test vectors, including the empty vector, vectors exactly
 equal to one/two input block size, vectors equal to one/two input block
 sizes plus or minus one, a single-character input, and something with a
 NUL in the middle.

 Ok, for both SHA3-256 and 512?

 >      * Although we don't do this with the other incremental digests, I'd
 like to see is do a pretty rigorous test on the incremental update
 function -- possibly randomized tests where we generate a big random
 buffer and add it incrementally in random-sized chunks and make sure we
 get the same output each time.  (I think this is a good idea because in
 this case the incremental update code is our own.)

 Hm, ok.

 >   * Fourth commit (the one to add xof):
 >     * Here you test the return value of keccak_init; in the previous
 commit you don't. Probably best to be consistent.

 Ok.

 I'll make all the changes when I can hear myself think.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/17783#comment:8>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online