Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make wrap_module patch socket.getaddrinfo. #91

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

JeremyRand
Copy link

Good day!

I'm developing a proxy leak detection tool, and thought it would be a fun challenge to use it to investigate #22. And happily it looks like I was successful. This PR makes wrap_module patch socket.getaddrinfo (preventing DNS leaks from that function), and updates the monkeypatching documentation accordingly.

I opted to use the workaround at #22 (comment) rather than the workaround at #22 (comment) , because the latter only works with SOCKS proxies that support Tor's nonstandard protocol extensions. The latter is definitely preferable if the user is certain that their proxy supports Tor's extensions; a future PR could give the user an option to choose which is used.

It's entirely possible that I've subtly broken something, as I'm not intimately familiar with the Python socket codebase, but it seems to work acceptably in my testing.

Cheers!

@Anorov
Copy link
Owner

Anorov commented Sep 18, 2017

Thanks for this PR. Could you please write a test for this, covering different kinds of addresses? We don't currently have any coverage for getaddrinfo().

And does this also prevent DNS leaks from gethostbyname()?

@JeremyRand
Copy link
Author

Could you please write a test for this, covering different kinds of addresses?

Are you looking for tests that make sure it doesn't leak outside of the proxy, or tests that make sure it still behaves correctly in terms of what an application expects?

And does this also prevent DNS leaks from gethostbyname()?

I don't think it does, although I haven't tried.

@Anorov
Copy link
Owner

Anorov commented Dec 18, 2017

Ideally tests for both. Thanks.

@JeremyRand
Copy link
Author

Do any of the existing tests actually check for leaks outside of the proxy? If so, could you point me to the relevant lines? If not, how do you prefer me to do that? The leak detector I wrote uses some strace magic to detect socket syscalls; if you like I can just publish the code for the leak detector and then incorporate it into the tests.

@Anorov
Copy link
Owner

Anorov commented Jul 24, 2018

Do any of the existing tests actually check for leaks outside of the proxy?

Not currently.

if you like I can just publish the code for the leak detector and then incorporate it into the tests.

Please do, thanks. Ideally tests shouldn't have any non-Python dependencies, but we've already broken that rule pretty badly, so whatever works should be fine for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants