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.

[step-security-bot]

Please find StepSecurity AI-CodeWise code comments below.

Code Comments

firewall.go

• [High]Validate and sanitize all input parameters before usage
  The function addBlockRules uses variables such as filterTable, chain, direction, netInterface, protocol, etc., in iptables rule construction without evident validation or sanitization. Unsanitized or malicious inputs can lead to command injection vulnerabilities or unintended firewall rules. According to OWASP Recommendation for Command Injection Prevention, all inputs used to construct commands or system calls must be validated and sanitized. Implement strict validation and sanitization for all input parameters used in iptables commands. For example, restrict protocol to allowed values ('tcp', 'udp', etc.), validate interface names, and ensure chain and target strings conform to expected firewall syntax before appending rules.
• [High]Check for duplicate or conflicting firewall rules before appending new ones
  Appending iptables rules without checking may cause rule duplication or conflicts, leading to firewall misconfiguration or performance degradation. According to the Netfilter/Iptables best practices guide, ensure rules are idempotent and avoid duplicates to maintain predictable firewall behavior. Before appending the UDP NFLOG rule, query existing iptables rules to check if an equivalent rule already exists. If yes, skip appending; otherwise, add the rule.
• [Medium]Log errors with sufficient context for easier debugging
  The error wrapping adds some context, but logging the exact rule parameters and state when an error occurs improves maintainability and troubleshooting, as recommended by Google’s error handling best practices. Enhance error handling by logging full details of rule parameters (e.g., values of filterTable, chain, protocol) alongside the error message before returning or wrap the error including this context.
• [Medium]Use constants or enumerations for protocols and action strings to avoid magic strings
  Currently, strings like "udp", "-j", "NFLOG", and "--nflog-group" are used inline, increasing the likelihood of typos or inconsistencies. The use of constants or enumerations improves code readability and reduces errors, per Clean Code principles by Robert C. Martin. Define constants for these strings at the package level, e.g., const ProtocolUDP = "udp", TargetNFLOG = "NFLOG", NFLGroup = "100" and use them wherever applicable.
• [Medium]Add comments explaining why the UDP NFLOG rule is added separately
  The comment "Log blocked traffic - UDP packets" is brief. Explaining reasoning or implications helps other developers. As per general coding standards, clear comments improve maintainability. Expand the comment to explain that UDP packets are separately logged with NFLOG group 100 for monitoring or troubleshooting and clarify how it fits into the firewall rule set.
• [Low]Handle the iptables Append operation atomically or within a transactional context if possible
  Appending multiple firewall rules independently can leave the firewall in an inconsistent state if one append fails mid-way. According to iptables best practices, rule modifications should be atomic to avoid partial application. Consider batch appending or using iptables-restore with a complete ruleset, or implement rollback logic if appending the UDP NFLOG rule fails after other rules have been added.
• [Low]Use more descriptive variable names for iptables options and arguments
  Using variable names like 'direction', 'protocol', etc., is good, but 'direction' in iptables context can be ambiguous. Clarity helps maintainability and reduces errors, following Clean Code principles. Rename variables to something clearer like 'packetDirectionFlag' or add inline comments indicating what these values represent.
• [Low]Add unit or integration tests covering the addition of UDP NFLOG rules
  Testing firewall rule additions ensures correctness and prevents regressions, aligning with general software engineering best practices on testing (IEEE Std 829-2008). Add tests that simulate or mock iptables interaction and verify that UDP NFLOG rules are added correctly and error handling works as intended.

netmon.go

• [High]Verify type assertions to prevent panics
  In Go, using the type assertion form 'x.(T)' without checking the boolean 'ok' value can cause a panic if the assertion fails. It is a best practice to use the 'comma-ok' idiom to safely handle type assertions to maintain application stability. (Source: Go Blog - "The Laws of Reflection" and Effective Go). Modify the type assertions to use 'comma-ok' idiom, e.g., 'tcp, ok := tcpLayer.(*layers.TCP); if !ok { return }' and similarly for UDP layer to handle any unexpected type assertion failure gracefully.
