Merge pull request #586 from safing/fix/reduce-compat-false-positive

Reduce compat false positivies

compat/module.go

@@ -28,6 +28,10 @@ var (
 	selfcheckFails int
 )
 
+// selfcheckFailThreshold holds the threshold of how many times the selfcheck
+// must fail before it is reported.
+const selfcheckFailThreshold = 5
+
 func init() {
 	module = modules.Register("compat", prep, start, stop, "base", "network", "interception", "netenv", "notifications")
 
@@ -48,6 +52,9 @@ func start() error {
 		MaxDelay(selfcheckTaskRetryAfter).
 		Schedule(time.Now().Add(selfcheckTaskRetryAfter))
 
+	module.NewTask("clean notify thresholds", cleanNotifyThreshold).
+		Repeat(10 * time.Minute)
+
 	return module.RegisterEventHook(
 		netenv.ModuleName,
 		netenv.NetworkChangedEvent,
@@ -82,7 +89,7 @@ func selfcheckTaskFunc(ctx context.Context, task *modules.Task) error {
 		selfcheckFails++
 
 		log.Errorf("compat: %s", err)
-		if selfcheckFails >= 3 {
+		if selfcheckFails >= selfcheckFailThreshold {
 			issue.notify(err)
 		}
 

compat/notify.go

@@ -8,6 +8,7 @@ import (
 	"time"
 
 	"github.com/safing/portbase/log"
+	"github.com/safing/portbase/modules"
 	"github.com/safing/portbase/notifications"
 	"github.com/safing/portmaster/process"
 	"github.com/safing/portmaster/profile"
@@ -42,10 +43,9 @@ var (
 	}
 
 	secureDNSBypassIssue = &appIssue{
-		id:    "compat:secure-dns-bypass-%s",
+		id:      "compat:secure-dns-bypass-%s",
-		title: "Detected %s Bypass Attempt",
+		title:   "Blocked Bypass Attempt by %s",
-		message: `[APPNAME] is bypassing Portmaster's firewall functions through its Secure DNS resolver. Portmaster can no longer protect or filter connections coming from [APPNAME]. Disable Secure DNS within [APPNAME] to restore functionality.  
+		message: `[APPNAME] is using its own Secure DNS resolver, which would bypass Portmaster's firewall protections. If [APPNAME] experiences problems, disable Secure DNS within [APPNAME] to restore functionality. Rest assured that Portmaster handles Secure DNS for your whole device, including [APPNAME].`,
-Rest assured that Portmaster already handles Secure DNS for your whole device.`,
 		// TODO: Add this when the new docs page is finished:
 		// , or [find out about other options](link to new docs page)
 		level: notifications.Warning,
@@ -124,9 +124,6 @@ func (issue *appIssue) notify(proc *process.Process) {
 		proc.Path,
 	)
 
-	// Build message.
-	message := strings.ReplaceAll(issue.message, "[APPNAME]", p.Name)
-
 	// Check if we already have this notification.
 	eventID := fmt.Sprintf(issue.id, p.ID)
 	n := notifications.Get(eventID)
@@ -134,7 +131,15 @@ func (issue *appIssue) notify(proc *process.Process) {
 		return
 	}
 
-	// Otherwise, create a new one.
+	// Check if we reach the threshold to actually send a notification.
+	if !isOverThreshold(eventID) {
+		return
+	}
+
+	// Build message.
+	message := strings.ReplaceAll(issue.message, "[APPNAME]", p.Name)
+
+	// Create a new notification.
 	n = &notifications.Notification{
 		EventID:      eventID,
 		Type:         issue.level,
@@ -171,3 +176,54 @@ func (issue *appIssue) notify(proc *process.Process) {
 		return nil
 	})
 }
+
+const (
+	notifyThresholdMinIncidents = 11
+	notifyThresholdResetAfter   = 2 * time.Minute
+)
+
+var (
+	notifyThresholds     = make(map[string]*notifyThreshold)
+	notifyThresholdsLock sync.Mutex
+)
+
+type notifyThreshold struct {
+	FirstSeen time.Time
+	Incidents uint
+}
+
+func (nt *notifyThreshold) expired() bool {
+	return time.Now().Add(-notifyThresholdResetAfter).After(nt.FirstSeen)
+}
+
+func isOverThreshold(id string) bool {
+	notifyThresholdsLock.Lock()
+	defer notifyThresholdsLock.Unlock()
+
+	// Get notify threshold and check if we reach the minimum incidents.
+	nt, ok := notifyThresholds[id]
+	if ok && !nt.expired() {
+		nt.Incidents++
+		return nt.Incidents >= notifyThresholdMinIncidents
+	}
+
+	// Add new entry.
+	notifyThresholds[id] = &notifyThreshold{
+		FirstSeen: time.Now(),
+		Incidents: 1,
+	}
+	return false
+}
+
+func cleanNotifyThreshold(ctx context.Context, task *modules.Task) error {
+	notifyThresholdsLock.Lock()
+	defer notifyThresholdsLock.Unlock()
+
+	for id, nt := range notifyThresholds {
+		if nt.expired() {
+			delete(notifyThresholds, id)
+		}
+	}
+
+	return nil
+}

compat/selfcheck.go

@@ -28,12 +28,12 @@ var (
 	systemIntegrationCheckDialNet      = fmt.Sprintf("ip4:%d", uint8(SystemIntegrationCheckProtocol))
 	systemIntegrationCheckDialIP       = SystemIntegrationCheckDstIP.String()
 	systemIntegrationCheckPackets      = make(chan packet.Packet, 1)
-	systemIntegrationCheckWaitDuration = 10 * time.Second
+	systemIntegrationCheckWaitDuration = 30 * time.Second
 
 	// DNSCheckInternalDomainScope is the domain scope to use for dns checks.
 	DNSCheckInternalDomainScope = ".self-check." + resolver.InternalSpecialUseDomain
 	dnsCheckReceivedDomain      = make(chan string, 1)
-	dnsCheckWaitDuration        = 10 * time.Second
+	dnsCheckWaitDuration        = 30 * time.Second
 	dnsCheckAnswerLock          sync.Mutex
 	dnsCheckAnswer              net.IP
 )

network/connection.go

@@ -580,10 +580,7 @@ func (conn *Connection) SetFirewallHandler(handler FirewallHandler) {
 		conn.pktQueue = make(chan packet.Packet, 1000)
 
 		// start handling
-		module.StartWorker("packet handler", func(ctx context.Context) error {
+		module.StartWorker("packet handler", conn.packetHandlerWorker)
-			conn.packetHandler()
-			return nil
-		})
 	}
 	conn.firewallHandler = handler
 }
@@ -608,35 +605,46 @@ func (conn *Connection) HandlePacket(pkt packet.Packet) {
 	}
 }
 
-// packetHandler sequentially handles queued packets.
+// packetHandlerWorker sequentially handles queued packets.
-func (conn *Connection) packetHandler() {
+func (conn *Connection) packetHandlerWorker(ctx context.Context) error {
-	for pkt := range conn.pktQueue {
+	for {
-		if pkt == nil {
+		select {
-			return
+		case pkt := <-conn.pktQueue:
+			if pkt == nil {
+				return nil
+			}
+			packetHandlerHandleConn(conn, pkt)
+
+		case <-ctx.Done():
+			conn.Lock()
+			defer conn.Unlock()
+			conn.firewallHandler = nil
+			return nil
 		}
-		// get handler
+	}
-		conn.Lock()
+}
 
-		// execute handler or verdict
+func packetHandlerHandleConn(conn *Connection, pkt packet.Packet) {
-		if conn.firewallHandler != nil {
+	conn.Lock()
-			conn.firewallHandler(conn, pkt)
+	defer conn.Unlock()
-		} else {
-			defaultFirewallHandler(conn, pkt)
-		}
 
-		// log verdict
+	// Handle packet with appropriate handler.
-		log.Tracer(pkt.Ctx()).Infof("filter: connection %s %s: %s", conn, conn.Verdict.Verb(), conn.Reason.Msg)
+	if conn.firewallHandler != nil {
-		// submit trace logs
+		conn.firewallHandler(conn, pkt)
-		log.Tracer(pkt.Ctx()).Submit()
+	} else {
+		defaultFirewallHandler(conn, pkt)
+	}
 
-		// save does not touch any changing data
+	// Log verdict.
-		// must not be locked, will deadlock with cleaner functions
+	log.Tracer(pkt.Ctx()).Infof("filter: connection %s %s: %s", conn, conn.Verdict.Verb(), conn.Reason.Msg)
-		if conn.saveWhenFinished {
+	// Submit trace logs.
-			conn.saveWhenFinished = false
+	log.Tracer(pkt.Ctx()).Submit()
-			conn.Save()
-		}
 
-		conn.Unlock()
+	// Save() itself does not touch any changing data.
+	// Must not be locked - would deadlock with cleaner functions.
+	if conn.saveWhenFinished {
+		conn.saveWhenFinished = false
+		conn.Save()
 	}
 }
 

profile/config.go

@@ -575,7 +575,9 @@ The lists are automatically updated every hour using incremental updates.
 	err = config.Register(&config.Option{
 		Name: "Block Bypassing",
 		Key:  CfgOptionPreventBypassingKey,
-		Description: `Prevent apps from bypassing the privacy filter.  
+		Description: `Prevent apps from bypassing Portmaster's privacy protections.  
+If Block Bypassing is disabled, Portmaster can no longer protect you or filter connections from the affected applications.
+
 Current Features:  
 - Disable Firefox' internal DNS-over-HTTPs resolver
 - Block direct access to public DNS resolvers