Skip to content
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.

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

Suricata XDP Syncookie #10268

Closed
wants to merge 3 commits into from
Closed

Conversation

vincentmli
Copy link
Contributor

Make sure these boxes are signed before submitting your Pull Request -- thank you.

Describe changes:

  • Add XDP Syncookie feature to prevent host from SYN flooding attack in AF_PACKET IDS

Provide values to any of the below to override the defaults.

To use a pull request use a branch name like pr/N where N is the
pull request number.

Alternatively, SV_BRANCH may also be a link to an
OISF/suricata-verify pull-request.

SV_REPO=
SV_BRANCH=
SU_REPO=
SU_BRANCH=
LIBHTP_REPO=
LIBHTP_BRANCH=

Copy link

codecov bot commented Jan 29, 2024

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (9fe00ff) 82.52% compared to head (906b316) 82.53%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #10268   +/-   ##
=======================================
  Coverage   82.52%   82.53%           
=======================================
  Files         978      978           
  Lines      272148   272166   +18     
=======================================
+ Hits       224595   224619   +24     
+ Misses      47553    47547    -6     
Flag Coverage Δ
fuzzcorpus 63.61% <0.00%> (+0.01%) ⬆️
suricata-verify 61.86% <0.00%> (-0.03%) ⬇️
unittests 62.83% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@vincentmli vincentmli force-pushed the suricata-xdpsyncookie branch from 4fb38b9 to 575d37c Compare January 29, 2024 16:34
@vincentmli
Copy link
Contributor Author

Hi @victorjulien

