feat: enhance network monitoring for UDP packets

[rohan-stepsecurity]

No description provided.

[step-security-bot]

Please find StepSecurity AI-CodeWise code comments below.

Code Comments

firewall.go

• [High]Handle return errors properly after iptables operations
  The code calls ipt.Append to add an iptables rule for logging UDP packets but does not check if the operation succeeded before proceeding. Proper error handling is critical in security-sensitive code to avoid silent failures which can cause the firewall to not function as intended. According to the Go error handling best practices (effective go, Go official docs), all possible errors should be handled explicitly. Immediately check the error returned from ipt.Append and return or handle it accordingly. For example:

err = ipt.Append(filterTable, chain, direction, netInterface, protocol, "udp", "-j", "NFLOG", "--nflog-group", "100")
if err != nil {
return errors.Wrap(err, "failed to add UDP NFLOG rule")
}

• [High]Avoid hardcoding iptables NFLOG group numbers
  The NFLOG group number 100 is hardcoded. Hardcoding magic numbers can lead to conflicts if the number is already in use or if the number has special meaning in some contexts. According to OWASP secure coding practices, constants representing external system configuration should be parameterized or documented. Define the NFLOG group number as a constant or configuration parameter at the top of the file or passed to the function, e.g.:

const nflogGroup = "100"

and use this variable in ipt.Append calls. Also, consider validating this number does not conflict with other system configurations.

• [Medium]Clarify the function parameters for ipt.Append for readability and correctness
  The call to ipt.Append passes several variables, e.g. 'direction', 'netInterface', 'protocol', 'udp', but it is unclear whether these are well-formed and comply with iptables command syntax. From the snippet, the mix of variables and literals might lead to incorrect rules if values are misplaced or redundant. According to Go and secure coding best practices, explicit is better than implicit; code clarity prevents bugs in security rules. Review and clearly comment on the parameters passed to ipt.Append. Consider constructing the arguments as a slice of strings with explicit labels to ensure correctness and maintainability. For example:

args := []string{chain, direction, netInterface, protocol, "udp", "-j", "NFLOG", "--nflog-group", nflogGroup}
err = ipt.Append(filterTable, args...)

This also helps during debugging.

• [Medium]Use structured logging or consistent logging for blocked packets
  The rule adds UDP packets to be logged via NFLOG group 100, but it is unclear if other protocols have similar logging rules or if the logs are structured consistently. According to industry best practices such as the CIS benchmarks, network logs should be comprehensive and consistently formatted for effective monitoring and incident response. Ensure that logging rules exist for all critical protocols or traffic types, not just UDP. Also, configure NFLOG or another logging mechanism to provide structured output with timestamp and metadata to aid analysis.
• [Low]Add comments to clarify the purpose of the new rule
  The newly added rule to log UDP blocked packets lacks detailed comments explaining why the NFLOG group 100 is used and how this log will be consumed. According to clean code practices, comments should explain the why behind decisions, especially in security-relevant code. Improve the comment above the new code block, for example:

// Log all blocked UDP packets to NFLOG group 100 for centralized monitoring and alerting.

• [Low]Consider atomic addition of firewall rules
  Adding firewall rules one by one can lead to partial application if any step fails. This can leave the firewall in an inconsistent state. Best practices suggest applying rules atomically or rolling back on failure. (Reference: Linux nftables atomic rule sets, iptables-restore). Consider using iptables-restore or batching all the rules so that the entire set is applied atomically. If this is not feasible due to library constraints, implement rollback logic to remove already added rules if subsequent adds fail.

netmon.go

• [High]Check error returns when type asserting layers
  The code performs type assertions like tcpLayer.(*layers.TCP) and udpLayer.(*layers.UDP) without verifying the success boolean. According to Go best practices (https://golang.org/doc/effective_go#type_assertions), type assertions should be checked to avoid panics in case of unexpected types. Change lines like 'tcp, _ := tcpLayer.(*layers.TCP)' to 'tcp, ok := tcpLayer.(*layers.TCP)'; then check if 'ok' is true before using 'tcp'. Do the same for 'udp, _ := udpLayer.(*layers.UDP)'. For example:

if tcpLayer := packet.Layer(layers.LayerTypeTCP); tcpLayer != nil {
tcp, ok := tcpLayer.(*layers.TCP)
if !ok {
// handle error or 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
}
port = udp.DstPort.String()
isUDP = true
}

• [High]Avoid calling sendNetConnection without validating critical fields
  The code calls 'netMonitor.ApiClient.sendNetConnection' with fields like CorrelationId and Repo that appear to come from the context but are not validated before use. Per security best practices (OWASP ASVS v4.0, section 4.3), unvalidated inputs can lead to injection or runtime errors. Before calling sendNetConnection, validate that CorrelationId and Repo are not empty and conform to expected formats. For example:

if netMonitor.CorrelationId == "" || netMonitor.Repo == "" {
// log or handle invalid values
return
}

netMonitor.ApiClient.sendNetConnection(...)

• [Medium]Add comments clarifying the rationale for sending connections only on 'Dropped' status
  The code sends network connection data only if netMonitor.Status equals 'Dropped' when isSYN or isUDP is true. Without comments explaining this business logic, maintainers may find it confusing, impacting maintainability per Clean Code principles by Robert C. Martin. Add a comment above the if condition explaining why sending occurs only on 'Dropped' status. For example:

// Only send connection events for initial TCP SYN packets or UDP packets when the network status is 'Dropped' to track potentially blocked attempts.

• [Medium]Handle unsupported or malformed packets gracefully
  The code only processes TCP and UDP packets but does not handle cases where the packet is neither, possibly leading to silent failures or untracked packets. Robust network parsing requires handling all cases (RFC 791 for IPv4). Add an else clause to log or handle unsupported packet types. For example:

} else {
// Log unsupported or non-TCP/UDP packet for analysis
}

