Fix DNS forward/cache path for relay-driven poisoning

lib/rex/proto/dns/cache.rb

@@ -57,7 +57,7 @@ def cache_record(record, expire: true)
       end
 
       unless record.name.to_s.match(MATCH_HOSTNAME)
-        raise "Invalid record for cache entry (invalid hostname) - #{record.inspect}"
+        return # skip non-cacheable record: " - #{record.inspect}"
       end
 
       add(record, expire ? (::Time.now.to_i + record.ttl) : 0)

lib/rex/proto/dns/server.rb

@@ -163,32 +163,35 @@ def default_dispatch_request(cli,data)
     return if data.strip.empty?
     req = Packet.encode_drb(data)
     forward = req.dup
-    # Find cached items, remove request from forwarded packet
+    # Dnsruby#dup is shallow - req and forward share the same @question Array.
+    # Give forward its own copy so deleting forwarded questions doesn't also
+    # empty req.question, which we need intact when echoing it in the response.
+    forward.instance_variable_set(:@question, req.question.dup)
+    answers = []
+    # Find cached items, remove question from forwarded packet
     req.question.each do |ques|
       cached = self.cache.find(ques.qname, ques.qtype)
-      if cached.empty?
-        next
-      else
-        req.instance_variable_set(:@answer, (req.answer + cached).uniq)
+      unless cached.empty?
+        answers.concat(cached)
         forward.question.delete(ques)
       end
     end
     # Forward remaining requests, cache responses
-    if forward.question.count > 0 and @fwd_res
+    if forward.question.count > 0 && @fwd_res
       forwarded = self.fwd_res.send(forward)
-      req.instance_variable_set(:@answer, (req.answer + forwarded.answer).uniq)
-      forwarded.answer.each do |ans|
-        self.cache.cache_record(ans)
-      end
-      req.header.ra = true # Set recursion bit
-    end
-    # Finalize answers in response
-    # Check for empty response prior to sending
-    if req.answer.size < 1
-      req.header.rcode = Dnsruby::RCode::NOERROR
+      answers.concat(forwarded.answer)
+      forwarded.answer.each { |ans| self.cache.cache_record(ans) }
     end
-    req.header.qr = true # Set response bit
-    send_response(cli, req.encode)
+    # Build a fresh response message to avoid a Dnsruby stale-state encoding bug
+    # where instance_variable_set(:@answer) on a decoded request appends answer
+    # bytes after the packet end rather than inside the answer section.
+    resp = Dnsruby::Message.new
+    resp.header.id = req.header.id
+    resp.header.qr = true
+    resp.header.ra = req.header.rd
+    req.question.each { |q| resp.add_question(q.qname, q.qtype, q.qclass) }
+    answers.uniq.each { |a| resp.add_answer(a) }
+    send_response(cli, resp.encode)
   end
 
   #
@@ -220,8 +223,12 @@ def monitor_listener
       r,_,_ = ::IO.select(rds,wds,eds,1)
 
       if (r != nil and r[0] == self.udp_sock)
-        buf, addr = self.udp_sock.recvfrom(65535)
-        host, port = addr[3], addr[1]
+        buf, addr, source_port = self.udp_sock.recvfrom(65535)
+        if source_port
+          host, port = addr, source_port
+        else
+          host, port = addr[3], addr[1]
+        end
         # Mock up a client object for sending back data
         cli = MockDnsClient.new(host, port, r[0])
         dispatch_request(cli, buf)

spec/lib/rex/proto/dns/server_spec.rb

@@ -36,5 +36,65 @@ def query_for(name)
         server.default_dispatch_request(cli, query_for('empty.example.com'))
       end
     end
+
+    context 'when the forwarded response carries an answer' do
+      let(:answer) do
+        Dnsruby::RR.create(name: 'poisoned.example.com.', type: 'A', address: '192.0.2.10')
+      end
+
+      before do
+        # the resolver forwards the query and returns one answer, which is the
+        # path that has to survive re-encoding intact
+        forwarded = Dnsruby::Message.new
+        forwarded.add_answer(answer)
+        server.fwd_res = double('resolver', send: forwarded)
+      end
+
+      it 'encodes the answer inside the answer section rather than past the packet end' do
+        # Regression: mutating a decoded request's @answer and re-encoding it
+        # appended the answer bytes after the packet, so the reply decoded with
+        # zero answers. Building a fresh response keeps the answer readable.
+        expect(cli).to receive(:write) do |data|
+          reply = Dnsruby::Message.decode(data)
+          expect(reply.answer.count).to eq(1)
+          expect(reply.answer.first.address.to_s).to eq('192.0.2.10')
+        end
+        server.default_dispatch_request(cli, query_for('poisoned.example.com'))
+      end
+    end
+
+    context 'when one of several questions is served from cache' do
+      it 'forwards only the uncached question and keeps the request questions intact' do
+        # Regression: Dnsruby#dup is shallow, so deleting a served question from
+        # the forwarded packet also emptied the original request's question list.
+        # Only the uncached question should be forwarded, and both questions must
+        # still be echoed back in the response.
+        cached_answer = Dnsruby::RR.create(name: 'a.example.com.', type: 'A', address: '192.0.2.30')
+        allow(server.cache).to receive(:find) do |qname, _qtype|
+          qname.to_s == 'a.example.com' ? [cached_answer] : []
+        end
+
+        forwarded = Dnsruby::Message.new
+        forwarded.add_answer(Dnsruby::RR.create(name: 'b.example.com.', type: 'A', address: '192.0.2.20'))
+        resolver = double('resolver')
+        server.fwd_res = resolver
+
+        query = Dnsruby::Message.new
+        query.add_question(Dnsruby::Name.create('a.example.com.'), Dnsruby::Types::A)
+        query.add_question(Dnsruby::Name.create('b.example.com.'), Dnsruby::Types::A)
+
+        expect(resolver).to receive(:send) do |fwd|
+          expect(fwd.question.map { |q| q.qname.to_s }).to eq(['b.example.com'])
+          forwarded
+        end
+
+        expect(cli).to receive(:write) do |data|
+          reply = Dnsruby::Message.decode(data)
+          expect(reply.question.map { |q| q.qname.to_s }).to contain_exactly('a.example.com', 'b.example.com')
+          expect(reply.answer.map { |a| a.address.to_s }).to contain_exactly('192.0.2.30', '192.0.2.20')
+        end
+        server.default_dispatch_request(cli, query.encode)
+      end
+    end
   end
 end