From 042171d1a1084eae082f125501d5535de00c3e3a Mon Sep 17 00:00:00 2001 From: David Sheets Date: Tue, 6 Oct 2020 16:34:06 +0100 Subject: [PATCH 1/4] dnsforward/ipset: factor ipset command execution out of ipset processing --- dnsforward/ipset.go | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/dnsforward/ipset.go b/dnsforward/ipset.go index b07556d5..8bb5ce73 100644 --- a/dnsforward/ipset.go +++ b/dnsforward/ipset.go @@ -89,8 +89,22 @@ func (c *ipsetCtx) getIP(rr dns.RR) net.IP { } } -// Add IP addresses of the specified in configuration domain names to an ipset list -func (c *ipsetCtx) process(ctx *dnsContext) int { +func addToIpset(host string, ipsetName string, ipStr string) { + code, out, err := util.RunCommand("ipset", "add", ipsetName, ipStr) + if err != nil { + log.Info("IPSET: %s(%s) -> %s: %s", host, ipStr, ipsetName, err) + return + } + if code != 0 { + log.Info("IPSET: ipset add: code:%d output:'%s'", code, out) + return + } + log.Debug("IPSET: added %s(%s) -> %s", host, ipStr, ipsetName) +} + +// Compute which addresses to add to which ipsets for a particular DNS query response +// Call addMember for each (host, ipset, ip) triple +func (c *ipsetCtx) processMembers(ctx *dnsContext, addMember func(string, string, string)) int { req := ctx.proxyCtx.Req if !(req.Question[0].Qtype == dns.TypeA || req.Question[0].Qtype == dns.TypeAAAA) || @@ -116,18 +130,14 @@ func (c *ipsetCtx) process(ctx *dnsContext) int { ipStr := ip.String() for _, name := range ipsetNames { - code, out, err := util.RunCommand("ipset", "add", name, ipStr) - if err != nil { - log.Info("IPSET: %s(%s) -> %s: %s", host, ipStr, name, err) - continue - } - if code != 0 { - log.Info("IPSET: ipset add: code:%d output:'%s'", code, out) - continue - } - log.Debug("IPSET: added %s(%s) -> %s", host, ipStr, name) + addMember(host, name, ipStr) } } return resultDone } + +// Add IP addresses of the specified in configuration domain names to an ipset list +func (c *ipsetCtx) process(ctx *dnsContext) int { + return c.processMembers(ctx, addToIpset) +} From 2db79bf7ec7b3826cba92f1d7f86cafd3eeb4a0e Mon Sep 17 00:00:00 2001 From: David Sheets Date: Tue, 6 Oct 2020 16:39:34 +0100 Subject: [PATCH 2/4] dnsforward/ipset: rewrite ipset tests to check dnsmasq behavior In the process, found a segfault from assuming the Res field of proxyCtx is non-nil. I added a defensive check as I'm not sure if it's guaranteed to be non-nil in regular execution in this context. --- dnsforward/ipset.go | 18 +++-- dnsforward/ipset_test.go | 169 +++++++++++++++++++++++++++++++++++---- 2 files changed, 162 insertions(+), 25 deletions(-) diff --git a/dnsforward/ipset.go b/dnsforward/ipset.go index 8bb5ce73..c6b3de5a 100644 --- a/dnsforward/ipset.go +++ b/dnsforward/ipset.go @@ -122,15 +122,17 @@ func (c *ipsetCtx) processMembers(ctx *dnsContext, addMember func(string, string log.Debug("IPSET: found ipsets %v for host %s", ipsetNames, host) - for _, it := range ctx.proxyCtx.Res.Answer { - ip := c.getIP(it) - if ip == nil { - continue - } + if ctx.proxyCtx.Res != nil { + for _, it := range ctx.proxyCtx.Res.Answer { + ip := c.getIP(it) + if ip == nil { + continue + } - ipStr := ip.String() - for _, name := range ipsetNames { - addMember(host, name, ipStr) + ipStr := ip.String() + for _, name := range ipsetNames { + addMember(host, name, ipStr) + } } } diff --git a/dnsforward/ipset_test.go b/dnsforward/ipset_test.go index 41be83d2..c6794790 100644 --- a/dnsforward/ipset_test.go +++ b/dnsforward/ipset_test.go @@ -1,6 +1,7 @@ package dnsforward import ( + "net" "testing" "github.com/AdguardTeam/dnsproxy/proxy" @@ -8,14 +9,92 @@ import ( "github.com/stretchr/testify/assert" ) -func TestIPSET(t *testing.T) { - s := Server{} - s.conf.IPSETList = append(s.conf.IPSETList, "HOST.com/name") - s.conf.IPSETList = append(s.conf.IPSETList, "host2.com,host3.com/name23") - s.conf.IPSETList = append(s.conf.IPSETList, "host4.com/name4,name41") - c := ipsetCtx{} +var s Server +var c ipsetCtx +var ctx *dnsContext + +type Binding struct { + host string + ipset string + ipStr string +} + +var b map[Binding]int + +func setup() { + s = Server{} + s.conf.IPSETList = []string{ + "HOST.com/name", + "host2.com,host3.com/name23", + "host4.com/name4,name41", + "sub.host4.com/subhost4", + } + + c = ipsetCtx{} c.init(s.conf.IPSETList) + ctx = &dnsContext{ + srv: &s, + } + ctx.responseFromUpstream = true + ctx.proxyCtx = &proxy.DNSContext{} + + b = make(map[Binding]int) +} + +func makeReq(fqdn string, qtype uint16) *dns.Msg { + return &dns.Msg{ + Question: []dns.Question{ + { + Name: fqdn, + Qtype: qtype, + }, + }, + } +} + +func makeReqA(fqdn string) *dns.Msg { + return makeReq(fqdn, dns.TypeA) +} + +func makeReqAAAA(fqdn string) *dns.Msg { + return makeReq(fqdn, dns.TypeAAAA) +} + +func makeA(fqdn string, ip net.IP) *dns.A { + return &dns.A{ + Hdr: dns.RR_Header{Name: fqdn, Rrtype: dns.TypeA, Class: dns.ClassINET, Ttl: 0}, + A: ip, + } +} + +func makeAAAA(fqdn string, ip net.IP) *dns.AAAA { + return &dns.AAAA{ + Hdr: dns.RR_Header{Name: fqdn, Rrtype: dns.TypeAAAA, Class: dns.ClassINET, Ttl: 0}, + AAAA: ip, + } +} + +func makeCNAME(fqdn string, cnameFqdn string) *dns.CNAME { + return &dns.CNAME{ + Hdr: dns.RR_Header{Name: fqdn, Rrtype: dns.TypeCNAME, Class: dns.ClassINET, Ttl: 0}, + Target: cnameFqdn, + } +} + +func addToBindings(host string, ipset string, ipStr string) { + binding := Binding{host, ipset, ipStr} + count := b[binding] + b[binding] = count + 1 +} + +func doProcess(t *testing.T) { + assert.Equal(t, resultDone, c.processMembers(ctx, addToBindings)) +} + +func TestIpsetParsing(t *testing.T) { + setup() + assert.Equal(t, "name", c.ipsetList["host.com"][0]) assert.Equal(t, "name23", c.ipsetList["host2.com"][0]) assert.Equal(t, "name23", c.ipsetList["host3.com"][0]) @@ -24,18 +103,74 @@ func TestIPSET(t *testing.T) { _, ok := c.ipsetList["host0.com"] assert.False(t, ok) +} - ctx := &dnsContext{ - srv: &s, - } - ctx.proxyCtx = &proxy.DNSContext{} - ctx.proxyCtx.Req = &dns.Msg{ - Question: []dns.Question{ - { - Name: "host.com.", - Qtype: dns.TypeA, - }, +func TestIpsetNoAnswer(t *testing.T) { + setup() + + ctx.proxyCtx.Req = makeReqA("HOST4.COM.") + + doProcess(t) + assert.Equal(t, 0, len(b)) +} + +func TestIpsetCache(t *testing.T) { + setup() + + ctx.proxyCtx.Req = makeReqA("HOST4.COM.") + ctx.proxyCtx.Res = &dns.Msg{ + Answer: []dns.RR{ + makeA("HOST4.COM.", net.IPv4(127, 0, 0, 1)), + makeAAAA("HOST4.COM.", net.IPv6loopback), }, } - assert.Equal(t, resultDone, c.process(ctx)) + + doProcess(t) + + assert.Equal(t, 1, b[Binding{"host4.com", "name4", "127.0.0.1"}]) + assert.Equal(t, 1, b[Binding{"host4.com", "name41", "127.0.0.1"}]) + assert.Equal(t, 1, b[Binding{"host4.com", "name4", net.IPv6loopback.String()}]) + assert.Equal(t, 1, b[Binding{"host4.com", "name41", net.IPv6loopback.String()}]) + assert.Equal(t, 4, len(b)) + + doProcess(t) + + assert.Equal(t, 1, b[Binding{"host4.com", "name4", "127.0.0.1"}]) + assert.Equal(t, 1, b[Binding{"host4.com", "name41", "127.0.0.1"}]) + assert.Equal(t, 1, b[Binding{"host4.com", "name4", net.IPv6loopback.String()}]) + assert.Equal(t, 1, b[Binding{"host4.com", "name41", net.IPv6loopback.String()}]) + assert.Equal(t, 4, len(b)) +} + +func TestIpsetSubdomainOverride(t *testing.T) { + setup() + + ctx.proxyCtx.Req = makeReqA("sub.host4.com.") + ctx.proxyCtx.Res = &dns.Msg{ + Answer: []dns.RR{ + makeA("sub.host4.com.", net.IPv4(127, 0, 0, 1)), + }, + } + + doProcess(t) + + assert.Equal(t, 1, b[Binding{"sub.host4.com", "subhost4", "127.0.0.1"}]) + assert.Equal(t, 1, len(b)) +} + +func TestIpsetCnameThirdParty(t *testing.T) { + setup() + + ctx.proxyCtx.Req = makeReqA("host.com.") + ctx.proxyCtx.Res = &dns.Msg{ + Answer: []dns.RR{ + makeCNAME("host.com.", "foo.bar.baz.elb.amazonaws.com."), + makeA("foo.bar.baz.elb.amazonaws.com.", net.IPv4(8, 8, 8, 8)), + }, + } + + doProcess(t) + + assert.Equal(t, 1, b[Binding{"host.com", "name", "8.8.8.8"}]) + assert.Equal(t, 1, len(b)) } From a93c6b67753c50fb29f333e5a91e95a4c15c5610 Mon Sep 17 00:00:00 2001 From: David Sheets Date: Tue, 6 Oct 2020 16:45:11 +0100 Subject: [PATCH 3/4] dnsforward/ipset: add support for wildcard subdomain ipset matches This matches dnsmasq behavior and the alternative is not really useful. See http://thekelleys.org.uk/gitweb/?p=dnsmasq.git;a=blob;f=src/forward.c;hb=f60fea1fb0a288011f57a25dfb653b8f6f8b46b9#l588 --- dnsforward/ipset.go | 27 ++++++++++++++++++++++++++- dnsforward/ipset_test.go | 16 ++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) diff --git a/dnsforward/ipset.go b/dnsforward/ipset.go index c6b3de5a..f5dadac0 100644 --- a/dnsforward/ipset.go +++ b/dnsforward/ipset.go @@ -89,6 +89,31 @@ func (c *ipsetCtx) getIP(rr dns.RR) net.IP { } } +// Find the ipsets for a given host (accounting for subdomain wildcards) +func (c *ipsetCtx) getIpsetNames(host string) ([]string, bool) { + var ipsetNames []string + var found bool + + // search for matching ipset hosts starting with most specific subdomain + i := 0 + for i != -1 { + host = host[i:] + + ipsetNames, found = c.ipsetList[host] + if found { + break + } + + // move slice up to the parent domain + i = strings.Index(host, ".") + if i != -1 { + i++ + } + } + + return ipsetNames, found +} + func addToIpset(host string, ipsetName string, ipStr string) { code, out, err := util.RunCommand("ipset", "add", ipsetName, ipStr) if err != nil { @@ -115,7 +140,7 @@ func (c *ipsetCtx) processMembers(ctx *dnsContext, addMember func(string, string host := req.Question[0].Name host = strings.TrimSuffix(host, ".") host = strings.ToLower(host) - ipsetNames, found := c.ipsetList[host] + ipsetNames, found := c.getIpsetNames(host) if !found { return resultDone } diff --git a/dnsforward/ipset_test.go b/dnsforward/ipset_test.go index c6794790..bf8f362d 100644 --- a/dnsforward/ipset_test.go +++ b/dnsforward/ipset_test.go @@ -158,6 +158,22 @@ func TestIpsetSubdomainOverride(t *testing.T) { assert.Equal(t, 1, len(b)) } +func TestIpsetSubdomainWildcard(t *testing.T) { + setup() + + ctx.proxyCtx.Req = makeReqA("sub.host.com.") + ctx.proxyCtx.Res = &dns.Msg{ + Answer: []dns.RR{ + makeA("sub.host.com.", net.IPv4(127, 0, 0, 1)), + }, + } + + doProcess(t) + + assert.Equal(t, 1, b[Binding{"sub.host.com", "name", "127.0.0.1"}]) + assert.Equal(t, 1, len(b)) +} + func TestIpsetCnameThirdParty(t *testing.T) { setup() From d39c1b0be6bd66d27d93129a52662e6e93e4bc3e Mon Sep 17 00:00:00 2001 From: David Sheets Date: Wed, 7 Oct 2020 09:53:30 +0100 Subject: [PATCH 4/4] dnsforward/ipset: add segfault defense for missing DNS question section --- dnsforward/ipset.go | 2 +- dnsforward/ipset_test.go | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/dnsforward/ipset.go b/dnsforward/ipset.go index f5dadac0..f0c49973 100644 --- a/dnsforward/ipset.go +++ b/dnsforward/ipset.go @@ -131,7 +131,7 @@ func addToIpset(host string, ipsetName string, ipStr string) { // Call addMember for each (host, ipset, ip) triple func (c *ipsetCtx) processMembers(ctx *dnsContext, addMember func(string, string, string)) int { req := ctx.proxyCtx.Req - if !(req.Question[0].Qtype == dns.TypeA || + if req == nil || !(req.Question[0].Qtype == dns.TypeA || req.Question[0].Qtype == dns.TypeAAAA) || !ctx.responseFromUpstream { return resultDone diff --git a/dnsforward/ipset_test.go b/dnsforward/ipset_test.go index bf8f362d..72721325 100644 --- a/dnsforward/ipset_test.go +++ b/dnsforward/ipset_test.go @@ -105,6 +105,13 @@ func TestIpsetParsing(t *testing.T) { assert.False(t, ok) } +func TestIpsetNoQuestion(t *testing.T) { + setup() + + doProcess(t) + assert.Equal(t, 0, len(b)) +} + func TestIpsetNoAnswer(t *testing.T) { setup()