Merge pull request #327 from safing/fix/patch-set-4

Improve system resolver and ICMP handling

firewall/interception.go

@@ -9,6 +9,7 @@ import (
 	"sync/atomic"
 	"time"
 
+	"github.com/google/gopacket/layers"
 	"github.com/safing/portmaster/netenv"
 	"golang.org/x/sync/singleflight"
 
@@ -184,30 +185,39 @@ func fastTrackedPermit(pkt packet.Packet) (handled bool) {
 	}
 
 	switch meta.Protocol {
-	case packet.ICMP:
-		// Submit to ICMP listener.
-		submitted := netenv.SubmitPacketToICMPListener(pkt)
-
-		// 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 {
+	case packet.ICMP, packet.ICMPv6:
+		// Load packet data.
+		err := pkt.LoadPacketData()
+		if err != nil {
+			log.Debugf("filter: failed to load ICMP packet data: %s", err)
 			_ = pkt.PermanentAccept()
+			return true
 		}
-		return true
 
-	case packet.ICMPv6:
+		// Handle echo request and replies regularly.
+		// Other ICMP packets are considered system business.
+		icmpLayers := pkt.Layers().LayerClass(layers.LayerClassIPControl)
+		switch icmpLayer := icmpLayers.(type) {
+		case *layers.ICMPv4:
+			switch icmpLayer.TypeCode.Type() {
+			case layers.ICMPv4TypeEchoRequest,
+				layers.ICMPv4TypeEchoReply:
+				return false
+			}
+		case *layers.ICMPv6:
+			switch icmpLayer.TypeCode.Type() {
+			case layers.ICMPv6TypeEchoRequest,
+				layers.ICMPv6TypeEchoReply:
+				return false
+			}
+		}
+
+		// Premit all ICMP/v6 packets that are not echo requests or replies.
+		log.Debugf("filter: fast-track accepting ICMP/v6: %s", pkt)
+
 		// Submit to ICMP listener.
 		submitted := netenv.SubmitPacketToICMPListener(pkt)
 
-		// 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.

firewall/interception/nfq/packet.go

@@ -145,7 +145,7 @@ func (pkt *packet) PermanentAccept() error {
 }
 
 func (pkt *packet) PermanentBlock() error {
-	if pkt.Info().Protocol == pmpacket.ICMP {
+	if pkt.Info().Protocol == pmpacket.ICMP || pkt.Info().Protocol == pmpacket.ICMPv6 {
 		// ICMP packets attributed to a blocked connection are always allowed, as
 		// rejection ICMP packets will have the same mark as the blocked
 		// connection. This is why we need to drop blocked ICMP packets instead.

firewall/master.go

@@ -55,8 +55,10 @@ var defaultDeciders = []deciderFn{
 }
 
 var dnsFromSystemResolverDeciders = []deciderFn{
+	checkEndpointListsForSystemResolverDNSRequests,
 	checkConnectivityDomain,
 	checkBypassPrevention,
+	checkFilterLists,
 }
 
 // DecideOnConnection makes a decision about a connection.
@@ -214,6 +216,29 @@ func checkEndpointLists(ctx context.Context, conn *network.Connection, p *profil
 	return false
 }
 
+// checkEndpointListsForSystemResolverDNSRequests is a special version of
+// 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 {
+	profileEndpoints := p.LocalProfile().GetEndpoints()
+	if profileEndpoints.IsSet() {
+		result, reason := profileEndpoints.Match(ctx, conn.Entity)
+		if endpoints.IsDecision(result) {
+			switch result {
+			case endpoints.Denied:
+				conn.DenyWithContext(reason.String(), profile.CfgOptionEndpointsKey, reason.Context())
+				return true
+			case endpoints.Permitted:
+				conn.AcceptWithContext(reason.String(), profile.CfgOptionEndpointsKey, reason.Context())
+				return true
+			}
+		}
+	}
+
+	return false
+}
+
 func checkConnectionType(ctx context.Context, conn *network.Connection, p *profile.LayeredProfile, _ packet.Packet) bool {
 	switch {
 	case conn.Type != network.IPConnection:

intel/filterlists/module.go

@@ -107,6 +107,6 @@ func warnAboutDisabledFilterLists() {
 	module.Warning(
 		filterlistsDisabled,
 		"Filter Lists Are Initializing",
-		"Filter lists are being downloaded and set up in the background. Until this initialization is finished, the filter lists are disabled and will not block anything.",
+		"Filter lists are being downloaded and set up in the background. They will be activated as configured when finished.",
 	)
 }

process/find.go

@@ -27,7 +27,7 @@ func GetProcessByConnection(ctx context.Context, pktInfo *packet.Info) (process
 	var pid int
 	pid, connInbound, err = state.Lookup(pktInfo, fastSearch)
 	if err != nil {
-		log.Tracer(ctx).Debugf("process: failed to find PID of connection: %s", err)
+		log.Tracer(ctx).Tracef("process: failed to find PID of connection: %s", err)
 		return nil, pktInfo.Inbound, err
 	}
 

profile/endpoints/endpoint.go

@@ -37,7 +37,7 @@ func (ep *EndpointBase) match(s fmt.Stringer, entity *intel.Entity, value, desc
 func (ep *EndpointBase) makeReason(s fmt.Stringer, value, desc string, keyval ...interface{}) Reason {
 	r := &reason{
 		description: desc,
-		Filter:      ep.renderPPP(s.String()),
+		Filter:      s.String(),
 		Permitted:   ep.Permitted,
 		Value:       value,
 	}

profile/profile.go

@@ -301,6 +301,22 @@ func (profile *Profile) IsOutdated() bool {
 	return profile.outdated.IsSet()
 }
 
+// GetEndpoints returns the endpoint list of the profile.
+func (profile *Profile) GetEndpoints() endpoints.Endpoints {
+	profile.RLock()
+	defer profile.RUnlock()
+
+	return profile.endpoints
+}
+
+// GetServiceEndpoints returns the service endpoint list of the profile.
+func (profile *Profile) GetServiceEndpoints() endpoints.Endpoints {
+	profile.RLock()
+	defer profile.RUnlock()
+
+	return profile.serviceEndpoints
+}
+
 // AddEndpoint adds an endpoint to the endpoint list, saves the profile and reloads the configuration.
 func (profile *Profile) AddEndpoint(newEntry string) {
 	profile.addEndpointyEntry(CfgOptionEndpointsKey, newEntry)

profile/special.go

@@ -85,19 +85,45 @@ func getSpecialProfile(profileID, linkedPath string) *Profile {
 		return New(SourceLocal, SystemProfileID, linkedPath, nil)
 
 	case SystemResolverProfileID:
-		return New(
+		systemResolverProfile := New(
 			SourceLocal,
 			SystemResolverProfileID,
 			linkedPath,
 			map[string]interface{}{
+				// Explicitly setting the default action to "permit" will improve the
+				// user experience for people who set the global default to "prompt".
+				// Resolved domain from the system resolver are checked again when
+				// attributed to a connection of a regular process. Otherwise, users
+				// would see two connection prompts for the same domain.
+				CfgOptionDefaultActionKey: "permit",
+				// Explicitly allow localhost and answers to multicast protocols that
+				// are commonly used by system resolvers.
+				// TODO: When the Portmaster gains the ability to attribute multicast
+				// responses to their requests, these rules can probably be removed
+				// again.
 				CfgOptionServiceEndpointsKey: []string{
 					"+ Localhost",    // Allow everything from localhost.
 					"+ LAN UDP/5353", // Allow inbound mDNS requests and multicast replies.
 					"+ LAN UDP/5355", // Allow inbound LLMNR requests and multicast replies.
 					"+ LAN UDP/1900", // Allow inbound SSDP requests and multicast replies.
 				},
+				// Explicitly disable all filter lists, as these will be checked later
+				// with the attributed connection. As this is the system resolver, this
+				// list can instead be used as a global enforcement of filter lists, if
+				// the system resolver is used. Users who want to
+				CfgOptionFilterListsKey: []string{},
 			},
 		)
+		// Add description to tell users about the quirks of this profile.
+		systemResolverProfile.Description = `The System DNS Client is a system service that requires special handling. For regular network connections, the configured settings will apply as usual, but DNS requests coming from the System DNS Client are handled in a special way, as they could actually be coming from any other application on the system.
+		
+In order to respect the app settings of the actual application, DNS requests from the System DNS Client are only subject to the following settings:
+
+- Outgoing Rules (without global rules)
+- Block Bypassing
+- Filter Lists
+`
+		return systemResolverProfile
 
 	case PortmasterProfileID:
 		profile := New(SourceLocal, PortmasterProfileID, linkedPath, nil)
@@ -155,7 +181,7 @@ func specialProfileNeedsReset(profile *Profile) bool {
 
 	switch profile.ID {
 	case SystemResolverProfileID:
-		return canBeUpgraded(profile, "18.5.2021")
+		return canBeUpgraded(profile, "1.6.2021")
 	default:
 		// Not a special profile or no upgrade available yet.
 		return false
@@ -175,7 +201,7 @@ func canBeUpgraded(profile *Profile, upgradeDate string) bool {
 		log.Infof("profile: upgrading special profile %s", profile.ScopedID())
 
 		notifications.NotifyInfo(
-			"profiles:upgraded-special-profile-"+profile.ID,
+			"profiles:upgraded-special-profile:"+profile.ID,
 			profile.Name+" Settings Upgraded",
 			// TODO: Remove disclaimer.
 			fmt.Sprintf(