From 8f162078458d94528b71c8a44b7bd0f80fa8e0e2 Mon Sep 17 00:00:00 2001 From: Eugene Burkov Date: Mon, 14 Feb 2022 16:56:14 +0300 Subject: [PATCH] Pull request: 3381 check private domains Merge in DNS/adguard-home from 3381-validate-privateness to master Closes #3381. Squashed commit of the following: commit 21cb12d10b07bb0bf0578db74ca9ac7b3ac5ae14 Author: Eugene Burkov Date: Mon Feb 14 16:29:59 2022 +0300 all: imp code, docs commit 39793551438cbea71e6ec78d0e05bee2d8dba3e5 Author: Eugene Burkov Date: Mon Feb 14 15:08:36 2022 +0300 all: imp code, docs commit 6b71848fd0980582b1bfe24a34f48608795e9b7d Author: Eugene Burkov Date: Mon Feb 14 14:22:00 2022 +0300 all: check private domains --- CHANGELOG.md | 6 + go.mod | 2 +- go.sum | 3 +- internal/aghnet/subnetdetector.go | 10 +- internal/dnsforward/http.go | 247 +++++++++++------- internal/dnsforward/http_test.go | 62 ++++- .../TestDNSForwardHTTP_handleSetConfig.json | 37 +++ 7 files changed, 260 insertions(+), 107 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 54c916d8938..2976e276d6c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,11 @@ and this project adheres to ### Changed +- Domain-specific private reverse DNS upstream servers are now validated to + allow only `*.in-addr.arpa` and `*.ip6.arpa` domains pointing to + locally-served networks ([#3381]). **Note:** If you already have invalid + entires in your configuration, consider removing them manually, since they + essentially had no effect. - Response filtering is now performed using the record types of the answer section of messages as opposed to the type of the question ([#4238]). - Instead of adding the build time information, the build scripts now use the @@ -80,6 +85,7 @@ In this release, the schema version has changed from 12 to 13. [#2993]: https://github.com/AdguardTeam/AdGuardHome/issues/2993 [#3057]: https://github.com/AdguardTeam/AdGuardHome/issues/3057 [#3367]: https://github.com/AdguardTeam/AdGuardHome/issues/3367 +[#3381]: https://github.com/AdguardTeam/AdGuardHome/issues/3381 [#3503]: https://github.com/AdguardTeam/AdGuardHome/issues/3503 [#4216]: https://github.com/AdguardTeam/AdGuardHome/issues/4216 [#4221]: https://github.com/AdguardTeam/AdGuardHome/issues/4221 diff --git a/go.mod b/go.mod index d75e299e05f..0018223537c 100644 --- a/go.mod +++ b/go.mod @@ -4,7 +4,7 @@ go 1.17 require ( github.com/AdguardTeam/dnsproxy v0.41.1 - github.com/AdguardTeam/golibs v0.10.4 + github.com/AdguardTeam/golibs v0.10.5 github.com/AdguardTeam/urlfilter v0.15.2 github.com/NYTimes/gziphandler v1.1.1 github.com/ameshkov/dnscrypt/v2 v2.2.3 diff --git a/go.sum b/go.sum index 65128977ed4..99f20fd1afa 100644 --- a/go.sum +++ b/go.sum @@ -12,8 +12,9 @@ github.com/AdguardTeam/dnsproxy v0.41.1/go.mod h1:PZ9l22h3Er+5mxFQB7oHZMTvx+aa9R github.com/AdguardTeam/golibs v0.4.0/go.mod h1:skKsDKIBB7kkFflLJBpfGX+G8QFTx0WKUzB6TIgtUj4= github.com/AdguardTeam/golibs v0.4.2/go.mod h1:skKsDKIBB7kkFflLJBpfGX+G8QFTx0WKUzB6TIgtUj4= github.com/AdguardTeam/golibs v0.10.3/go.mod h1:rSfQRGHIdgfxriDDNgNJ7HmE5zRoURq8R+VdR81Zuzw= -github.com/AdguardTeam/golibs v0.10.4 h1:TMBkablZC0IZOpRgg9fzAKlxxNhSN2YJq7qbgtuZ7PQ= github.com/AdguardTeam/golibs v0.10.4/go.mod h1:rSfQRGHIdgfxriDDNgNJ7HmE5zRoURq8R+VdR81Zuzw= +github.com/AdguardTeam/golibs v0.10.5 h1:4/nl1yIBJOv5luVu9SURW8LfgOjI3zQ2moIUy/1k0y4= +github.com/AdguardTeam/golibs v0.10.5/go.mod h1:rSfQRGHIdgfxriDDNgNJ7HmE5zRoURq8R+VdR81Zuzw= github.com/AdguardTeam/gomitmproxy v0.2.0/go.mod h1:Qdv0Mktnzer5zpdpi5rAwixNJzW2FN91LjKJCkVbYGU= github.com/AdguardTeam/urlfilter v0.15.2 h1:LZGgrm4l4Ys9eAqB+UUmZfiC6vHlDlYFhx0WXqo6LtQ= github.com/AdguardTeam/urlfilter v0.15.2/go.mod h1:46YZDOV1+qtdRDuhZKVPSSp7JWWes0KayqHrKAFBdEI= diff --git a/internal/aghnet/subnetdetector.go b/internal/aghnet/subnetdetector.go index e353903b68e..fd338aaa7a6 100644 --- a/internal/aghnet/subnetdetector.go +++ b/internal/aghnet/subnetdetector.go @@ -6,16 +6,18 @@ import ( // SubnetDetector describes IP address properties. type SubnetDetector struct { - // spNets is the slice of special-purpose address registries as defined - // by RFC-6890 (https://tools.ietf.org/html/rfc6890). + // spNets is the collection of special-purpose address registries as defined + // by RFC 6890. spNets []*net.IPNet - // locServedNets is the slice of locally-served networks as defined by - // RFC-6303 (https://tools.ietf.org/html/rfc6303). + // locServedNets is the collection of locally-served networks as defined by + // RFC 6303. locServedNets []*net.IPNet } // NewSubnetDetector returns a new IP detector. +// +// TODO(a.garipov): Decide whether an error is actually needed. func NewSubnetDetector() (snd *SubnetDetector, err error) { spNets := []string{ // "This" network. diff --git a/internal/dnsforward/http.go b/internal/dnsforward/http.go index b0de11b9b4d..d653fa1edc8 100644 --- a/internal/dnsforward/http.go +++ b/internal/dnsforward/http.go @@ -5,10 +5,12 @@ import ( "fmt" "net" "net/http" + "sort" "strings" "time" "github.com/AdguardTeam/AdGuardHome/internal/aghhttp" + "github.com/AdguardTeam/AdGuardHome/internal/aghnet" "github.com/AdguardTeam/dnsproxy/proxy" "github.com/AdguardTeam/dnsproxy/upstream" "github.com/AdguardTeam/golibs/errors" @@ -41,7 +43,7 @@ type dnsConfig struct { LocalPTRUpstreams *[]string `json:"local_ptr_upstreams"` } -func (s *Server) getDNSConfig() dnsConfig { +func (s *Server) getDNSConfig() (c *dnsConfig) { s.serverLock.RLock() defer s.serverLock.RUnlock() @@ -70,7 +72,7 @@ func (s *Server) getDNSConfig() dnsConfig { upstreamMode = "parallel" } - return dnsConfig{ + return &dnsConfig{ Upstreams: &upstreams, UpstreamsFile: &upstreamFile, Bootstraps: &bootstraps, @@ -106,7 +108,7 @@ func (s *Server) handleGetConfig(w http.ResponseWriter, r *http.Request) { // since there is no need to omit it while decoding from JSON. DefautLocalPTRUpstreams []string `json:"default_local_ptr_upstreams,omitempty"` }{ - dnsConfig: s.getDNSConfig(), + dnsConfig: *s.getDNSConfig(), DefautLocalPTRUpstreams: defLocalPTRUps, } @@ -138,39 +140,63 @@ func (req *dnsConfig) checkBlockingMode() bool { } func (req *dnsConfig) checkUpstreamsMode() bool { - if req.UpstreamMode == nil { - return true + valid := []string{"", "fastest_addr", "parallel"} + + return req.UpstreamMode == nil || stringutil.InSlice(valid, *req.UpstreamMode) +} + +func (req *dnsConfig) checkBootstrap() (err error) { + if req.Bootstraps == nil { + return nil } - for _, valid := range []string{ - "", - "fastest_addr", - "parallel", - } { - if *req.UpstreamMode == valid { - return true + var b string + defer func() { err = errors.Annotate(err, "checking bootstrap %s: invalid address: %w", b) }() + + for _, b = range *req.Bootstraps { + if b == "" { + return errors.Error("empty") + } + + if _, err = upstream.NewResolver(b, nil); err != nil { + return err } } - return false + return nil } -func (req *dnsConfig) checkBootstrap() (string, error) { - if req.Bootstraps == nil { - return "", nil +// validate returns an error if any field of req is invalid. +func (req *dnsConfig) validate(snd *aghnet.SubnetDetector) (err error) { + if req.Upstreams != nil { + err = ValidateUpstreams(*req.Upstreams) + if err != nil { + return fmt.Errorf("validating upstream servers: %w", err) + } } - for _, boot := range *req.Bootstraps { - if boot == "" { - return boot, fmt.Errorf("invalid bootstrap server address: empty") + if req.LocalPTRUpstreams != nil { + err = ValidateUpstreamsPrivate(*req.LocalPTRUpstreams, snd) + if err != nil { + return fmt.Errorf("validating private upstream servers: %w", err) } + } - if _, err := upstream.NewResolver(boot, nil); err != nil { - return boot, fmt.Errorf("invalid bootstrap server address: %w", err) - } + err = req.checkBootstrap() + if err != nil { + return err } - return "", nil + switch { + case !req.checkBlockingMode(): + return errors.Error("blocking_mode: incorrect value") + case !req.checkUpstreamsMode(): + return errors.Error("upstream_mode: incorrect value") + case !req.checkCacheTTL(): + return errors.Error("cache_ttl_min must be less or equal than cache_ttl_max") + default: + return nil + } } func (req *dnsConfig) checkCacheTTL() bool { @@ -190,69 +216,33 @@ func (req *dnsConfig) checkCacheTTL() bool { } func (s *Server) handleSetConfig(w http.ResponseWriter, r *http.Request) { - req := dnsConfig{} - err := json.NewDecoder(r.Body).Decode(&req) + req := &dnsConfig{} + err := json.NewDecoder(r.Body).Decode(req) if err != nil { - aghhttp.Error(r, w, http.StatusBadRequest, "json Encode: %s", err) + aghhttp.Error(r, w, http.StatusBadRequest, "decoding request: %s", err) return } - if req.Upstreams != nil { - if err = ValidateUpstreams(*req.Upstreams); err != nil { - aghhttp.Error(r, w, http.StatusBadRequest, "wrong upstreams specification: %s", err) - - return - } - } - - var errBoot string - if errBoot, err = req.checkBootstrap(); err != nil { - aghhttp.Error( - r, - w, - http.StatusBadRequest, - "%s can not be used as bootstrap dns cause: %s", - errBoot, - err, - ) - - return - } - - switch { - case !req.checkBlockingMode(): - aghhttp.Error(r, w, http.StatusBadRequest, "blocking_mode: incorrect value") - - return - case !req.checkUpstreamsMode(): - aghhttp.Error(r, w, http.StatusBadRequest, "upstream_mode: incorrect value") - - return - case !req.checkCacheTTL(): - aghhttp.Error( - r, - w, - http.StatusBadRequest, - "cache_ttl_min must be less or equal than cache_ttl_max", - ) + err = req.validate(s.subnetDetector) + if err != nil { + aghhttp.Error(r, w, http.StatusBadRequest, "%s", err) return - default: - // Go on. } restart := s.setConfig(req) s.conf.ConfigModified() if restart { - if err = s.Reconfigure(nil); err != nil { + err = s.Reconfigure(nil) + if err != nil { aghhttp.Error(r, w, http.StatusInternalServerError, "%s", err) } } } -func (s *Server) setConfigRestartable(dc dnsConfig) (restart bool) { +func (s *Server) setConfigRestartable(dc *dnsConfig) (restart bool) { if dc.Upstreams != nil { s.conf.UpstreamDNS = *dc.Upstreams restart = true @@ -273,9 +263,9 @@ func (s *Server) setConfigRestartable(dc dnsConfig) (restart bool) { restart = true } - if dc.RateLimit != nil { - restart = restart || s.conf.Ratelimit != *dc.RateLimit + if dc.RateLimit != nil && s.conf.Ratelimit != *dc.RateLimit { s.conf.Ratelimit = *dc.RateLimit + restart = true } if dc.EDNSCSEnabled != nil { @@ -306,7 +296,7 @@ func (s *Server) setConfigRestartable(dc dnsConfig) (restart bool) { return restart } -func (s *Server) setConfig(dc dnsConfig) (restart bool) { +func (s *Server) setConfig(dc *dnsConfig) (restart bool) { s.serverLock.Lock() defer s.serverLock.Unlock() @@ -353,52 +343,117 @@ type upstreamJSON struct { PrivateUpstreams []string `json:"private_upstream"` } -// IsCommentOrEmpty returns true of the string starts with a "#" character or is -// an empty string. This function is useful for filtering out non-upstream -// lines from upstream configs. +// IsCommentOrEmpty returns true if s starts with a "#" character or is empty. +// This function is useful for filtering out non-upstream lines from upstream +// configs. func IsCommentOrEmpty(s string) (ok bool) { return len(s) == 0 || s[0] == '#' } -// ValidateUpstreams validates each upstream and returns an error if any -// upstream is invalid or if there are no default upstreams specified. +// LocalNetChecker is used to check if the IP address belongs to a local +// network. +type LocalNetChecker interface { + // IsLocallyServedNetwork returns true if ip is contained in any of address + // registries defined by RFC 6303. + IsLocallyServedNetwork(ip net.IP) (ok bool) +} + +// type check +var _ LocalNetChecker = (*aghnet.SubnetDetector)(nil) + +// newUpstreamConfig validates upstreams and returns an appropriate upstream +// configuration or nil if it can't be built. // -// TODO(e.burkov): Move into aghnet or even into dnsproxy. -func ValidateUpstreams(upstreams []string) (err error) { - // No need to validate comments +// TODO(e.burkov): Perhaps proxy.ParseUpstreamsConfig should validate upstreams +// slice already so that this function may be considered useless. +func newUpstreamConfig(upstreams []string) (conf *proxy.UpstreamConfig, err error) { + // No need to validate comments and empty lines. upstreams = stringutil.FilterOut(upstreams, IsCommentOrEmpty) - - // Consider this case valid because defaultDNS will be used if len(upstreams) == 0 { - return nil + // Consider this case valid since it means the default server should be + // used. + return nil, nil } - _, err = proxy.ParseUpstreamsConfig( + conf, err = proxy.ParseUpstreamsConfig( upstreams, - &upstream.Options{ - Bootstrap: []string{}, - Timeout: DefaultTimeout, - }, + &upstream.Options{Bootstrap: []string{}, Timeout: DefaultTimeout}, ) if err != nil { - return err + return nil, err + } else if len(conf.Upstreams) == 0 { + return nil, errors.Error("no default upstreams specified") } - var defaultUpstreamFound bool for _, u := range upstreams { - var useDefault bool - useDefault, err = validateUpstream(u) + _, err = validateUpstream(u) if err != nil { - return err + return nil, err + } + } + + return conf, nil +} + +// ValidateUpstreams validates each upstream and returns an error if any +// upstream is invalid or if there are no default upstreams specified. +// +// TODO(e.burkov): Move into aghnet or even into dnsproxy. +func ValidateUpstreams(upstreams []string) (err error) { + _, err = newUpstreamConfig(upstreams) + + return err +} + +// stringKeysSorted returns the sorted slice of string keys of m. +// +// TODO(e.burkov): Use generics in Go 1.18. Move into golibs. +func stringKeysSorted(m map[string][]upstream.Upstream) (sorted []string) { + sorted = make([]string, 0, len(m)) + for s := range m { + sorted = append(sorted, s) + } + + sort.Strings(sorted) + + return sorted +} + +// ValidateUpstreamsPrivate validates each upstream and returns an error if any +// upstream is invalid or if there are no default upstreams specified. It also +// checks each domain of domain-specific upstreams for being ARPA pointing to +// a locally-served network. lnc must not be nil. +func ValidateUpstreamsPrivate(upstreams []string, lnc LocalNetChecker) (err error) { + conf, err := newUpstreamConfig(upstreams) + if err != nil { + return err + } + + if conf == nil { + return nil + } + + var errs []error + + for _, domain := range stringKeysSorted(conf.DomainReservedUpstreams) { + var subnet *net.IPNet + subnet, err = netutil.SubnetFromReversedAddr(domain) + if err != nil { + errs = append(errs, err) + + continue } - if !defaultUpstreamFound { - defaultUpstreamFound = useDefault + if !lnc.IsLocallyServedNetwork(subnet.IP) { + errs = append( + errs, + fmt.Errorf("arpa domain %q should point to a locally-served network", domain), + ) } } - if !defaultUpstreamFound { - return fmt.Errorf("no default upstreams specified") + if len(errs) > 0 { + return errors.List("checking domain-specific upstreams", errs...) } return nil diff --git a/internal/dnsforward/http_test.go b/internal/dnsforward/http_test.go index 876cfdcf584..66931f4dd8a 100644 --- a/internal/dnsforward/http_test.go +++ b/internal/dnsforward/http_test.go @@ -184,12 +184,11 @@ func TestDNSForwardHTTP_handleSetConfig(t *testing.T) { wantSet: "", }, { name: "upstream_dns_bad", - wantSet: `wrong upstreams specification: bad ipport address "!!!": address !!!: ` + - `missing port in address`, + wantSet: `validating upstream servers: bad ipport address "!!!": ` + + `address !!!: missing port in address`, }, { name: "bootstraps_bad", - wantSet: `a can not be used as bootstrap dns cause: ` + - `invalid bootstrap server address: ` + + wantSet: `checking bootstrap a: invalid address: ` + `Resolver a is not eligible to be a bootstrap DNS server`, }, { name: "cache_bad_ttl", @@ -200,6 +199,10 @@ func TestDNSForwardHTTP_handleSetConfig(t *testing.T) { }, { name: "local_ptr_upstreams_good", wantSet: "", + }, { + name: "local_ptr_upstreams_bad", + wantSet: `validating private upstream servers: checking domain-specific upstreams: ` + + `bad arpa domain name "non.arpa": not a reversed ip network`, }, { name: "local_ptr_upstreams_null", wantSet: "", @@ -350,7 +353,7 @@ func TestValidateUpstream(t *testing.T) { } } -func TestValidateUpstreamsSet(t *testing.T) { +func TestValidateUpstreams(t *testing.T) { testCases := []struct { name string wantErr string @@ -397,3 +400,52 @@ func TestValidateUpstreamsSet(t *testing.T) { }) } } + +func TestValidateUpstreamsPrivate(t *testing.T) { + snd, err := aghnet.NewSubnetDetector() + require.NoError(t, err) + + testCases := []struct { + name string + wantErr string + u string + }{{ + name: "success_address", + wantErr: ``, + u: "[/1.0.0.127.in-addr.arpa/]#", + }, { + name: "success_subnet", + wantErr: ``, + u: "[/127.in-addr.arpa/]#", + }, { + name: "not_arpa_subnet", + wantErr: `checking domain-specific upstreams: ` + + `bad arpa domain name "hello.world": not a reversed ip network`, + u: "[/hello.world/]#", + }, { + name: "non-private_arpa_address", + wantErr: `checking domain-specific upstreams: ` + + `arpa domain "1.2.3.4.in-addr.arpa." should point to a locally-served network`, + u: "[/1.2.3.4.in-addr.arpa/]#", + }, { + name: "non-private_arpa_subnet", + wantErr: `checking domain-specific upstreams: ` + + `arpa domain "128.in-addr.arpa." should point to a locally-served network`, + u: "[/128.in-addr.arpa/]#", + }, { + name: "several_bad", + wantErr: `checking domain-specific upstreams: 2 errors: ` + + `"arpa domain \"1.2.3.4.in-addr.arpa.\" should point to a locally-served network", ` + + `"bad arpa domain name \"non.arpa\": not a reversed ip network"`, + u: "[/non.arpa/1.2.3.4.in-addr.arpa/127.in-addr.arpa/]#", + }} + + for _, tc := range testCases { + set := []string{"192.168.0.1", tc.u} + + t.Run(tc.name, func(t *testing.T) { + err = ValidateUpstreamsPrivate(set, snd) + testutil.AssertErrorMsg(t, tc.wantErr, err) + }) + } +} diff --git a/internal/dnsforward/testdata/TestDNSForwardHTTP_handleSetConfig.json b/internal/dnsforward/testdata/TestDNSForwardHTTP_handleSetConfig.json index b594029ced8..830bf491420 100644 --- a/internal/dnsforward/testdata/TestDNSForwardHTTP_handleSetConfig.json +++ b/internal/dnsforward/testdata/TestDNSForwardHTTP_handleSetConfig.json @@ -520,6 +520,43 @@ ] } }, + "local_ptr_upstreams_bad": { + "req": { + "local_ptr_upstreams": [ + "123.123.123.123", + "[/non.arpa/]#" + ] + }, + "want": { + "upstream_dns": [ + "8.8.8.8:53", + "8.8.4.4:53" + ], + "upstream_dns_file": "", + "bootstrap_dns": [ + "9.9.9.10", + "149.112.112.10", + "2620:fe::10", + "2620:fe::fe:10" + ], + "protection_enabled": true, + "ratelimit": 0, + "blocking_mode": "", + "blocking_ipv4": "", + "blocking_ipv6": "", + "edns_cs_enabled": false, + "dnssec_enabled": false, + "disable_ipv6": false, + "upstream_mode": "", + "cache_size": 0, + "cache_ttl_min": 0, + "cache_ttl_max": 0, + "cache_optimistic": false, + "resolve_clients": false, + "use_private_ptr_resolvers": false, + "local_ptr_upstreams": [] + } + }, "local_ptr_upstreams_null": { "req": { "local_ptr_upstreams": null