feat: enhance network monitoring for UDP packets

[rohan-stepsecurity]

Added support for monitoring UDP packets in the NetworkMonitor by introducing handling for the sendto and sendmsg syscalls. Updated logging to reflect the addition of UDP monitoring alongside existing TCP functionality.

[step-security-bot]

Please find StepSecurity AI-CodeWise code comments below.

Code Comments

netmon.go

• [High]Always check for errors immediately after type assertions to avoid potential runtime panics
  The code performs type assertions like tcpLayer.(*layers.TCP) and udpLayer.(*layers.UDP) without verifying if the assertion was successful. According to the Go language specification and best practices, ignoring the second boolean result of a type assertion can lead to runtime panics if the type assertion fails. (Go Blog: 'Effective Go' - Type assertions). Change the type assertions to capture and check the assertion success, for example:

if tcp, ok := tcpLayer.(*layers.TCP); ok {
port = tcp.DstPort.String()
isSYN = tcp.SYN
}

Similarly for UDP:

if udp, ok := udpLayer.(*layers.UDP); ok {
port = udp.DstPort.String()
isUDP = true
}

• [High]Avoid embedding protocol-specific logic in a manner that could cause future maintenance complexity by tightly coupling TCP and UDP handling
  Currently, the code handles TCP and UDP layers differently but then merges the logic in a way that might result in confusion (e.g., using isSYN || isUDP to decide if a connection is sent). Best practices suggest encapsulating protocol handling separately or using interfaces to improve maintainability and readability. (Clean Code by Robert C. Martin). Refactor packet handling to use separate functions or methods for TCP and UDP packet processing. For example:

func (netMonitor *NetworkMonitor) handleTCPPacket(tcp *layers.TCP) { ... }
func (netMonitor *NetworkMonitor) handleUDPPacket(udp *layers.UDP) { ... }

Then call them conditionally within handlePacket.

• [Medium]Use constant or typed string values for fixed status comparisons instead of raw string literals
  The code compares netMonitor.Status to "Dropped" as a raw string literal. Using constants or iota enumerations improves code robustness, reduces bugs from typos, and improves readability. (Effective Go - Constants and Enumerations). Define a constant for the status string, for example:

const StatusDropped = "Dropped"

Then replace string literals 'Dropped' with StatusDropped in conditions.

• [Medium]Consider using more descriptive or explicit variable names to improve code readability
  The variable 'port' is assigned using tcp.DstPort.String() or udp.DstPort.String(), but it might not be clear if this port is the source or destination port and whether it's a string representation or numeric port. Descriptive names improve clarity, reducing cognitive load and potential bugs. (Clean Code - Meaningful Names). Rename 'port' to 'dstPortString' or 'destinationPortString' to clarify its meaning.

For example:

var dstPortString string
// Then assign accordingly

port = tcp.DstPort.String() // becomes dstPortString = tcp.DstPort.String()

• [Low]Add comments clarifying the intent of boolean flags especially those that affect control flow
  While the code adds some comments, flags such as isSYN and isUDP are set and then used in conditions which may not be immediately clear to new maintainers. Commenting or using more descriptive flags helps maintainability. (Clean Code - Comments that explain why, not what). Add comments near flag declarations and usages explaining the rationale, e.g.,

// isSYN indicates this packet is a TCP SYN packet, used to detect connection initiation
// isUDP flags the presence of a UDP packet, treated similarly to SYN for connection logic

• [Low]Avoid redundant boolean variables when the data can be inferred directly where needed
  The boolean variable isUDP is set to true if the UDP layer exists but is later only used once combined with isSYN. Sometimes, directly using the condition in the branching logic improves code succinctness. (Go idiomatic coding guidelines). Replace the combined condition 'if isSYN || isUDP' with explicit checks such as:

if isSYN || packet.Layer(layers.LayerTypeUDP) != nil {
// do something
}

Alternatively, keep isUDP if expected to be used repeatedly.

procmon_linux.go