This ensures visibility into unexpected network traffic.

• [Medium]Use consistent naming for boolean flags indicating protocol types
  The code uses 'isSYN' for TCP SYN packets and 'isUDP' for UDP packets, mixing protocol-specific flag with protocol presence indicator. Clear naming improves code readability and reduces misunderstanding (Google Go Style Guide). Rename 'isUDP' to 'hasUDP' or 'isUDPPacket' to parallel 'isSYN'. Ideally, separate flags like 'isTCP' and 'isUDP', and use 'isSYN' solely for TCP SYN checking.
• [Low]Initialize variables specifically with explicit zero values
  The variable 'port' is initialized as an empty string at the start. While it works, per Go best practices for clarity (Effective Go), consider declaring with a zero value only when necessary. Declare 'var port string' without explicit initialization, or assign only when the port is extracted. For example:

var port string
// ... later if layer found, assign port = ...

• [Low]Consider using constants or enums for status values like 'Dropped'
  Using string literals directly in code for status comparisons can lead to typos and maintenance issues. According to Effective Go, defining constants improves reliability and self-documentation. Define const StatusDropped = "Dropped" at package level and replace string literal checks with this constant:

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

• [Low]Add error handling or logging inside packet parsing failures
  gopacket.NewPacket may silently drop malformed packets. Logging parse errors improves observability and debugging (12 Factor App, Log Everything). Check for errors or use DecodingLayerParser with error returns, and log or handle them accordingly to detect malformed packets.
• [Low]Add unit tests for UDP packet handling logic
  The patch adds UDP packet processing, which should be covered by unit tests to ensure correct behavior and prevent regressions (Testing Best Practices). Write unit tests that create sample UDP packets and verify that 'port' is correctly extracted and that 'isUDP' is set, and that subsequent processing logic triggers as expected.
• [Low]Use context and cancelation to control packet handling
  Long running network monitoring should support context-based cancellation to allow clean shutdowns (Go concurrency patterns). Consider adding context.Context parameters to handlePacket and related calls to enable timely cancellation.

procmon_linux.go

• [High]Check and handle errors returned from flags.Parse and rule.Build before proceeding
  Ignoring errors from flags.Parse and rule.Build can lead to silent failures and unpredictable behavior. According to the Go best practices and Effective Go documentation, error handling is critical for maintaining robust and reliable programs. Add error checks after parsing flags and building rules, and handle errors appropriately, returning or logging them before proceeding.
• [High]Avoid ignoring errors by replacing blank identifier (_) with proper error handling
  Assigning errors to the blank identifier discards error information, making debugging difficult. Go error handling best practices emphasize explicitly checking and handling errors. Replace underscores with error variables and handle errors, as in:

r, err := flags.Parse(...)
if err != nil {
errc <- errors.Wrap(err, "failed to parse audit flags")
return
}

Similarly for rule.Build.

• [Medium]Avoid repeated code by creating a helper function to add audit rules, improving maintainability
  Duplicated code for adding audit rules for different syscalls violates the DRY (Don't Repeat Yourself) principle. According to Clean Code by Robert C. Martin, code duplication reduces maintainability and can introduce inconsistencies. Create a helper method that accepts syscall names and tags as arguments, parses flags, builds the rule, adds the rule, logs success/failure, and reports errors to errc.
• [Medium]Include more detailed contextual information in error logs to improve debugging
  Providing detailed context in logs such as syscall names and parameters helps in faster root cause analysis. This aligns with best practices for effective logging as recommended by the Google Logging Guide. Update logging statements to include the syscall and tag involved, e.g.,

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

• [Medium]Use structured logging instead of plain string formatting for better log analysis
  Structured logs facilitate automated parsing and analysis, improving observability. Authoritative sources like the Elastic logging documentation recommend structured logging. Replace WriteLog(fmt.Sprintf(...)) with a structured logging approach if supported, e.g.,

log.WithFields(log.Fields{"syscall": "sendto", "tag": netMonitorTag, "error": err}).Error("failed to add audit rule")

• [Low]Add comments explaining the purpose of the added audit rules for clarity
  Adding comments improves code readability and helps other developers understand the intent, consistent with Go documentation standards. Add comments like:

// Adding audit rule for UDP syscalls sendto and sendmsg to monitor network activity.

• [Low]Use constants or typed variables for syscall names to avoid typos and improve maintainability
  Hardcoding string literals increases the risk of typos and complicates updates. The use of constants is recommended in Go style guides such as Effective Go. Define constants for syscall names, e.g.,

const (
SyscallSendto = "sendto"
SyscallSendmsg = "sendmsg"
)

and use them in flags.Parse.

• [Low]Ensure consistent logging style throughout the function
  Uniform logging style improves readability and standardizes outputs, as recommended by best logging practices. Replace "WriteLog("Net monitor added for TCP (connect)")" with a consistent logging format similar to UDP logs, e.g., including tags or context.

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.