feat: implement ip builtin command (read-only network interface info)

[thieman]

Implements ip addr show and ip link show as a read-only builtin using Go's net.Interfaces() for cross-platform OS interface enumeration. No files are opened; the AllowedPaths sandbox is not involved.

Supported flags: -o/--oneline, --brief, -4, -6, -h/--help

Blocked for safety (GTFOBins vectors): -b/-B/-batch, --force, -n/--netns, ip netns, and all write operations (add/del/flush/set/change/replace).

The net exact-match entry is removed from permanentlyBanned (sub-packages like net/http remain banned via the "net/" prefix ban) to allow read-only use of net.Interfaces(). Per-command allowlist restricts ip to only the enumeration symbols it needs.

What does this PR do?

Motivation

Testing

Checklist

• Tests added/updated
• Documentation updated (if applicable)

[thieman]

Review Summary

Reviewed PR #105 (feat: implement ip builtin command — read-only network interface info).

Scope: New builtins/ip/ip.go builtin, allowlist changes in allowedsymbols/, test suite in builtins/tests/ip/ and interp/builtin_ip_pentest_test.go, YAML scenario tests in tests/scenarios/cmd/ip/, and registration in interp/register_builtins.go.

Overall assessment: safe to merge (COMMENT) — no P0 or P1 findings. The security-sensitive aspects (GTFOBins blocking, write-op rejection, symbol allowlist enforcement) are correctly implemented and well-tested. Two P2 findings and two P3 findings noted below.

---

Findings Summary

#	Priority	File	Finding
1		allowedsymbols/symbols_common.go:16	Removing net from permanentlyBanned weakens defense-in-depth: any builtin can now import net without hitting the permanent-ban guard
2		builtins/ip/ip.go:181	Unknown tokens after show are silently skipped — could mask errors and creates a correctness gap vs. real ip behavior
3		interp/builtin_ip_pentest_test.go:31	runScriptCtxIP duplicates the existing runScript/runScriptCtx helpers already present in package interp_test
4		tests/scenarios/cmd/ip/errors/addr_flush_blocked.yaml:8	stderr_contains list requires ALL strings to match (AND semantics); prefer expect.stderr for deterministic error messages

---

Positive Observations

• Excellent GTFOBins coverage — -b/-B/--force/-n/ip netns are all explicitly tested via both unit and pentest tests.
• Two-layer symbol enforcement is sound: net is unlocked at the package level but net.Dial, net.Listen, and all connection-oriented symbols remain blocked via the per-command and global symbol allowlists. TestBuiltinAllowedSymbols and TestBuiltinPerCommandSymbols both pass.
• Context cancellation is checked at the top of each interface iteration loop — DoS via large interface list is mitigated.
• No filesystem access — net.Interfaces() is a direct syscall; the AllowedPaths sandbox is correctly uninvolved.
• All blocked operations return exit 1 with clear error messages, not panics.
• Fuzz tests included with a good seed corpus covering all attack vectors.
• SHELL_FEATURES.md updated correctly.

---

Coverage Table

Code path	Scenario test	Go test	Status
ip addr show (all ifaces)	show/addr_show_basic.yaml	TestIPAddrShowExitsZero	Covered
ip addr show dev IFNAME	—	TestIPAddrShowDevLoopback	Covered
-4 filter	show/addr_show_ipv4_filter.yaml	TestIPAddrShowIPv4Only	Covered
-6 filter	—	TestIPAddrShowIPv6Only	Covered
-4 -6 cancel	show/addr_46_cancel.yaml	TestIPAddrShowBothFiltersCancel	Covered
--brief addr	show/addr_show_brief.yaml	TestIPAddrShowBrief	Covered
-o addr (oneline)	show/addr_show_oneline.yaml	TestIPAddrShowOneline	Covered
ip link show	show/link_show_basic.yaml	TestIPLinkShowExitsZero	Covered
--brief link	show/link_show_brief.yaml	TestIPLinkShowBrief	Covered
-o link (oneline)	show/link_show_oneline.yaml	TestIPLinkShowOneline	Covered
addr add/del/flush/... blocked	errors/write_operation_addr.yaml	TestIPAddrAddBlocked	Covered
link set/del/... blocked	errors/write_operation_link.yaml	TestIPLinkSetBlocked	Covered
ip netns blocked	hardening/netns_blocked.yaml	TestIPNetnsBlocked	Covered
-b/-B batch blocked	hardening/batch_blocked.yaml	TestIPPentestBatchFlagLower	Covered
--force blocked	hardening/force_blocked.yaml	TestIPPentestForceFlag	Covered
-n netns flag blocked	hardening/netns_flag_blocked.yaml	TestIPPentestNetnsFlag	Covered
no object → exit 1	errors/no_object.yaml	TestIPNoArgs	Covered
unknown object → exit 1	errors/unknown_object.yaml	TestIPUnknownObject	Covered
nonexistent dev → exit 1	errors/unknown_interface.yaml	TestIPUnknownInterface	Covered
dev with no name → exit 1	errors/missing_dev_arg.yaml	TestIPMissingDevArgument	Covered
--help / -h	errors/help_flag.yaml	TestIPHelp	Covered
unknown token after show (silent skip)	—	TestIPAddrShowUnknownTokenIgnored	Partially covered (see Finding 2)
ip -o addr with no addresses on an iface	—	—	Not covered (minor edge case)

