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

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#17783: Pull in a SHA-3 implementation.
-----------------------------+------------------------------------
 Reporter:  yawning          |          Owner:  yawning
     Type:  enhancement      |         Status:  new
 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 nickm):

 Reviewing...

 Looking good!

  * 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?
  * 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?
    * 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.
    * There should be a comment explaining that nobody should ever look at
 keccak_state's internals.
  * 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.
    * 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.
      * 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.)
   * 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.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/17783#comment:4>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs