Opened 9 years ago

Closed 8 years ago

#3342 defect closed fixed (fixed)

twisted.names dns-spoofing vulnerability

Reported by: jknight Owned by:
Priority: normal Milestone:
Component: names Keywords:
Cc: jknight Branch: branches/dns-random-source-port-3342
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun



We apparently need to use a cryptographically secure random source port number.

Change History (10)

comment:1 Changed 9 years ago by Jean-Paul Calderone

I think this came up once before. Last time, someone (probably me, I don't really remember) decided that using a random (hopefully cryptographically so, but the code will fall back to a non-cryptographically secure random source if a cryptographically secure random source is unavailable on the system) message identifier would be a good enough fix. Looking at the vulnerabilities linked from that page, I see three pieces of software which failed to use cryptographically secure random sources, but no vulnerabilities based on a server not selecting source port numbers with a cryptographic random number generator. I agree with the page's assertion that using a random port for each message increases security, but it is harder to implement, slower at runtime (perhaps not significantly so), and likely not practically more secure.


comment:2 Changed 9 years ago by jknight

Cc: jknight added

That's an incorrect reading of the vulnerability report. That first section talks about previous vulnerabilities that had already been fixed a year ago.

t.names already (I think...) uses a secure random source for its message ids. Hm, actually, it looks like it's being really stupid and cutting down the effective number of bits to 10 instead of 16. That seems really bad....

But anyways, the new issue is about randomizing the port used. It has not yet been disclosed why this is so important, only that it is a relatively effective mitigation technique for the so far undisclosed vulnerability.

And t.names does not randomize the port used for each query. It should.

comment:3 Changed 9 years ago by jknight

The first comment here: has speculation as to the reason for the newly discovered incredible level of brokenness in not randomizing the port.

comment:4 Changed 8 years ago by Jean-Paul Calderone

author: exarkun
Branch: branches/dns-random-source-port-3342

(In [24306]) Branching to 'dns-random-source-port-3342'

comment:5 Changed 8 years ago by jknight

Probably #3217 will get fixed as a side-effect of fixing this.

comment:6 Changed 8 years ago by Jean-Paul Calderone

Keywords: review added

Ready for review. I think #3217 is indeed fixed here. I didn't change any APIs or add any tests for this in particular, but I did delete the cleanup code in the tests which reached into the resolver and disconnected its port - that happens by itself now, unless the deprecated API is used.

For the rest, it's pretty straightforward - client.Resolver tries not to use its protocol attribute anymore, instead of it binds a UDP port for each query it needs to do and unbinds it afterwards.

Build results at

comment:7 Changed 8 years ago by Jean-Paul Calderone

Owner: Jean-Paul Calderone deleted

comment:8 Changed 8 years ago by therve

Keywords: review removed
Owner: set to Jean-Paul Calderone

Please update copyright of test_names, and merge.

comment:9 Changed 8 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [24401]) Merge dns-random-source-port-3342

Author: exarkun Reviewer: therve Fixes: #3342

Change twisted.names.client.Resolver to use a new random UDP source port for each DNS request it makes. Also the function responsible for generating DNS message identifier to use the full 16 bit range instead of only using 10 bits of it.


comment:10 Changed 6 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.