Ticket #3342 defect closed fixed

Opened 6 years ago

Last modified 6 years ago

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

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?

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.

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.

4

Changed 6 years ago by exarkun

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

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

5

Changed 6 years ago by jknight

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

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

7

Changed 6 years ago by exarkun

  • owner exarkun deleted

8

Changed 6 years ago by therve

  • owner set to exarkun
  • keywords review removed

Please update copyright of test_names, and merge.

9

Changed 6 years ago by exarkun

  • status changed from new to closed
  • resolution set to fixed

(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

10

Changed 3 years ago by <automation>

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