• [High]Check and handle errors from flags.Parse and rule.Build functions
  The flags.Parse and rule.Build function calls return errors that are currently being ignored (assigned to '_'). Ignoring errors can cause unnoticed failures and unstable behavior. According to the Go error handling best practices (Effective Go - https://golang.org/doc/effective_go.html#errors), every error returned by a function should be checked and handled appropriately. Capture and check the errors returned by flags.Parse and rule.Build calls. If an error occurs, log it and send it to the errc channel or handle accordingly.
  Example:

r, err := flags.Parse(fmt.Sprintf("-a exit,always -S sendto -k %s", netMonitorTag))
if err != nil {
WriteLog(fmt.Sprintf("failed to parse audit flags for sendto: %v", err))
errc <- errors.Wrap(err, "failed to parse audit flags for syscall sendto")
return
}

actualBytes, err := rule.Build(r)
if err != nil {
WriteLog(fmt.Sprintf("failed to build audit rule for sendto: %v", err))
errc <- errors.Wrap(err, "failed to build audit rule for syscall sendto")
return
}

• [High]Avoid multiple writes to the errc channel without synchronization or returning immediately
  In case adding multiple rules fails, multiple errors could be sent to errc channel by multiple goroutines or sequentially without a return. This may cause deadlocks or data races if the error channel is unbuffered and read only once. According to Go concurrency best practices (Go Blog - https://blog.golang.org/pipelines), care must be taken when sending errors on channels to avoid blocking and deadlocks. Send the error to errc and return immediately to avoid multiple sends causing deadlock or race conditions.
  Example:

if err = client.AddRule(actualBytes); err != nil {
WriteLog(fmt.Sprintf("failed to add audit rule for sendto %v", err))
errc <- errors.Wrap(err, "failed to add audit rule for syscall sendto")
return
}

• [Medium]Use consistent error logging format and message clarity
  Some error logs include variable substitution using fmt.Sprintf, while others directly log simple strings. This inconsistency can be confusing and harder to maintain. According to logging best practices (Google Logging Guide - https://cloud.google.com/logging/docs/reference/v2/rest/v2/LogEntry), clear, consistent, and structured logging helps with log readability and analysis. Use a consistent logging method, preferably structured logging if supported, or consistent fmt.Sprintf usage for all error logs.
  Example:

WriteLog(fmt.Sprintf("failed to add audit rule for syscall sendto: %v", err))

• [Medium]Factor repeated code into a helper function to reduce duplication
  The code for adding audit rules for different syscalls repeats similar logic: parse flags, build rule, add rule, log errors, and write success logs. According to the DRY principle (Don't Repeat Yourself) (Martin Fowler - https://martinfowler.com/bliki/DRY.html), repeated code should be refactored into a function to improve maintainability and reduce bugs. Create a helper function, e.g., addAuditRule(syscallName string, key string), that encapsulates this logic and call it with 'connect', 'sendto', and 'sendmsg' syscall names and respective keys.
• [Low]Consider adding context to logs for better observability
  Logs like "Net monitor added for UDP (sendmsg)" are simple but lack context such as timestamps or identifiers, which may help in debugging and tracking. According to logging best practices (Microsoft Docs - https://docs.microsoft.com/en-us/azure/architecture/best-practices/observability), including context in logs improves diagnosability. Include timestamps, process IDs, or more detailed context in logging messages or use a logging framework that appends such metadata automatically.
• [Low]Avoid using underscores to ignore errors in production-level code
  Assigning errors to '_' to ignore them does not allow for potential errors to be noticed during execution. This can lead to bugs or security issues. This is covered in many language best practices including Go Effective practices. Assign returned errors to variables and properly handle them as per suggestion 1.

Feedback

We appreciate your feedback in helping us improve the service! To provide feedback, please use emojis on this comment. If you find the comments helpful, give them a 👍. If they aren't useful, kindly express that with a 👎. If you have questions or detailed feedback, please create n GitHub issue in StepSecurity/AI-CodeWise.

[step-security-bot]

Please find StepSecurity AI-CodeWise code comments below.

Code Comments

netmon.go

• [High]Check for errors when type asserting packet layers
  The code performs type assertions like tcp, _ := tcpLayer.(*layers.TCP) and udp, _ := udpLayer.(*layers.UDP) without checking the success of these assertions. According to the Go Programming Language Specification and Effective Go guidelines, ignoring errors in type assertions can lead to runtime panics or subtle bugs if assertion fails. Modify the type assertion to check for success and handle the failure gracefully. For example:

if tcpLayer := packet.Layer(layers.LayerTypeTCP); tcpLayer != nil {
    tcp, ok := tcpLayer.(*layers.TCP)
    if !ok {
        // Handle error or return
        return
    }
    port = tcp.DstPort.String()
    isSYN = tcp.SYN
} else if udpLayer := packet.Layer(layers.LayerTypeUDP); udpLayer != nil {
    udp, ok := udpLayer.(*layers.UDP)
    if !ok {
        // Handle error or return
        return
    }
    port = udp.DstPort.String()
    isUDP = true
}

• [High]Avoid conflating protocol behavior by treating UDP packets as SYN equivalents
  The logic treats UDP packets similarly to TCP SYN packets (if isSYN || isUDP), which incorrectly assumes that a UDP packet initiation is equivalent to a TCP SYN packet that initiates connections. According to RFC 793 (TCP) and RFC 768 (UDP), UDP is connectionless and does not have connection initiation flags. Misinterpreting UDP behavior can lead to inaccurate network monitoring and incorrect state assumptions. Separate the handling logic for UDP packets from TCP SYN flags to reflect protocol semantics. For example, handle UDP packets with their own logic or a clearly defined state rather than combining in the SYN condition:

if isSYN {
    // TCP connection initiation handling
} else if isUDP {
    // UDP packet handling if needed
}

• [Medium]Handle the case when the packet does not contain TCP or UDP layers explicitly
  Currently, if the packet does not contain a TCP or UDP layer, port remains an empty string and isSYN and isUDP remain false, but this case is not explicitly handled. According to robust programming practices and error handling principles (Effective Go), explicitly handling all cases improves code clarity and reduces possibility of unexpected behavior. Add an else clause or a default handling for packets without TCP or UDP layers. For example:

// after TCP and UDP checks
else {
    // Log unexpected packet type or return early
    return
}

• [Medium]Avoid magic strings for status comparison; use constants or enums
  The code compares netMonitor.Status == "Dropped" using a hardcoded string. According to Go best practices and Clean Code principles, using constants or iota-enums for status codes improves maintainability and reduces bugs caused by typos. Define constants for status values and use them instead of string literals:

const StatusDropped = "Dropped"

// Then
if netMonitor.Status == StatusDropped {
    // ...
}

• [Low]Initialize boolean variables closer to usage for clarity
  The variables isSYN and isUDP are declared and initialized far from their usage, reducing code readability. Effective Go advocate for declaring and initializing variables as close as possible to where they are used for better clarity. Move declarations of isSYN and isUDP closer to the blocks where they are set and used, or inline their usage if possible.
• [Low]Avoid redundant map operations by using Go's map presence idiom
  The code uses found := false with a for loop to check for key existence in the map which is inefficient. According to Go idioms, it's better to use the comma-ok form for direct lookup in maps. Replace the loop to check if _, found := ipAddresses[ipv4Address]; !found { ... } instead of iterating manually.
• [Low]Add comments explaining rationale for key decisions
  Certain logic such as combining UDP with SYN packets is not explained in comments. According to Clean Code principles, adding descriptive comments helps maintainers understand non-obvious design choices. Document why UDP packets are treated like SYN packets or any other non-trivial logic.

procmon_linux.go

• [High]Check and handle errors returned by flags.Parse and rule.Build
  The code ignores errors returned by flags.Parse and rule.Build functions. According to the Go error handling best practices outlined in "Effective Go" (https://golang.org/doc/effective_go#errors), errors must be explicitly checked and handled to avoid runtime failures and unexpected behavior. Modify the code to check errors returned from flags.Parse and rule.Build, and handle them appropriately, for example by logging and sending the error on errc channel before returning.

Example:

r, err = flags.Parse(fmt.Sprintf("-a exit,always -S sendto -S sendmsg -k %s", netMonitorTag))
if err != nil {
WriteLog(fmt.Sprintf("failed to parse audit rule flags: %v", err))
errc <- errors.Wrap(err, "failed to parse audit rule flags for sendto")
return
}

actualBytes, err = rule.Build(r)
if err != nil {
WriteLog(fmt.Sprintf("failed to build audit rule: %v", err))
errc <- errors.Wrap(err, "failed to build audit rule for sendto")
return
}

• [High]Ensure early return after error is sent on errc channel to stop further processing
  In the error handling code where errc is sent an error, the function continues execution instead of returning immediately. This may cause multiple errors to be sent or inconsistent state. "Effective Go" advises that once an unrecoverable error is detected, the function should return immediately to avoid deeper issues. Add return statements immediately after sending error to the errc channel to stop further execution.

Example:

if err != nil {
WriteLog(fmt.Sprintf("failed to add audit rule for sendto %v", err))
errc <- errors.Wrap(err, "failed to add audit rule for syscall sendto")
return
}

• [Medium]Avoid using fmt.Sprintf for static strings by using string concatenation or constants
  Using fmt.Sprintf solely to format strings without variables introduces unnecessary overhead. Go best practices recommend using string concatenation for static strings or simple cases as described in "Effective Go" (https://golang.org/doc/effective_go#formatting). Replace fmt.Sprintf calls that only use static strings (e.g. WriteLog and logging statements) with string concatenation or static string constants.

Example:

WriteLog("failed to add audit rule for sendto " + err.Error())

• [Medium]Use named error variables for easier error identification and testing
  Defining and returning named error variables improves error inspection and testing, which is recommended in Go error handling guidelines (https://blog.golang.org/go1.13-errors). Currently, errors.Wrap produces dynamic errors which are hard to compare. Define package-level named errors or use error wrapping with sentinel errors to better identify specific audit rule failures.

Example:

var ErrAddAuditRuleSendto = errors.New("failed to add audit rule for sendto")

// then wrap or return it
errc <- errors.Wrap(ErrAddAuditRuleSendto, err.Error())

• [Low]Add comments explaining the purpose of each audit rule added and its significance
  Adding descriptive comments improves code readability and maintainability for future developers as aligned with documentation best practices (https://golang.org/doc/effective_go#commentary). Add comments above each audit rule addition explaining why it is added (e.g., monitoring UDP send operations via sendto and sendmsg to track outbound network calls).
• [Low]Consistently use error variables or concrete types for errc to improve error management
  Sending different types of errors or wrapping inconsistently on errc can complicate error processing by the receiver. Consistency is a best practice per error handling guidelines. Standardize error wrapping and type before sending on errc channel, or define a struct/type for all errors sent.

Example:

errc <- &AuditRuleError{Op: "sendto", Err: err}

Feedback

We appreciate your feedback in helping us improve the service! To provide feedback, please use emojis on this comment. If you find the comments helpful, give them a 👍. If they aren't useful, kindly express that with a 👎. If you have questions or detailed feedback, please create n GitHub issue in StepSecurity/AI-CodeWise.