Adress requested changes

firewall/inspection/portscan/detect.go

@@ -2,7 +2,9 @@ package portscan
 
 import (
 	"context"
-	"net"
+	"fmt"
+	"strconv"
+	"strings"
 	"sync"
 	"time"
 
@@ -11,7 +13,6 @@ import (
 	"github.com/safing/portmaster/firewall/inspection"
 	"github.com/safing/portmaster/netenv"
 	"github.com/safing/portmaster/network"
-	"github.com/safing/portmaster/network/netutils"
 	"github.com/safing/portmaster/network/packet"
 	"github.com/safing/portmaster/process"
 	"github.com/safing/portmaster/status"
@@ -23,7 +24,7 @@ type tcpUDPport struct {
 }
 
 type ipData struct {
-	score int //score needs to be big enough to keep maxScore + addScore... to prevent overflow
+	score int // score needs to be big enough to keep maxScore + addScore... to prevent overflow
 	//	greylistingWorked bool
 	previousOffender bool
 	blocked          bool
@@ -34,18 +35,16 @@ type ipData struct {
 }
 
 const (
-	//fixme: Which production-values do we want to have?
-	cleanUpInterval = 1 * time.Minute //fixme: Debug-Value
+	cleanUpInterval = 5 * time.Minute
 	cleanUpMaxDelay = 5 * time.Minute
 
-	startAfter            = 1 * time.Second //fixme: Debug Value; When should the Portscan Detection start to prevent blocking Apps that just try to reconnect?
 	decreaseInterval      = 11 * time.Second
 	unblockIdleTime       = 1 * time.Hour
 	undoSuspicionIdleTime = 24 * time.Hour
 	unignoreTime          = 24 * time.Hour
 
-	startRegisteredPorts = 1024
-	startDynamicPorts    = 32768
+	registeredPortsStart = 1024
+	dynamicPortsStart    = 32768
 
 	addScoreWellKnownPort  = 40
 	addScoreRegisteredPort = 20
@@ -54,15 +53,14 @@ const (
 	scoreBlock = 160
 	maxScore   = 320
 
-	threadPrefix = "portscan: "
+	threatIDPrefix = "portscan:"
 )
 
 var (
-	ips    map[string]*ipData
-	ownIPs []net.IP
+	ips map[string]*ipData
 
-	module     *modules.Module
-	runOnlyOne sync.Mutex
+	module        *modules.Module
+	detectorMutex sync.Mutex
 )
 
 // Detector detects if a connection is part of a portscan which already sent some packets.
@@ -74,30 +72,26 @@ func (d *Detector) Name() string {
 }
 
 // Inspect implements the inspection interface.
-func (d *Detector) Inspect(conn *network.Connection, pkt packet.Packet) (pktVerdict network.Verdict, proceed bool, err error) {
-	runOnlyOne.Lock()
-	defer runOnlyOne.Unlock()
+func (d *Detector) Inspect(conn *network.Connection, pkt packet.Packet) (network.Verdict, bool, error) {
+	detectorMutex.Lock()
+	defer detectorMutex.Unlock()
 
 	ctx := pkt.Ctx()
 
-	//Delete for production. This just reduces the amount of Debug Messages significantly
-	//	if conn.LocalIP.Equal(net.IP([]byte{255, 255, 255, 255})) {
-	//		return network.VerdictUndecided, false, nil
-	//	}
-	log.Tracer(ctx).Debugf("new connection for Portscan detection")
+	log.Tracer(ctx).Debugf("portscan-detection: new connection")
 
-	rIP, ok := conn.Entity.GetIP() //remote IP
-	if !ok {                       //No IP => return undecided
+	rIP, ok := conn.Entity.GetIP() // remote IP
+	if !ok {                       // No IP => return undecided
 		return network.VerdictUndecided, false, nil
 	}
 
-	ipString := rIP.String()
+	ipString := conn.LocalIP.String() + "-" + rIP.String() //localip-remoteip
 	entry, inMap := ips[ipString]
 
-	log.Tracer(ctx).Debugf("Conn: %v, Entity: %#v, Protocol: %v, LocalIP: %s, LocalPort: %d, inMap: %v, entry: %+v", conn, conn.Entity, conn.IPProtocol, conn.LocalIP.String(), conn.LocalPort, inMap, entry)
+	log.Tracer(ctx).Debugf("portscan-detection: Conn: %s, remotePort: %d, IP: %s, Protocol: %s, LocalIP: %s, LocalPort: %d, inMap: %t, entry: %s", conn, conn.Entity.Port, conn.Entity.IP, conn.IPProtocol, conn.LocalIP, conn.LocalPort, inMap, entry)
 
 	if inMap {
-		inMap = entry.updateScoreIgnoreBlockPrevOffender(ipString)
+		inMap = entry.updateIPstate(ipString) // needs to be run before updating lastSeen (lastUpdated is updated within)
 	}
 
 	if inMap {
@@ -108,129 +102,130 @@ func (d *Detector) Inspect(conn *network.Connection, pkt packet.Packet) (pktVerd
 		}
 	}
 
-	ipClass := netutils.ClassifyIP(conn.LocalIP)
 	proc := conn.Process()
+	myip, _ := netenv.IsMyIP(conn.LocalIP)
 
-	log.Tracer(ctx).Debugf("PID: %+v", proc)
-
-	//malicious Packet?
-	if (proc == nil || proc.Pid == process.UnidentifiedProcessID) && //Port unused
-		conn.Inbound &&
-		(conn.IPProtocol == packet.TCP || conn.IPProtocol == packet.UDP) &&
-		!foreignIPv4(conn.LocalIP) &&
-		(ipClass == netutils.LinkLocal ||
-			ipClass == netutils.SiteLocal ||
-			ipClass == netutils.Invalid ||
-			ipClass == netutils.Global) &&
-		!isNetBIOSoverTCPIP(conn) &&
-		!(conn.IPProtocol == packet.UDP && (conn.LocalPort == 67 || conn.LocalPort == 68)) { // DHCP
-
+	// malicious Packet? This if checks all conditions for a malicious packet
+	switch {
+	case proc != nil && proc.Pid != process.UnidentifiedProcessID:
+		//We don't handle connections to running apps
+	case !conn.Inbound:
+		//We don't handle outbound connections
+	case !(conn.IPProtocol == packet.TCP || conn.IPProtocol == packet.UDP):
+		//We only handle TCP and UDP
+	case !myip:
+		//we only handle connections to our own IP
+	case isNetBIOSoverTCPIP(conn):
+		//we currently ignore NetBIOS
+	case (conn.IPProtocol == packet.UDP && (conn.LocalPort == 67 || conn.LocalPort == 68)):
+		//we ignore DHCP
+	default:
+		//We count this packet as a malicious packet
 		handleMaliciousPacket(ctx, inMap, conn, entry, ipString)
 	}
 
 	if inMap && entry.blocked {
-		log.Tracer(ctx).Debugf("blocking")
+		log.Tracer(ctx).Debugf("portscan-detection: blocking")
 		conn.SetVerdict(network.VerdictDrop, "Portscan", nil)
 	} else {
-		log.Tracer(ctx).Debugf("let through")
+		log.Tracer(ctx).Debugf("portscan-detection: let through")
 	}
 
-	return network.VerdictUndecided, false, nil
+	return network.VerdictUndecided, false, nil // If dropped, the whole connection is already dropped by conn.SetVerdict above
 }
 
 func handleMaliciousPacket(ctx context.Context, inMap bool, conn *network.Connection, entry *ipData, ipString string) {
-	//define Portscore
+	// define Portscore
 	var addScore int
 	switch {
-	case conn.LocalPort < startRegisteredPorts:
+	case conn.LocalPort < registeredPortsStart:
 		addScore = addScoreWellKnownPort
-	case conn.LocalPort < startDynamicPorts:
+	case conn.LocalPort < dynamicPortsStart:
 		addScore = addScoreRegisteredPort
 	default:
 		addScore = addScoreDynamicPort
 	}
 
+	port := tcpUDPport{protocol: conn.IPProtocol, port: conn.LocalPort}
+
 	if !inMap {
-		//new IP => add to List
+		// new IP => add to List
 		ips[ipString] = &ipData{
-			score: addScore,
-			blockedPorts: []tcpUDPport{
-				tcpUDPport{
-					protocol: conn.IPProtocol,
-					port:     conn.LocalPort,
-				},
-			},
-			lastSeen:    time.Now(),
-			lastUpdated: time.Now(),
+			score:        addScore,
+			blockedPorts: []tcpUDPport{port},
+			lastSeen:     time.Now(),
+			lastUpdated:  time.Now(),
 		}
-		log.Tracer(ctx).Debugf("New Entry: %+v", ips[ipString])
-	} else {
-		//Port in list of tried ports?
-		triedPort := false
-		port := tcpUDPport{protocol: conn.IPProtocol, port: conn.LocalPort}
-		for _, e := range entry.blockedPorts {
-			if e == port {
-				triedPort = true
-				break
-			}
-		}
-
-		if !triedPort {
-			entry.blockedPorts = append(entry.blockedPorts, tcpUDPport{protocol: conn.IPProtocol, port: conn.LocalPort})
-			entry.score = intMin(entry.score+addScore, maxScore)
-
-			if entry.previousOffender || entry.score >= scoreBlock {
-				entry.blocked = true
-				entry.previousOffender = true
-
-				//TODO: actually I just want to know if THIS threat exists - I don't need prefixing. Maybe we can do it simpler ... (less CPU-intensive)
-				if t, _ := status.GetThreats(threadPrefix + ipString); len(t) == 0 {
-					log.Tracer(ctx).Debugf("new Threat")
-					status.AddOrUpdateThreat(&status.Threat{
-						ID:              threadPrefix + ipString,
-						Name:            "Portscan by " + ipString,
-						Description:     "Someone tries to connect to a lot of closed Ports (non-running Services). Probably he wants to find out the services running on the maschine to determine which services to attack", //fixme: to long
-						MitigationLevel: status.SecurityLevelHigh,
-						Started:         time.Now().Unix(),
-					})
-				}
-			}
-		}
-
-		log.Tracer(ctx).Debugf("changed Entry: %+v", entry)
+		log.Tracer(ctx).Debugf("portscan-detection: New Entry: %s", ips[ipString])
+		return
 	}
+
+	// the Port in list of tried ports - otherwise it would have already returned
+	triedPort := false
+	for _, e := range entry.blockedPorts {
+		if e == port {
+			triedPort = true
+			break
+		}
+	}
+
+	if !triedPort {
+		entry.blockedPorts = append(entry.blockedPorts, port)
+		entry.score = intMin(entry.score+addScore, maxScore)
+
+		if entry.previousOffender || entry.score >= scoreBlock {
+			entry.blocked = true
+			entry.previousOffender = true
+
+			// FIXME: actually I just want to know if THIS threat exists - I don't need prefixing. Maybe we can do it simpler ... (less CPU-intensive)
+			if t, _ := status.GetThreats(threatIDPrefix + ipString); len(t) == 0 {
+				log.Tracer(ctx).Infof("portscan-detection: new Threat %s", extractRemoteFromIPString(ipString))
+				status.AddOrUpdateThreat(&status.Threat{
+					ID:              threatIDPrefix + ipString,
+					Name:            "Detected portscan from " + extractRemoteFromIPString(ipString),
+					Description:     "The device with the IP address " + extractRemoteFromIPString(ipString) + " is scanning network ports on your device.",
+					MitigationLevel: status.SecurityLevelHigh,
+					Started:         time.Now().Unix(),
+				})
+			}
+		}
+	}
+
+	log.Tracer(ctx).Debugf("portscan-detection: changed Entry: %s", entry)
 }
 
-//updateScoreIgnoreBlockPrevOffender updates this 4 Values of the Struct
-//ipString needs to correspond to the key of the entry in the map ips
-//WARNING: This function maybe deletes the entry ipString from the Map ips. (look at the returncode)
-//return: still in map? (bool)
-func (d *ipData) updateScoreIgnoreBlockPrevOffender(ipString string) bool {
-	d.score -= intMin(int(time.Since(d.lastUpdated)/decreaseInterval), d.score)
+// updateIPstate updates this 4 Values of the Struct
+// ipString needs to correspond to the key of the entry in the map ips
+// needs to be run before updating lastSeen (lastUpdated is updated within)
+// WARNING: This function maybe deletes the entry ipString from the Map ips. (look at the returncode)
+// return: still in map? (bool)
+func (ip *ipData) updateIPstate(ipString string) bool {
+	ip.score -= intMin(int(time.Since(ip.lastUpdated)/decreaseInterval), ip.score)
 
-	if d.ignore {
-		if time.Since(d.lastSeen) > unignoreTime {
-			d.ignore = false
+	if ip.ignore {
+		if time.Since(ip.lastSeen) > unignoreTime {
+			ip.ignore = false
 		}
 	}
 
-	if d.previousOffender && time.Since(d.lastSeen) > undoSuspicionIdleTime {
-		d.previousOffender = false
+	if ip.previousOffender && time.Since(ip.lastSeen) > undoSuspicionIdleTime {
+		ip.previousOffender = false
 	}
 
-	if d.blocked && time.Since(d.lastSeen) > unblockIdleTime {
-		d.blocked = false
-		d.blockedPorts = []tcpUDPport{}
+	if ip.blocked && time.Since(ip.lastSeen) > unblockIdleTime {
+		ip.blocked = false
+		ip.blockedPorts = []tcpUDPport{}
 
-		status.DeleteThreat(threadPrefix + ipString)
+		status.DeleteThreat(threatIDPrefix + ipString)
 	}
 
-	if !d.blocked && !d.ignore && !d.previousOffender && d.score == 0 {
+	ip.lastUpdated = time.Now()
+
+	if !ip.blocked && !ip.ignore && !ip.previousOffender && ip.score == 0 {
 		delete(ips, ipString)
 		return false
 	}
 
-	d.lastUpdated = time.Now()
 	return true
 }
 
@@ -239,66 +234,45 @@ func (d *Detector) Destroy() error {
 	return nil
 }
 
-// DetectorFactory is a primitive detection method that runs within the factory only.
+// DetectorFactory creates&returns a detector for a connection
 func DetectorFactory(conn *network.Connection, pkt packet.Packet) (network.Inspector, error) {
 	return &Detector{}, nil
 }
 
 // Register registers the encryption detection inspector with the inspection framework.
 func init() {
-	ips = make(map[string]*ipData)
-
-	//cleanup old Threads
-	threads, _ := status.GetThreats(threadPrefix)
-	for _, t := range threads {
-		status.DeleteThreat(t.ID)
-	}
-
 	module = modules.Register("portscan-detection", nil, start, nil, "base", "netenv")
-	module.Enable()
+	module.Enable() // FIXME
 }
 
 func updateWholeList() {
-	log.Debugf("Portscan detection: update list&cleanup")
-	for ip, entry := range ips {
-		//done inside the loop to give other goroutines time in between to access the list (and during that time block this task)
-		runOnlyOne.Lock()
-		defer runOnlyOne.Unlock()
+	log.Debugf("portscan-detection: update list&cleanup")
 
-		if entry.updateScoreIgnoreBlockPrevOffender(ip) {
-			log.Debugf("%s: %v", ip, entry)
+	detectorMutex.Lock()
+	defer detectorMutex.Unlock()
+
+	for ip, entry := range ips {
+
+		if entry.updateIPstate(ip) {
+			log.Debugf("portscan-detection: %s: %s", ip, entry)
 		} else {
-			log.Debugf("Removed %s from the list", ip)
+			log.Debugf("portscan-detection: Removed %s from the list", ip)
 		}
 	}
-	log.Debugf("Portscan detection: finished update list&cleanup")
+	log.Debugf("portscan-detection: finished update list&cleanup")
 
 }
 
-func start() (err error) {
-	go delayedStart()
+func start() error {
+	ips = make(map[string]*ipData)
 
-	// Reload own IP List on Network change
-	err = module.RegisterEventHook(
-		"netenv",
-		"network changed",
-		"Reload List of own IPs on Network change for Portscan detection",
-		func(_ context.Context, _ interface{}) (err error) {
-			fillOwnIPs()
+	// cleanup old Threats
+	threats, _ := status.GetThreats(threatIDPrefix)
+	for _, t := range threats {
+		status.DeleteThreat(t.ID)
+	}
 
-			return
-		},
-	)
-
-	fillOwnIPs()
-
-	return
-}
-
-func delayedStart() {
-	time.Sleep(startAfter)
-
-	log.Debugf("starting Portscan detection")
+	log.Debugf("portscan-detection: starting")
 	err := inspection.RegisterInspector(&inspection.Registration{
 		Name:    "Portscan Detection",
 		Order:   0,
@@ -306,52 +280,44 @@ func delayedStart() {
 	})
 
 	if err != nil {
-		panic(err)
+		return err
 	}
 
-	module.NewTask("portscan score update", func(ctx context.Context, task *modules.Task) (err error) {
+	module.NewTask("portscan score update", func(ctx context.Context, task *modules.Task) error {
 		updateWholeList()
-		return
+		return nil
 	}).Repeat(cleanUpInterval).MaxDelay(cleanUpMaxDelay)
-}
 
-func fillOwnIPs() {
-	var err error
-	ownIPs, _, err = netenv.GetAssignedAddresses()
-
-	if err != nil {
-		log.Errorf("Couldn't obtain List of IPs: %v", err)
-	}
-
-	log.Debugf("Portscan detection: ownIPs: %v", ownIPs)
-}
-
-//Does NOT check localhost range!!
-func foreignIPv4(ip net.IP) bool {
-	if ip.To4() == nil {
-		return false
-	}
-
-	for _, ownIP := range ownIPs {
-		if ip.Equal(ownIP) {
-			return false
-		}
-	}
-
-	return true
+	return nil
 }
 
 func isNetBIOSoverTCPIP(conn *network.Connection) bool {
-	return conn.LocalPort == 138 ||
+	return conn.LocalPort == 137 || // maybe we could limit this to UDP ... RFC1002 defines NAME_SERVICE_TCP_PORT but dosn't use it (in contrast to the other ports that are also only defined TCP or UDP)
 		(conn.IPProtocol == packet.UDP && conn.LocalPort == 138) ||
 		(conn.IPProtocol == packet.TCP && conn.LocalPort == 139)
 
 }
 
-// Source: https://stackoverflow.com/questions/27516387/what-is-the-correct-way-to-find-the-min-between-two-integers-in-go#27516559
 func intMin(a, b int) int {
 	if a < b {
 		return a
 	}
 	return b
 }
+
+func (ip *ipData) String() string {
+	var blockedPorts strings.Builder
+	for k, v := range ip.blockedPorts {
+		if k > 0 {
+			blockedPorts.WriteString(", ")
+		}
+
+		blockedPorts.WriteString(v.protocol.String() + " " + strconv.Itoa(int(v.port)))
+	}
+
+	return fmt.Sprintf("Score: %d, previousOffender: %t, blocked: %t, ignored: %t, lastSeen: %s, lastUpdated: %s, blockedPorts: [%s]", ip.score, ip.previousOffender, ip.blocked, ip.ignore, ip.lastSeen, ip.lastUpdated, blockedPorts.String())
+}
+
+func extractRemoteFromIPString(ipString string) string {
+	return strings.SplitAfterN(ipString, "-", 2)[1]
+}

firewall/inspection/portscan/doc.go

@@ -0,0 +1,67 @@
+package portscan
+
+/*
+* delay start by 1 Minutes (in order to let answer-packets from old sockets arrive (at reboot))
+* if Portscan detected: secure mode; IP-Block
+* Whitelist outgoing connections
+* Whitelist DHCP, ICMP, IGM, NetBios, foreign destination IPs (including especially Broadcast&Multicast)
+* Score >= 160: Portscan; set previous offender-flag which is persistent until 24 hours of inactivity
+* ability to set ignore-flag (persistent until 24 hours of inactivity)
+* previous offender is blocked on 1st probed closed port
+
+flowchart:
+----------
+
+function inspect() {
+	if can't get IP {
+	return undecided;
+	}
+
+	if IP listed {
+		call updateIPstate();
+		update last seen;
+
+		if IP ignored {
+			return undecided;
+		}
+	}
+
+	if no process attached
+	&& inbound && tcp/udp
+	&& going to own singlecast-address
+	&& not NetBIOS over TCP/IP
+	&& not DHCP {
+		call handleMaliciousPacket();
+	}
+
+	return blocked if blocked, otherwise undecided;
+}
+
+function updateIPstate() {
+  recalculate score;
+  reset ignore-flag if expired;
+  reset block-flag if expired and delete own threat;
+  update lastUpdated;
+  if nothing important in entry{
+    delete entry;
+  }
+}
+
+function handleMaliciousPacket() {
+	set score depending on type of port;
+
+	if IP not listed listed {
+	  add to List;
+	  return;
+	}
+
+	if probed port is not in th List of already ports by that IP {
+	  add to List of Ports;
+	  update score;
+	  update wether IP is is blocked;
+
+	  if blocked and no threat-warning {
+	    create threat-warning;
+	  }
+	}
+} */