• [High]Add explicit handling for non-TCP/UDP packets
  The current code treats packets without TCP or UDP layers by ignoring port information and is not clearly handling those cases. Explicitly handling or logging non-TCP/UDP packets improves observability and maintains consistent packet processing. (Source: OWASP Secure Coding Practices, defensive programming principles). Add an else block to handle packets that are neither TCP nor UDP, either by logging or explicitly skipping processing, to avoid ambiguous behavior.
• [Medium]Avoid redundant variables that are not used meaningfully
  The variable 'isUDP' is set but never used to influence any logic besides being declared. Unused variables clutter code and cause confusion. (Source: Effective Go - Code Simplicity and Clarity). If 'isUDP' is required for future logic, use it accordingly; otherwise, remove 'isUDP' to simplify the code.
• [Medium]Use consistent and descriptive variable naming
  Naming variables like 'port' which is a string may cause confusion since port is typically represented as an integer. Converting ports early and keeping consistent types improves readability and reduces errors. (Source: Clean Code by Robert C. Martin). Store port as an integer (dstPort := int(tcp.DstPort)) when appropriate and convert to string only when needed for output or API calls.
• [Low]Document the purpose and logic of extended packet handling
  Maintaining clear comments explaining why UDP packets are treated similarly to SYN TCP packets improves maintainability and developer understanding. (Source: Google Go Style Guide and general best practices on code documentation). Add comments around the conditional that checks for 'isSYN || isUDP' explaining why UDP packets are treated in this context.
• [Low]Check for nil packets or incomplete data before processing
  Packets could be malformed or nil, which may lead to unexpected panics or errors. Defensive checking before accessing layers improves robustness. (Source: Defensive Programming Practices). Add a check after NewPacket to verify 'packet' is not nil or packet.ErrorLayer() is nil before accessing layers.

procmon_linux.go

• [High]Proper error handling for flags.Parse
  In Go, ignoring errors from function calls like flags.Parse can lead to unexpected behaviors and security issues. The errors returned should be properly checked and handled to ensure the audit rules are correctly parsed and applied. This is aligned with the Go error handling best practices as described in Effective Go and Go Code Review Comments. Check for errors returned by flags.Parse and handle them appropriately. For 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 flags: %v", err))
errc <- errors.Wrap(err, "failed to parse audit flags for syscall sendto")
return
}

• [High]Proper error handling for rule.Build function
  Ignoring errors from rule.Build can mean the audit rule is malformed or incomplete, potentially leading to an insecure monitor setup. As per Go error handling guidelines (Effective Go), all errors should be handled to preserve program correctness and security. Check for errors returned by rule.Build and handle them properly. For example:

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 syscall sendto")
return
}

• [High]Return after sending error on errc channel to prevent further execution
  After encountering an error, continuing execution may cause unintended side-effects or inconsistent state. Returning immediately after sending the error on the channel ensures that the function does not run subsequent logic that may depend on successful prior operations. This aligns with defensive programming best practices. Add a return statement immediately after sending an error on errc. For 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 logging format and avoid concatenation
  Prefer structured logging or formatted error messages for consistency and easier parsing. Use fmt.Errorf or structured logging libraries instead of string concatenation or fmt.Sprintf inside logs. This aligns with common logging best practices to improve debugging and monitoring. Replace WriteLog(fmt.Sprintf(...)) with consistent formatting or use a logging library that supports structured logs. For example:

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

Could be changed to:

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

• [Medium]Comment clarity and completeness
  Comments should clearly describe the purpose and scope of the code they annotate. The current comment '// syscall sendto (for UDP)' misses 'sendmsg' syscall that is also included. Proper comments improve maintainability and developer understanding. Improve the comment for clarity:

// Add audit rules for UDP syscalls sendto and sendmsg for network monitoring

• [Low]Check errc channel capacity and possible blocking
  Sending errors on errc without confirmation or proper channel size can risk blocking the goroutine if the receiver is not ready. To prevent deadlocks, ensure errc has sufficient buffer or use non-blocking sends where appropriate. This is recommended by concurrency best practices in Go. Verify that errc channel is buffered or consider launching a goroutine for sending errors. For example:

select {
case errc <- errors.Wrap(err, "..."):
default:
WriteLog("could not send error to errc channel")
}

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.

[h0x0er]

Looks good