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?

IMAGE_ORG ?= $(USER)

# CHANNELS define the bundle channels used in the bundle.
Expand Down
5 changes: 5 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,9 @@ struct rulesVal_st {
struct ruleType_st rules[MAX_RULES_PER_TARGET];
} __attribute__((packed));

// 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
28 changes: 24 additions & 4 deletions bpf/ingress_node_firewall_kernel.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ volatile const __u32 debug_lookup = 0;
* Return:
* 0 for Success.
* -1 for Failure.
* -2 for non-first IP fragment (security: deny fragments without L4 headers).

@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.

Should probably have #defined return values.

And this should be clearer about what the different statuses imply:

  • 0: success (extracted L4 info)
  • -1: packet is truncated; allow it to continue to the kernel, where it should be rejected
  • -2: packet is fragmented; reject and do not pass to kernel (because INF does not support reassembling and checking fragmented packets, so we have to assume all fragmented packets are denied).

*/
__attribute__((__always_inline__)) static inline int
ip_extract_l4info(void *data, void *dataEnd, __u8 *proto, __u16 *dstPort,
Expand All @@ -110,6 +111,15 @@ ip_extract_l4info(void *data, void *dataEnd, __u8 *proto, __u16 *dstPort,
return -1;
}
*proto = iph->protocol;

// Security: Check for IP fragmentation (RFC 791)
// Non-first fragments don't have L4 headers, deny them to prevent bypass
__u16 frag_off = bpf_ntohs(iph->frag_off);
if (unlikely(frag_off & IP_OFFSET_MASK)) {
// Fragment offset > 0: this is a non-first fragment
// No L4 header present, deny by policy
return -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.

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))

} else {
struct ipv6hdr *iph = dataStart;
dataStart += sizeof(struct ipv6hdr);
Expand Down Expand Up @@ -195,8 +205,13 @@ 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 < 0)) {
if (l4_result == -2) {
// Non-first IP fragment: deny by security policy
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 +306,13 @@ 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 < 0)) {
if (l4_result == -2) {
// Non-first IP fragment: deny by security policy
return SET_ACTIONRULE_RESPONSE(DENY, INVALID_RULE_ID);
}
ingress_node_firewall_printk("failed to extract l4 info");
return SET_ACTION(UNDEF);
}
Comment on lines +309 to 318

