Skip to content

OCPBUGS-67363: fixes ip traffic fragmentation verification #730

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
raphaelvrosa wants to merge 1 commit into
base: master
Choose a base branch
from
Open

OCPBUGS-67363: fixes ip traffic fragmentation verification #730

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ CSV_VERSION = $(shell echo $(VERSION) | sed 's/v//')
ifeq ($(VERSION), latest)
CSV_VERSION := 0.0.0
endif
CERT_MANAGER_VERSION=v1.9.1
CERT_MANAGER_VERSION=v1.20.2

@danwinship danwinship Jul 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where does this change come from?

@raphaelvrosa raphaelvrosa Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated for local testing.

IMAGE_ORG ?= $(USER)

# CHANNELS define the bundle channels used in the bundle.
Expand Down
10 changes: 10 additions & 0 deletions bpf/ingress_node_firewall.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,4 +90,14 @@ struct rulesVal_st {
struct ruleType_st rules[MAX_RULES_PER_TARGET];
} __attribute__((packed));

// ip_extract_l4info return codes
#define L4_OK 0 // extracted L4 info successfully
#define L4_TRUNCATED -1 // packet too short; pass to kernel for rejection
#define L4_FRAGMENTED -2 // fragmented packet; deny (INF cannot reassemble)

// IPv4 fragmentation constants (RFC 791)
#define IP_MF 0x2000 // More Fragments flag
#define IP_OFFSET_MASK 0x1FFF // Fragment offset mask (bits 0-12, in 8-byte units)
#define IP_DF 0x4000 // Don't Fragment flag

