-
Notifications
You must be signed in to change notification settings - Fork 1.6k
decoder/l2tp: initial implementation of decoder #14325
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
base: main
Are you sure you want to change the base?
Conversation
|
NOTE: This PR may contain new authors. |
victorjulien
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few quick comments as I checked if I could enable the CI
src/decode-l2tp.c
Outdated
| uint16_t proto = SCNtohs(ethh->eth_type); | ||
| switch (proto) { | ||
| case ETHERNET_TYPE_IP: | ||
| L2TP_CHECK_INNER_TUNNEL(DECODE_TUNNEL_IPV4) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing ;
src/decode-l2tp.h
Outdated
| eth_found = true; \ | ||
| } \ | ||
| break; \ | ||
| } while (0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would like to drop the ; here and force the caller to set it
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #14325 +/- ##
========================================
Coverage 84.16% 84.17%
========================================
Files 1012 1014 +2
Lines 261869 262231 +362
========================================
+ Hits 220405 220721 +316
- Misses 41464 41510 +46
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
WARNING:
Pipeline = 28461 |
|
I've made it even more less strict to stop the excessive number invalid packets on QA and added a SV test for this (decode-l2tp-v3-15). |
|
NOTE: This PR may contain new authors. |
Would have to check the pcaps, but I suspect we need to opposite? If we detect other traffic as l2tp initially, but then it fails to decode the encapsulated protocol. That would trigger lots of invalid packets potentially. This has been our experience with vxlan as well. With Suricon next week it will probably a while before we can dig up pcaps. @ct0br0 can we share any of the pcaps? |
|
WARNING:
Pipeline = 28466 |
|
This result is a lot better already. Lets see if we can extract the pcap and review if they can be shared or that we can use them to assist in some other way. |
Issue: 7986
Make sure these boxes are checked accordingly before submitting your Pull Request -- thank you.
Contribution style:
https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html
Our Contribution agreements:
https://suricata.io/about/contribution-agreement/ (note: this is only required once)
Changes (if applicable):
(including schema descriptions)
https://redmine.openinfosecfoundation.org/projects/suricata/issues
Link to ticket: https://redmine.openinfosecfoundation.org/issues/7986
Describe changes:
Provide values to any of the below to override the defaults.
link to the pull request in the respective
_BRANCHvariable.SV_REPO=
SV_BRANCH=OISF/suricata-verify#2748
SU_REPO=
SU_BRANCH=