[REVIEW] firewall-review: add rule evidence matrix

[yunrongy424-oss]

Skill Being Reviewed

Skill name: firewall-review
Skill path: skills/network/firewall-review/

False Positive Analysis

Benign code/configuration that can be over-reported without more evidence context:

resource "aws_security_group_rule" "private_endpoint" {
  type              = "ingress"
  protocol          = "tcp"
  from_port         = 443
  to_port           = 443
  cidr_blocks       = ["10.20.0.0/16"]
  description       = "Private east-west app traffic approved by CHG-1842"
}

Why this is a false positive risk:
The current skill correctly flags broad permits and rule hygiene issues, but it does not require a structured evidence record for asset zone, business owner, change ticket, hit-count age, source/destination role, or compensating controls. A rule can look broad in isolation while still being scoped to a private segment and approved flow. Without an evidence matrix, review output may overstate severity or under-document why a rule is acceptable.

Coverage Gaps

Missed variant 1: NAT/security policy mismatch

NAT policy:
  source 10.10.0.0/16 -> translated public pool

Security policy:
  allow any -> 10.10.0.0/16 tcp/443

Why it should be caught:
Firewall reviews often need both NAT and security policy context. Reviewing the security rule alone can miss effective exposure after translation, reverse-path behavior, or a policy that protects pre-NAT addresses but not post-NAT traffic.

Missed variant 2: Object-group expansion hides overly broad members

object-group network APP_PROD
  network-object 10.0.0.0 255.0.0.0
  network-object host 10.2.3.4

access-list OUTSIDE_IN extended permit tcp any object-group APP_PROD eq 443

Why it should be caught:
The skill asks for source/destination documentation, but it does not explicitly require expanded object/member evidence. Reviewers can miss that a friendly object name includes a broad CIDR, stale host, or mixed environment member.

Missed variant 3: Rule lifecycle and hit-count evidence uncertainty

Rule 120: permit tcp any host 10.5.10.20 eq 22
Hit count: 0
Counter reset: last firewall failover 2 hours ago

Why it should be caught:
The skill mentions hit counters resetting as a pitfall, but the output format does not require counter-baseline evidence or a confidence rating. This can lead to unsafe removal recommendations or weak bounty review evidence.

Edge Cases

• Cloud security groups, network ACLs, Kubernetes NetworkPolicy, and hardware firewalls expose rule evidence in different shapes. A normalized evidence matrix would make the review comparable across platforms.
• First-match policy analysis needs explicit source of truth for rule order, object expansion, NAT stage, and default policy. Otherwise a shadowed-rule finding may be speculative.
• IPv6, DNS egress, management-plane, and emergency-access rules are often stored in separate policy sets. The skill should make out-of-scope policy sets visible as "Not Evaluable" instead of silently omitting them.
• Logged permit traffic can be unavailable by design because of cost or privacy controls. Findings should separate "not configured" from "not evidenced".

Remediation Quality

• Fix resolves the vulnerability
• Fix doesn't introduce new security issues
• Fix doesn't break functionality
• Issues found: The skill has a strong baseline checklist, but it needs evidence confidence, Not Evaluable reason codes, and a normalized rule matrix so findings are auditable instead of only narrative.

Comparison to Other Tools

Tool	Catches this?	Notes
Semgrep	No	Not a general fit for ordered firewall rule semantics, NAT stage, or object expansion evidence.
CodeQL	No	CodeQL is strong for source-code dataflow, but not for firewall rulebase policy evaluation.
Cloud-native config scanners	Partial	Can flag open security groups or missing logging, but often miss business justification, hit-count age, shadowing context, NAT stage, and evidence confidence.
Firewall policy managers	Partial	Can detect shadowed/unused rules when integrated with devices, but this skill still needs portable report fields for manual/local review.

Overall Assessment

Strengths:

• Clear CIS Controls v8 and NIST SP 800-41 framing.
• Good coverage of default deny, any/any, shadowed rules, unused rules, logging, egress, IPv6, and cloud firewall pitfalls.
• Useful output structure for a prioritized firewall audit report.

Needs improvement:

• Add a normalized rule evidence matrix for rule ID, direction, zone, source, destination, service, action, owner, ticket, hit count, last-used age, logging, NAT/security policy relationship, and confidence.
• Require evidence confidence levels so findings distinguish source-controlled IaC, exported device config, firewall manager exports, runtime counters, flow logs, SIEM evidence, and policy narratives.
• Add Not Evaluable reason codes for missing object expansion, missing runtime counters, missing NAT stage, missing IPv6 policy, missing egress path, or stale/incomplete exports.

Priority recommendations:

1. Add a "Rule Evidence Matrix" section before final reporting.
2. Add confidence levels and Not Evaluable reason codes to reduce speculative findings.
3. Expand output fields and pitfalls for object groups, NAT stage, counter baseline, and policy/export freshness.

Bounty Info

• I have read and agree to the CONTRIBUTING.md bounty terms
• Preferred payment method: PayPal - 1005150221@qq.com

[yunrongy424-oss]

Improvement PR submitted for this review: #61

Preferred payment method remains PayPal: 1005150221@qq.com