fix: don't pass EDNS0 DNS Cookies through (#2252)

resolver/ecs_resolver.go

@@ -116,7 +116,7 @@ func (r *ECSResolver) Resolve(ctx context.Context, request *model.Request) (*mod
 		// and the forwardEcs option is not enabled
 		if r.cfg.IPv4Mask == 0 && r.cfg.IPv6Mask == 0 && so != nil && !r.cfg.Forward {
 			logger.Debug("remove edns0 subnet option")
-			util.RemoveEdns0Option[*dns.EDNS0_SUBNET](request.Req)
+			util.RemoveEdns0OptionKeepRecord[*dns.EDNS0_SUBNET](request.Req)
 		}
 	}
 

resolver/ecs_resolver_test.go

@@ -209,6 +209,43 @@ var _ = Describe("EcsResolver", func() {
 							HaveReason("Test")))
 			})
 		})
+
+		When("remove ECS information", func() {
+			// no mask configured and forwarding disabled: the option the client sent is dropped
+			BeforeEach(func() {
+				sutConfig.IPv4Mask = 0
+				sutConfig.IPv6Mask = 0
+				sutConfig.Forward = false
+			})
+
+			It("should keep the OPT record when the subnet was the only option", func(ctx context.Context) {
+				request := newRequest("example.com.", A)
+				request.ClientIP = origIP
+
+				request.Req.SetEdns0(1232, true)
+				addEcsOption(request.Req, ecsIP, 32)
+
+				m.ResolveFn = func(ctx context.Context, req *Request) (*Response, error) {
+					Expect(req.Req).ShouldNot(HaveEdnsOption(dns.EDNS0SUBNET))
+
+					// the OPT record still carries the DO bit and the buffer size the client advertised
+					opt := req.Req.IsEdns0()
+					Expect(opt).ShouldNot(BeNil())
+					Expect(opt.Do()).Should(BeTrue())
+					Expect(opt.UDPSize()).Should(BeNumerically("==", 1232))
+
+					return respondWith(mockAnswer), nil
+				}
+
+				Expect(sut.Resolve(ctx, request)).
+					Should(
+						SatisfyAll(
+							HaveNoAnswer(),
+							HaveResponseType(ResponseTypeRESOLVED),
+							HaveReturnCode(dns.RcodeSuccess),
+							HaveReason("Test")))
+			})
+		})
 	})
 
 	Context("maskIP", func() {

server/client_query.go

@@ -80,6 +80,14 @@ func (q clientQuery) normalizeResponse(res *dns.Msg) {
 		// don't return an OPT record to a client that didn't use EDNS0 (RFC 6891 section 7)
 		util.RemoveEdns0Record(res)
 	} else if opt := res.IsEdns0(); opt != nil {
+		// Blocky doesn't implement DNS Cookies (RFC 7873), so a Server Cookie in the response is
+		// one an upstream issued for blocky itself, and blocky can't validate it when the client
+		// returns it. Passing it on also makes the presence of a cookie depend on which upstream
+		// answered and on whether the answer came from the cache (stored without an OPT record),
+		// and a client that tracks cookie support per server address — c-ares does — discards the
+		// cookieless answers of such a flip-flopping server as spoofed.
+		util.RemoveEdns0OptionKeepRecord[*dns.EDNS0_COOKIE](res)
+
 		// RFC 3225 §3: the DO bit of the query is copied into the response
 		opt.SetDo(q.wantsDNSSEC)
 

server/server.go

@@ -829,6 +829,14 @@ func (s *Server) resolve(ctx context.Context, request *model.Request) (response
 	// up front: the response is normalized against that, not the mutated request.
 	query := newClientQuery(request)
 
+	// Blocky doesn't implement DNS Cookies (RFC 7873), so a Server Cookie a client returns is one
+	// an upstream issued and blocky can neither validate nor reissue it. Forwarding it is worse
+	// than dropping it: it is likely to reach a different upstream than the one that issued it
+	// (`parallel_best` picks at random), which can't validate it either and may answer BADCOOKIE.
+	// The OPT record itself is kept, since it still carries the DO bit and the buffer size the
+	// client advertised.
+	util.RemoveEdns0OptionKeepRecord[*dns.EDNS0_COOKIE](request.Req)
+
 	switch {
 	case len(request.Req.Question) == 0:
 		m := new(dns.Msg)

server/server_test.go

@@ -1329,6 +1329,74 @@ var _ = Describe("Running DNS server", func() {
 			})
 		})
 
+		When("the client sent a DNS Cookie", func() {
+			// Blocky doesn't implement DNS Cookies (RFC 7873): it can neither produce a Server
+			// Cookie of its own nor validate one a client returns. An upstream's cookie must
+			// therefore not reach the client, and the client's cookie must not reach an upstream.
+			const (
+				clientCookie = "0102030405060708"
+				serverCookie = "1112131415161718"
+			)
+
+			var upstreamRequest *dns.Msg
+
+			// chainWithUpstreamCookie records the request as the chain saw it and answers like a
+			// cookie-supporting upstream.
+			chainWithUpstreamCookie := func(req *model.Request) *dns.Msg {
+				upstreamRequest = req.Req.Copy()
+
+				res := chainResponse.SetReply(req.Req)
+				res.SetEdns0(4096, false)
+				util.SetEdns0Option(res, &dns.EDNS0_COOKIE{
+					Code: dns.EDNS0COOKIE, Cookie: clientCookie + serverCookie,
+				})
+
+				return res
+			}
+
+			resolveWithCookie := func(udpSize uint16, do bool) *model.Response {
+				s := newServerWithChain(chainWithUpstreamCookie)
+
+				clientMsg := util.NewMsgWithQuestion("example.com.", A)
+				clientMsg.SetEdns0(udpSize, do)
+				util.SetEdns0Option(clientMsg, &dns.EDNS0_COOKIE{Code: dns.EDNS0COOKIE, Cookie: clientCookie})
+
+				_, req := newRequest(ctx, net.ParseIP("1.2.3.4"), "", model.RequestProtocolUDP, clientMsg)
+
+				resp, err := s.resolve(ctx, req)
+				Expect(err).Should(Succeed())
+
+				return resp
+			}
+
+			BeforeEach(func() {
+				upstreamRequest = nil
+			})
+
+			It("removes the upstream's COOKIE option from the response", func() {
+				resp := resolveWithCookie(1232, false)
+
+				Expect(resp.Res).ShouldNot(HaveEdnsOption(dns.EDNS0COOKIE))
+				Expect(resp.Res.IsEdns0()).ShouldNot(BeNil())
+			})
+
+			It("does not forward the client's COOKIE option upstream", func() {
+				resolveWithCookie(1232, false)
+
+				Expect(upstreamRequest).ShouldNot(BeNil())
+				Expect(upstreamRequest).ShouldNot(HaveEdnsOption(dns.EDNS0COOKIE))
+			})
+
+			It("keeps the OPT record sent upstream when the cookie was the only option", func() {
+				resolveWithCookie(1232, true)
+
+				Expect(upstreamRequest).ShouldNot(BeNil())
+				Expect(upstreamRequest.IsEdns0()).ShouldNot(BeNil())
+				Expect(upstreamRequest.IsEdns0().Do()).Should(BeTrue())
+				Expect(upstreamRequest.IsEdns0().UDPSize()).Should(BeNumerically("==", 1232))
+			})
+		})
+
 		When("the client left the DO bit clear", func() {
 			// RFC 4035 section 3.2.1: the DNSSEC records the chain requested upstream on the
 			// client's behalf must not be added to the response of a client that didn't ask.

util/edns0.go

@@ -65,6 +65,19 @@ func GetEdns0Option[T EDNS0Option](msg *dns.Msg) T {
 // If there are no more options in the OPT record, the OPT record will be removed.
 // If the option is successfully removed, true will be returned.
 func RemoveEdns0Option[T EDNS0Option](msg *dns.Msg) bool {
+	return removeEdns0Option[T](msg, true)
+}
+
+// RemoveEdns0OptionKeepRecord removes the option according to the given type from the OPT record
+// in the Extra section of the given message, keeping the OPT record itself even when it becomes
+// empty: on a request its header still carries the DO bit and the UDP buffer size the client
+// advertised, and a response to an EDNS0 query must have one (RFC 6891 section 6.1.1).
+// If the option is successfully removed, true will be returned.
+func RemoveEdns0OptionKeepRecord[T EDNS0Option](msg *dns.Msg) bool {
+	return removeEdns0Option[T](msg, false)
+}
+
+func removeEdns0Option[T EDNS0Option](msg *dns.Msg, dropEmptyRecord bool) bool {
 	if msg == nil {
 		return false
 	}
@@ -88,7 +101,7 @@ func RemoveEdns0Option[T EDNS0Option](msg *dns.Msg) bool {
 		}
 	}
 
-	if len(opt.Option) == 0 {
+	if dropEmptyRecord && len(opt.Option) == 0 {
 		RemoveEdns0Record(msg)
 	}
 

util/edns0_test.go

@@ -167,6 +167,80 @@ var _ = Describe("EDNS0 utils", func() {
 		})
 	})
 
+	Describe("RemoveEdns0OptionKeepRecord", func() {
+		When("the removed option is the only one in the OPT record", func() {
+			BeforeEach(func() {
+				opt := new(dns.OPT)
+				opt.Hdr.Name = "."
+				opt.Hdr.Rrtype = dns.TypeOPT
+				opt.SetUDPSize(1232)
+				opt.SetDo(true)
+				opt.Option = append(opt.Option, new(dns.EDNS0_COOKIE))
+				baseMsg.Extra = append(baseMsg.Extra, opt)
+			})
+
+			It("should keep the OPT record with its UDP size and DO bit", func() {
+				Expect(RemoveEdns0OptionKeepRecord[*dns.EDNS0_COOKIE](baseMsg)).Should(BeTrue())
+
+				Expect(baseMsg).ShouldNot(HaveEdnsOption(dns.EDNS0COOKIE))
+
+				opt := baseMsg.IsEdns0()
+				Expect(opt).ShouldNot(BeNil())
+				Expect(opt.UDPSize()).Should(BeNumerically("==", 1232))
+				Expect(opt.Do()).Should(BeTrue())
+			})
+		})
+
+		When("other options are present", func() {
+			BeforeEach(func() {
+				opt := new(dns.OPT)
+				opt.Hdr.Name = "."
+				opt.Hdr.Rrtype = dns.TypeOPT
+				opt.Option = append(opt.Option, new(dns.EDNS0_COOKIE), new(dns.EDNS0_SUBNET))
+				baseMsg.Extra = append(baseMsg.Extra, opt)
+			})
+
+			It("should remove only the given option", func() {
+				Expect(RemoveEdns0OptionKeepRecord[*dns.EDNS0_COOKIE](baseMsg)).Should(BeTrue())
+
+				Expect(baseMsg).ShouldNot(HaveEdnsOption(dns.EDNS0COOKIE))
+				Expect(baseMsg).Should(HaveEdnsOption(dns.EDNS0SUBNET))
+			})
+		})
+
+		When("Option is not present", func() {
+			BeforeEach(func() {
+				opt := new(dns.OPT)
+				opt.Hdr.Name = "."
+				opt.Hdr.Rrtype = dns.TypeOPT
+				opt.Option = append(opt.Option, new(dns.EDNS0_EDE))
+				baseMsg.Extra = append(baseMsg.Extra, opt)
+			})
+
+			It("should return false and keep the OPT record", func() {
+				Expect(RemoveEdns0OptionKeepRecord[*dns.EDNS0_COOKIE](baseMsg)).Should(BeFalse())
+
+				Expect(baseMsg.IsEdns0()).ShouldNot(BeNil())
+			})
+		})
+
+		When("Extra is nil", func() {
+			BeforeEach(func() {
+				baseMsg.Extra = nil
+			})
+
+			It("should return false", func() {
+				Expect(RemoveEdns0OptionKeepRecord[*dns.EDNS0_COOKIE](baseMsg)).Should(BeFalse())
+			})
+		})
+
+		When("message is nil", func() {
+			It("should return false", func() {
+				Expect(RemoveEdns0OptionKeepRecord[*dns.EDNS0_COOKIE](nil)).Should(BeFalse())
+			})
+		})
+	})
+
 	Describe("SetEdns0Option", func() {
 		When("Option is not present", func() {
 			var eso *dns.EDNS0_SUBNET