Opened 6 years ago

Closed 6 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
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

Description

See:
http://www.kb.cert.org/vuls/id/800113

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

Change History (10)

comment:1 Changed 6 years ago by exarkun

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.

Thoughts?

comment:2 Changed 6 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 6 years ago by jknight

The first comment here: http://blog.netherlabs.nl/articles/2008/07/09/some-thoughts-on-the-recent-dns-vulnerability has speculation as to the reason for the newly discovered incredible level of brokenness in not randomizing the port.

comment:4 Changed 6 years ago by exarkun

  • author set to exarkun
  • Branch set to branches/dns-random-source-port-3342

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

comment:5 Changed 6 years ago by jknight

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

comment:6 Changed 6 years ago by exarkun

  • 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 http://buildbot.twistedmatrix.com/boxes-supported?branch=/branches/dns-random-source-port-3342

comment:7 Changed 6 years ago by exarkun

  • Owner exarkun deleted

comment:8 Changed 6 years ago by therve

  • Keywords review removed
  • Owner set to exarkun

Please update copyright of test_names, and merge.

comment:9 Changed 6 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

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

See http://www.kb.cert.org/vuls/id/800113

comment:10 Changed 3 years ago by <automation>

  • Owner exarkun deleted
Note: See TracTickets for help on using tickets.