Validate DNS responses before accepting a resolved address

DNSResolver built every query with the constant transaction ID 2 and
authenticated the reply with nothing but `id == 2`, while on_readable read
the datagram off a never-connected UDPSocket with
`recvfrom_nonblock(...).first`, discarding the sender. Any host that could
land a UDP packet on the resolver's ephemeral port could therefore choose
the address a hostname resolved to, and Coolio::TCPSocket.connect would
open the application's session to it.

Give each query a random 16-bit transaction ID from SecureRandom, and
accept a datagram only when it carries that ID and arrives from port 53 of
an address this query was actually sent to. The check runs before the
response is parsed. A datagram that fails it is ignored rather than turned
into a failure, so a single spoofed packet can no longer deny the
resolution either; the existing TIMEOUT/RETRIES budget still bounds the
wait.

Nameservers may be given as hostnames, so send_request resolves the
nameserver to a numeric address and remembers it, caching only successful
lookups. The constructor keeps accepting whatever it accepted before, an
unresolvable nameserver still surfaces as SocketError from the same place,
and one that is only transiently unresolvable recovers on the next retry.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: Watson1978

lib/cool.io/dns_resolver.rb

@@ -18,6 +18,8 @@
 #++
 
 require 'resolv'
+require 'securerandom'
+require 'socket'
 
 module Coolio
   # A non-blocking DNS resolver.  It provides interfaces for querying both
@@ -76,6 +78,13 @@ def initialize(hostname, *nameservers)
       @nameservers = nameservers.dup
       @question = request_question hostname
 
+      # A guessable ID would let an off-path attacker forge a response
+      @request_id = SecureRandom.random_number(1 << 16)
+
+      # Numeric addresses this query was sent to, and the lookups behind them
+      @queried_addresses = []
+      @numeric_addresses = {}
+
       @socket = UDPSocket.new
       @timer = Timeout.new(self)
 
@@ -115,25 +124,43 @@ def on_timeout
     # Send a request to the DNS server
     def send_request
       @nameservers.rotate!
+
+      # Send to the numeric address, so we know where a response must come from
+      address = numeric_address(@nameservers.first)
+      @queried_addresses << address unless @queried_addresses.include?(address)
+
       begin
-        @socket.send request_message, 0, @nameservers.first, DNS_PORT
+        @socket.send request_message, 0, address, DNS_PORT
       rescue Errno::EHOSTUNREACH # TODO figure out why it has to be wrapper here, when the other wrapper should be wrapping this one!
       end
     end
 
     # Called by the subclass when the DNS response is available
     def on_readable
-      datagram = nil
+      datagram = sender = nil
       begin
-        datagram = @socket.recvfrom_nonblock(DATAGRAM_SIZE).first
+        datagram, sender = @socket.recvfrom_nonblock(DATAGRAM_SIZE)
       rescue Errno::ECONNREFUSED
       end
 
+      # Ignore anything we didn't ask for, rather than resolving or failing on it.
+      # The query stays outstanding, so the retry timer still bounds us.
+      return if datagram and not solicited_response?(datagram, sender)
+
       address = response_address datagram rescue nil
       address ? on_success(address) : on_failure
       detach
     end
 
+    # Is this a reply to our query, from an address we sent it to?
+    # Retries rotate through @nameservers, so any address already queried counts.
+    def solicited_response?(datagram, sender)
+      return false unless datagram.size >= 12
+      return false unless sender and sender[1] == DNS_PORT and @queried_addresses.include?(sender[3])
+
+      datagram[0..1].unpack('n').first.to_i == @request_id
+    end
+
     def request_question(hostname)
       raise ArgumentError, "hostname cannot be nil" if hostname.nil?
 
@@ -151,7 +178,7 @@ def request_question(hostname)
 
     def request_message
       # Standard query header
-      message = [2, 1, 0].pack('nCC')
+      message = [@request_id, 1, 0].pack('nCC')
 
       # One entry
       qdcount = 1
@@ -166,7 +193,7 @@ def request_message
     def response_address(message)
       # Confirm the ID field
       id = message[0..1].unpack('n').first.to_i
