Fixing L4 offset for IP packets with Options field #41

evershalik · 2023-04-01T08:52:29Z

This PR fixed #30

Problem Statement:
The current implementation of the codebase does not handle IP and TCP packets with a header length greater than 20 bytes correctly. This issue is particularly problematic for packets containing options, which increase the header length beyond 20 bytes.

Approach:
To address this issue, we have taken the following approaches:

Firstly, I have modified the code to use the l4_offset variable(IP Header Length) instead of sizeof(iphdr struct) when adding to the iph pointer. This ensures that the code correctly traverses to the end of the IP header( or the start of the transport layer header), even in cases where the header length is greater than 20 bytes.

Secondly, I have modified the code to use the data_offset variable(TCP Header Length) instead of sizeof(tcphdr struct) when adding to the tcph pointer. This ensures that the code correctly traverses to the end of the TCP header (or start of the payload), even in cases where the header length is greater than 20 bytes.

By implementing these changes, I can ensure that IP and TCP packets with options are handled correctly, which improves the reliability and stability of the system.

connection-limit/connection_limit_kern.c

aka320 · 2023-04-02T19:38:04Z

connection-limit/connection_limit_kern.c

    struct iphdr *iph = (struct iphdr *)(data + sizeof(struct ethhdr));
-    if (iph + 1 > data_end)
+    __u8 l4_offset = iph->ihl * 4; // ip header length


Endianness is handled in the definition of 'struct iphdr'. However please have a test-case to verify that the value for IHL is read correctly from the packet.

What sort of check did you have in mind? I am not sure an additional one is necessary beyond what line 281 checks.

ipfix-flow-exporter/bpf_ipfix_egress_kern.c

aka320

The comments given are applicable for all the eBPF applications. Please fix in all files even though its not explicitly commented in that specific file.
Please include the tests results (different values of IHL including negative cases like invalid IHL value)
Add a Commit log message with a short description of RootCause (pointing to the source Issue) & Details of the Fix

evershalik · 2023-04-05T19:30:51Z

I have gone ahead and fixed the issue, as you suggested.
I have added a commit log message above that provides a short description of the root cause of the issue (as pointed out in the source issue), along with details of the fix that has been implemented. This should help with tracking the changes and understanding the reasons for the changes made.
Regarding the test results, it may take some time to complete these tests and ensure that everything is working as expected.
Thank you for your feedback.

aka320 · 2023-04-12T03:32:20Z

You can remove the diagrams in the Description. Text is enough.
Next time when you address review comments, do not overwrite the previous commit. It doesnt give option to review the additional changes made for the review comments.

evershalik · 2023-04-12T07:24:58Z

Thank you for your feedback.
I have removed the diagrams in the Description.
And I apologize for overwriting the previous commit. I will take care of this in the future.

aka320 · 2023-04-12T16:24:13Z

Changes look good. Please update once testing is done.

connection-limit/connection_limit_kern.c

dthaler

This code isn't safe because it doesn't check whether the packet is IPv4 before trying to interpret the IPv4 header.

dthaler · 2023-04-19T17:02:30Z

ipfix-flow-exporter/bpf_ipfix_egress_kern.c


+    /* Check if its valid ip packet */


Suggested change

/* Check if its valid ip packet */

/* Check if it's a valid IPv4 packet */

dthaler · 2023-04-19T17:03:32Z

ipfix-flow-exporter/bpf_ipfix_ingress_kern.c


+    /* Check if its valid ip packet */


Suggested change

/* Check if its valid ip packet */

/* Check if it's a valid IPv4 packet */

dthaler · 2023-04-19T17:04:28Z

ratelimiting/ratelimiting_kern.c

        return XDP_PASS;

    /* Ignore other than TCP packets */
    if (iph->protocol != IPPROTO_TCP)
        return XDP_PASS;

+    struct tcphdr *tcph = (struct tcphdr *)((unsigned char *)iph + l4_offset);
+    __u16 data_offset = tcph->doff * 4; // tcp header length
+
    /* Check if its valid tcp packet */


Suggested change

/* Check if its valid tcp packet */

/* Check if it's a valid TCP packet */

dthaler · 2023-04-20T14:10:42Z

.github/workflows/ci-build-traffic-mirroring.yaml

+          make defconfig
+
+      - name: Clone kernel function repository
+        uses: actions/checkout@v2


Suggested change

uses: actions/checkout@v2

uses: actions/checkout@b0e28b5ac45a892f91e7d036f8200cf5ed489415

use hash instead of version. I see some programs use version and others use hash, we should be consistent at always using hash.

dthaler · 2023-04-20T14:19:22Z

.github/workflows/codeql.yml