@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/*
97 changes: 96 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 @@ -828,6 +828,43 @@ var _ = Describe("Ingress Node Firewall", func() {
},
},
},
{
"should deny fragmented TCP packets to blocked port",

@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.

This is just a dup of the "block a port with a single rule defining the destinations port" case isn't it?

[]testIngressNodeFirewall{
{
[]string{testInterface},
[]testRule{
{
[]sourceCIDRsEntry{{clientOnePodName, v4SubnetLen, v6SubnetLen}},
[]func(proto ingressnodefwv1alpha1.IngressNodeFirewallRuleProtocolType, order uint32) ingressnodefwv1alpha1.IngressNodeFirewallProtocolRule{
func(proto ingressnodefwv1alpha1.IngressNodeFirewallRuleProtocolType, order uint32) ingressnodefwv1alpha1.IngressNodeFirewallProtocolRule {
return infwutils.GetTransportProtocolBlockPortRule(proto, order, serverOnePort)
},
},
},
},
},
},
[]reachable{
{
clientOnePodName,
serverOnePodName,
serverOnePort,
false,
},
},
transportProtocols,
[]func() (*corev1.Pod, func(), error){
func() (*corev1.Pod, func(), error) {
return transport.GetAndEnsureRunningClient(testclient.Client, clientOnePodName, OperatorNameSpace, clientPodLabel, clientPodLabel,
serverPodLabel, retryInterval, timeout)
},
func() (*corev1.Pod, func(), error) {
return transport.GetAndEnsureRunningTransportServer(testclient.Client, serverOnePodName, OperatorNameSpace,
serverPodLabel, serverPodLabel, clientPodLabel, retryInterval, timeout)
},
},
},
}

BeforeEach(func() {
Expand Down Expand Up @@ -973,6 +1010,11 @@ var _ = Describe("Ingress Node Firewall", func() {
reach := reach
reachabilityCheck(reach, podNameObj, entry.protocols, reach.connectivity)
}
// test fragmented packets are always denied
for _, reach := range entry.reachables {
reach := reach

@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.

the reach := reach is not needed with go >= 1.22

reachabilityCheckFragmentation(reach, podNameObj, entry.protocols)
}

@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.

why is this loop needed if you also check this from isConnectivitySeen?

// Make sure all rules are deleted before running next test
deleteAllTestRules(testINFs, testclient.Client, timeout, nodeStateList)
})
Expand Down Expand Up @@ -1515,7 +1557,17 @@ 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 fragmented connectivity test (runs side by side with ncat)

@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.

I would expect "side by side" to mean that they run in parallel, but that's not what's happening here. (Also, it's not clear what "ncat" refers to since nothing else in this file mentions "ncat"... if you mean the transport.ConnectToPortFromPod test, say that.)

hpingStdout, hpingStderr, hpingErr := transport.HpingFragmentedConnect(client, protocol, v6, sourcePod, destinationIP, destinationPort)
if hpingErr != nil && !strings.Contains(hpingErr.Error(), "exit code 1") {
log.Printf("hping3 frag probe failed to execute: %v", hpingErr)
}
hpingResult := transport.IsHpingResponseSeen(hpingStdout, hpingStderr)
Comment on lines +1564 to +1567

@danwinship danwinship Jul 17, 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.

I assume the !strings.Contains(hpingErr.Error(), "exit code 1") is because that error code means "failed to connect"? It's weird to have some of the logic for parsing the return value here, and some in IsHpingResponseSeen. It would be better to just have transport.HpingFragmentedConnect parse the return value itself and return (connected bool, err error)

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 +1672,49 @@ 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 {
stdout, stderr, err := transport.HpingFragmentedConnect(
testclient.Client, protocol, false, sourcePod, destinationPodV4IP, reach.port)
if err != nil && !strings.Contains(err.Error(), "exit code 1") {
Fail(fmt.Sprintf("hping3 IPv4 frag probe failed to execute: %v", err))
}
return transport.IsHpingResponseSeen(stdout, stderr)
}, 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 {
stdout, stderr, err := transport.HpingFragmentedConnect(
testclient.Client, protocol, true, sourcePod, destinationPodV6IP, reach.port)
if err != nil && !strings.Contains(err.Error(), "exit code 1") {
Fail(fmt.Sprintf("hping3 IPv6 frag probe failed to execute: %v", err))
}
return transport.IsHpingResponseSeen(stdout, stderr)
}, 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
64 changes: 64 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,62 @@ 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) {
switch proto {
case ingressnodefwv1alpha1.ProtocolTypeTCP:
if v6 {
return hpingTCPFragV6(client, sourcePod, destinationIP, destinationPort)
}
return hpingTCPFragV4(client, sourcePod, destinationIP, destinationPort)
case ingressnodefwv1alpha1.ProtocolTypeUDP:
if v6 {
return hpingUDPFragV6(client, sourcePod, destinationIP, destinationPort)
}
return hpingUDPFragV4(client, sourcePod, destinationIP, destinationPort)
case ingressnodefwv1alpha1.ProtocolTypeSCTP:
return "", "", fmt.Errorf("hping3 does not support SCTP")
default:
panic("Unsupported protocol for hping3")

@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.

weird to return an error for SCTP but panic for ICMP

}
}

// IsHpingResponseSeen parses hping3 output to determine if a response was received.
// Returns true if the target responded (packet reached it), false if dropped.
func IsHpingResponseSeen(stdout, stderr string) bool {
combined := stdout + stderr
// hping3 prints "flags=SA" (SYN-ACK) or "flags=RA" (RST-ACK) when a response is received.
if strings.Contains(combined, "flags=") {

@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.

This seems potentially flaky... can we distinguish based on exit status 0 vs non-0?

return true
}
// "0 packets received" or "100% packet loss" means no response
if strings.Contains(combined, "0 packets received") || strings.Contains(combined, "100% packet loss") {
return false
}
return false
}

func hpingTCPFragV4(client *testclient.ClientSet, sourcePod *corev1.Pod, destinationIP, dPort string) (string, string, error) {
return hpingClient(client, sourcePod, destinationIP, dPort, "-S", "-f", "-c", "1")
}

func hpingTCPFragV6(client *testclient.ClientSet, sourcePod *corev1.Pod, destinationIP, dPort string) (string, string, error) {
return hpingClient(client, sourcePod, destinationIP, dPort, "-S", "-f", "-c", "1", "-6")
}

@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.

Not sure all these helper functions actually make things clearer. It seems like it might be better to just have HpingFragmentedConnect assemble the hping args based on proto, then add a "-6" if v6, then call exec.RunExecCommand?


func hpingUDPFragV4(client *testclient.ClientSet, sourcePod *corev1.Pod, destinationIP, dPort string) (string, string, error) {
return hpingClient(client, sourcePod, destinationIP, dPort, "--udp", "-f", "-c", "1")
}

func hpingUDPFragV6(client *testclient.ClientSet, sourcePod *corev1.Pod, destinationIP, dPort string) (string, string, error) {
return hpingClient(client, sourcePod, destinationIP, dPort, "--udp", "-f", "-c", "1", "-6")
}

func hpingClient(client *testclient.ClientSet, sourcePod *corev1.Pod, destinationIP, dPort string, flags ...string) (string, string, error) {
command := append([]string{"hping3"}, flags...)
command = append(command, "-p", dPort, destinationIP)
return exec.RunExecCommand(client, sourcePod, command...)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}