Re: [tor-bugs] #10872 [Pluggable transport]: review of obfsclient

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

#10872: review of obfsclient
-------------------------------------+-----------------------
     Reporter:  infinity0            |      Owner:  infinity0
         Type:  task                 |     Status:  assigned
     Priority:  normal               |  Milestone:
    Component:  Pluggable transport  |    Version:
   Resolution:                       |   Keywords:
Actual Points:                       |  Parent ID:
       Points:                       |
-------------------------------------+-----------------------

Comment (by yawning):

 Replying to [comment:1 infinity0]:
 > *..c7400b87e3ec8d8380c3fbf9919b08438b3725cf:
 >
 > Brief comments about the crypto code, excluding uniform DH:
 >
 > - aes_ctr128 should be renamed aes128_ctr

 I can, though the naming I used is consistent with OpenSSL and how the
 obfs3 spec refers to the algorithm (Eg:
 EVP_aes_ctr_128()/AES_ctr128_encrypt(), AES-CTR-128).  Maybe I should use
 the SP 800-38A name (CTR-AES128)?

 >   - call clear_state() in aes_ctr128 destructor?

 block_ and ctr_ get wiped when their dtors are called (That's the point of
 the class), explicitly calling clear_state() is a bit redundant (I could
 explicitly wipe has_state_ and offset_).

 >   - please annotate AesCtr128::process() in more detail, I couldn't get
 my head around it.
 >   - in the header file, please also document
 >     - how is the counter updated for the next block? I couldn't figure
 this out
 >     - the significance of "offset" in terms of the algorithm, how this
 is used

 Both of these are related, offset_ maintains a count of how many bytes of
 block_ the algorithm has consumed.  When offset_ == 0, I generate a new
 block and increment the ctr_ by 1.

 I guess this part is hard to read, it works like this:
  * For each byte of ctr_ (in reverse)
    * byte += 1
      * if (byte != 0) break

 > - it's valid to request a hash/mac/encryption of an empty buffer
 >   - remove the len == 0 test from hmac_sha256.cc and sha256.cc
 >   - in aes_ctr128 the test can remain, but it should probably "return
 true" rather than "return false"

 Ok.  All the uses of these classes in the code currently require failure
 when length is 0, so I would be checking that there instead, but that is
 indeed the right thing to do.

 > - I don't know about C++ conventions, but it's probably clearer to have
 bool memequals (and return !ret), instead of int memequals

 I can do this.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/10872#comment:3>
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