@@ -0,0 +1,104 @@
+# Copyright Contributors to the L3AF Project.
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)


is there a reason the codeql file needs to have GPL in the SPDX?

dthaler · 2023-04-20T14:19:49Z

.github/workflows/codeql.yml

+
+    # Initializes the CodeQL tools for scanning.
+    - name: Initialize CodeQL
+      uses: github/codeql-action/init@v2


Suggested change

uses: github/codeql-action/init@v2

uses: github/codeql-action/init@04df1262e6247151b5ac09cd2c303ac36ad3f62b

dthaler · 2023-04-20T14:20:33Z

.github/workflows/codeql.yml

+        make defconfig
+
+    - name: Clone eBPF-Package-Repository
+      uses: actions/checkout@v2


Suggested change

uses: actions/checkout@v2

uses: actions/checkout@b0e28b5ac45a892f91e7d036f8200cf5ed489415

dthaler · 2023-04-20T14:22:02Z

buildscript.sh

@@ -0,0 +1,14 @@
+#!/usr/bin/env bash
+# Copyright Contributors to the L3AF Project.
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)


Any reason GPL is needed in the build script SPDX for all programs?

dthaler · 2023-04-20T14:36:07Z

traffic-mirroring/mirroring_egress_kern.c

+            }
+
+            if (allow_all_src_ports == true && allow_all_dst_ports == true) {
+                return bpf_clone_redirect(skb, *ifindex, 0); // __bpf_rx_skb


What is the comment on the end for? Avoid putting Linux-only things in comments where possible.

dthaler · 2023-04-20T14:36:38Z

traffic-mirroring/mirroring_ingress_kern.c

+    __u8 data[4];
+};
+
+/* TODO: Describe what this PIN_GLOBAL_NS value 2 means???


Can this TODO be done before approving the PR?

dthaler · 2023-04-20T14:36:54Z

traffic-mirroring/mirroring_ingress_kern.c

+    int *ifany;
+    int iface_key = 1;
+
+    /* Lookup what ifindex to redirect packets to */


Suggested change

/* Lookup what ifindex to redirect packets to */

/* Look up what ifindex to redirect packets to */

dthaler · 2023-04-20T14:37:17Z

traffic-mirroring/mirroring_ingress_kern.c

