Merge pull request #379 from safing/fix/endpoint-list-matching

Improve endpoint/rule lists and filtering of DNS requests

firewall/bypassing.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/safing/portmaster/nameserver/nsutil"
 	"github.com/safing/portmaster/network"
+	"github.com/safing/portmaster/network/packet"
 	"github.com/safing/portmaster/profile/endpoints"
 )
 
@@ -16,17 +17,23 @@ var (
 // PreventBypassing checks if the connection should be denied or permitted
 // based on some bypass protection checks.
 func PreventBypassing(ctx context.Context, conn *network.Connection) (endpoints.EPResult, string, nsutil.Responder) {
-	// Block firefox canary domain to disable DoH
+	// Block firefox canary domain to disable DoH.
 	if strings.ToLower(conn.Entity.Domain) == "use-application-dns.net." {
 		return endpoints.Denied,
 			"blocked canary domain to prevent enabling of DNS-over-HTTPs",
 			nsutil.NxDomain()
 	}
 
+	// Block direct connections to known DNS resolvers.
+	switch packet.IPProtocol(conn.Entity.Protocol) {
+	case packet.ICMP, packet.ICMPv6:
+		// Make an exception for ICMP, as these IPs are also often used for debugging.
+	default:
 		if conn.Entity.MatchLists(resolverFilterLists) {
 			return endpoints.Denied,
 				"blocked rogue connection to DNS resolver",
-			nsutil.ZeroIP()
+				nsutil.BlockIP()
+		}
 	}
 
 	return endpoints.NoMatch, "", nil

firewall/master.go

@@ -54,13 +54,6 @@ var defaultDeciders = []deciderFn{
 	checkAutoPermitRelated,
 }
 
-var dnsFromSystemResolverDeciders = []deciderFn{
-	checkEndpointListsForSystemResolverDNSRequests,
-	checkConnectivityDomain,
-	checkBypassPrevention,
-	checkFilterLists,
-}
-
 // DecideOnConnection makes a decision about a connection.
 // When called, the connection and profile is already locked.
 func DecideOnConnection(ctx context.Context, conn *network.Connection, pkt packet.Packet) {
@@ -99,25 +92,19 @@ func DecideOnConnection(ctx context.Context, conn *network.Connection, pkt packe
 	conn.Entity.EnableCNAMECheck(ctx, layeredProfile.FilterCNAMEs())
 	conn.Entity.LoadLists(ctx)
 
-	// DNS request from the system resolver require a special decision process,
-	// because the original requesting process is not known. Here, we only check
-	// global-only and the most important per-app aspects. The resulting
-	// connection is then blocked when the original requesting process is known.
-	if conn.Type == network.DNSRequest && conn.Process().IsSystemResolver() {
-		// Run all deciders and return if they came to a conclusion.
-		done, _ := runDeciders(ctx, dnsFromSystemResolverDeciders, conn, layeredProfile, pkt)
-		if !done {
-			conn.Accept("allowing system resolver dns request", noReasonOptionKey)
-		}
-		return
-	}
-
 	// Run all deciders and return if they came to a conclusion.
 	done, defaultAction := runDeciders(ctx, defaultDeciders, conn, layeredProfile, pkt)
 	if done {
 		return
 	}
 
+	// DNS Request are always default allowed, as the endpoint lists could not
+	// be checked fully.
+	if conn.Type == network.DNSRequest {
+		conn.Accept("allowing dns request", noReasonOptionKey)
+		return
+	}
+
 	// Deciders did not conclude, use default action.
 	switch defaultAction {
 	case profile.DefaultActionPermit:
@@ -197,6 +184,14 @@ func checkSelfCommunication(ctx context.Context, conn *network.Connection, _ *pr
 }
 
 func checkEndpointLists(ctx context.Context, conn *network.Connection, p *profile.LayeredProfile, _ packet.Packet) bool {
+	// DNS request from the system resolver require a special decision process,
+	// because the original requesting process is not known. Here, we only check
+	// global-only and the most important per-app aspects. The resulting
+	// connection is then blocked when the original requesting process is known.
+	if conn.Type == network.DNSRequest && conn.Process().IsSystemResolver() {
+		return checkEndpointListsForSystemResolverDNSRequests(ctx, conn, p)
+	}
+
 	var result endpoints.EPResult
 	var reason endpoints.Reason
 
@@ -210,7 +205,7 @@ func checkEndpointLists(ctx context.Context, conn *network.Connection, p *profil
 		optionKey = profile.CfgOptionEndpointsKey
 	}
 	switch result {
-	case endpoints.Denied:
+	case endpoints.Denied, endpoints.MatchError:
 		conn.DenyWithContext(reason.String(), optionKey, reason.Context())
 		return true
 	case endpoints.Permitted:
@@ -225,13 +220,13 @@ func checkEndpointLists(ctx context.Context, conn *network.Connection, p *profil
 // checkEndpointLists that is only meant for DNS queries by the system
 // resolver. It only checks the endpoint filter list of the local profile and
 // does not include the global profile.
-func checkEndpointListsForSystemResolverDNSRequests(ctx context.Context, conn *network.Connection, p *profile.LayeredProfile, _ packet.Packet) bool {
+func checkEndpointListsForSystemResolverDNSRequests(ctx context.Context, conn *network.Connection, p *profile.LayeredProfile) bool {
 	profileEndpoints := p.LocalProfile().GetEndpoints()
 	if profileEndpoints.IsSet() {
 		result, reason := profileEndpoints.Match(ctx, conn.Entity)
 		if endpoints.IsDecision(result) {
 			switch result {
-			case endpoints.Denied:
+			case endpoints.Denied, endpoints.MatchError:
 				conn.DenyWithContext(reason.String(), profile.CfgOptionEndpointsKey, reason.Context())
 				return true
 			case endpoints.Permitted:
@@ -396,11 +391,13 @@ func checkResolverScope(_ context.Context, conn *network.Connection, p *profile.
 }
 
 func checkDomainHeuristics(ctx context.Context, conn *network.Connection, p *profile.LayeredProfile, _ packet.Packet) bool {
-	if !p.DomainHeuristics() {
+	// Don't check domain heuristics if no domain is available.
+	if conn.Entity.Domain == "" {
 		return false
 	}
 
-	if conn.Entity.Domain == "" {
+	// Check if domain heuristics are enabled.
+	if !p.DomainHeuristics() {
 		return false
 	}
 
@@ -485,10 +482,11 @@ func checkAutoPermitRelated(_ context.Context, conn *network.Connection, p *prof
 
 // checkRelation tries to find a relation between a process and a communication. This is for better out of the box experience and is _not_ meant to thwart intentional malware.
 func checkRelation(conn *network.Connection) (related bool, reason string) {
-	if conn.Entity.Domain != "" {
+	// Don't check relation if no domain is available.
+	if conn.Entity.Domain == "" {
 		return false, ""
 	}
-	// don't check for unknown processes
+	// Don't check for unknown processes.
 	if conn.Process().Pid < 0 {
 		return false, ""
 	}

intel/entity.go

@@ -71,6 +71,9 @@ type Entity struct {
 	// ASOrg holds the owner's name of the autonomous system.
 	ASOrg string
 
+	// LocationError holds an error message if fetching the location failed.
+	LocationError string
+
 	location *geoip.Location
 
 	// BlockedByLists holds list source IDs that
@@ -86,12 +89,15 @@ type Entity struct {
 	// to a list of sources where the entity has been observed in.
 	ListOccurences map[string][]string
 
+	// ListsError holds an error message if fetching the lists failed.
+	ListsError string
+
 	// we only load each data above at most once
 	fetchLocationOnce   sync.Once
 	reverseResolveOnce  sync.Once
 	loadDomainListOnce  sync.Once
 	loadIPListOnce      sync.Once
-	loadCoutryListOnce sync.Once
+	loadCountryListOnce sync.Once
 	loadAsnListOnce     sync.Once
 }
 
@@ -142,7 +148,7 @@ func (e *Entity) ResetLists() {
 	e.checkCNAMEs = false
 	e.loadDomainListOnce = sync.Once{}
 	e.loadIPListOnce = sync.Once{}
-	e.loadCoutryListOnce = sync.Once{}
+	e.loadCountryListOnce = sync.Once{}
 	e.loadAsnListOnce = sync.Once{}
 }
 
@@ -236,6 +242,7 @@ func (e *Entity) getLocation(ctx context.Context) {
 		loc, err := geoip.GetLocation(e.IP)
 		if err != nil {
 			log.Tracer(ctx).Warningf("intel: failed to get location data for %s: %s", e.IP, err)
+			e.LocationError = err.Error()
 			return
 		}
 		e.location = loc
@@ -259,7 +266,7 @@ func (e *Entity) GetLocation(ctx context.Context) (*geoip.Location, bool) {
 func (e *Entity) GetCountry(ctx context.Context) (string, bool) {
 	e.getLocation(ctx)
 
-	if e.Country == "" {
+	if e.LocationError != "" {
 		return "", false
 	}
 	return e.Country, true
@@ -269,13 +276,14 @@ func (e *Entity) GetCountry(ctx context.Context) (string, bool) {
 func (e *Entity) GetASN(ctx context.Context) (uint, bool) {
 	e.getLocation(ctx)
 
-	if e.ASN == 0 {
+	if e.LocationError != "" {
 		return 0, false
 	}
 	return e.ASN, true
 }
 
 // Lists
+
 func (e *Entity) getLists(ctx context.Context) {
 	e.getDomainLists(ctx)
 	e.getASNLists(ctx)
@@ -305,11 +313,11 @@ func (e *Entity) getDomainLists(ctx context.Context) {
 		return
 	}
 
-	var err error
+	log.Tracer(ctx).Tracef("intel: loading domain list for %s", domain)
 	e.loadDomainListOnce.Do(func() {
 		var domainsToInspect = []string{domain}
 
-		if e.checkCNAMEs {
+		if e.checkCNAMEs && len(e.CNAME) > 0 {
 			log.Tracer(ctx).Tracef("intel: CNAME filtering enabled, checking %v too", e.CNAME)
 			domainsToInspect = append(domainsToInspect, e.CNAME...)
 		}
@@ -327,11 +335,10 @@ func (e *Entity) getDomainLists(ctx context.Context) {
 		domains = makeDistinct(domains)
 
 		for _, d := range domains {
-			log.Tracer(ctx).Tracef("intel: loading domain list for %s", d)
-			var list []string
-			list, err = filterlists.LookupDomain(d)
+			list, err := filterlists.LookupDomain(d)
 			if err != nil {
 				log.Tracer(ctx).Errorf("intel: failed to get domain blocklists for %s: %s", d, err)
+				e.ListsError = err.Error()
 				return
 			}
 
@@ -339,10 +346,6 @@ func (e *Entity) getDomainLists(ctx context.Context) {
 		}
 		e.domainListLoaded = true
 	})
-
-	if err != nil {
-		e.loadDomainListOnce = sync.Once{}
-	}
 }
 
 func splitDomain(domain string) []string {
@@ -377,7 +380,7 @@ func (e *Entity) getASNLists(ctx context.Context) {
 	}
 
 	asn, ok := e.GetASN(ctx)
-	if !ok {
+	if !ok || asn == 0 {
 		return
 	}
 
@@ -387,7 +390,7 @@ func (e *Entity) getASNLists(ctx context.Context) {
 		list, err := filterlists.LookupASNString(asnStr)
 		if err != nil {
 			log.Tracer(ctx).Errorf("intel: failed to get ASN blocklist for %d: %s", asn, err)
-			e.loadAsnListOnce = sync.Once{}
+			e.ListsError = err.Error()
 			return
 		}
 
@@ -402,16 +405,16 @@ func (e *Entity) getCountryLists(ctx context.Context) {
 	}
 
 	country, ok := e.GetCountry(ctx)
-	if !ok {
+	if !ok || country == "" {
 		return
 	}
 
 	log.Tracer(ctx).Tracef("intel: loading country list for %s", country)
-	e.loadCoutryListOnce.Do(func() {
+	e.loadCountryListOnce.Do(func() {
 		list, err := filterlists.LookupCountry(country)
 		if err != nil {
 			log.Tracer(ctx).Errorf("intel: failed to load country blocklist for %s: %s", country, err)
-			e.loadCoutryListOnce = sync.Once{}
+			e.ListsError = err.Error()
 			return
 		}
 
@@ -426,11 +429,7 @@ func (e *Entity) getIPLists(ctx context.Context) {
 	}
 
 	ip, ok := e.GetIP()
-	if !ok {
-		return
-	}
-
-	if ip == nil {
+	if !ok || ip == nil {
 		return
 	}
 
@@ -442,12 +441,12 @@ func (e *Entity) getIPLists(ctx context.Context) {
 	log.Tracer(ctx).Tracef("intel: loading IP list for %s", ip)
 	e.loadIPListOnce.Do(func() {
 		list, err := filterlists.LookupIP(ip)
-
 		if err != nil {
 			log.Tracer(ctx).Errorf("intel: failed to get IP blocklist for %s: %s", ip.String(), err)
-			e.loadIPListOnce = sync.Once{}
+			e.ListsError = err.Error()
 			return
 		}
+
 		e.ipListLoaded = true
 		e.mergeList(ip.String(), list)
 	})

nameserver/nameserver.go

@@ -197,7 +197,7 @@ func handleRequest(ctx context.Context, w dns.ResponseWriter, request *dns.Msg)
 			return reply(nsutil.NxDomain("nxdomain: " + err.Error()))
 		case errors.Is(err, resolver.ErrBlocked):
 			tracer.Tracef("nameserver: %s", err)
-			return reply(nsutil.ZeroIP("blocked: " + err.Error()))
+			return reply(nsutil.BlockIP("blocked: " + err.Error()))
 		case errors.Is(err, resolver.ErrLocalhost):
 			tracer.Tracef("nameserver: returning localhost records")
 			return reply(nsutil.Localhost())

nameserver/nsutil/nsutil.go

@@ -45,50 +45,42 @@ func (rf ResponderFunc) ReplyWithDNS(ctx context.Context, request *dns.Msg) *dns
 	return rf(ctx, request)
 }
 
+// BlockIP is a ResponderFunc than replies with either 0.0.0.17 or ::17 for
+// each A or AAAA question respectively. If there is no A or AAAA question, it
+// defaults to replying with NXDomain.
+func BlockIP(msgs ...string) ResponderFunc {
+	return createResponderFunc(
+		"blocking",
+		"0.0.0.17",
+		"::17",
+		msgs...,
+	)
+}
+
 // ZeroIP is a ResponderFunc than replies with either 0.0.0.0 or :: for each A
 // or AAAA question respectively. If there is no A or AAAA question, it
 // defaults to replying with NXDomain.
 func ZeroIP(msgs ...string) ResponderFunc {
-	return func(ctx context.Context, request *dns.Msg) *dns.Msg {
-		reply := new(dns.Msg)
-		hasErr := false
-
-		for _, question := range request.Question {
-			var rr dns.RR
-			var err error
-
-			switch question.Qtype {
-			case dns.TypeA:
-				rr, err = dns.NewRR(question.Name + " 1 IN A 0.0.0.17")
-			case dns.TypeAAAA:
-				rr, err = dns.NewRR(question.Name + " 1 IN AAAA ::17")
-			}
-
-			switch {
-			case err != nil:
-				log.Tracer(ctx).Errorf("nameserver: failed to create zero-ip response for %s: %s", question.Name, err)
-				hasErr = true
-			case rr != nil:
-				reply.Answer = append(reply.Answer, rr)
-			}
-		}
-
-		switch {
-		case hasErr || len(reply.Answer) == 0:
-			reply.SetRcode(request, dns.RcodeServerFailure)
-		default:
-			reply.SetRcode(request, dns.RcodeSuccess)
-		}
-
-		AddMessagesToReply(ctx, reply, log.InfoLevel, msgs...)
-
-		return reply
-	}
+	return createResponderFunc(
+		"zero ip",
+		"0.0.0.0",
+		"::",
+		msgs...,
+	)
 }
 
 // Localhost is a ResponderFunc than replies with localhost IP addresses.
 // If there is no A or AAAA question, it defaults to replying with NXDomain.
 func Localhost(msgs ...string) ResponderFunc {
+	return createResponderFunc(
+		"localhost",
+		"127.0.0.1",
+		"::1",
+		msgs...,
+	)
+}
+
+func createResponderFunc(responderName, aAnswer, aaaaAnswer string, msgs ...string) ResponderFunc {
 	return func(ctx context.Context, request *dns.Msg) *dns.Msg {
 		reply := new(dns.Msg)
 		hasErr := false
@@ -99,14 +91,14 @@ func Localhost(msgs ...string) ResponderFunc {
 
 			switch question.Qtype {
 			case dns.TypeA:
-				rr, err = dns.NewRR("localhost. 1 IN A 127.0.0.1")
+				rr, err = dns.NewRR(question.Name + " 1 IN A " + aAnswer)
 			case dns.TypeAAAA:
-				rr, err = dns.NewRR("localhost. 1 IN AAAA ::1")
+				rr, err = dns.NewRR(question.Name + " 1 IN AAAA " + aaaaAnswer)
 			}
 
 			switch {
 			case err != nil:
-				log.Tracer(ctx).Errorf("nameserver: failed to create localhost response for %s: %s", question.Name, err)
+				log.Tracer(ctx).Errorf("nameserver: failed to create %s response for %s: %s", responderName, question.Name, err)
 				hasErr = true
 			case rr != nil:
 				reply.Answer = append(reply.Answer, rr)
@@ -114,8 +106,10 @@ func Localhost(msgs ...string) ResponderFunc {
 		}
 
 		switch {
-		case hasErr || len(reply.Answer) == 0:
+		case hasErr && len(reply.Answer) == 0:
 			reply.SetRcode(request, dns.RcodeServerFailure)
+		case len(reply.Answer) == 0:
+			reply.SetRcode(request, dns.RcodeNameError)
 		default:
 			reply.SetRcode(request, dns.RcodeSuccess)
 		}

network/dns.go

@@ -103,11 +103,11 @@ func (conn *Connection) ReplyWithDNS(ctx context.Context, request *dns.Msg) *dns
 	// Select request responder.
 	switch conn.Verdict {
 	case VerdictBlock:
-		return nsutil.ZeroIP().ReplyWithDNS(ctx, request)
+		return nsutil.BlockIP().ReplyWithDNS(ctx, request)
 	case VerdictDrop:
 		return nil // Do not respond to request.
 	case VerdictFailed:
-		return nsutil.ZeroIP().ReplyWithDNS(ctx, request)
+		return nsutil.BlockIP().ReplyWithDNS(ctx, request)
 	default:
 		reply := nsutil.ServerFailure().ReplyWithDNS(ctx, request)
 		nsutil.AddMessagesToReply(ctx, reply, log.ErrorLevel, "INTERNAL ERROR: incorrect use of Connection DNS Responder")

profile/config.go

@@ -182,6 +182,8 @@ func registerConfiguration() error {
 Additionally, you may supply a protocol and port just behind that using numbers ("6/80") or names ("TCP/HTTP").  
 In this case the rule is only matched if the protocol and port also match.  
 Example: "192.168.0.1 TCP/HTTP"
+
+Important: DNS Requests are only matched against domain and filter list rules, all others require an IP address and are checked only with the following IP connection.
 `, `"`, "`")
 
 	// Endpoint Filter List

profile/endpoints/endpoint-asn.go

@@ -22,9 +22,18 @@ type EndpointASN struct {
 
 // Matches checks whether the given entity matches this endpoint definition.
 func (ep *EndpointASN) Matches(ctx context.Context, entity *intel.Entity) (EPResult, Reason) {
+	if entity.IP == nil {
+		return NoMatch, nil
+	}
+
+	if !entity.IPScope.IsGlobal() {
+		return NoMatch, nil
+	}
+
 	asn, ok := entity.GetASN(ctx)
 	if !ok {
-		return Undeterminable, nil
+		asnStr := strconv.Itoa(int(ep.ASN))
+		return MatchError, ep.makeReason(ep, asnStr, "ASN data not available to match")
 	}
 
 	if asn == ep.ASN {

profile/endpoints/endpoint-country.go

@@ -21,9 +21,17 @@ type EndpointCountry struct {
 
 // Matches checks whether the given entity matches this endpoint definition.
 func (ep *EndpointCountry) Matches(ctx context.Context, entity *intel.Entity) (EPResult, Reason) {
+	if entity.IP == nil {
+		return NoMatch, nil
+	}
+
+	if !entity.IPScope.IsGlobal() {
+		return NoMatch, nil
+	}
+
 	country, ok := entity.GetCountry(ctx)
 	if !ok {
-		return Undeterminable, nil
+		return MatchError, ep.makeReason(ep, country, "country data not available to match")
 	}
 
 	if country == ep.Country {

profile/endpoints/endpoint-ip.go

@@ -17,7 +17,7 @@ type EndpointIP struct {
 // Matches checks whether the given entity matches this endpoint definition.
 func (ep *EndpointIP) Matches(_ context.Context, entity *intel.Entity) (EPResult, Reason) {
 	if entity.IP == nil {
-		return Undeterminable, nil
+		return NoMatch, nil
 	}
 
 	if ep.IP.Equal(entity.IP) {

profile/endpoints/endpoint-iprange.go

@@ -17,8 +17,9 @@ type EndpointIPRange struct {
 // Matches checks whether the given entity matches this endpoint definition.
 func (ep *EndpointIPRange) Matches(_ context.Context, entity *intel.Entity) (EPResult, Reason) {
 	if entity.IP == nil {
-		return Undeterminable, nil
+		return NoMatch, nil
 	}
+
 	if ep.Net.Contains(entity.IP) {
 		return ep.match(ep, entity, ep.Net.String(), "IP is in")
 	}

profile/endpoints/endpoint-scopes.go

@@ -33,7 +33,7 @@ type EndpointScope struct {
 // Matches checks whether the given entity matches this endpoint definition.
 func (ep *EndpointScope) Matches(_ context.Context, entity *intel.Entity) (EPResult, Reason) {
 	if entity.IP == nil {
-		return Undeterminable, nil
+		return NoMatch, nil
 	}
 
 	var scope uint8

profile/endpoints/endpoint.go

@@ -27,7 +27,7 @@ type EndpointBase struct { //nolint:maligned // TODO
 
 func (ep *EndpointBase) match(s fmt.Stringer, entity *intel.Entity, value, desc string, keyval ...interface{}) (EPResult, Reason) {
 	result := ep.matchesPPP(entity)
-	if result == Undeterminable || result == NoMatch {
+	if result == NoMatch {
 		return result, nil
 	}
 
@@ -57,10 +57,6 @@ func (ep *EndpointBase) makeReason(s fmt.Stringer, value, desc string, keyval ..
 func (ep *EndpointBase) matchesPPP(entity *intel.Entity) (result EPResult) {
 	// only check if protocol is defined
 	if ep.Protocol > 0 {
-		// if protocol is unknown, return Undeterminable
-		if entity.Protocol == 0 {
-			return Undeterminable
-		}
 		// if protocol does not match, return NoMatch
 		if entity.Protocol != ep.Protocol {
 			return NoMatch
@@ -69,10 +65,6 @@ func (ep *EndpointBase) matchesPPP(entity *intel.Entity) (result EPResult) {
 
 	// only check if port is defined
 	if ep.StartPort > 0 {
-		// if port is unknown, return Undeterminable
-		if entity.DstPort() == 0 {
-			return Undeterminable
-		}
 		// if port does not match, return NoMatch
 		if entity.DstPort() < ep.StartPort || entity.DstPort() > ep.EndPort {
 			return NoMatch

profile/endpoints/endpoints.go

@@ -17,7 +17,7 @@ type EPResult uint8
 // Endpoint matching return values
 const (
 	NoMatch EPResult = iota
-	Undeterminable
+	MatchError
 	Denied
 	Permitted
 )
@@ -25,7 +25,7 @@ const (
 // IsDecision returns true if result represents a decision
 // and false if result is NoMatch or Undeterminable.
 func IsDecision(result EPResult) bool {
-	return result == Denied || result == Permitted || result == Undeterminable
+	return result == Denied || result == Permitted || result == MatchError
 }
 
 // ParseEndpoints parses a list of endpoints and returns a list of Endpoints for matching.
@@ -88,8 +88,8 @@ func (epr EPResult) String() string {
 	switch epr {
 	case NoMatch:
 		return "No Match"
-	case Undeterminable:
-		return "Undeterminable"
+	case MatchError:
+		return "Match Error"
 	case Denied:
 		return "Denied"
 	case Permitted: