OCPBUGS-67363: fixes ip traffic fragmentation verification#730
OCPBUGS-67363: fixes ip traffic fragmentation verification#730raphaelvrosa wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@raphaelvrosa: This pull request references Jira Issue OCPBUGS-67363, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/invalid-bug
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe firewall now identifies and denies non-first IPv4 fragments. E2E transport tooling adds hping3-based fragmented probes, raw-network capability, and validation for blocked fragmented TCP traffic. The Makefile also updates the cert-manager version. ChangesFragmented Traffic Denial
Cert-manager Version Update
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant E2ETest
participant Transport
participant Hping3
participant Firewall
E2ETest->>Transport: HpingFragmentedConnect()
Transport->>Hping3: Execute fragmented probe
Hping3->>Firewall: Send fragmented packets
Firewall-->>Hping3: Deny non-first fragments
Hping3-->>Transport: Return command output
Transport-->>E2ETest: Report response presence
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci-robot
added
jira/valid-bug
|
@raphaelvrosa: This pull request references Jira Issue OCPBUGS-67363, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: raphaelvrosa The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@bpf/ingress_node_firewall_kernel.c`:
- Around line 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).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d4ba6ea4-4664-4622-95ab-3845b7991dd2
⛔ Files ignored due to path filters (4)
pkg/ebpf/bpf_arm64_bpfel.ois excluded by!**/*.opkg/ebpf/bpf_powerpc_bpfel.ois excluded by!**/*.opkg/ebpf/bpf_s390_bpfeb.ois excluded by!**/*.opkg/ebpf/bpf_x86_bpfel.ois excluded by!**/*.o
📒 Files selected for processing (3)
Makefilebpf/ingress_node_firewall.hbpf/ingress_node_firewall_kernel.c
| 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); | ||
| } |
There was a problem hiding this comment.
🔒 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.cRepository: 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.cRepository: 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).
There was a problem hiding this comment.
@danwinship should this PR address this?
There was a problem hiding this comment.
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!
raphaelvrosa
changed the title
|
@raphaelvrosa: This pull request references Jira Issue OCPBUGS-67363, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In `@test/e2e/functional/tests/e2e.go`:
- Around line 1679-1681: Update the protocol filtering in the Hping
fragmented-connect probe loop to explicitly exclude SCTP after the
IsTransportProtocol check. Ensure SCTP is skipped or reported as unsupported so
HpingFragmentedConnect does not produce a false successful assertion without
sending traffic.
In `@test/e2e/transport/transport.go`:
- Around line 287-289: Preserve hping3 execution errors in hpingClient by
removing the shell fallback that appends “|| true”
(test/e2e/transport/transport.go:287-289). In
test/e2e/functional/tests/e2e.go:1562-1565, handle and log or fail on the
returned error; in the IPv4 assertion at 1687-1692 and IPv6 assertion at
1699-1704, fail the assertion when the probe cannot execute instead of parsing
its output as no response. Ensure no error return is ignored.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0e4a1a7b-9965-4789-a6b5-e881dfa4adcc
⛔ Files ignored due to path filters (4)
pkg/ebpf/bpf_arm64_bpfel.ois excluded by!**/*.opkg/ebpf/bpf_powerpc_bpfel.ois excluded by!**/*.opkg/ebpf/bpf_s390_bpfeb.ois excluded by!**/*.opkg/ebpf/bpf_x86_bpfel.ois excluded by!**/*.o
📒 Files selected for processing (6)
Makefilebpf/ingress_node_firewall.hbpf/ingress_node_firewall_kernel.ctest/Dockerfile.netcattest/e2e/functional/tests/e2e.gotest/e2e/transport/transport.go
🚧 Files skipped from review as they are similar to previous changes (3)
- bpf/ingress_node_firewall.h
- Makefile
- bpf/ingress_node_firewall_kernel.c
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@config/manager/kustomization.yaml`:
- Around line 12-13: Keep config/manager/kustomization.yaml lines 12-13 using
the registry-backed controller image, moving the localhost/...:dev override to
the local kind/development overlay. Keep config/manager/env.yaml line 13 using
the deployable DaemonSet image in the shared configuration, moving the local
DAEMONSET_IMAGE override to that same local overlay.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7daa3340-9028-4d87-89aa-e7d87d8dc635
⛔ Files ignored due to path filters (4)
pkg/ebpf/bpf_arm64_bpfel.ois excluded by!**/*.opkg/ebpf/bpf_powerpc_bpfel.ois excluded by!**/*.opkg/ebpf/bpf_s390_bpfeb.ois excluded by!**/*.opkg/ebpf/bpf_x86_bpfel.ois excluded by!**/*.o
📒 Files selected for processing (8)
Makefilebpf/ingress_node_firewall.hbpf/ingress_node_firewall_kernel.cconfig/manager/env.yamlconfig/manager/kustomization.yamltest/Dockerfile.netcattest/e2e/functional/tests/e2e.gotest/e2e/transport/transport.go
🚧 Files skipped from review as they are similar to previous changes (6)
- test/Dockerfile.netcat
- bpf/ingress_node_firewall.h
- Makefile
- test/e2e/transport/transport.go
- bpf/ingress_node_firewall_kernel.c
- test/e2e/functional/tests/e2e.go
c83761f to
888780d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@test/Dockerfile.netcat`:
- Around line 3-4: Update the Dockerfile’s Alpine base image reference to a
fixed digest and replace the floating edge/testing repository in the
package-install command with a repository matching the pinned Alpine release; if
hping3 requires testing, scope that repository only to hping3 while keeping bash
and nmap-ncat on the release repository.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d23b7778-891e-4e10-bae4-12716ccb9bd0
⛔ Files ignored due to path filters (4)
pkg/ebpf/bpf_arm64_bpfel.ois excluded by!**/*.opkg/ebpf/bpf_powerpc_bpfel.ois excluded by!**/*.opkg/ebpf/bpf_s390_bpfeb.ois excluded by!**/*.opkg/ebpf/bpf_x86_bpfel.ois excluded by!**/*.o
📒 Files selected for processing (8)
Makefilebpf/ingress_node_firewall.hbpf/ingress_node_firewall_kernel.cconfig/manager/env.yamlconfig/manager/kustomization.yamltest/Dockerfile.netcattest/e2e/functional/tests/e2e.gotest/e2e/transport/transport.go
🚧 Files skipped from review as they are similar to previous changes (5)
- config/manager/env.yaml
- bpf/ingress_node_firewall.h
- bpf/ingress_node_firewall_kernel.c
- test/e2e/functional/tests/e2e.go
- test/e2e/transport/transport.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In `@test/e2e/transport/transport.go`:
- Around line 248-250: Update the SCTP branch in the hping probe helper to
return a non-nil error instead of empty output with nil, ensuring unsupported
SCTP probes cannot be interpreted as denied packets. Preserve the existing
behavior for supported protocol types and follow the Go error-handling
requirement.
- Around line 287-289: Update hpingClient to remove the sh -c wrapper and avoid
interpolating destinationIP or dPort into a shell command. Build the hping3
argument list as separate arguments, including flags, port, and destination IP,
then pass it directly to exec.RunExecCommand while preserving the existing
return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e1a1f3e3-6951-48fb-a9a6-ee7c44488f61
⛔ Files ignored due to path filters (4)
pkg/ebpf/bpf_arm64_bpfel.ois excluded by!**/*.opkg/ebpf/bpf_powerpc_bpfel.ois excluded by!**/*.opkg/ebpf/bpf_s390_bpfeb.ois excluded by!**/*.opkg/ebpf/bpf_x86_bpfel.ois excluded by!**/*.o
📒 Files selected for processing (6)
Makefilebpf/ingress_node_firewall.hbpf/ingress_node_firewall_kernel.ctest/Dockerfile.netcattest/e2e/functional/tests/e2e.gotest/e2e/transport/transport.go
🚧 Files skipped from review as they are similar to previous changes (3)
- bpf/ingress_node_firewall.h
- test/e2e/functional/tests/e2e.go
- bpf/ingress_node_firewall_kernel.c
Impact: Attackers can access blocked ports by fragmenting IP packets in specific ways that exploit the firewall's failure to validate IP fragmentation headers before extracting Layer 4 port information. Root Cause: The eBPF code assumes all packets have complete TCP/UDP/SCTP headers immediately following the IP header, without checking if the packet is a non-first fragment that contains only payload data. Solution: - Check iph->frag_off before extracting L4 headers - Deny all non-first fragments (secure by default) - First fragments (offset = 0) still processed normally with L4 inspection The solution was verified and validate locally with kind and ovn-kubernetes as CNI. The ebpf program was loaded, and traffic was blocked. The verification of the fragmentation issue could not be fully verified, as docker and host were probably dropping fragmented packets before it reached the cluster. Signed-off-by: Raphael Rosa <raprosa@redhat.com>
|
/assign @danwinship |
|
/retest required |
|
@raphaelvrosa: The Use DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@raphaelvrosa: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| // Fragment offset > 0: this is a non-first fragment | ||
| // No L4 header present, deny by policy | ||
| return -2; | ||
| } |
There was a problem hiding this comment.
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))
| * Return: | ||
| * 0 for Success. | ||
| * -1 for Failure. | ||
| * -2 for non-first IP fragment (security: deny fragments without L4 headers). |
There was a problem hiding this comment.
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).
| 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); | ||
| } |
| }, | ||
| }, | ||
| { | ||
| "should deny fragmented TCP packets to blocked port", |
There was a problem hiding this comment.
This is just a dup of the "block a port with a single rule defining the destinations port" case isn't it?
| } | ||
| // test fragmented packets are always denied | ||
| for _, reach := range entry.reachables { | ||
| reach := reach |
There was a problem hiding this comment.
the reach := reach is not needed with go >= 1.22
| for _, reach := range entry.reachables { | ||
| reach := reach | ||
| reachabilityCheckFragmentation(reach, podNameObj, entry.protocols) | ||
| } |
There was a problem hiding this comment.
why is this loop needed if you also check this from isConnectivitySeen?
| case ingressnodefwv1alpha1.ProtocolTypeSCTP: | ||
| return "", "", fmt.Errorf("hping3 does not support SCTP") | ||
| default: | ||
| panic("Unsupported protocol for hping3") |
There was a problem hiding this comment.
weird to return an error for SCTP but panic for ICMP
| 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=") { |
There was a problem hiding this comment.
This seems potentially flaky... can we distinguish based on exit status 0 vs non-0?
|
|
||
| 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") | ||
| } |
There was a problem hiding this comment.
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?
| CSV_VERSION := 0.0.0 | ||
| endif | ||
| CERT_MANAGER_VERSION=v1.9.1 | ||
| CERT_MANAGER_VERSION=v1.20.2 |
3 participants
Impact: Attackers can access blocked ports by fragmenting IP packets in specific ways that exploit the firewall's failure to validate IP fragmentation headers before extracting Layer 4 port information.
Root Cause: The eBPF code assumes all packets have complete TCP/UDP/SCTP headers immediately following the IP header, without checking if the packet is a non-first fragment that contains only payload data.
Solution:
The solution was verified and validate locally with kind and ovn-kubernetes as CNI. The ebpf program was loaded, and traffic was blocked.
End to end tests with hping3 are added to validate the solution.
- What this PR does and why is it needed
- Special notes for reviewers
- How to verify it
- Description for the changelog
Summary by CodeRabbit
Bug Fixes
Chores