+        l7_off = l4_off + sizeof(struct tcphdr); // L7 (e.g. HTTP) header offset
+    } else if (iph->protocol == IPPROTO_UDP) {
+        l7_off = l4_off + sizeof(struct udphdr); // L7 (e.g. HTTP) header offset
+    } else if (iph->protocol == IPPROTO_ICMP) {
+        l7_off =
+            l4_off + sizeof(struct icmphdr); // L7 (e.g. HTTP) header offset


Suggested change

l7_off = l4_off + sizeof(struct tcphdr); // L7 (e.g. HTTP) header offset

} else if (iph->protocol == IPPROTO_UDP) {

l7_off = l4_off + sizeof(struct udphdr); // L7 (e.g. HTTP) header offset

} else if (iph->protocol == IPPROTO_ICMP) {

l7_off =

l4_off + sizeof(struct icmphdr); // L7 (e.g. HTTP) header offset

l7_off = l4_off + sizeof(struct tcphdr); // L7 (e.g., HTTP) header offset

} else if (iph->protocol == IPPROTO_UDP) {

l7_off = l4_off + sizeof(struct udphdr); // L7 (e.g., HTTP) header offset

} else if (iph->protocol == IPPROTO_ICMP) {

l7_off =

l4_off + sizeof(struct icmphdr); // L7 (e.g., HTTP) header offset

dthaler · 2023-04-20T14:37:32Z

traffic-mirroring/mirroring_ingress_kern.c

+    //      1th bit True: allow all/any source port
+    //      2th bit True: allow all/any destination port


Suggested change

// 1th bit True: allow all/any source port

// 2th bit True: allow all/any destination port

// 1st bit True: allow all/any source port

// 2nd bit True: allow all/any destination port

Updated

evershalik · 2023-04-21T09:41:26Z

Sorry for this. I have synced the forked repository by mistake due to which commits made by other people are also showing in this PR. What should I do now to revert this?

Signed-off-by: Santhosh Fernandes <[email protected]> Signed-off-by: evershalik <[email protected]>

He's moved on to other things. Signed-off-by: evershalik <[email protected]>

Signed-off-by: Santhosh Fernandes <[email protected]> Signed-off-by: evershalik <[email protected]>

Signed-off-by: Dave Thaler <[email protected]> Signed-off-by: evershalik <[email protected]>

Signed-off-by: evershalik <[email protected]>

* Adding Traffic Mirroring Signed-off-by: Jay Sheth <[email protected]> * Fixing Readme Signed-off-by: Jay Sheth <[email protected]> * Removing unnecessary comments and code Signed-off-by: Jay Sheth <[email protected]> * Removing unnecessary files Signed-off-by: Jay Sheth <[email protected]> * Minor Fixes Signed-off-by: [email protected] <[email protected]> --------- Signed-off-by: Jay Sheth <[email protected]> Signed-off-by: [email protected] <[email protected]> Signed-off-by: evershalik <[email protected]>

Signed-off-by: sferna1 <[email protected]> Signed-off-by: evershalik <[email protected]>

Signed-off-by: evershalik <[email protected]>

Signed-off-by: Santhosh Fernandes <[email protected]> Signed-off-by: sanfern <[email protected]> Signed-off-by: SHANKAR <[email protected]> Signed-off-by: evershalik <[email protected]>

Signed-off-by: Santhosh Fernandes <[email protected]> Signed-off-by: sferna1 <[email protected]> Signed-off-by: SHANKAR <[email protected]> Signed-off-by: evershalik <[email protected]>

Signed-off-by: sanfern <[email protected]> Signed-off-by: SHANKAR <[email protected]> Signed-off-by: evershalik <[email protected]>

Signed-off-by: Santhosh Fernandes <[email protected]> Signed-off-by: sanfern <[email protected]> Signed-off-by: SHANKAR <[email protected]> Signed-off-by: evershalik <[email protected]>

Signed-off-by: SHANKAR <[email protected]> Signed-off-by: evershalik <[email protected]>

Signed-off-by: evershalik <[email protected]>

evershalik requested review from jniesz, sanfern, jaysheth2 and aka320 as code owners April 1, 2023 08:52

aka320 reviewed Apr 2, 2023

View reviewed changes

connection-limit/connection_limit_kern.c Show resolved Hide resolved

aka320 reviewed Apr 2, 2023

View reviewed changes

connection-limit/connection_limit_kern.c Outdated Show resolved Hide resolved

aka320 reviewed Apr 2, 2023

View reviewed changes

ipfix-flow-exporter/bpf_ipfix_egress_kern.c Outdated Show resolved Hide resolved

aka320 requested changes Apr 2, 2023

View reviewed changes

evershalik requested a review from aka320 April 6, 2023 06:25

dthaler reviewed Apr 12, 2023

View reviewed changes

connection-limit/connection_limit_kern.c Show resolved Hide resolved

dthaler previously requested changes Apr 13, 2023

View reviewed changes

dthaler reviewed Apr 19, 2023

View reviewed changes

evershalik requested a review from dthaler April 20, 2023 09:04

dthaler reviewed Apr 20, 2023

View reviewed changes

sanfern and others added 9 commits June 18, 2023 13:02

Update names

1b93cc3

Signed-off-by: Santhosh Fernandes <[email protected]> Signed-off-by: evershalik <[email protected]>

Remove Karan from CODEOWNERS

c42fd6d

He's moved on to other things. Signed-off-by: evershalik <[email protected]>

Remove unused files

216aba2

Signed-off-by: Santhosh Fernandes <[email protected]> Signed-off-by: evershalik <[email protected]>

Update license string

53e8f32

Signed-off-by: Santhosh Fernandes <[email protected]> Signed-off-by: evershalik <[email protected]>

Add windows submodule

ca0702a

Signed-off-by: Dave Thaler <[email protected]> Signed-off-by: evershalik <[email protected]>

Make xdp_root and ratelimiting compile for windows

f0120a9

Signed-off-by: Dave Thaler <[email protected]> Signed-off-by: evershalik <[email protected]>

Update windows CICD workflows

970f980

Signed-off-by: Dave Thaler <[email protected]> Signed-off-by: evershalik <[email protected]>

Build eBPF for Windows until we have a devkit available

8bccc1a

Signed-off-by: Dave Thaler <[email protected]> Signed-off-by: evershalik <[email protected]>

Consume eBPF for Windows via a nuget package instead of a submodule

5439851

Signed-off-by: Dave Thaler <[email protected]> Signed-off-by: evershalik <[email protected]>

dthaler and others added 17 commits June 18, 2023 13:02

Update github workflows to not build ebpf-for-windows

c8b7245

Signed-off-by: Dave Thaler <[email protected]> Signed-off-by: evershalik <[email protected]>

Use paths from nuget props file

9b4cc04

Signed-off-by: Dave Thaler <[email protected]> Signed-off-by: evershalik <[email protected]>

Update nuget package references

2529f73

Signed-off-by: Dave Thaler <[email protected]> Signed-off-by: evershalik <[email protected]>

Upgrade ebpf-for-windows dependency to 0.1.4

416be38

Signed-off-by: Dave Thaler <[email protected]> Signed-off-by: evershalik <[email protected]>

Update code owners

3fe08af

Signed-off-by: evershalik <[email protected]>

update codeowners list

1f197b1

Signed-off-by: sferna1 <[email protected]> Signed-off-by: evershalik <[email protected]>

Fixing L4 offset for IP packets with Options field

f95e11d

Signed-off-by: evershalik <[email protected]>

Updating Comments

60451ba

Signed-off-by: evershalik <[email protected]>

Update codeQL

cf0b9e7

Signed-off-by: Santhosh Fernandes <[email protected]> Signed-off-by: sanfern <[email protected]> Signed-off-by: SHANKAR <[email protected]> Signed-off-by: evershalik <[email protected]>

Update traffic-mirroring workflow

80517da

Signed-off-by: Santhosh Fernandes <[email protected]> Signed-off-by: sferna1 <[email protected]> Signed-off-by: SHANKAR <[email protected]> Signed-off-by: evershalik <[email protected]>

Update codeql errors

8d72c73

Signed-off-by: sanfern <[email protected]> Signed-off-by: SHANKAR <[email protected]> Signed-off-by: evershalik <[email protected]>

Update map rl_window_map type to BPF_MAP_TYPE_LRU_HASH

4a77555

Signed-off-by: Santhosh Fernandes <[email protected]> Signed-off-by: sanfern <[email protected]> Signed-off-by: SHANKAR <[email protected]> Signed-off-by: evershalik <[email protected]>

ipv6 check

74a9e67

Signed-off-by: SHANKAR <[email protected]> Signed-off-by: evershalik <[email protected]>

fixing L4-offset issue

fb45984

Signed-off-by: SHANKAR <[email protected]> Signed-off-by: evershalik <[email protected]>

Add files via upload

95cecc8

Signed-off-by: evershalik <[email protected]>

Merge branch 'main' into main

cb8c20c

evershalik closed this Jun 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing L4 offset for IP packets with Options field #41

Fixing L4 offset for IP packets with Options field #41

evershalik commented Apr 1, 2023 •

edited

Loading

aka320 Apr 2, 2023

dthaler Apr 19, 2023 •

edited

Loading

aka320 left a comment

evershalik commented Apr 5, 2023 •

edited

Loading

aka320 commented Apr 12, 2023

evershalik commented Apr 12, 2023

aka320 commented Apr 12, 2023

dthaler left a comment

dthaler Apr 19, 2023

dthaler Apr 19, 2023

dthaler Apr 19, 2023

dthaler Apr 20, 2023

dthaler Apr 20, 2023

dthaler Apr 20, 2023

dthaler Apr 20, 2023

dthaler Apr 20, 2023

dthaler Apr 20, 2023

dthaler Apr 20, 2023

dthaler Apr 20, 2023

dthaler Apr 20, 2023

dthaler Apr 20, 2023

evershalik commented Apr 21, 2023 •

edited

Loading

	/* Check if its valid ip packet */
	/* Check if it's a valid IPv4 packet */

	/* Check if its valid tcp packet */
	/* Check if it's a valid TCP packet */

	uses: actions/checkout@v2
	uses: actions/checkout@b0e28b5ac45a892f91e7d036f8200cf5ed489415

		@@ -0,0 +1,104 @@
		# Copyright Contributors to the L3AF Project.
		# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)

	uses: github/codeql-action/init@v2
	uses: github/codeql-action/init@04df1262e6247151b5ac09cd2c303ac36ad3f62b

	/* Lookup what ifindex to redirect packets to */
	/* Look up what ifindex to redirect packets to */

		// 1th bit True: allow all/any source port
		// 2th bit True: allow all/any destination port

Fixing L4 offset for IP packets with Options field #41

Fixing L4 offset for IP packets with Options field #41

Conversation

evershalik commented Apr 1, 2023 • edited Loading

Choose a reason for hiding this comment

dthaler Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

aka320 left a comment

Choose a reason for hiding this comment

evershalik commented Apr 5, 2023 • edited Loading

aka320 commented Apr 12, 2023

evershalik commented Apr 12, 2023

aka320 commented Apr 12, 2023

dthaler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evershalik commented Apr 21, 2023 • edited Loading

evershalik commented Apr 1, 2023 •

edited

Loading

dthaler Apr 19, 2023 •

edited

Loading

evershalik commented Apr 5, 2023 •

edited

Loading

evershalik commented Apr 21, 2023 •

edited

Loading