Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/systemd query events #1728

Merged
merged 15 commits into from
Nov 27, 2024
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -52,3 +52,4 @@ go.work.sum

# Kext releases
windows_kext/release/kext_release_*.zip
windows_core_dll/.vs/windows_core_dll
1 change: 0 additions & 1 deletion .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ linters:
- interfacer
- ireturn
- lll
- mnd
- musttag
- nestif
- nilnil
Expand Down
2 changes: 2 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ require (
github.com/tidwall/gjson v1.18.0
github.com/tidwall/sjson v1.2.5
github.com/umahmood/haversine v0.0.0-20151105152445-808ab04add26
github.com/varlink/go v0.4.0
github.com/vincent-petithory/dataurl v1.0.0
go.etcd.io/bbolt v1.3.11
golang.org/x/exp v0.0.0-20241108190413-2d47ceb2692f
Expand Down Expand Up @@ -91,6 +92,7 @@ require (
github.com/inconshreveable/mousetrap v1.1.0 // indirect
github.com/josharian/native v1.1.0 // indirect
github.com/klauspost/cpuid/v2 v2.2.9 // indirect
github.com/maruel/panicparse/v2 v2.3.1 // indirect
github.com/mdlayher/netlink v1.7.2 // indirect
github.com/mdlayher/socket v0.5.1 // indirect
github.com/mitchellh/reflectwalk v1.0.2 // indirect
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,8 @@ github.com/valyala/fastrand v1.1.0 h1:f+5HkLW4rsgzdNoleUOB69hyT9IlD2ZQh9GyDMfb5G
github.com/valyala/fastrand v1.1.0/go.mod h1:HWqCzkrkg6QXT8V2EXWvXCoow7vLwOFN002oeRzjapQ=
github.com/valyala/histogram v1.2.0 h1:wyYGAZZt3CpwUiIb9AU/Zbllg1llXyrtApRS815OLoQ=
github.com/valyala/histogram v1.2.0/go.mod h1:Hb4kBwb4UxsaNbbbh+RRz8ZR6pdodR57tzWUS3BUzXY=
github.com/varlink/go v0.4.0 h1:+/BQoUO9eJK/+MTSHwFcJch7TMsb6N6Dqp6g0qaXXRo=
github.com/varlink/go v0.4.0/go.mod h1:DKg9Y2ctoNkesREGAEak58l+jOC6JU2aqZvUYs5DynU=
github.com/vincent-petithory/dataurl v1.0.0 h1:cXw+kPto8NLuJtlMsI152irrVw9fRDX8AbShPRpg2CI=
github.com/vincent-petithory/dataurl v1.0.0/go.mod h1:FHafX5vmDzyP+1CQATJn7WFKc9CvnvxyvZy6I1MrG/U=
github.com/vmihailenco/msgpack/v5 v5.3.5/go.mod h1:7xyJ9e+0+9SaZT0Wt1RGleJXzli6Q/V5KbhBonMG9jc=
Expand Down
9 changes: 9 additions & 0 deletions service/compat/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,12 @@ func stop() error {
}

func selfcheckTaskFunc(wc *mgr.WorkerCtx) error {
res := module.instance.Resolver()
if res.IsDisabled() {
log.Debugf("compat: skipping self-check: resolver is disabled")
return nil
}

// Create tracing logger.
ctx, tracer := log.AddTracer(wc.Ctx())
defer tracer.Submit()
Expand All @@ -118,6 +124,8 @@ func selfcheckTaskFunc(wc *mgr.WorkerCtx) error {
tracer.Warningf("compat: %s", err)
case selfcheckNetworkChangedFlag.IsSet():
// The network changed, ignore the issue.
case res.IsDisabled():
// Portmaster resolver is disabled, ignore this issue.
default:
// The self-check failed.

Expand Down Expand Up @@ -181,4 +189,5 @@ func New(instance instance) (*Compat, error) {

type instance interface {
NetEnv() *netenv.NetEnv
Resolver() *resolver.ResolverModule
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Missing Resolver() implementation in spn/instance.go

The verification reveals that spn/instance.go implements NetEnv() but lacks the newly added Resolver() method. This implementation needs to be updated to maintain interface compatibility.

  • spn/instance.go: Add the missing Resolver() method to match the interface requirements
  • Other implementations in service/instance.go and test files are correctly updated
🔗 Analysis chain

Verify interface implementation across the codebase.

The addition of Resolver() method to the interface looks good, but we should verify that all types implementing this interface have been updated.


The previous attempt might have failed due to complex AST pattern. Let's try using ripgrep to:

  1. Find the interface definition to confirm the methods we need to check
  2. Search for implementations of key methods (NetEnv and Resolver) to identify structs that implement them

The previous output shows we have potentially found an issue - the spn/instance.go implementation has NetEnv() but might be missing Resolver(). Let's verify the complete interface definition and check the full implementation in spn/instance.go to confirm this.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for types that implement the instance interface
ast-grep --pattern 'type $_ struct {
  $$$
}

func ($_ $_) NetEnv() *netenv.NetEnv {
  $$$
}'

# This will help identify structs that implement NetEnv() but might be missing Resolver()

Length of output: 95


Script:

#!/bin/bash
# Let's try a different approach to find interface implementations

# First, let's find the interface definition
echo "=== Interface Definition ==="
rg -A 5 "type Instance interface" 

# Then search for struct types that might implement it
echo -e "\n=== Potential Implementations ==="
rg "func \(.*?\) (NetEnv|Resolver)\(\)" --type go -A 1

Length of output: 695


Script:

#!/bin/bash
# Let's find the complete interface definition first
echo "=== Complete Interface Definition ==="
rg -B 2 -A 10 "type Instance interface"

# Let's also check the implementation in spn/instance.go since it's missing Resolver()
echo -e "\n=== SPN Instance Implementation ==="
rg -B 2 -A 15 "type Instance struct" spn/instance.go

Length of output: 649

}
30 changes: 30 additions & 0 deletions service/firewall/bypassing.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,24 @@ func PreventBypassing(ctx context.Context, conn *network.Connection) (endpoints.
return endpoints.NoMatch, "", nil
}

// If Portmaster resolver is disabled allow requests going to system dns resolver.
// And allow all connections out of the System Resolver.
if module.instance.Resolver().IsDisabled() {
// TODO(vladimir): Is there a more specific check that can be done?
if conn.Process().IsSystemResolver() {
return endpoints.NoMatch, "", nil
}
if conn.Entity.Port == 53 && conn.Entity.IPScope.IsLocalhost() {
return endpoints.NoMatch, "", nil
}
Comment on lines +53 to +55
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can allow any normal DNS, as we can see the packets. We only need to prevent encrypted DNS connections from apps.

This is getting complicated. Maybe we can simplify and regroup the logic here? Let's talk about options.

}
vlabo marked this conversation as resolved.
Show resolved Hide resolved

// Block bypass attempts using an (encrypted) DNS server.
switch {
case looksLikeOutgoingDNSRequest(conn) && module.instance.Resolver().IsDisabled():
// Allow. Packet will be analyzed and blocked if its not a dns request, before sent.
conn.Inspecting = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

conn.Inspecting is currently unused. Are you using it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return endpoints.NoMatch, "", nil
Comment on lines +60 to +63
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Security: Enhance DNS inspection mechanism

The current implementation allows packets before inspection completes, which could create a small window of opportunity for attacks.

Consider these improvements:

  1. Buffer initial packets until inspection confirms DNS validity
  2. Add timeout for inspection to prevent hanging connections
  3. Consider implementing pre-inspection validation
 case looksLikeOutgoingDNSRequest(conn) && module.instance.Resolver().IsDisabled():
-    // Allow. Packet will be analyzed and blocked if its not a dns request, before sent.
     conn.Inspecting = true
+    conn.SetInspectionTimeout(time.Second * 5)  // Add timeout
+    conn.BufferInitialPackets = true           // Buffer until inspection
     return endpoints.NoMatch, "", nil

Committable suggestion skipped: line range outside the PR's diff.

case conn.Entity.Port == 53:
return endpoints.Denied,
"blocked DNS query, manual dns setup required",
Expand All @@ -62,3 +78,17 @@ func PreventBypassing(ctx context.Context, conn *network.Connection) (endpoints.

return endpoints.NoMatch, "", nil
}

func looksLikeOutgoingDNSRequest(conn *network.Connection) bool {
// Outbound on remote port 53, UDP.
if conn.Inbound {
return false
}
if conn.Entity.Port != 53 {
return false
}
if conn.IPProtocol != packet.UDP {
return false
}
return true
}
vlabo marked this conversation as resolved.
Show resolved Hide resolved
52 changes: 22 additions & 30 deletions service/firewall/dns.go
Original file line number Diff line number Diff line change
Expand Up @@ -287,38 +287,35 @@ func UpdateIPsAndCNAMEs(q *resolver.Query, rrCache *resolver.RRCache, conn *netw
}
}

// Create new record for this IP.
record := resolver.ResolvedDomain{
Domain: q.FQDN,
Resolver: rrCache.Resolver,
DNSRequestContext: rrCache.ToDNSRequestContext(),
Expires: rrCache.Expires,
}
// Process CNAMEs
record.AddCNAMEs(cnames)
// Link connection with cnames.
conn.Entity.CNAME = record.CNAMEs

SaveIPsInCache(ips, profileID, record)
}

// formatRR is a friendlier alternative to miekg/dns.RR.String().
func formatRR(rr dns.RR) string {
return strings.ReplaceAll(rr.String(), "\t", " ")
}

// SaveIPsInCache saves the provided ips in the dns cashe assoseted with the record Domain and CNAMEs.
func SaveIPsInCache(ips []net.IP, profileID string, record resolver.ResolvedDomain) {
// Package IPs and CNAMEs into IPInfo structs.
for _, ip := range ips {
// Never save domain attributions for localhost IPs.
if netutils.GetIPScope(ip) == netutils.HostLocal {
continue
}

// Create new record for this IP.
record := resolver.ResolvedDomain{
Domain: q.FQDN,
Resolver: rrCache.Resolver,
DNSRequestContext: rrCache.ToDNSRequestContext(),
Expires: rrCache.Expires,
}

// Resolve all CNAMEs in the correct order and add the to the record - up to max 50 layers.
domain := q.FQDN
for range 50 {
nextDomain, isCNAME := cnames[domain]
if !isCNAME || nextDomain == domain {
break
}

record.CNAMEs = append(record.CNAMEs, nextDomain)
domain = nextDomain
}

// Update the entity to include the CNAMEs of the query response.
conn.Entity.CNAME = record.CNAMEs

// Check if there is an existing record for this DNS response.
// Else create a new one.
ipString := ip.String()
info, err := resolver.GetIPInfo(profileID, ipString)
if err != nil {
Expand All @@ -341,8 +338,3 @@ func UpdateIPsAndCNAMEs(q *resolver.Query, rrCache *resolver.RRCache, conn *netw
}
}
}

// formatRR is a friendlier alternative to miekg/dns.RR.String().
func formatRR(rr dns.RR) string {
return strings.ReplaceAll(rr.String(), "\t", " ")
}
99 changes: 99 additions & 0 deletions service/firewall/interception/dnsmonitor/etwlink_windows.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
//go:build windows
// +build windows

package dnsmonitor

import (
"fmt"
"runtime"
"sync"
"sync/atomic"

"github.com/safing/portmaster/service/integration"
"golang.org/x/sys/windows"
)

type ETWSession struct {
i integration.ETWFunctions

shutdownGuard atomic.Bool
shutdownMutex sync.Mutex

state uintptr
}

// NewSession creates new ETW event listener and initilizes it. This is a low level interface, make sure to call DestorySession when you are done using it.
func NewSession(etwInterface integration.ETWFunctions, callback func(domain string, result string)) (*ETWSession, error) {
etwSession := &ETWSession{
i: etwInterface,
}

// Make sure session from previous instances are not running.
_ = etwSession.i.StopOldSession()

// Initialize notification activated callback
win32Callback := windows.NewCallback(func(domain *uint16, result *uint16) uintptr {
callback(windows.UTF16PtrToString(domain), windows.UTF16PtrToString(result))
return 0
})
// The function only allocates memory it will not fail.
etwSession.state = etwSession.i.CreateState(win32Callback)

// Make sure DestroySession is called even if caller forgets to call it.
runtime.SetFinalizer(etwSession, func(s *ETWSession) {
_ = s.i.DestroySession(s.state)
})

// Initialize session.
err := etwSession.i.InitializeSession(etwSession.state)
if err != nil {
return nil, fmt.Errorf("failed to initialzie session: %q", err)
}

return etwSession, nil
}

// StartTrace starts the tracing session of dns events. This is a blocking call. It will not return until the trace is stopped.
func (l *ETWSession) StartTrace() error {
return l.i.StartTrace(l.state)
}
Comment on lines +57 to +59
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Prevent method calls after session is destroyed

The methods StartTrace (lines 57-59~) and StopTrace (lines 81-82~) do not check if the session has been destroyed before proceeding. Calling these methods after DestroySession could result in undefined behavior or errors due to the invalid l.state.

Add checks to ensure the session is still running before executing these methods.

Apply the following changes to add safety checks:

 func (l *ETWSession) StartTrace() error {
+	if l.shutdownGuard.Load() {
+		return fmt.Errorf("cannot start trace; session has been destroyed")
+	}
 	return l.i.StartTrace(l.state)
 }

 func (l *ETWSession) StopTrace() error {
+	if l.shutdownGuard.Load() {
+		return fmt.Errorf("cannot stop trace; session has been destroyed")
+	}
 	return l.i.StopTrace(l.state)
 }

Also applies to: 81-82


// IsRunning returns true if DestroySession has NOT been called.
func (l *ETWSession) IsRunning() bool {
return !l.shutdownGuard.Load()
}

// FlushTrace flushes the trace buffer.
func (l *ETWSession) FlushTrace() error {
l.shutdownMutex.Lock()
defer l.shutdownMutex.Unlock()

// Make sure session is still running.
if l.shutdownGuard.Load() {
return nil
}

return l.i.FlushTrace(l.state)
}

// StopTrace stopes the trace. This will cause StartTrace to return.
func (l *ETWSession) StopTrace() error {
return l.i.StopTrace(l.state)
}

// DestroySession closes the session and frees the allocated memory. Listener cannot be used after this function is called.
func (l *ETWSession) DestroySession() error {
l.shutdownMutex.Lock()
defer l.shutdownMutex.Unlock()

if l.shutdownGuard.Swap(true) {
return nil
}

err := l.i.DestroySession(l.state)
if err != nil {
return err
}
l.state = 0
return nil
}
19 changes: 19 additions & 0 deletions service/firewall/interception/dnsmonitor/eventlistener.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
//go:build !linux && !windows
// +build !linux,!windows

package dnsmonitor

type Listener struct{}

func newListener(_ *DNSMonitor) (*Listener, error) {
return &Listener{}, nil
}

func (l *Listener) flush() error {
// Nothing to flush
return nil
}

func (l *Listener) stop() error {
return nil
}
Loading
Loading