Port IPMatcher from firewall-node - #357
Conversation
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| private static void normalizeMappedNetwork(ParsedNetwork parsed) { | ||
| if (parsed.version == IPV6 | ||
| && parsed.prefix >= 96 | ||
| && isIPv4Mapped(parsed.ipv6HighAddress, parsed.ipv6LowAddress)) { | ||
| parsed.version = IPV4; | ||
| parsed.ipv4Address = (int) parsed.ipv6LowAddress; | ||
| parsed.prefix -= 96; | ||
| } | ||
| } | ||
|
|
||
| private static boolean isIPv4Mapped(long high, long low) { | ||
| return high == 0 && (low >>> 32) == 0x0000ffffL; |
There was a problem hiding this comment.
🟠 High - IPMatcher stops treating IPv4-compatible IPv6 literals as the equivalent private/IMDS IPv4 address
The new matcher only bridges ::ffff/96 addresses back into the IPv4 tables, so IPv4-compatible IPv6 literals such as ::169.254.169.254 or ::10.0.0.1 remain in the IPv6 path and never match the stored IPv4 private, IMDS, allowlist, or blocklist entries. DNSRecordCollector feeds InetAddress.getHostAddress() output directly into SSRFDetector, which returns early when containsPrivateIP misses, so an attacker-controlled AAAA record using this notation can bypass the PR's SSRF/private-IP protections; the same family mismatch also weakens configured inbound IP allow/block checks.
Show fix
Normalize all IPv4-convertible IPv6 forms, not just ::ffff/96, before storing or matching networks, and add regression tests for IPv4-compatible forms such as ::169.254.169.254 and ::10.0.0.1.
More info - Reply on this comment to give feedback or ignore the issue.
There was a problem hiding this comment.
Identical behaviour as https://github.com/AikidoSec/firewall-node/blob/main/library/helpers/extractIPv4FromMapped.ts (and also the previous IPList implementation)
58d384e to
f1d76dc
Compare
|
|
||
| public IPList() { | ||
| this.ipAddresses = new DualIPv4v6Tries(); | ||
| matcher = IPMatcher.from(List.of()); |
There was a problem hiding this comment.
The no-argument constructor duplicates initialization logic from the collection constructor. Delegate with this(List.of()) to keep one initialization path.
| matcher = IPMatcher.from(List.of()); | |
| this(List.of()); |
Details
✨ AI Reasoning
Both constructors initialize the same field through the same factory with an empty collection in one case. Delegating the no-argument constructor to the collection constructor would provide a single initialization path without changing behavior.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
| private static boolean parseIPv6(String value, int start, int end, ParsedNetwork output) { | ||
| if (start < end && value.charAt(start) == '[') { | ||
| int closingBracket = lastIndexOf(value, ']', start + 1, end); | ||
| if (closingBracket >= 0) { | ||
| start++; | ||
| end = closingBracket; | ||
| } | ||
| } | ||
| if (start == end) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🟡 Medium - Scoped IPv6 link-local literals are no longer classified as private addresses
The new parser rejects the %zone suffix used by valid scoped IPv6 literals such as fe80::1%eth0, because parseIPv6 passes the suffix into parseHextet and removePortInfo never strips it. DNSRecordCollector forwards InetAddress.getHostAddress() strings into SSRFDetector, and that detector returns immediately when containsPrivateIP misses, so outbound requests to scoped link-local targets can bypass the PR's private-IP SSRF guard instead of being blocked or reported.
Show fix
| private static boolean parseIPv6(String value, int start, int end, ParsedNetwork output) { | |
| if (start < end && value.charAt(start) == '[') { | |
| int closingBracket = lastIndexOf(value, ']', start + 1, end); | |
| if (closingBracket >= 0) { | |
| start++; | |
| end = closingBracket; | |
| } | |
| } | |
| if (start == end) { | |
| return false; | |
| } | |
| private static boolean parseIPv6(String value, int start, int end, ParsedNetwork output) { | |
| if (start < end && value.charAt(start) == '[') { | |
| int closingBracket = lastIndexOf(value, ']', start + 1, end); | |
| if (closingBracket >= 0) { | |
| start++; | |
| end = closingBracket; | |
| } | |
| } | |
| int zoneSeparator = indexOf(value, '%', start, end); | |
| if (zoneSeparator >= 0) { | |
| end = zoneSeparator; | |
| } | |
| if (start == end) { | |
| return false; | |
| } |
More info - Reply on this comment to give feedback or ignore the issue.
| if (!Parser.parseBaseNetwork(value, parsed) | ||
| || parsed.version != IPV6 | ||
| || !isIPv4Mapped(parsed.ipv6HighAddress, parsed.ipv6LowAddress)) { |
There was a problem hiding this comment.
The compound validation condition combines three independent rejection cases, making the mapped-address path harder to read.
Show fix
| if (!Parser.parseBaseNetwork(value, parsed) | |
| || parsed.version != IPV6 | |
| || !isIPv4Mapped(parsed.ipv6HighAddress, parsed.ipv6LowAddress)) { | |
| if (!Parser.parseBaseNetwork(value, parsed)) { | |
| return false; | |
| } | |
| if (parsed.version != IPV6) { | |
| return false; | |
| } | |
| if (!isIPv4Mapped(parsed.ipv6HighAddress, parsed.ipv6LowAddress)) { |
Details
✨ AI Reasoning
The matching method first performs a successful match check, then uses one compound condition to reject null, unparsable, and non-IPv6/non-mapped values. These are independent validation failures, so presenting them as separate guard clauses would make the remaining mapped-address logic easier to follow.
Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info
91d65d3 to
d899e40
Compare
|
|
||
| List<String> addresses = new ArrayList<>(ips); | ||
| for (String ip : ips) { | ||
| String mappedAddress = mapIPv4ToIPv6(ip); |
There was a problem hiding this comment.
🟡 Medium - Mapped-address expansion breaks legacy IPv4 allowlist and bypass entries
createIPListWithMappedAddresses now prepends ::ffff: to every colon-free entry, but shorthand IPv4 forms that this matcher still accepts as valid configuration, such as 127.1 or 10.*.*.*, are not valid embedded-IPv4 syntax inside IPv6 literals. Those synthesized entries are ignored, and the affected call sites then use plain matches(...) instead of the mapped fallback, so an incoming ::ffff: client address no longer matches the equivalent configured allowlist or bypass rule. In deployments where reverse proxies or the JVM expose clients as IPv4-mapped IPv6, this causes endpoint allowlists to deny legitimate traffic and bypass lists to stop exempting the intended clients.
Show fix
Canonicalize supported legacy IPv4 forms to a real IPv4 address/network before generating ::ffff: variants, or perform mapped-address fallback at lookup time for these call sites instead of synthesizing mapped strings from the raw configuration text.
More info - Reply on this comment to give feedback or ignore the issue.
2 participants
No description provided.