reject IPv4-embedded IPv6 prefixes.

[consuelo2210]

Reject IPv4-compatible and IPv4-mapped IPv6 prefixes when parsing IPv6 packet filtering rules.

IPv4-compatible addresses have been deprecated since RFC 4291. RFC 6890 further specifies that IPv4-mapped addresses are not valid IPv6 packet source or destination addresses (Source=False, Destination=False). Therefore, neither format is expected to appear in legitimate IPv6 packet traffic.

Rejecting these prefixes also simplifies GeoIP lookups. MaxMind DBs do not contain IPv4-compatible or IPv4-mapped IPv6 prefixes: IPv4 networks are stored exclusively in the IPv4 search tree. Users should use native IPv4 prefixes and IPv4 GeoIP databases instead of IPv4-embedded IPv6 forms.

Closes 336(escudo), 331(escudo)

[consuelo2210]

FAILED profiles/tests/test_xfw.py::test_zeroed_ipv6 - ValueError:
Server returned error (code 13): Internal server error.

[krizhanovsky]

LGTM, but a more careful review in terms of RFC is required. @EvgeniiMekhanik could you please have a look

[krizhanovsky]

~The RFC is update by https://datatracker.ietf.org/doc/html/rfc8190 ~ - it seems there are no relevant for us updates in 8190. Also please specify particular RFC chapter and table

[krizhanovsky]

Could you also reference RFC 4291 2.5.5 in the comment

[krizhanovsky]

We have a duplicate logic - can we adjust addr and use the routine from bpf_uapi/ip_helpers.h ?

[EvgeniiMekhanik]

static bool
xfw_is_ipv4_embedded_ipv6(const Ip6Addr &addr,
unsigned prefixlen) noexcept
{
if (prefixlen < 96)
return false;

...

} at least we should use such function to adjust prefixlen

[krizhanovsky]

Well, actually probably there are more things to check against the RFCs...

[krizhanovsky]

IIUC from our discussion the addresses are deprecated, but we still may receive them, i.e. we should not block them right?

RFC 6890 table 20 says that we should not accept IPv6-mapped addresses (Source=False, Destination=False), but I don't see the check here.

And I do not see an appropriate table in the RFC for embedded addresses (::0/96)

[krizhanovsky]

GPT-5.6-sol review:

• Found one functional issue:

• [P1] GeoIP import rejects broad, valid IPv6 prefixes — manager/xfw/geoip.cc:373

  xfw_is_ipv4_embedded_ipv6() examines only the canonical address and ignores prefixlen. Consequently, valid prefixes such as ::/0, ::/1, or ::/80 are classified as IPv4-embedded
  because their base address begins with 96 zero bits. An IPv6 MMDB containing a catch-all/root record will therefore throw and abort GeoIP configuration loading. Pass prefixlen
  into the check and reject only prefixes wholly inside the relevant /96, or handle broader prefixes while excluding the embedded subrange.

Also, git diff --check reports trailing whitespace in bpf_uapi/ip_helpers.h:44, though that is non-functional.

[krizhanovsky]

Review from Fable 5 (~20k tokens):

Review: irinak/deny_embedded_addresses vs main (f6917f2)

The change moves handling of IPv4-embedded IPv6 addresses (::a.b.c.d, ::ffff:a.b.c.d) from "convert and match against IPv4 rules" to "drop the packets and reject such prefixes in
config". The overall direction is sound and I verified the mechanics: the BPF programs compile cleanly (clang-18, tc.o/xdp.o), the changed manager files pass g++ -fsyntax-only
with the generated flatbuffers headers (full manager build isn't possible here — no gRPC dev headers installed), the guarded add_matchers_to<ProtoNetRule, NetIp> specialization
covers src, dst, and prot_net alike, and pytest.raises(ValueError) in the tests matches what the framework raises when the CLI push fails. The helper correctly excludes ::
(needed for NDP/DAD sources) and ::1, and does not match the NAT64 prefix 64:ff9b::/96, which is legitimate on-wire traffic — good.

Findings, most important first

1. Egress now silently bypasses dst policy for embedded-address packets — bpf/tc.c:62. On ingress, parse_ip6hdr returning -EINVAL drops the packet; on egress the same error is a
   PASS (XFW_IP6_BADHDR_EGRESS). Before this change, an egress packet to ::ffff:1.2.3.4 went through the normal dst lookup and was subject to defaults { dst: block; }; now it passes
   unfiltered. Externally-originated traffic is covered by the ingress XDP drop, so the hole is limited to host-generated packets (e.g. a raw socket), but a firewall passing
   traffic that a default-block policy used to catch deserves a deliberate decision. If "egress parsing never drops" is the intended invariant, consider at least a dedicated counter
   so this is visible.

2. Drop reason is lumped into IP6_BADHDR — bpf/parsing_helpers.h:207. Embedded-address drops share the XFW_IP6_BADHDR_INGRESS counter with genuinely malformed headers. Given the
   project's otherwise fine-grained stats, a distinct reason would help debugging when legitimate-looking traffic starts disappearing (e.g. a misconfigured tunnel emitting mapped
   addresses).

3. Geoip reimplements the helper with the same name but different semantics — manager/xfw/geoip.cc:349. The byte-based copy does not exclude the all-zero address, so a data
   record covering ::/N (N ≤ 96) — or a trivial single-record IPv6 MMDB — throws and aborts the entire geoip import. Real MaxMind DBs alias these ranges to the IPv4 subtree (which
   the walk skips), so it won't fire in practice, but hard-failing the whole DB load on a harmless prefix (those addresses can never match traffic anymore) is aggressive;
   skip-with-warning would be more robust. Also, two functions named xfw_is_ipv4_embedded_ipv6 with diverging edge-case behavior is a maintenance trap — either reuse the shared one
   via std::bit_cast to __be32[4], or give the local one a distinct name.

4. Error message says "IPv4-compatible" but rejects mapped too — manager/xfw_config_updater.cc:695. Should say "IPv4-embedded". Also, sibling validation errors in this function
   throw UpdateError; Except is caught upstream so it works, but UpdateError is the local idiom for config rejections.

5. Trailing whitespace — bpf_uapi/ip_helpers.h:44 has a tab-only line after return false;.

6. Test coverage gaps (minor): rejection is only exercised via rules_patch add/replace — there's no test that a full rules_set config containing an embedded address is rejected,
   and test_dst_del_block_by_ip_mapped was deleted without a reject counterpart (defensible, since you can't delete what can't be added). Test name
   test_dst_reject_on_replace_block_by_ip_by_allow_by_ip_mapped and its docstring read awkwardly ("by_allow" → "with allow"; "error has to appear on rules applying").

Also noted but fine: the guard is address-exact, so a src prefix like ::/96 that covers the embedded range still passes config validation — harmless since the packets are dropped
at parse time; and the configuration.hh hunks are pure indentation fixes.

Nothing here is a blocker; #1 is the one I'd want an explicit decision on before merge.

[EvgeniiMekhanik]

static __always_inline bool
xfw_is_legacy_ipv4_embedded_ipv6(const __be32 addr[4])
{
/* IPv4-compatible IPv6: ::/96 */
if (!addr[0] && !addr[1] && !addr[2])
return addr[3] != 0 && addr[3] != HTONL(1);

/* IPv4-mapped IPv6: ::ffff:0:0/96 */
return !addr[0] && !addr[1] &&
       addr[2] == HTONL(0xffff);

} may be in till be more clear? But up to you