Implement review suggestions

firewall/interception.go

@@ -144,6 +144,10 @@ func fastTrackedPermit(pkt packet.Packet) (handled bool) {
 
 		// Always permit ICMP.
 		log.Debugf("filter: fast-track accepting ICMP: %s", pkt)
+
+		// If the packet was submitted to the listener, we must not do a
+		// permanent accept, because then we won't see any future packets of that
+		// connection and thus cannot continue to submit them.
 		if submitted {
 			_ = pkt.Accept()
 		} else {
@@ -157,6 +161,10 @@ func fastTrackedPermit(pkt packet.Packet) (handled bool) {
 
 		// Always permit ICMPv6.
 		log.Debugf("filter: fast-track accepting ICMPv6: %s", pkt)
+
+		// If the packet was submitted to the listener, we must not do a
+		// permanent accept, because then we won't see any future packets of that
+		// connection and thus cannot continue to submit them.
 		if submitted {
 			_ = pkt.Accept()
 		} else {

netenv/icmp_listener.go

@@ -9,12 +9,25 @@ import (
 	"github.com/safing/portmaster/network/packet"
 )
 
+/*
+This ICMP listening system is a simple system for components to listen to ICMP
+packets via the firewall.
+
+The main use case for this is to receive ICMP packets that are not always
+delivered correctly, or need special permissions and or sockets to receive
+them. This is the case when doing a traceroute.
+
+In order to keep it simple, the system is only designed to be used by one
+"user" at at time. Further calls to ListenToICMP will wait for the previous
+operation to complete.
+*/
+
 var (
 	// listenICMPLock locks the ICMP listening system for one user at a time.
 	listenICMPLock sync.Mutex
 
-	// listenICMPEnabled defines whether or not the firewall should send ICMP
-	// packets through this interface.
+	// listenICMPEnabled defines whether or not the firewall should submit ICMP
+	// packets to this interface.
 	listenICMPEnabled = abool.New()
 
 	// listenICMPInput is created for every use of the ICMP listenting system.
@@ -22,28 +35,35 @@ var (
 	listenICMPInputLock sync.Mutex
 )
 
+// ListenToICMP returns a new channel for listenting to icmp packets. Please
+// note that any icmp packet will be passed and filtering must be done on
+// the side of the caller. The caller must call the returned done function when
+// done with the listener.
 func ListenToICMP() (packets chan packet.Packet, done func()) {
 	// Lock for single use.
 	listenICMPLock.Lock()
 
 	// Create new input channel.
 	listenICMPInputLock.Lock()
-	listenICMPInput = make(chan packet.Packet, 10)
+	listenICMPInput = make(chan packet.Packet, 100)
 	listenICMPEnabled.Set()
 	listenICMPInputLock.Unlock()
 
 	return listenICMPInput, func() {
+		// Release for someone else to use.
+		defer listenICMPLock.Unlock()
+
 		// Close input channel.
 		listenICMPInputLock.Lock()
 		listenICMPEnabled.UnSet()
 		close(listenICMPInput)
 		listenICMPInputLock.Unlock()
-
-		// Release for someone else to use.
-		listenICMPLock.Unlock()
 	}
 }
 
+// SubmitPacketToICMPListener checks if an ICMP packet should be submitted to
+// the listener. If so, it is submitted right away. The function returns
+// whether or not the packet should be submitted, not if it was successful.
 func SubmitPacketToICMPListener(pkt packet.Packet) (submitted bool) {
 	// Hot path.
 	if !listenICMPEnabled.IsSet() {
@@ -71,8 +91,6 @@ func submitPacketToICMPListenerSlow(pkt packet.Packet) {
 		return
 	}
 
-	log.Criticalf("netenv: recvd ICMP packet: %s", pkt)
-
 	// Send to channel, if possible.
 	select {
 	case listenICMPInput <- pkt:

netenv/location.go

@@ -66,6 +66,24 @@ type DeviceLocation struct {
 	SourceAccuracy int
 }
 
+// IsMoreAccurateThan checks if the device location is more accurate than the
+// given one.
+func (dl *DeviceLocation) IsMoreAccurateThan(other *DeviceLocation) bool {
+	switch {
+	case dl.SourceAccuracy > other.SourceAccuracy:
+		// Higher accuracy is better.
+		return true
+	case dl.ASN != 0 && other.ASN == 0:
+		// Having an ASN is better than having none.
+		return true
+	case dl.Country == "" && other.Country != "":
+		// Having a Country is better than having none.
+		return true
+	}
+
+	return false
+}
+
 type DeviceLocationSource string
 
 const (
@@ -106,28 +124,6 @@ func SetInternetLocation(ip net.IP, source DeviceLocationSource) (ok bool) {
 		SourceAccuracy: source.Accuracy(),
 	}
 
-	locationsLock.Lock()
-	defer locationsLock.Unlock()
-
-	// Add to locations, if better.
-	key := loc.IP.String()
-	existing, ok := locations.All[key]
-	if ok && existing.SourceAccuracy > loc.SourceAccuracy {
-		// Existing entry is better.
-		// Return true, because the IP address is part of the locations.
-		return true
-	}
-	locations.All[key] = loc
-
-	// Find best location.
-	var best *DeviceLocation
-	for _, dl := range locations.All {
-		if best == nil || dl.SourceAccuracy > best.SourceAccuracy {
-			best = dl
-		}
-	}
-	locations.Best = best
-
 	// Get geoip information, but continue if it fails.
 	geoLoc, err := geoip.GetLocation(ip)
 	if err != nil {
@@ -139,6 +135,28 @@ func SetInternetLocation(ip net.IP, source DeviceLocationSource) (ok bool) {
 		loc.ASOrg = geoLoc.AutonomousSystemOrganization
 	}
 
+	locationsLock.Lock()
+	defer locationsLock.Unlock()
+
+	// Add to locations, if better.
+	key := loc.IP.String()
+	existing, ok := locations.All[key]
+	if ok && existing.IsMoreAccurateThan(loc) {
+		// Existing entry is more accurate, abort adding.
+		// Return true, because the IP address is already part of the locations.
+		return true
+	}
+	locations.All[key] = loc
+
+	// Find best location.
+	best := loc
+	for _, dl := range locations.All {
+		if dl.IsMoreAccurateThan(best) {
+			best = dl
+		}
+	}
+	locations.Best = best
+
 	return true
 }
 

netenv/network-change.go

@@ -14,15 +14,15 @@ import (
 
 var (
 	networkChangeCheckTrigger   = make(chan struct{}, 1)
-	networkChangedFlagController = utils.NewFlagController()
+	networkChangedBroadcastFlag = utils.NewBroadcastFlag()
 )
 
 func GetNetworkChangedFlag() *utils.Flag {
-	return networkChangedFlagController.NewFlag()
+	return networkChangedBroadcastFlag.NewFlag()
 }
 
 func notifyOfNetworkChange() {
-	networkChangedFlagController.NotifyAndReset()
+	networkChangedBroadcastFlag.NotifyAndReset()
 	module.TriggerEvent(NetworkChangedEvent, nil)
 }
 

network/packet/parse.go

@@ -144,8 +144,8 @@ func Parse(packetData []byte, pktBase *Base) (err error) {
 	}
 
 	pktBase.layers = packet
-	if packet.TransportLayer() != nil {
-		pktBase.layer5Data = packet.TransportLayer().LayerPayload()
+	if transport := packet.TransportLayer(); transport != nil {
+		pktBase.layer5Data = transport.LayerPayload()
 	}
 	return nil
 }

process/profile.go

@@ -58,8 +58,13 @@ func (p *Process) GetProfile(ctx context.Context) (changed bool, err error) {
 		// Check if this is the system resolver.
 		switch runtime.GOOS {
 		case "windows":
-			if (p.Path == `C:\Windows\System32\svchost.exe` || p.Path == `C:\Windows\system32\svchost.exe`) &&
-				(strings.Contains(p.SpecialDetail, "Dnscache") || strings.Contains(p.CmdLine, "-s Dnscache")) {
+			// Depending on the OS version System32 may be capitalized or not.
+			if (p.Path == `C:\Windows\System32\svchost.exe` ||
+				p.Path == `C:\Windows\system32\svchost.exe`) &&
+				// This comes from the windows tasklist command and should be pretty consistent.
+				(strings.Contains(p.SpecialDetail, "Dnscache") ||
+					// As an alternative in case of failure, we try to match the svchost.exe service parameter.
+					strings.Contains(p.CmdLine, "-s Dnscache")) {
 				profileID = profile.SystemResolverProfileID
 			}
 		case "linux":