Update the package lists before installing Valgrind

.github/workflows/valgrind.yaml

@@ -36,7 +36,9 @@ jobs:
         ruby-version: ${{matrix.ruby}}
         bundler-cache: true
     - name: Install Valgrind
-      run: sudo apt-get install -y valgrind
+      run: |
+        sudo apt-get update
+        sudo apt-get install -y valgrind
     - name: Run tests
       timeout-minutes: 10
       run: bundle exec rake spec:valgrind

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