plugin/rewrite: prevent illegal names (#1972)

Log and returns an error when the name rewrite creates a name that is
illegal. Add test in name_test.go to see if an error is returned.

Possible followup could be the only check this if a name-rewrite is
done.

Fixes: #1638

Signed-off-by: Miek Gieben <miek@miek.nl>

plugin/rewrite/README.md

@@ -21,7 +21,8 @@ rewrite [continue|stop] FIELD FROM TO
    * `type` - the type field of the request will be rewritten. FROM/TO must be a DNS record type (`A`, `MX`, etc);
 e.g., to rewrite ANY queries to HINFO, use `rewrite type ANY HINFO`.
    * `class` - the class of the message will be rewritten. FROM/TO must be a DNS class type (`IN`, `CH`, or `HS`) e.g., to rewrite CH queries to IN use `rewrite class CH IN`.
-   * `name` - the query name in the _request_ is rewritten; by default this is a full match of the name, e.g., `rewrite name miek.nl example.org`. Other match types are supported, see the **Name Field Rewrites** section below.
+   * `name` - the query name in the _request_ is rewritten; by default this is a full match of the
+     name, e.g., `rewrite name example.net example.org`. Other match types are supported, see the **Name Field Rewrites** section below.
    * `answer name` - the query name in the _response_ is rewritten.  This option has special restrictions and requirements, in particular it must always combined with a `name` rewrite.  See below in the **Response Rewrites** section.
    *  `edns0` - an EDNS0 option can be appended to the request as described below in the **EDNS0 Options** section.
 
@@ -38,7 +39,8 @@ for not specifying this rule processing mode is `stop`
 
 The `rewrite` plugin offers the ability to match on the name in the question section of
 a DNS request. The match could be exact, substring, or based on a prefix, suffix, or regular
-expression.
+expression. If the newly used name is not a legal domain name the plugin returns an error to the
+client.
 
 The syntax for the name re-writing is as follows:
 

plugin/rewrite/name.go

@@ -9,6 +9,8 @@ import (
 
 	"github.com/coredns/coredns/plugin"
 	"github.com/coredns/coredns/request"
+
+	"github.com/miekg/dns"
 )
 
 type nameRule struct {
@@ -194,3 +196,16 @@ func (rule *substringNameRule) GetResponseRule() ResponseRule { return ResponseR
 
 // GetResponseRule return a rule to rewrite the response with.
 func (rule *regexNameRule) GetResponseRule() ResponseRule { return rule.ResponseRule }
+
+// validName returns true if s is valid domain name and shortern than 256 characters.
+func validName(s string) bool {
+	_, ok := dns.IsDomainName(s)
+	if !ok {
+		return false
+	}
+	if len(dns.Name(s).String()) > 255 {
+		return false
+	}
+
+	return true
+}

plugin/rewrite/name_test.go

@@ -0,0 +1,33 @@
+package rewrite
+
+import (
+	"context"
+	"strings"
+	"testing"
+
+	"github.com/coredns/coredns/plugin"
+	"github.com/coredns/coredns/plugin/pkg/dnstest"
+	"github.com/coredns/coredns/plugin/test"
+
+	"github.com/miekg/dns"
+)
+
+func TestRewriteIllegalName(t *testing.T) {
+	r, _ := newNameRule("stop", "example.org.", "example..org.")
+
+	rw := Rewrite{
+		Next:     plugin.HandlerFunc(msgPrinter),
+		Rules:    []Rule{r},
+		noRevert: true,
+	}
+
+	ctx := context.TODO()
+	m := new(dns.Msg)
+	m.SetQuestion("example.org.", dns.TypeA)
+
+	rec := dnstest.NewRecorder(&test.ResponseWriter{})
+	_, err := rw.ServeDNS(ctx, rec, m)
+	if !strings.Contains(err.Error(), "invalid name") {
+		t.Errorf("Expected invalid name, got %s", err.Error())
+	}
+}

plugin/rewrite/reverter.go

@@ -17,7 +17,7 @@ type ResponseRule struct {
 
 // ResponseReverter reverses the operations done on the question section of a packet.
 // This is need because the client will otherwise disregards the response, i.e.
-// dig will complain with ';; Question section mismatch: got miek.nl/HINFO/IN'
+// dig will complain with ';; Question section mismatch: got example.org/HINFO/IN'
 type ResponseReverter struct {
 	dns.ResponseWriter
 	originalQuestion dns.Question
@@ -64,10 +64,3 @@ func (r *ResponseReverter) Write(buf []byte) (int, error) {
 	n, err := r.ResponseWriter.Write(buf)
 	return n, err
 }
-
-// Hijack implements dns.Hijacker. It simply wraps the underlying
-// ResponseWriter's Hijack method if there is one, or returns an error.
-func (r *ResponseReverter) Hijack() {
-	r.ResponseWriter.Hijack()
-	return
-}

plugin/rewrite/rewrite.go

@@ -44,6 +44,13 @@ func (rw Rewrite) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg
 	for _, rule := range rw.Rules {
 		switch result := rule.Rewrite(ctx, state); result {
 		case RewriteDone:
+			if !validName(state.Req.Question[0].Name) {
+				x := state.Req.Question[0].Name
+				log.Errorf("Invalid name after rewrite: %s", x)
+				state.Req.Question[0] = wr.originalQuestion
+				return dns.RcodeServerFailure, fmt.Errorf("invalid name after rewrite: %s", x)
+			}
+
 			respRule := rule.GetResponseRule()
 			if respRule.Active == true {
 				wr.ResponseRewrite = true

plugin/rewrite/setup.go

@@ -3,10 +3,13 @@ package rewrite
 import (
 	"github.com/coredns/coredns/core/dnsserver"
 	"github.com/coredns/coredns/plugin"
+	clog "github.com/coredns/coredns/plugin/pkg/log"
 
 	"github.com/mholt/caddy"
 )
 
+var log = clog.NewWithPlugin("rewrite")
+
 func init() {
 	caddy.RegisterPlugin("rewrite", caddy.Plugin{
 		ServerType: "dns",