-      return unless id == 2
+      return unless id == @request_id
 
       # Check the QR value and confirm this message is a response
       qr = message[2..2].unpack('B1').first.to_i
@@ -210,6 +237,17 @@ def reject_ipv6_nameservers(nameservers)
       nameservers.reject { |ns| ns.include?(':') }
     end
 
+    # The address of a nameserver, which may be given as a hostname.
+    # Only successful lookups are cached, so a transient failure is looked up again.
+    def numeric_address(nameserver)
+      @numeric_addresses[nameserver] ||= begin
+        addrinfo = Addrinfo.getaddrinfo(nameserver, nil, ::Socket::AF_INET, ::Socket::SOCK_DGRAM).first
+        raise SocketError, "getaddrinfo: no IPv4 address for #{nameserver}" if addrinfo.nil?
+
+        addrinfo.ip_address
+      end
+    end
+
     class Timeout < TimerWatcher
       def initialize(resolver)
         @resolver = resolver

spec/dns_spec.rb

@@ -76,4 +76,169 @@ def on_resolve_failed
       expect(nameservers).to eq(["8.8.4.4"])
     end
   end
+
+  describe "nameserver normalization" do
+    let(:localhost_address) do
+      Addrinfo.getaddrinfo("localhost", nil, ::Socket::AF_INET, ::Socket::SOCK_DGRAM).first.ip_address
+    end
+
+    it "keeps the nameserver list as given" do
+      resolver = Coolio::DNSResolver.new("example.com", "localhost")
+
+      expect(resolver.instance_variable_get(:@nameservers)).to eq(["localhost"])
+    end
+
+    it "queries the numeric address of a nameserver given as a hostname" do
+      resolver = Coolio::DNSResolver.new("example.com", "localhost")
+      resolver.__send__(:send_request)
+
+      expect(resolver.instance_variable_get(:@queried_addresses)).to eq([localhost_address])
+    end
+
+    it "accepts responses from a nameserver which was given as a hostname" do
+      resolver = Coolio::DNSResolver.new("example.com", "localhost")
+      resolver.__send__(:send_request)
+      response = dns_response_for(resolver)
+
+      expect(
+        resolver.__send__(:solicited_response?, response, ["AF_INET", 53, localhost_address, localhost_address])
+      ).to be true
+    end
+
+    it "looks a nameserver up once and reuses the result on retries" do
+      resolved = Addrinfo.getaddrinfo("127.0.0.1", nil, ::Socket::AF_INET, ::Socket::SOCK_DGRAM)
+      resolver = Coolio::DNSResolver.new("example.com", "127.0.0.1")
+
+      expect(Addrinfo).to receive(:getaddrinfo).once.and_return(resolved)
+
+      3.times { resolver.__send__(:send_request) }
+    end
+
+    it "does not reject an unresolvable nameserver at construction" do
+      allow(Addrinfo).to receive(:getaddrinfo).and_raise(SocketError, "getaddrinfo: Name or service not known")
+
+      expect do
+        Coolio::DNSResolver.new("example.com", "no-such-nameserver.invalid")
+      end.to_not raise_error
+    end
+
+    it "surfaces an unresolvable nameserver as a SocketError from the request" do
+      allow(Addrinfo).to receive(:getaddrinfo).and_raise(SocketError, "getaddrinfo: Name or service not known")
+      resolver = Coolio::DNSResolver.new("example.com", "no-such-nameserver.invalid")
+
+      expect { resolver.attach(@loop) }.to raise_error(SocketError)
+      expect(@loop.watchers).to be_empty
+    end
+
+    it "recovers when a nameserver is only transiently unresolvable" do
+      resolved = Addrinfo.getaddrinfo("127.0.0.1", nil, ::Socket::AF_INET, ::Socket::SOCK_DGRAM)
+      resolver = Coolio::DNSResolver.new("example.com", "ns.example.test")
+
+      attempts = 0
+      allow(Addrinfo).to receive(:getaddrinfo) do
+        attempts += 1
+        raise SocketError, "getaddrinfo: Name or service not known" if attempts == 1
+
+        resolved
+      end
+
+      expect { resolver.__send__(:send_request) }.to raise_error(SocketError)
+      expect { resolver.__send__(:send_request) }.to_not raise_error
+      expect(resolver.instance_variable_get(:@queried_addresses)).to eq(["127.0.0.1"])
+    end
+  end
+
+  describe "response validation" do
+    let(:nameserver) { "127.0.0.1" }
+    let(:sender) { ["AF_INET", 53, nameserver, nameserver] }
+    let(:resolver) do
+      Coolio::DNSResolver.new("example.com", nameserver).tap { |r| r.__send__(:send_request) }
+    end
+
+    it "uses an unpredictable transaction ID for each query" do
+      ids = 10.times.map do
+        request_id_of(Coolio::DNSResolver.new("example.com", nameserver))
+      end
+
+      expect(ids.uniq.size).to be > 1
+    end
+
+    it "accepts a response carrying our transaction ID from the queried nameserver" do
+      expect(
+        resolver.__send__(:solicited_response?, dns_response_for(resolver), sender)
+      ).to be true
+    end
+
+    it "rejects a response carrying a different transaction ID" do
+      forged = dns_response_for(resolver, id: (request_id_of(resolver) + 1) % 65536)
+
+      expect(resolver.__send__(:solicited_response?, forged, sender)).to be false
+    end
+
+    it "rejects a response from a source address we did not query" do
+      response = dns_response_for(resolver)
+
+      expect(
+        resolver.__send__(:solicited_response?, response, ["AF_INET", 53, "10.11.12.13", "10.11.12.13"])
+      ).to be false
+    end
+
+    it "rejects a response arriving before the request was sent" do
+      unsent = Coolio::DNSResolver.new("example.com", nameserver)
+
+      expect(unsent.__send__(:solicited_response?, dns_response_for(unsent), sender)).to be false
+    end
+
+    it "rejects a response from a source port other than the DNS port" do
+      response = dns_response_for(resolver)
+
+      expect(
+        resolver.__send__(:solicited_response?, response, ["AF_INET", 4444, nameserver, nameserver])
+      ).to be false
+    end
+
+    it "rejects a truncated datagram" do
+      expect(resolver.__send__(:solicited_response?, "\0\0", sender)).to be false
+    end
+
+    it "resolves from a response sent by the queried nameserver" do
+      response = dns_response_for(resolver, address: "1.2.3.4")
+      allow(resolver.instance_variable_get(:@socket)).to receive(:recvfrom_nonblock).and_return([response, sender])
+
+      expect(resolver).to receive(:on_success).with("1.2.3.4")
+      expect(resolver).to receive(:detach)
+
+      resolver.__send__(:on_readable)
+    end
+
+    it "ignores a spoofed response instead of resolving or failing it" do
+      forged = dns_response_for(resolver, address: "6.6.6.6")
+      allow(resolver.instance_variable_get(:@socket)).to receive(:recvfrom_nonblock)
+        .and_return([forged, ["AF_INET", 53, "10.11.12.13", "10.11.12.13"]])
+
+      expect(resolver).to_not receive(:on_success)
+      expect(resolver).to_not receive(:on_failure)
+      expect(resolver).to_not receive(:detach)
+
+      resolver.__send__(:on_readable)
+    end
+  end
+
+  def request_id_of(resolver)
+    resolver.__send__(:request_message)[0..1].unpack('n').first
+  end
+
+  # A response to the resolver's own query: header plus the echoed question,
+  # and an A record when an address is given.
+  def dns_response_for(resolver, id: request_id_of(resolver), address: nil)
+    question = resolver.instance_variable_get(:@question)
+    answer = if address
+      # Compressed name pointer, type A, class IN, TTL, RDLENGTH, RDATA
+      [0xc00c, 1, 1, 60, 4].pack('nnnNn') + address.split('.').map(&:to_i).pack('CCCC')
+    else
+      ""
+    end
+
+    [id, 0x81, 0x80, 1, answer.empty? ? 0 : 1, 0, 0].pack('nCCnnnn') + question + answer
+  end
 end