Opened 7 years ago

Closed 7 years ago

#11611 closed defect (fixed)

obfs2/obfs3 AES counter initialization is incorrect.

Reported by: yawning Owned by: asn
Priority: Medium Milestone:
Component: Archived/Obfsproxy Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


From obfsproxy/common/

        self.ctr =, initial_value=long(iv.encode('hex'), 16))

From the Crypto.Util.Counter docstring:

allow_wraparound : boolean
If *True*, the counter will automatically restart from zero after
reaching the maximum value (``2**nbits-1``).
If *False* (default), the object will raise an *OverflowError*.

The docs on the pycrypto web page are incorrect ( and haven't been regenerated yet.

The obfs2/3 protocols uses a initial value derived from the UniformDH handshake, allow_wraparound=True should be passed to the constructor here to avoid mysterious (though extremely unlikely) connection failures.

Child Tickets

Change History (5)

comment:1 Changed 7 years ago by asn

Hm, thanks for catching this.

PyCrypto increments the counter by one for each use, right? If yes, I don't see a big problem with switching allow_wraparound to True.

OTOH, maybe for other protocols, where the IV is not derived from the protocol, the current behavior of bailing on wraparound is more sane.

comment:2 Changed 7 years ago by yawning

PyCrypto will increment the counter by one per every block size worth of data processed.

ScrambleSuit actually uses a prefix | counter style non-wrapping counter, but the code explicitly sets allow_wraparound to False (and doesn't use common/ I do not see why future protocols could not use pycrypto's AES directly if they want different behavior like ScrambleSuit does.

comment:3 Changed 7 years ago by asn

Please see branch bug11611 in my repo:

As I understand it, the counter will now overlfow after 2128 block encryptions. This sounds unlikely to ever happen, but because I don't like this behavior I disabled it by default. obfs2 and obfs3 explicitly enable the wraparound behavior. Future protocols should consider using an initial counter value of 0 :/

comment:4 Changed 7 years ago by asn

Status: newneeds_review

comment:5 Changed 7 years ago by asn

Resolution: fixed
Status: needs_reviewclosed

Merged after an ACK from yawning. Thanks :)

Note: See TracTickets for help on using tickets.