I send this XDP syncookie PR and noticed the build failed on Debian 11 for the XDP syncookie program and user space program, I suspect Debian 11 may have old libbpf and kernel that does not support the XDP program and user space program. (https://github.com/OISF/suricata/actions/runs/7699348152/job/20980796747?pr=10268).

the XDP kernel program contains bpf kernel function that is supported from kernel 6.2 and above, I suspect none of existing Suricata distribution CI test has 6.2 kernel, Could you share your suggestion on this?

@vincentmli vincentmli force-pushed the suricata-xdpsyncookie branch 5 times, most recently from 6e84cfc to 63136f0 Compare February 5, 2024 02:15
Copy link
Member

@victorjulien victorjulien left a comment

Choose a reason for hiding this comment

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

Interesting work. Some comments and questions inline.

Big question for me: can you share some results?

@@ -0,0 +1,11 @@
#ifndef __VMLINUX_COMMON_H__
Copy link
Member

Choose a reason for hiding this comment

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

what is the origin of this file? similar for the other headers below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

these header files are from https://github.com/xdp-project/bpf-examples/tree/master/headers/vmlinux with modification, the aim is to include minimum header files for the bpf program, not include full vmlinux.h header file which is too big

@@ -0,0 +1,283 @@
// SPDX-License-Identifier: LGPL-2.1 OR BSD-2-Clause
Copy link
Member

Choose a reason for hiding this comment

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

where does this come from? Did you modify it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is from the Linux kernel tree bpf selftests https://elixir.bootlin.com/linux/latest/source/tools/testing/selftests/bpf/xdp_synproxy.c, I did modify it by removing some code from it.

@@ -0,0 +1,799 @@
// SPDX-License-Identifier: LGPL-2.1 OR BSD-2-Clause
Copy link
Member

Choose a reason for hiding this comment

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

these files look like they are pulled in from somewhere. Where are they from? Did you modify them? Is it necessary to have them in our repo?

Copy link
Contributor Author

@vincentmli vincentmli Feb 12, 2024

Choose a reason for hiding this comment

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

this is from https://elixir.bootlin.com/linux/latest/source/tools/testing/selftests/bpf/progs/xdp_synproxy_kern.c with modification by removing some code, and change code behavior, I think it is easier to be included in Suricata repo for user to use, if user get the code from kernel tree separately, it will not work since code behavior changes.

@vincentmli
Copy link
Contributor Author

Interesting work. Some comments and questions inline.

Big question for me: can you share some results?

what result you expect to see? for the XDP bpf program performance, the original author has good performance result in
https://netdevconf.info/0x15/slides/30/Netdev%200x15%20Accelerating%20synproxy%20with%20XDP.pdf, maybe add a reference link to it in doc ?

@vincentmli vincentmli force-pushed the suricata-xdpsyncookie branch 2 times, most recently from 3231c39 to 949bd5f Compare February 12, 2024 17:25
@victorjulien
Copy link
Member

Interesting work. Some comments and questions inline.
Big question for me: can you share some results?

what result you expect to see? for the XDP bpf program performance, the original author has good performance result in https://netdevconf.info/0x15/slides/30/Netdev%200x15%20Accelerating%20synproxy%20with%20XDP.pdf, maybe add a reference link to it in doc ?

Suricata with and without this, under some kind of stress test.

@vincentmli
Copy link
Contributor Author

Suricata with and without this, under some kind of stress test.

I did SYN flooding test with and without this with Suricata, Suricata CPU spikes near 100% without this and enter into emergency mode, with this, Suricata CPU usage is about 6% under SYN flood as normal. or you are concerned if this feature introduce any overhead to Suricata itself in data path and need some other kind of stress test? I am not familiar with Suricata stress test, so any reference documentation on stress test would be great.

Add XDP Syncookie program to enable Suricata
in af-packet IDS mode to stop host from SYN
flooding attack.

with this feature, CPU usage under SYN flood

top - 23:25:07 up 20 min,  2 users,  load average: 0.02, 0.28, 0.27
Threads:   7 total,   0 running,   7 sleeping,   0 stopped,   0 zombie

    PID USER      PR  NI    VIRT    RES    SHR S  %CPU  %MEM     TIME+ COMMAND
   1703 root      20   0  752860 372940 329216 S   0.3   1.1   0:00.32 FM#01
   1699 root      20   0  752860 372940 329216 S   0.0   1.1   0:00.52 Suricata-Main
   1702 root      20   0  752860 372940 329216 S   0.0   1.1   0:00.06 W#01-eno1
   1704 root      20   0  752860 372940 329216 S   0.0   1.1   0:00.00 FR#01
   1705 root      20   0  752860 372940 329216 S   0.0   1.1   0:00.00 CW
   1706 root      20   0  752860 372940 329216 S   0.0   1.1   0:00.00 CS
   1707 root      20   0  752860 372940 329216 S   0.0   1.1   0:00.00 US

without this feature CPU usage under SYN flood

top - 23:26:12 up 21 min,  2 users,  load average: 0.21, 0.28, 0.27
Threads:   7 total,   1 running,   6 sleeping,   0 stopped,   0 zombie

    PID USER      PR  NI    VIRT    RES    SHR S  %CPU  %MEM     TIME+ COMMAND
   1702 root      20   0  752860 413900 329216 R  99.9   1.3   0:11.12 W#01-eno1
   1703 root      20   0  752860 413900 329216 S   0.7   1.3   0:00.60 FM#01
   1699 root      20   0  752860 413900 329216 S   0.0   1.3   0:00.64 Suricata-Main
   1704 root      20   0  752860 413900 329216 S   0.0   1.3   0:00.00 FR#01
   1705 root      20   0  752860 413900 329216 S   0.0   1.3   0:00.00 CW
   1706 root      20   0  752860 413900 329216 S   0.0   1.3   0:00.01 CS
   1707 root      20   0  752860 413900 329216 S   0.0   1.3   0:00.01 US

Signed-off-by: Vincent Li <[email protected]>
add xdp_synproxy user program to manage
syncookie tcp options and ports protected
by XDP Syncookie program

Signed-off-by: Vincent Li <[email protected]>
Add XDP Syncookie user guide and original
performance result referrence link

Signed-off-by: Vincent Li <[email protected]>
@vincentmli vincentmli force-pushed the suricata-xdpsyncookie branch from 949bd5f to 906b316 Compare February 12, 2024 23:36
@vincentmli
Copy link
Contributor Author

vincentmli commented Feb 12, 2024

I updated the commit message with Suricata CPU usage when under SYN flooding attack without and with this feature, the CPU usage difference is from nearly 99% to 0.0%, this is expected since XDP is in early data path before Suricata AF_PACKET, Suricata will not even see the SYN flood packet since XDP already dropped SYN flood attach

with:

    PID USER      PR  NI    VIRT    RES    SHR S  %CPU  %MEM     TIME+ COMMAND                                       
   1703 root      20   0  752860 372940 329216 S   0.3   1.1   0:00.32 FM#01                                         
   1699 root      20   0  752860 372940 329216 S   0.0   1.1   0:00.52 Suricata-Main                                 
   1702 root      20   0  752860 372940 329216 S   0.0   1.1   0:00.06 W#01-eno1                                     
   1704 root      20   0  752860 372940 329216 S   0.0   1.1   0:00.00 FR#01                                         
   1705 root      20   0  752860 372940 329216 S   0.0   1.1   0:00.00 CW                                            
   1706 root      20   0  752860 372940 329216 S   0.0   1.1   0:00.00 CS                                            
   1707 root      20   0  752860 372940 329216 S   0.0   1.1   0:00.00 US     

without:

    PID USER      PR  NI    VIRT    RES    SHR S  %CPU  %MEM     TIME+ COMMAND                                       
   1702 root      20   0  752860 413900 329216 R  99.9   1.3   0:11.12 W#01-eno1                                     
   1703 root      20   0  752860 413900 329216 S   0.7   1.3   0:00.60 FM#01                                         
   1699 root      20   0  752860 413900 329216 S   0.0   1.3   0:00.64 Suricata-Main                                 
   1704 root      20   0  752860 413900 329216 S   0.0   1.3   0:00.00 FR#01                                         
   1705 root      20   0  752860 413900 329216 S   0.0   1.3   0:00.00 CW                                            
   1706 root      20   0  752860 413900 329216 S   0.0   1.3   0:00.01 CS                                            
   1707 root      20   0  752860 413900 329216 S   0.0   1.3   0:00.01 US                                            

@vincentmli
Copy link
Contributor Author

@victorjulien any further feedback?

@catenacyber
Copy link
Contributor

I think Victor expects a new rebased PR (instead of pushes on the existing one which has changes-requested status)

@vincentmli
Copy link
Contributor Author

I think Victor expects a new rebased PR (instead of pushes on the existing one which has changes-requested status)

ok, the process works different for other project, I will do rebase then and send new PR.

@vincentmli vincentmli closed this Mar 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants