Opened 5 years ago

Closed 10 months ago

#10872 closed task (wontfix)

review of obfsclient

Reported by: infinity0 Owned by: infinity0
Priority: Medium Milestone:
Component: Archived/obfsclient Version:
Severity: Normal Keywords: archived-closed-2018-07-04
Cc: yawning Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Child Tickets

Change History (8)

comment:1 Changed 5 years ago by infinity0

*..c7400b87e3ec8d8380c3fbf9919b08438b3725cf:

Brief comments about the crypto code, excluding uniform DH:

  • aes_ctr128 should be renamed aes128_ctr
    • call clear_state() in aes_ctr128 destructor?
    • 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
  • 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"
  • I don't know about C++ conventions, but it's probably clearer to have bool memequals (and return !ret), instead of int memequals

comment:2 Changed 5 years ago by infinity0

Cc: yawning added
Owner: changed from Yawning to infinity0
Status: newassigned

comment:3 in reply to:  1 Changed 5 years ago by yawning

Replying to 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.

The AES code will be modified when I change it to support both AES128 and AES256 (Needed for ScrambleSuit).

https://github.com/Yawning/obfsclient/commit/c28c3780d2edf908a30c7ef6fa4f4ac6c0522805

  • 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.

https://github.com/Yawning/obfsclient/commit/c27683a523ded976f680aaa64824c7775345a229

Last edited 5 years ago by yawning (previous) (diff)

comment:4 Changed 5 years ago by yawning

Re: The Aes128Ctr situation.

My (unpushed) scramblesuit branch has a ever so slightly better documented version of the code, and the classes are named Aes128Ctr/Aes192Ctr/Aes256Ctr (Technically they're a bunch of typedefs since "AesCtr<AesEcb<::EVP_aes_128_ecb, kAes128KeyLength>>" is horrendous to type.

In theory this will let me more easily change out the CTR mode implementation to something that isn't ECB based later as long as I use the typedefs (I don't use OpenSSL's because I don't particularly care for eviscerating the required number of goats and reading their entrails to figure out what versions actually provide working EVP_aes_FOO_ctr.).

Update: The latest and greatest has the refactored AES code now (Along with a bunch of other things).

Last edited 5 years ago by yawning (previous) (diff)

comment:5 Changed 5 years ago by yawning

The latest version has logging now (yay). I *hope* I stamped out the FreeBSD/compiler related portability issues (though I still require GCC 4.7.x at the oldest).

Back to expanding the ScrambleSuit support.

comment:6 Changed 5 years ago by yawning

Component: Pluggable transportobfsclient
Description: modified (diff)

Changing the component to the obfsclient one, and pointing the description at my user repo.

Also for what it's worth, since this was last looked at:

  • asn did a cursory look over the code (not in depth)
  • I did some basic static analysis (cppcheck)
  • I switched the build process to include -Wextra, though that didn't find anything major.

comment:7 Changed 17 months ago by teor

Severity: Normal

Set all open tickets without a severity to "Normal"

comment:8 Changed 10 months ago by teor

Keywords: archived-closed-2018-07-04 added
Resolution: wontfix
Status: assignedclosed

Close all tickets in archived components

Note: See TracTickets for help on using tickets.