diff --git a/.github/workflows/valgrind.yaml b/.github/workflows/valgrind.yaml index 249a3c8..065569c 100644 --- a/.github/workflows/valgrind.yaml +++ b/.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 diff --git a/lib/cool.io/dns_resolver.rb b/lib/cool.io/dns_resolver.rb index af7f299..4dbe959 100644 --- a/lib/cool.io/dns_resolver.rb +++ b/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 diff --git a/spec/dns_spec.rb b/spec/dns_spec.rb index 50d3e31..5118808 100644 --- a/spec/dns_spec.rb +++ b/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