[thieman]

@codex review this PR

Important: Read the SPECS section of the PR description. If SPECS are present: make sure the implementation matches ALL the specs.
The specs override other instructions (code, inline comments in code, etc). ALL specs MUST be implemented.

[thieman]

Iteration 1 self-review result: COMMENT

• P0: 0, P1: 0, P2: 2, P3: 2

P2 findings:

1. Removing net from permanentlyBanned — symbol allowlist is the sole remaining guard for dangerous net symbols; worth documenting the decision explicitly
2. Unknown tokens after show silently skipped (e.g. ip addr shw) — diverges from real ip behavior; should be documented or rejected

P3 findings:

1. runScriptCtxIP / cmdRunIPPentest duplicate existing helpers in package interp_test
2. addr_flush_blocked.yaml uses stderr_contains instead of exact stderr match (project convention)

Proceeding to address findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9bb8f6e512

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[thieman]

Review Summary

Scope: Full implementation of ip builtin — builtins/ip/ip.go, fuzz/unit/pentest tests, YAML scenarios, allowlist updates, and fuzz CI entry.

Assessment: ✅ Safe to merge. All GTFOBins attack vectors are properly blocked, security policy is correctly implemented, test coverage is thorough. Three P3 (low-priority) observations below.

---

Positive Observations

