Add ARP spoofing/poisoning detection

[pirate]

Implements two complementary ARP spoofing detection methods:

1. Gateway MAC Spoofing Detection:

   • Monitors the default gateway's MAC address
   • Alerts if the gateway MAC changes unexpectedly
   • Detects MITM attacks intercepting outgoing traffic

2. Own IP Spoofing Detection:

   • Checks ARP table for other devices claiming your IP
   • Alerts if another MAC is impersonating your machine
   • Detects attacks intercepting your incoming traffic

Features:

• Configurable via MONITOR_ARP_SPOOF environment variable
• Integrated with existing notification system
• State tracking to prevent duplicate alerts
• Displays gateway MAC in menubar
• Includes response guidelines in documentation

Closes #30

Generated with Claude Code

[claude]

Pull Request Review: ARP Spoofing Detection

This is a solid implementation of ARP spoofing detection that adds valuable security monitoring capabilities. The code is well-structured and follows the existing patterns in the codebase. Here's my detailed review:

---

✅ Strengths

1. Good Architecture: The implementation follows the existing pattern of separate detection functions that integrate cleanly into the event collection system.

2. Dual Detection Methods: Covers both common ARP spoofing scenarios:

   • Gateway MAC spoofing (MITM attacks)
   • Own IP spoofing (identity theft on local network)

3. State Management: Properly uses the state dictionary to track known values and prevent duplicate alerts.

4. Error Handling: Good use of try-except blocks with appropriate timeouts to prevent hanging.

5. User Experience: Includes helpful response guidelines in the README and displays gateway MAC in the menubar.

---

🐛 Potential Issues

1. Command Injection Vulnerability (CRITICAL) 🔴

Location: security-growler.30s.py:1174, 1235

The use of f-strings with user-controlled data in shell commands creates command injection risks:

# Line 1174
cmd = f"arp -n {ip} 2>/dev/null | grep -v 'no entry' | grep -v 'incomplete' | tail -1 | awk '{{print $4}}'"

# Line 1235
cmd = f"arp -a 2>/dev/null | grep '({our_ip})' | awk '{{print $4}}'"

While ip values come from system commands (reducing risk), an attacker who can manipulate routing tables or ARP responses could potentially inject malicious IP values containing shell metacharacters.

Recommendation: Use shlex.quote() or pass parameters as arrays:

import shlex
cmd = f"arp -n {shlex.quote(ip)} 2>/dev/null | grep -v 'no entry' ..."

2. MAC Address Validation is Weak 🟡

Location: security-growler.30s.py:1187, 1249

The MAC address validation only checks for : and length >= 14:

if mac and ":" in mac and len(mac) >= 14:

This could accept malformed MACs like :::::::::::::: or partial matches.

Recommendation: Use regex validation:

import re
MAC_PATTERN = re.compile(r'^([0-9a-f]{2}:){5}[0-9a-f]{2}$', re.IGNORECASE)
if MAC_PATTERN.match(mac):
    return mac.lower()

3. Gateway Change Alert Lacks Deduplication 🟡

Location: security-growler.30s.py:1293-1296

Unlike the "Own IP Claimed" detection (which uses seen_events), gateway MAC changes will alert on every check until the state updates. If the gateway legitimately changes (network switch, load balancer failover), users will get repeated alerts every 30 seconds.

Recommendation: Add deduplication for gateway changes:

event_id = f"gateway_mac_change_{gateway_ip}_{known_gateway_mac}_{current_gateway_mac}"
if event_id not in state["seen_events"]:
    # ... alert ...
    state["seen_events"].append(event_id)

4. Interface Assumption 🟡

Location: security-growler.30s.py:1303

The code assumes the gateway interface is the same one to check for own IP spoofing, which is usually correct but not guaranteed in complex routing scenarios (VPNs, multiple interfaces).

Mitigation: This is acceptable for the common case, but could add a check to verify the interface has an IP address assigned.

5. Race Condition in Gateway Detection 🟢

Location: security-growler.30s.py:1160-1164

The function assumes route -n get default output order is always gateway then interface, but command output parsing can be fragile.

Recommendation: Parse by key rather than position:

gateway_ip = None
interface = None
for line in result.stdout.strip().splitlines():
    if 'gateway:' in line:
        gateway_ip = line.split()[-1]
    elif 'interface:' in line:
        interface = line.split()[-1]

---

⚡ Performance Considerations

1. Ping Overhead (security-growler.30s.py:1277-1281): The ping to refresh ARP cache adds ~1 second latency every 30 seconds. This is acceptable but consider:

   • Using -W 1 timeout (already done ✓)
   • Documenting this behavior for users on slow networks

2. Multiple Subprocess Calls: Each check spawns 4-6 subprocesses. For a 30-second polling interval, this is fine, but consider caching results if checks become more frequent.

3. No Batch Processing: The check_arp_table_for_duplicates function could be optimized to check all IPs in one arp -a call if monitoring multiple IPs in the future.

Overall: Performance is appropriate for the use case. ✓

---

🔒 Security Concerns

1. Command Injection Risk: See issue Sometimes quitting the menubar doesnt kill the secnotify process, and ghost growl notifications will occur #1 above - needs addressing before production use.

2. False Positives: Legitimate scenarios could trigger alerts:

   • Switching between WAN connections with different gateway IPs
   • DHCP lease renewal changing gateway
   • Enterprise networks with redundant gateways (VRRP/HSRP)

   The state management helps, but users should be educated about these scenarios (README addresses this ✓).

3. ARP Cache Timing: The check relies on ARP cache freshness. If cache entries expire between checks, false negatives are possible. The ping helps mitigate this. ✓

4. Privilege Requirements: Commands like arp, route, and ping don't require root, which is good for security. ✓

---

🧪 Test Coverage

Missing: No unit tests for the new functionality.

Recommendations:

• Add tests for MAC address validation edge cases
• Mock subprocess calls to test gateway change detection logic
• Test state persistence across runs
• Test behavior when commands fail/timeout

Note: I understand this is a menubar script where integration testing is challenging, but core validation/parsing logic could be unit tested.

---

📝 Code Quality Notes

1. Consistent Style: Matches existing codebase patterns ✓
2. Documentation: Good docstrings and inline comments ✓
3. Type Hints: Proper use of Optional, Dict, List types ✓
4. Error Handling: Graceful degradation when commands fail ✓
5. Configuration: Properly integrated with xbar config system ✓

---

🎯 Summary & Recommendations

Overall Assessment: This is a valuable feature that's well-implemented. The critical command injection issue should be fixed, but the risk is low given the data sources.

Priority Fixes:

1. 🔴 High: Add shlex.quote() to shell commands with variable interpolation
2. 🟡 Medium: Improve MAC address validation with regex
3. 🟡 Medium: Add deduplication for gateway change alerts
4. 🟢 Low: Make route parsing more robust

Approved with recommendations: I'd feel comfortable merging this after addressing the command injection concern, even if other improvements are deferred to future PRs.

Great work on implementing a frequently-requested security feature! 🎉