From 249261a3da8dc9752940021b15dd1d2a3b1b20ae Mon Sep 17 00:00:00 2001 From: Daniel Date: Wed, 8 Apr 2020 14:07:29 +0200 Subject: [PATCH] Implement review suggestions --- firewall/inspection/inspection.go | 2 ++ firewall/master.go | 44 ++++++++++++++++--------------- firewall/prompt.go | 4 --- network/clean.go | 2 +- network/connection.go | 15 +++++------ 5 files changed, 32 insertions(+), 35 deletions(-) diff --git a/firewall/inspection/inspection.go b/firewall/inspection/inspection.go index 750eb4ba..55629b19 100644 --- a/firewall/inspection/inspection.go +++ b/firewall/inspection/inspection.go @@ -62,6 +62,8 @@ func RunInspectors(conn *network.Connection, pkt packet.Packet) (network.Verdict if skip { continue } + + // check if the current verdict is already past the inspection criteria. if conn.Verdict > inspectVerdicts[key] { activeInspectors[key] = true continue diff --git a/firewall/master.go b/firewall/master.go index ba33d2db..ccac9664 100644 --- a/firewall/master.go +++ b/firewall/master.go @@ -22,17 +22,19 @@ import ( // Call order: // -// 1. DecideOnCommunicationBeforeIntel (if connecting to domain) -// is called when a DNS query is made, before the query is resolved -// 2. DecideOnCommunicationAfterIntel (if connecting to domain) -// is called when a DNS query is made, after the query is resolved -// 3. DecideOnCommunication -// is called when the first packet of the first link of the communication arrives -// 4. DecideOnLink -// is called when when the first packet of a link arrives only if communication has verdict UNDECIDED or CANTSAY +// DNS Query: +// 1. DecideOnConnection +// is called when a DNS query is made, may set verdict to Undeterminable to permit a DNS reply. +// is called with a nil packet. +// 2. FilterDNSResponse +// is called to (possibly) filter out A/AAAA records that the filter would deny later. +// +// Network Connection: +// 3. DecideOnConnection +// is called with the first packet of a network connection. // DecideOnConnection makes a decision about a connection. -func DecideOnConnection(conn *network.Connection, pkt packet.Packet) { +func DecideOnConnection(conn *network.Connection, pkt packet.Packet) { //nolint:gocognit,gocyclo // TODO // update profiles and check if communication needs reevaluation if conn.UpdateAndCheck() { log.Infof("filter: re-evaluating verdict on %s", conn) @@ -48,6 +50,7 @@ func DecideOnConnection(conn *network.Connection, pkt packet.Packet) { // check if process is communicating with itself if pkt != nil { + // TODO: evaluate the case where different IPs in the 127/8 net are used. if conn.Process().Pid >= 0 && pkt.Info().Src.Equal(pkt.Info().Dst) { // get PID otherPid, _, err := process.GetPidByEndpoints( @@ -57,16 +60,16 @@ func DecideOnConnection(conn *network.Connection, pkt packet.Packet) { pkt.Info().LocalPort(), pkt.Info().Protocol, ) - if err == nil { - + if err != nil { + log.Warningf("filter: failed to find local peer process PID: %s", err) + } else { // get primary process otherProcess, err := process.GetOrFindPrimaryProcess(pkt.Ctx(), otherPid) - if err == nil { - - if otherProcess.Pid == conn.Process().Pid { - conn.Accept("connection to self") - return - } + if err != nil { + log.Warningf("filter: failed to find load local peer process with PID %d: %s", otherPid, err) + } else if otherProcess.Pid == conn.Process().Pid { + conn.Accept("connection to self") + return } } } @@ -86,7 +89,7 @@ func DecideOnConnection(conn *network.Connection, pkt packet.Packet) { if conn.Scope == network.IncomingHost { conn.Block("inbound connections blocked") } else { - conn.Deny("inbound connections blocked") + conn.Drop("inbound connections blocked") } return } @@ -179,12 +182,11 @@ func DecideOnConnection(conn *network.Connection, pkt packet.Packet) { // DefaultAction == DefaultActionBlock conn.Deny("endpoint is not whitelisted (default=block)") - return } // FilterDNSResponse filters a dns response according to the application profile and settings. func FilterDNSResponse(conn *network.Connection, q *resolver.Query, rrCache *resolver.RRCache) *resolver.RRCache { //nolint:gocognit // TODO - // do not modify own queries - this should not happen anyway + // do not modify own queries if conn.Process().Pid == os.Getpid() { return rrCache } @@ -339,5 +341,5 @@ matchLoop: if related { reason = fmt.Sprintf("domain is related to process: %s is related to %s", domainElement, processElement) } - return + return related, reason } diff --git a/firewall/prompt.go b/firewall/prompt.go index 9805fb82..210b63a9 100644 --- a/firewall/prompt.go +++ b/firewall/prompt.go @@ -25,10 +25,6 @@ const ( denyServingIP = "deny-serving-ip" ) -var ( - mtSaveProfile = "save profile" -) - func prompt(conn *network.Connection, pkt packet.Packet) { //nolint:gocognit // TODO nTTL := time.Duration(promptTimeout()) * time.Second diff --git a/network/clean.go b/network/clean.go index 25fdfb94..70519614 100644 --- a/network/clean.go +++ b/network/clean.go @@ -32,7 +32,7 @@ func cleanConnections() (activePIDs map[int]struct{}) { activePIDs = make(map[int]struct{}) name := "clean connections" // TODO: change to new fn - module.RunMediumPriorityMicroTask(&name, func(ctx context.Context) error { + _ = module.RunMediumPriorityMicroTask(&name, func(ctx context.Context) error { activeIDs := make(map[string]struct{}) for _, cID := range process.GetActiveConnectionIDs() { activeIDs[cID] = struct{}{} diff --git a/network/connection.go b/network/connection.go index 9876193d..1577d5c8 100644 --- a/network/connection.go +++ b/network/connection.go @@ -52,7 +52,7 @@ type Connection struct { //nolint:maligned // TODO: fix alignment profileRevisionCounter uint64 } -// NewConnectionFromDNSRequest +// NewConnectionFromDNSRequest returns a new connection based on the given dns request. func NewConnectionFromDNSRequest(ctx context.Context, fqdn string, ip net.IP, port uint16) *Connection { // get Process proc, err := process.GetProcessByEndpoints(ctx, ip, port, dnsAddress, dnsPort, packet.UDP) @@ -75,6 +75,7 @@ func NewConnectionFromDNSRequest(ctx context.Context, fqdn string, ip net.IP, po return dnsConn } +// NewConnectionFromFirstPacket returns a new connection based on the given packet. func NewConnectionFromFirstPacket(pkt packet.Packet) *Connection { // get Process proc, inbound, err := process.GetProcessByPacket(pkt) @@ -229,14 +230,12 @@ func (conn *Connection) save() { // save to internal state // check if it already exists mapKey := strconv.Itoa(conn.process.Pid) + "/" + conn.Scope - dnsConnsLock.RLock() + dnsConnsLock.Lock() _, ok := dnsConns[mapKey] - dnsConnsLock.RUnlock() if !ok { - dnsConnsLock.Lock() dnsConns[mapKey] = conn - dnsConnsLock.Unlock() } + dnsConnsLock.Unlock() } else { @@ -247,14 +246,12 @@ func (conn *Connection) save() { } // save to internal state // check if it already exists - connsLock.RLock() + connsLock.Lock() _, ok := conns[conn.ID] - connsLock.RUnlock() if !ok { - connsLock.Lock() conns[conn.ID] = conn - connsLock.Unlock() } + connsLock.Unlock() }