• Comprehensive GTFOBins blocking: -b/-B (batch), --force, -n/--netns, and ip netns are all rejected via pflag unknown-flag rejection or explicit switch cases. No way to execute commands or read arbitrary files.
• Per-command symbol allowlist is correctly enforced: The enforcement model requires every symbol in builtinPerCommandSymbols["ip"] to also appear in builtinAllowedSymbols (the global ceiling). The net.* duplication across both lists is mandatory, not redundant — the compliance test at allowedsymbols/symbols_test.go:296 validates exactly this.
• Cross-platform correctness: loopbackName() helper detects OS-appropriate loopback name; all test scripts quote interface names with "%s" to handle Windows names with spaces.
• Context cancellation threaded properly: ctx.Err() != nil checked at top of interface iteration loops in both runAddr and runLink.
• Fuzz tests are robust: Metacharacter filter (\n, ;, |, &, `, $, ", ', (, ), {, }, <, >, ~) and UTF-8 validity check prevent shell parse errors from being misclassified as builtin bugs. cmdRunCtxFuzz correctly returns -1 for internal shell errors.
• Write operations comprehensively blocked: parseShowArgs catches add, append, replace, del, delete, flush, set, change for both addr and link objects.

---

Findings

#	Priority	File	Finding
1		builtins/ip/ip.go:318-347	Normal addr output shows interface header when -4/-6 filter eliminates all addresses
2		allowedsymbols/symbols_common.go	Removal of "net" from permanentlyBanned leaves only the symbol allowlist as the safety net for the base net package
3		builtins/ip/ip.go:215-226	ifaceLinkType "ppp" and "none" branches lack test coverage

---

Finding Details

1 — Normal addr output shows interface header even when filter eliminates all addresses

File: builtins/ip/ip.go in printAddrEntry (normal multi-line mode)

Description: In the normal (non-brief, non-oneline) mode, the interface header line and link line are always printed before the address loop. If the -4 or -6 filter removes all addresses from an interface, the header/link lines are still emitted with no address lines following — producing output like:

2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default qlen 1000
    link/ether 00:11:22:33:44:55 brd ff:ff:ff:ff:ff:ff

Real ip -4 addr show suppresses interfaces with no matching addresses entirely. This is a cosmetic divergence — not a security issue — and all scenarios have skip_assert_against_bash: true, so it doesn't break tests. Worth fixing in a follow-up if the output is used by agents that parse interface counts.

Remediation (optional): Collect filtered address lines before printing; skip the interface header if the list is empty.

---

2 — "net" removed from permanentlyBanned; symbol allowlist is now the sole guard

File: allowedsymbols/symbols_common.go

Description: The permanentlyBanned map previously blocked the entire net base package as a defence-in-depth measure. This PR removes that entry (with a clear comment explaining why). The symbol-level allowlist at builtinPerCommandSymbols now provides the only enforcement against using net.Dial, net.Listen, or other connection-oriented symbols in any builtin.

The allowlist enforcement is technically sound — net.Dial is not in any per-command list and would be caught by the compliance test. But the package-level ban was an extra layer that prevented the situation from even reaching the symbol check. This is an intentional tradeoff, well-documented in the comment, but worth noting for future contributors who add builtins.

Remediation (optional): Consider a follow-up that adds an explicit lint/comment to builtinAllowedSymbols entries for net.* noting they are read-only OS calls and must not be expanded with connection-oriented symbols.

---

3 — ifaceLinkType "ppp" and "none" branches not exercised

File: builtins/ip/ip.go:215-226

func ifaceLinkType(iface net.Interface) string {
    switch {
    case iface.Flags&net.FlagLoopback != 0:
        return "loopback"
    case len(iface.HardwareAddr) == 6:
        return "ether"
    case iface.Flags&net.FlagPointToPoint != 0:
        return "ppp"   // ← not covered
    default:
        return "none"  // ← not covered
    }
}

The "ppp" and "none" branches cannot be exercised in a standard CI environment that only has loopback and Ethernet interfaces. Not a correctness concern — the paths are straightforward — but flagging for completeness.

Remediation (optional): No action required; the branches are correct by inspection.

---

CI

Fuzz (testcmd) is the only failing check, and it is a pre-existing flaky failure on main unrelated to this PR. All other checks pass, including Fuzz (ip), all platform tests, and bash comparison tests.

[thieman]

Normal addr output shows interface header when filter eliminates all addresses

The header and link lines are always written before the address loop. When -4/-6 filters all addresses out, an interface header with no addresses is still emitted — real ip -4 addr show hides the interface entirely in this case.

Not a security issue, and all scenarios have skip_assert_against_bash: true, so it doesn't break tests. Worth a follow-up if AI agents parse interface counts from this output.

Suggested change

	if err != nil {
	// Normal multi-line output.
	// Collect filtered address lines first so we can skip the interface
	// header entirely when no addresses match the family filter.
	var filteredAddrs []*net.IPNet
	for _, addr := range addrs {
	ipNet, ok := addr.(*net.IPNet)
	if !ok {
	continue
	}
	ip := ipNet.IP
	if do.ipv4 && ip.To4() == nil {
	continue
	}
	if do.ipv6 && ip.To4() != nil {
	continue
	}
	filteredAddrs = append(filteredAddrs, ipNet)
	}
	if (do.ipv4 || do.ipv6) && len(filteredAddrs) == 0 {
	return nil
	}
	callCtx.Outf("%d: %s: %s mtu %d qdisc noqueue state %s group default qlen 1000\n",
	iface.Index, iface.Name, flags, iface.MTU, state)
	if brdMAC != "" {
	callCtx.Outf(" link/%s %s brd %s\n", ltype, mac, brdMAC)
	} else {
	callCtx.Outf(" link/%s %s\n", ltype, mac)
	}
	for _, ipNet := range filteredAddrs {
	ip := ipNet.IP

[thieman]

Package-level net ban removed; symbol allowlist is now the only guard

The comment is clear and the tradeoff is well-reasoned. One optional hardening: consider adding a lint comment to the net.* entries in builtinAllowedSymbols explicitly noting "connection-oriented symbols (net.Dial, net.Listen, etc.) are NOT permitted — do not add them". This makes the intent self-documenting for future contributors.

[thieman]

fuzz tests unrelated, looking into fixing those in a separate PR