#endif
44 changes: 29 additions & 15 deletions bpf/ingress_node_firewall_kernel.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,9 @@ volatile const __u32 debug_lookup = 0;
* __u8 *icmpType: pointer to ICMP or ICMPv6's type value.
* __u8 *icmpCode: pointer to ICMP or ICMPv6's code value.
* Return:
* 0 for Success.
* -1 for Failure.
* L4_OK (0): extracted L4 info successfully.
* L4_TRUNCATED (-1): packet too short; pass to kernel for rejection.
* L4_FRAGMENTED (-2): fragmented packet; deny (INF cannot reassemble).
*/
__attribute__((__always_inline__)) static inline int
ip_extract_l4info(void *data, void *dataEnd, __u8 *proto, __u16 *dstPort,
Expand All @@ -107,14 +108,19 @@ ip_extract_l4info(void *data, void *dataEnd, __u8 *proto, __u16 *dstPort,
struct iphdr *iph = dataStart;
dataStart += sizeof(struct iphdr);
if (unlikely(dataStart > dataEnd)) {
return -1;
return L4_TRUNCATED;
}
*proto = iph->protocol;

__u16 frag_off = bpf_ntohs(iph->frag_off);
if (unlikely((frag_off & IP_OFFSET_MASK) || (frag_off & IP_MF))) {
return L4_FRAGMENTED;
}

@danwinship danwinship Jul 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to deny non-first fragments, it seems like we should deny first fragments too, since without the followup fragments the initial fragment will be useless.

So something like unlikely((frag_off & IP_OFFSET_MASK) != 0 || (frag_off & IP_MF))

@raphaelvrosa raphaelvrosa Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, since it is stateless, better to drop all fragmented traffic.

} else {
struct ipv6hdr *iph = dataStart;
dataStart += sizeof(struct ipv6hdr);
if (unlikely(dataStart > dataEnd)) {
return -1;
return L4_TRUNCATED;
}
*proto = iph->nexthdr;
}
Expand All @@ -123,7 +129,7 @@ ip_extract_l4info(void *data, void *dataEnd, __u8 *proto, __u16 *dstPort,
struct tcphdr *tcph = (struct tcphdr *)dataStart;
dataStart += sizeof(struct tcphdr);
if (unlikely(dataStart > dataEnd)) {
return -1;
return L4_TRUNCATED;
}
*dstPort = tcph->dest;
break;
Expand All @@ -132,7 +138,7 @@ ip_extract_l4info(void *data, void *dataEnd, __u8 *proto, __u16 *dstPort,
struct udphdr *udph = (struct udphdr *)dataStart;
dataStart += sizeof(struct udphdr);
if (unlikely(dataStart > dataEnd)) {
return -1;
return L4_TRUNCATED;
}
*dstPort = udph->dest;
break;
Expand All @@ -141,7 +147,7 @@ ip_extract_l4info(void *data, void *dataEnd, __u8 *proto, __u16 *dstPort,
struct sctphdr *sctph = (struct sctphdr *)dataStart;
dataStart += sizeof(struct sctphdr);
if (unlikely(dataStart > dataEnd)) {
return -1;
return L4_TRUNCATED;
}
*dstPort = sctph->dest;
break;
Expand All @@ -150,7 +156,7 @@ ip_extract_l4info(void *data, void *dataEnd, __u8 *proto, __u16 *dstPort,
struct icmphdr *icmph = (struct icmphdr *)dataStart;
dataStart += sizeof(struct icmphdr);
if (unlikely(dataStart > dataEnd)) {
return -1;
return L4_TRUNCATED;
}
*icmpType = icmph->type;
*icmpCode = icmph->code;
Expand All @@ -160,16 +166,16 @@ ip_extract_l4info(void *data, void *dataEnd, __u8 *proto, __u16 *dstPort,
struct icmp6hdr *icmp6h = (struct icmp6hdr *)dataStart;
dataStart += sizeof(struct icmp6hdr);
if (unlikely(dataStart > dataEnd)) {
return -1;
return L4_TRUNCATED;
}
*icmpType = icmp6h->icmp6_type;
*icmpCode = icmp6h->icmp6_code;
break;
}
default:
return -1;
return L4_TRUNCATED;
}
return 0;
return L4_OK;
}

/*
Expand All @@ -195,8 +201,12 @@ ipv4_firewall_lookup(void *data, void *data_end, __u32 ifId) {
__u8 icmpCode = 0, icmpType = 0, proto = 0;
int i;

if (unlikely(ip_extract_l4info(data, data_end, &proto, &dstPort, &icmpType,
&icmpCode, 1) < 0)) {
int l4_result = ip_extract_l4info(data, data_end, &proto, &dstPort, &icmpType,
&icmpCode, 1);
if (unlikely(l4_result != L4_OK)) {
if (l4_result == L4_FRAGMENTED) {
return SET_ACTIONRULE_RESPONSE(DENY, INVALID_RULE_ID);
}
ingress_node_firewall_printk("failed to extract l4 info");
return SET_ACTION(UNDEF);
}
Expand Down Expand Up @@ -291,8 +301,12 @@ ipv6_firewall_lookup(void *data, void *data_end, __u32 ifId) {
__u8 icmpCode = 0, icmpType = 0, proto = 0;
int i;

if (unlikely(ip_extract_l4info(data, data_end, &proto, &dstPort, &icmpType,
&icmpCode, 0) < 0)) {
int l4_result = ip_extract_l4info(data, data_end, &proto, &dstPort, &icmpType,
&icmpCode, 0);
if (unlikely(l4_result != L4_OK)) {
if (l4_result == L4_FRAGMENTED) {
return SET_ACTIONRULE_RESPONSE(DENY, INVALID_RULE_ID);
}
ingress_node_firewall_printk("failed to extract l4 info");
return SET_ACTION(UNDEF);
}
Comment on lines +304 to 312

@coderabbitai coderabbitai Bot Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm there's no other IPv6 extension-header walking logic elsewhere in the bpf dir
rg -n 'nexthdr|NEXTHDR|ipv6hdr' bpf/

Repository: openshift/ingress-node-firewall

Length of output: 1822

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the relevant IPv4/IPv6 parsing and lookup flow.
sed -n '80,170p' bpf/ingress_node_firewall_kernel.c
printf '\n---\n'
sed -n '280,340p' bpf/ingress_node_firewall_kernel.c
printf '\n---\n'
sed -n '340,420p' bpf/ingress_node_firewall_kernel.c

# Look for any IPv6 fragment/extension-header handling in the repo.
printf '\n=== search ===\n'
rg -n 'NEXTHDR_FRAGMENT|fragment|ext header|extension header|ipv6 fragment|IPV6_EXT' bpf/ .

Repository: openshift/ingress-node-firewall

Length of output: 18706

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Find how UNDEF propagates to the final packet decision.
rg -n 'SET_ACTION\(UNDEF\)|UNDEF|DENY|ALLOW|SET_ACTIONRULE_RESPONSE' bpf/ingress_node_firewall_kernel.c bpf/ingress_node_firewall.h

printf '\n=== action helpers ===\n'
sed -n '1,140p' bpf/ingress_node_firewall.h

printf '\n=== lookup callers ===\n'
rg -n 'ipv6_firewall_lookup|ipv4_firewall_lookup|SET_ACTIONRULE_RESPONSE|SET_ACTION\(' bpf/ingress_node_firewall_kernel.c

Repository: openshift/ingress-node-firewall

Length of output: 7842

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Read the final action mapping for XDP and TCX paths.
sed -n '440,575p' bpf/ingress_node_firewall_kernel.c

Repository: openshift/ingress-node-firewall

Length of output: 4810

IPv6 fragments still bypass port matching
ip_extract_l4info() only returns -2 for IPv4; the IPv6 path never checks NEXTHDR_FRAGMENT or walks extension headers. IPv6 packets with a Fragment header fall through as UNDEF and are passed, so this does not close the fragmentation bypass for IPv6. Add IPv6 extension-header parsing and deny non-first fragments.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@bpf/ingress_node_firewall_kernel.c` around lines 309 - 318, The current
fragment-handling branch in ip_extract_l4info() only denies IPv4 non-first
fragments via the -2 return path, but IPv6 Fragment headers are still not
detected. Update the IPv6 parsing path in bpf/ingress_node_firewall_kernel.c so
ip_extract_l4info() walks IPv6 extension headers, explicitly checks for
NEXTHDR_FRAGMENT, and identifies non-first fragments as a deny case; then make
the existing caller logic in the l4_result check return DENY for those IPv6
fragments instead of falling through to SET_ACTION(UNDEF).

@raphaelvrosa raphaelvrosa Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danwinship should this PR address this?

@danwinship danwinship Jul 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, we'll do a followup PR

@coderabbitai coderabbitai Bot Jul 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Expand Down
Binary file modified pkg/ebpf/bpf_arm64_bpfel.o
View file
Open in desktop
Binary file not shown.
Binary file modified pkg/ebpf/bpf_powerpc_bpfel.o
View file
Open in desktop
Binary file not shown.
Binary file modified pkg/ebpf/bpf_s390_bpfeb.o
View file
Open in desktop
Binary file not shown.
Binary file modified pkg/ebpf/bpf_x86_bpfel.o
View file
Open in desktop
Binary file not shown.
2 changes: 2 additions & 0 deletions test/Dockerfile.netcat
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
FROM alpine:latest

RUN apk add --no-cache --update --verbose bash nmap-ncat && \
apk add --no-cache --verbose hping3 \
--repository=https://dl-cdn.alpinelinux.org/alpine/edge/testing && \
rm -rf /var/cache/apk/* /tmp/*
46 changes: 45 additions & 1 deletion test/e2e/functional/tests/e2e.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1515,7 +1515,14 @@ func isConnectivitySeen(client *testclient.ClientSet, protocol ingressnodefwv1al
// Connectivity will be confirmed with server output later and expecting server output to contain clients IP.
_, _, _ = transport.ConnectToPortFromPod(client, protocol, v6, sourcePod, sourceIP, destinationIP, destinationPort)
serverResult := <-serverResultsCh
return strings.Contains(serverResult, sourceIP)
ncatResult := strings.Contains(serverResult, sourceIP)

// hping3 connectivity test with IP fragmentation
_, hpingStderr, hpingErr := transport.HpingFragmentedConnect(client, protocol, v6, sourcePod, destinationIP, destinationPort)
hpingResult := transport.IsHpingResponseSeen(hpingStderr, hpingErr)
log.Printf("isConnectivitySeen from %s to %s:%s (hping3 = %v) && (ncat = %v)", sourceIP, destinationIP, destinationPort, hpingResult, ncatResult)

return ncatResult
} else {
panic("Unexpected protocol")
}
Expand Down Expand Up @@ -1620,6 +1627,43 @@ func reachabilityCheck(reach reachable, podNameObj map[string]*corev1.Pod,
}
}

func reachabilityCheckFragmentation(reach reachable, podNameObj map[string]*corev1.Pod,
protocols []ingressnodefwv1alpha1.IngressNodeFirewallRuleProtocolType) {
sourcePod := podNameObj[reach.source]
for _, protocol := range protocols {
if skipProtocol(protocol, !v6Enabled) {
continue
}
if !infwutils.IsTransportProtocol(protocol) || protocol == ingressnodefwv1alpha1.ProtocolTypeSCTP {
continue
}
// v4 fragmentation tests
if v4Enabled && protocol != ingressnodefwv1alpha1.ProtocolTypeICMP6 {
destinationPodV4IP := pods.GetIPV4(podNameObj[reach.destination].Status.PodIPs)
By(fmt.Sprintf("[IPV4] Fragmentation check for protocol %s from pod %q to %s:%s",
protocol, reach.source, destinationPodV4IP, reach.port))
Eventually(func() bool {
_, stderr, err := transport.HpingFragmentedConnect(
testclient.Client, protocol, false, sourcePod, destinationPodV4IP, reach.port)
return transport.IsHpingResponseSeen(stderr, err)
}, timeout, retryInterval).Should(BeFalse(),
"Failed: IPv4 fragmented packets should be denied")
}
// v6 fragmentation tests
if !isSingleStack && v6Enabled && protocol != ingressnodefwv1alpha1.ProtocolTypeICMP {
destinationPodV6IP := pods.GetIPV6(podNameObj[reach.destination].Status.PodIPs)
By(fmt.Sprintf("[IPV6] Fragmentation check for protocol %s from pod %q to %s:%s",
protocol, reach.source, destinationPodV6IP, reach.port))
Eventually(func() bool {
_, stderr, err := transport.HpingFragmentedConnect(
testclient.Client, protocol, true, sourcePod, destinationPodV6IP, reach.port)
return transport.IsHpingResponseSeen(stderr, err)
}, timeout, retryInterval).Should(BeFalse(),
"Failed: IPv6 fragmented packets should be denied")
}
}
}

func checkNodeStateCreate(client *testclient.ClientSet, nodeStateList *ingressnodefwv1alpha1.IngressNodeFirewallNodeStateList) {
Eventually(func() bool {
err := client.List(context.Background(), nodeStateList)
Expand Down
41 changes: 41 additions & 0 deletions test/e2e/transport/transport.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@ func getClient(clientPodName, namespace string, labels, affinity, antiAffinity m
Name: "client",
Image: images.NetcatImage(),
Command: []string{"/bin/sh", "-c", "sleep INF"},
SecurityContext: &corev1.SecurityContext{
Capabilities: &corev1.Capabilities{
Add: []corev1.Capability{"NET_RAW"},
},
},
Resources: corev1.ResourceRequirements{
Requests: map[corev1.ResourceName]resource.Quantity{corev1.ResourceMemory: resource.MustParse("256Mi")},
Limits: map[corev1.ResourceName]resource.Quantity{corev1.ResourceMemory: resource.MustParse("512Mi")},
Expand Down Expand Up @@ -224,3 +229,39 @@ func ncClientTransport(client *testclient.ClientSet, sourcePod *corev1.Pod, sour
command := []string{"sh", "-c", fmt.Sprintf("ncat %s --wait 1 %s %s --verbose", strings.Join(additionalFlag, " "), destinationIP, dPort)}
return exec.RunExecCommandWithStdin(client, sourcePod, sourceIP, command...)
}

// HpingFragmentedConnect sends a fragmented packet to the destination using hping3.
// Analogous to ConnectToPortFromPod but for fragmented traffic testing.
func HpingFragmentedConnect(client *testclient.ClientSet, proto ingressnodefwv1alpha1.IngressNodeFirewallRuleProtocolType,
v6 bool, sourcePod *corev1.Pod, destinationIP, destinationPort string) (string, string, error) {
hping3Flags := []string{"-f", "-c", "1"}
if v6 {
hping3Flags = append(hping3Flags, "-6")
}

switch proto {
case ingressnodefwv1alpha1.ProtocolTypeTCP:
hping3Flags = append(hping3Flags, "-S")
case ingressnodefwv1alpha1.ProtocolTypeUDP:
hping3Flags = append(hping3Flags, "--udp")
default:
return "", "", fmt.Errorf("Unsupported protocol for hping3")
}
command := append([]string{"hping3"}, hping3Flags...)
command = append(command, "-p", destinationPort, destinationIP)
return exec.RunExecCommand(client, sourcePod, command...)
}

// IsHpingResponseSeen determines if a response was received from hping3.
// Exit code 0 means a response was received (true).
// Exit code 1 means no response (false). Any other error is logged and
// treated as no response (false).
func IsHpingResponseSeen(stderr string, err error) bool {
if err != nil {
if !strings.Contains(err.Error(), "exit code 1") {
log.Printf("hping3 probe failed to execute: %v (stderr: %s)", err, stderr)
}
return false
}
return true
}