Repository navigation
refactor(security): harden mac_address_spoof_automator input validation and failure handling - #2
Conversation
krishsharma-code
left a comment
krishsharma-code
left a comment
There was a problem hiding this comment.
Self-review of PR #2 — mac_address_spoof_automator.py.
Submitted as COMMENT because GitHub does not allow approving your own pull request. Scope: one script and its new test module.
What was reviewed
Security boundaries (input validation, privilege handling), failure/edge-case handling, memory footprint, and docstring/comment clarity for python_scripts/mac_address_spoof_automator.py.
What was found
- Availability defect (high).
change_mac()brought the interface down, then set the address, then brought it up — with no rollback. Any failure on the middle step left the interface down. A tool whose whole job is to stay unnoticed should not knock the host off the network when it fails. - Input validation (medium). The MAC regex was applied with
re.match()and no end anchor, soaa:bb:cc:dd:ee:ff:zzpassed validation and was handed toip. The interface argument was not validated at all, so-xwould be parsed as an option. - Privilege handling (medium). No pre-flight root check; the script failed mid-sequence rather than before touching the interface.
- Correctness (low/medium).
get_random_mac()used the fixed Xen OUI00:16:3eand never set the locally-administered bit — a spoofed MAC that impersonates a real vendor prefix and can collide. - Standards (low). Raw
sys.argvparsing instead ofargparse(CONTRIBUTING.md), thin docstrings, and no test module.
What changed and why
See the inline comments. In short: up now runs in a finally; both inputs are validated with anchored, compiled patterns; a root/PATH pre-check runs before any state change; the generator now emits locally-administered unicast addresses; the CLI moved to argparse with a --dry-run; and 24 tests cover the validation, rollback and exit-code contract. Memory footprint is unchanged and O(1) — the script never buffers file contents.
Verification: python3 -m py_compile python_scripts/*.py and flake8 --select=E9,F63,F7,F82,F821,F823 --max-line-length=120 pass; python3 -m pytest tests/ -q reports 50 passed (24 new).
| except subprocess.CalledProcessError as exc: | ||
| print(f"[-] Error: Failed to change MAC address. " | ||
| f"Ensure you have root privileges: {exc}") | ||
| finally: |
There was a problem hiding this comment.
The key fix. Bringing the interface back up now happens in a finally, so a failure in the address step (bad MAC, lost privileges, driver rejection) can no longer leave the host offline. The inner try/except deliberately catches CalledProcessError only — if up itself fails we surface a warning rather than masking the original error. Is catching only CalledProcessError the right boundary here, or should a missing ip binary (FileNotFoundError) also be handled at this level?
| # A MAC address is six colon-separated hex octets. The trailing \Z anchors the | ||
| # match so junk such as "aa:bb:cc:dd:ee:ff:zz" or "aa:bb:cc:dd:ee:ff00" is | ||
| # rejected rather than silently accepted by an unanchored re.match(). | ||
| _MAC_RE = re.compile(r"[0-9a-f]{2}(?::[0-9a-f]{2}){5}\Z") |
There was a problem hiding this comment.
End-anchored with \Z and used via fullmatch(), so trailing junk (aa:bb:cc:dd:ee:ff:zz, aa:bb:cc:dd:ee:ff00) is now rejected. Previously re.match() accepted it and passed the value straight to ip link set.
| # consist of letters, digits and a few punctuation characters. The first | ||
| # character must be alphanumeric so a fat-fingered or hostile argument such as | ||
| # "-x" cannot be interpreted as an option by `ip`. | ||
| _INTERFACE_RE = re.compile(r"[A-Za-z0-9][A-Za-z0-9_.:-]{0,14}\Z") |
There was a problem hiding this comment.
New interface-name guard. Requiring an alphanumeric first character stops a value like -x being interpreted as an option by ip; the 15-character cap matches IFNAMSIZ-1. Should we also verify the interface actually exists (e.g. against /sys/class/net) before attempting the change, or is that better left to ip itself?
| return 1 | ||
|
|
||
| if not args.dry_run: | ||
| if os.geteuid() != 0: |
There was a problem hiding this comment.
Root and ip-on-PATH checks now run before any state change, so a non-root invocation fails cleanly instead of half-way through. --dry-run intentionally bypasses these so the commands can be inspected without privileges.
| random.randint(0x00, 0xff), | ||
| random.randint(0x00, 0xff)] | ||
| return ':'.join(map(lambda x: "%02x" % x, mac)) | ||
| first_octet = (random.randint(0x00, 0xFF) | _LOCALLY_ADMINISTERED) & ~_MULTICAST |
There was a problem hiding this comment.
Generator correctness fix: the locally-administered bit is set and the multicast bit cleared, and the whole address is randomised. The old fixed OUI 00:16:3e (Xen) made spoofed hosts impersonate a real vendor prefix. random is kept over secrets on purpose — a MAC address is broadcast on the wire, so unpredictability is not a security requirement here.
| True when the string is exactly six colon-separated hex octets. | ||
| """ | ||
| Configures the network interface with a new MAC address. | ||
| return bool(_MAC_RE.fullmatch(value.lower())) |
There was a problem hiding this comment.
fullmatch() + \Z makes the anchoring explicit and readable; the previous inline re.match(...) call both failed to anchor and recompiled the pattern on every invocation.
Summary
Static code-quality and security review of
python_scripts/mac_address_spoof_automator.py(Day 12 - System and Network Enumeration). The script brings an interface down, assigns a new MAC address and brings the interface back up. The review found one genuine availability defect, an unanchored input-validation regex, a missing privilege pre-check, and a MAC generator that emitted a real vendor OUI. This PR fixes those, adds a--dry-runmode, and adds a focused test module.Linked Issue
No tracking issue - this is a routine code-quality audit of an existing script.
Type of Change
What Changed
change_mac()randownthenaddressthenup. If theaddressstep failed (bad MAC, dropped privileges), the interface was left down - a network outage on the host. Theupstep now runs in afinallyblock, so the interface is always restored, and a failure to restore is reported rather than swallowed.re.match(r"[0-9a-f]{2}([:][0-9a-f]{2}){5}", ...), which accepted trailing junk such asaa:bb:cc:dd:ee:ff:zzoraa:bb:cc:dd:ee:ff00. Replaced with a compiled, end-anchored_MAC_REused viafullmatch().-xwould be read as an option byip. Added_INTERFACE_RE(first character alphanumeric,IFNAMSIZ-1length cap) and anis_valid_interface()guard.os.geteuid() != 0check and anip-on-PATH check, so the script fails cleanly before touching the interface instead of part-way through.get_random_mac()used the fixed Xen OUI00:16:3e, so spoofed hosts impersonated a real vendor prefix and could collide. It now sets the locally-administered bit (0x02), clears the multicast bit, and randomises the whole address.sys.argvparsing withargparse(perCONTRIBUTING.md), which supplies a--helppath and a new--dry-runflag that prints the exact commands without changing anything.tests/test_mac_address_spoof_automator.py(24 cases) covering detection-adjacent validation, redaction-equivalent safety (interface rollback), generation invariants and the exit-code contract.New or Modified Scripts
python_scripts/mac_address_spoof_automator.py--dry-run, argparse CLI, docstringstests/test_mac_address_spoof_automator.pySafety Review
--dry-runTesting Checklist
python3 -m py_compile python_scripts/*.pypassesflake8 python_scripts --select=E9,F63,F7,F82,F821,F823 --max-line-length=120passesfor f in bash_scripts/*.sh; do bash -n "$f"; donepasses (untouched)shellcheck bash_scripts/*.shreports no errors (advisory, untouched)--helpoutputLocal run:
python3 -m pytest tests/ -q-> 50 passed (24 new + 26 existing).Documentation
README.md(Day 12, Network Enumeration)Code Quality Checklist
Additional Notes
Behaviour is preserved except where a genuine defect is fixed (rollback on failure, stricter validation,
--dry-run, argparse). The MAC generator change means newly generated addresses are locally administered rather than impersonating the Xen OUI; this is a correctness fix for a spoofing tool, not a stylistic preference.randomis retained oversecretsbecause a MAC address is broadcast on the wire and carries no secrecy requirement.