Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#11015 closed defect (fixed)

UniformDH should not block the main event loop

Reported by: yawning Owned by: asn
Priority: Medium Milestone:
Component: Obfuscation/Obfsproxy Version:
Severity: Keywords: python, UniformDH, cpu exhaustion
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

This isn't as big of a deal when gmpy is installed, but depending on how determined the adversary is, it still might be a problem.

I got curious I went and benchmarked the obfsproxy UniformDH implementation. On the test machine I used (i5-3320M), the generate + exchange takes ~24 - 25 msec (With gmpy it takes ~3.8 msec).

Since the obfsproxy code does the key exchange in the main event loop, this means that on each incoming connection, the server will spend that much time doing the modular exponentiation and nothing else (To be pedantic for the obfs3 transport the attacker needs to only open a ton of TCP connections, even without sending anything to be successful).

Things that should be done:

  • Use twisted.internet.threads.deferToThread to do the modular exponentiation in a thread pool, leaving the main event loop free to process other connections.
  • Rate limit the number of incoming connections processed per interval to something sane. Also strongly consider rate limiting by source IP, so that an adversary at least has to get a bot net.
  • modexp.powMod should also support using gmpy2 (Different import). Per the authors "gmpy2 is now the recommended version, especially if you use the pre-compiled versions for Windows."
  • I do have a OpenSSL based implementation of the key exchange that is similar in performance to the gmpy based code. I could write a python module for it.

Child Tickets

Attachments (5)

0001-Also-support-gmpy2-for-doing-modular-exponentiation.patch (1.9 KB) - added by yawning 5 years ago.
Support gmpy2 in addition to gmpy1
0001-Use-twisted.internet.threads.deferToThread-in-the-ob.patch (3.9 KB) - added by yawning 5 years ago.
0002-Threaded-obfs3-UniformDH-incorporate-feedback-from-i.patch (2.9 KB) - added by yawning 5 years ago.
0001-Add-support-for-using-py-uniformdh.patch (2.1 KB) - added by yawning 5 years ago.
0001-Use-deferToThread-SQUASHED.patch (5.7 KB) - added by yawning 5 years ago.
Squahsed version of the UniformDH changes, with fixes (Integration tests pass)

Download all attachments as: .zip

Change History (15)

comment:1 Changed 5 years ago by yawning

Lunar added python-gmpy to the debian package Recomends, and asn wrote an email to tor-relays@, so this is mitigated somewhat (Thanks!). I still think that all of the things in the ticket should be done at some point, since gmpy only buys a ~6x increase in the number of handshakes that can be processed in a given interval.

Changed 5 years ago by yawning

Support gmpy2 in addition to gmpy1

comment:2 Changed 5 years ago by yawning

Sorry for the conflicting ChangeLogs, I used separate branches for each patch. Someone that's not me that knows the obfsproxy internals more should come up with a rate limiting scheme.

I would go and thread ScrambleSuit's UniformDH handshake as well, but it's less of an issue since it takes knowing the bridge key to be able to force obfsproxy to do the modular exponentiation.

comment:3 Changed 5 years ago by infinity0

LGTM both patches, with the following suggestions for the threads patch:

  • move the errback next to the callback so they are visually closer together
  • self.circuit.close() is only called when you trap ValueError; it is not called if the error is something else (in which case trap() raises). perhaps you should move it above the trap.
  • you may want to log e as well.
Last edited 5 years ago by infinity0 (previous) (diff)

comment:4 Changed 5 years ago by infinity0

Oh, another thing: you can just pass in simply self.dh.get_secret as the task to deferToThread, instead of spending 8 lines to define a separate _uniform_dh_task.

comment:5 Changed 5 years ago by yawning

The patch I just attached incorporates infinity0's feedback (though I don't log e as the behaviour when I don't is closest to the unmodified code (re-raise a new exception that logs what the threaded code logs and closes)).

Thanks for the feedback.

Changed 5 years ago by yawning

comment:6 Changed 5 years ago by yawning

And one more. Depends on https://github.com/Yawning/py-uniformdh

Probably preferable to using gmpy.

comment:7 Changed 5 years ago by asn

Thanks for the patches.

I tried a branch with the first three patches merged (the threading and gmpy2 changes) and it seems to break obfs3. Have you tested it?

Also, maybe it makes more sense to squash the two threading patches together.

Changed 5 years ago by yawning

Squahsed version of the UniformDH changes, with fixes (Integration tests pass)

comment:8 Changed 5 years ago by yawning

The latest patch obsoletes the previous 2 threading patches, and fixes things so that the integration tests run properly, and I can bootstrap using the modified obfsproxy both as a client and server.

Thanks!

comment:9 Changed 5 years ago by asn

Resolution: fixed
Status: newclosed

Merged! Thanks!

Closing this ticket. I opened #11093 for the C implementation of UniformDH.

comment:10 Changed 5 years ago by asn

FWIW I also merged & pushed the gmpy2 commit.

Note: See TracTickets for help on using tickets.