decoder/l2tp: initial implementation of decoder #14023

poodle-amazon · 2025-10-14T03:20:21Z

Issue: 7986

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

Contribution style:

I have read the contributing guide lines at
https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html

Our Contribution agreements:

I have signed the Open Information Security Foundation contribution agreement at
https://suricata.io/about/contribution-agreement/ (note: this is only required once)

Changes (if applicable):

I have updated the User Guide (in doc/userguide/) to reflect the changes made
I have updated the JSON schema (in etc/schema.json) to reflect all logging changes
(including schema descriptions)
I have created a ticket at
https://redmine.openinfosecfoundation.org/projects/suricata/issues

Link to ticket: https://redmine.openinfosecfoundation.org/issues/7986

Describe changes:

Initial commit of L2TP Feature for 7896

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

To use a Suricata-Verify or Suricata-Update pull request,
link to the pull request in the respective _BRANCH variable.
Leave unused overrides blank or remove.

SV_REPO=
SV_BRANCH=
SU_REPO=
SU_BRANCH=

victorjulien

Thanks for your contribution!

Some initial comments inline. I'll also let this run CI and our QA, I suspect there will be some more issues as at least the formatting seems off in a few places (run clang-format on the diff: git clang-format HEAD~) and I suspect the debug build is broken.

src/decode-l2tp.c

suricata.yaml.in

src/decode-l2tp.c

victorjulien · 2025-10-14T04:59:42Z

src/decode-l2tp.c

+            case ETHERNET_TYPE_VNTAG:
+                eth_found = true;
+                break;
+            default:


can you add a comment explaining why we consume 4 bytes here?

this code seems like dead code? If eth_found isn't set we should hit the !eth_found exit path directly after?

I don't think it's dead because it is i<4 && !eth_found;?

Comment added.

src/decode-l2tp.c

github-actions · 2025-10-14T05:11:09Z

NOTE: This PR may contain new authors.

victorjulien · 2025-10-14T05:13:38Z

Additionally we would love to see some pcap based Suricata Verify tests.

victorjulien · 2025-10-14T05:14:06Z

CI: the Cargo audit test failure is unrelated and expected.

Issue: 7986

poodle-amazon · 2025-10-14T06:34:28Z

Additionally we would love to see some pcap based Suricata Verify tests.

Happy to add some this week from some hardware we have on hand

github-actions · 2025-10-14T07:10:51Z

NOTE: This PR may contain new authors.

…coder.properties.event.properties

suricata-qa · 2025-10-14T07:40:03Z

WARNING:

field	baseline	test	%
	SURI_TLPR1_stats_chk
.uptime	654	628	96.02%
.decoder.invalid	123	4307	3501.63%
	IPS_AFP_stats_chk
.decoder.invalid	0	3692	-
	TREX_GENERIC_stats_chk
.decoder.invalid	0	8338	-

Pipeline = 27975

src/decode-l2tp.h

suricata.yaml.in

github-actions · 2025-10-14T07:45:56Z

NOTE: This PR may contain new authors.

victorjulien · 2025-10-14T07:46:35Z

QA result shows an increase of invalid packets. This suggests the logic to detect the protocol should be tighter. At least that is what VXLAN has shown us previously.

poodle-amazon · 2025-10-14T07:47:26Z

QA result shows an increase of invalid packets. This suggests the logic to detect the protocol should be tighter. At least that is what VXLAN has shown us previously.

Cool, I'll take a look and see if I can tighten it up later tonight/tomorrow

src/decode-l2tp.c

victorjulien · 2025-10-15T05:37:08Z

Think we're almost there. There is a compile warning left:

decode-l2tp.c:171:41: error: implicit conversion loses integer precision: 'int' to 'uint16_t' (aka 'unsigned short') [-Werror,-Wimplicit-int-conversion]
        uint16_t proto = (pkt[12] << 8) + pkt[13];
                 ~~~~~   ~~~~~~~~~~~~~~~^~~~~~~~~
1 error generated.

Plus a few schema things missing.

Then combined with a SV PR we'd like to see a new (rebased) PR.

- Add a strict option similar to VXLAN checking reserved bits and only return TM_ECODE_FAILED if it is set for invalid L2TP headers - Tighten to only IPv4, IPv6, VLAN, or ARP encapsulated packets (similar to VXLAN) - No longer return TM_ECODE_FAILED if we cannot find an encapsulated packet (similar to VXLAN)

poodle-amazon · 2025-10-15T06:59:05Z

I've fixed the compile warning by changing to EthernetHdr and SCNtohs, I've changed the decoding logic to only error on the reserve bits if enabled in the YAML and I've modified the logic to no longer error if it can not find an inner tunnel (all very similar to VXLAN in v9).

QA result shows an increase of invalid packets.

Can you please run the QA over this and see if the logic change is better? If it's acceptable then I'll then create a SV PR tomorrow and rebase this PR into a new PR.

inashivb · 2025-10-15T06:57:43Z

src/decode-l2tp.c

+                eth_found = true; /* breaks the loop */
+                break;
+            default: /* consume 4 bytes to detect all combinations of cookie/sublayer types */
+                len -= sizeof(uint32_t);


RFC https://datatracker.ietf.org/doc/html/rfc3931#section-4.1 says that Cookie could be at most 64 bits. Is there any chance this might underflow?

Yes, but the extra 32-bits is to detect the optional L2-Specific Sublayer, which can exist between the Cookie and the Tunnel itself: https://datatracker.ietf.org/doc/html/rfc3931#section-3.2.2

The suggested L2-Specific Sublayer is 32-bits in the RFC: https://datatracker.ietf.org/doc/html/rfc3931#section-4.6

Actually re: underflow we check for +14 bytes (Ethernet Header) on each iteration of the loop, so it would not underflow.

I don't see that the cookie should be a multiple of 4 bytes in the RFC. It just says variable length.

Correct, but the only implementations I've run into the wild with the cookie are multiples of 4 bytes. Take for example the iproute(2) documentation on L2TPv3:

cookie HEXSTR
sets an optional cookie value to be assigned to the session. This is a 4 or 8 byte value, specified as 8 or 16 hex digits, e.g. 014d3636deadbeef. The value must match the peer_cookie value set at the peer. The cookie value is carried in L2TP data packets and is checked for expected value at the peer. Default is to use no cookie.

Similar with the L2-Specific Sublayer, there's no custom option only the default (from the RFC) on or off:

l2spec_type L2SPECTYPE
set the layer2specific header type of the session.
Valid values are: none, default.

Mikrotik's documentation [1] is similar for Cookies (0, 4, 8 byte only) and the SubLayer (on (4 byte) / off (0 byte))

peer-cookie ( string; Default: disabled) | Sets optional peer cookie. To enable cookie enter remote cookie value (8 or 16 character hex string value expected) to disable leave empty.

send-cookie ( string; Default: disabled) | Sets optional cookie. To enable cookie enter remote cookie value (8 or 16 character hex string value expected) to disable leave empty.

use-l2-specific-sublayer ( yes | no; Default: no) |

[1] https://help.mikrotik.com/docs/spaces/ROS/pages/2031631/L2TP#L2TP-L2TPEther

Cisco as well [1]

The size of the cookie field can be 4 or 8 bytes. If you do not enter this command, no cookie value is included in the header of L2TP packets.

[1] https://www.cisco.com/c/en/us/td/docs/routers/ios/config/17-x/lan-wan/b-lan-wan/m_wan-l2-tun-pro-v3-xe.html#task_vnp_h51_jlb

inashivb · 2025-10-15T07:01:38Z

src/decode-l2tp.h

+#define L2TP_DEFAULT_PORT_S "1701"
+
+/* Although L2TPv3 can do non-IP (like ATM), assume that there's a trailing ethernet header */
+#define L2TP_MIN_HEADER_LEN sizeof(L2TPoverUDPDataHdr) + sizeof(EthernetHdr)


Is this trailing Ethernet header always in full i.e. 14 bytes ?

Yes from what I've observed.

I've seen one edge case where we had some fragmentation due to mis-configuration (9000 MTU host forwarding to a 1500 MTU host) and because the L2TPv3 payload was the inner-most encapsulation it split the ethernet header across two packets. We set tunnel depth to non-default, so I would not expect to see this in the wild.

victorjulien · 2025-10-16T09:18:54Z

src/decode-l2tp.c

+        }
+        /* check the protocol type */
+        EthernetHdr *ethh = (EthernetHdr *)(pkt);
+        uint16_t proto = SCNtohs(ethh->eth_type);


here we take bytes 12/13 as eth header and check if it is a known type, but what if the cookie is still present and we just interpret some bytes from the hwaddresses as eth type? Then we could interpret address bytes as eth type?

codecov · 2025-10-16T12:07:59Z

Codecov Report

❌ Patch coverage is 90.45936% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.44%. Comparing base (16d124c) to head (db7449a).
⚠️ Report is 128 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #14023      +/-   ##
==========================================
+ Coverage   83.87%   84.44%   +0.57%     
==========================================
  Files        1011     1013       +2     
  Lines      275671   272534    -3137     
==========================================
- Hits       231207   230154    -1053     
+ Misses      44464    42380    -2084

Flag	Coverage Δ
fuzzcorpus	`63.40% <54.83%> (-0.11%)`	⬇️
livemode	`19.39% <25.00%> (-0.07%)`	⬇️
pcap	`44.76% <31.45%> (+<0.01%)`	⬆️
suricata-verify	`65.15% <33.33%> (+<0.01%)`	⬆️
unittests	`59.48% <86.87%> (+0.33%)`	⬆️

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

victorjulien · 2025-10-16T15:03:26Z

Btw there is a public pcap here https://wiki.wireshark.org/uploads/__moin_import__/attachments/SampleCaptures/nb6-http.pcap

poodle-amazon requested review from a team and victorjulien as code owners October 14, 2025 03:20

victorjulien requested changes Oct 14, 2025

View reviewed changes

victorjulien added the waiting for qa label Oct 14, 2025

decoder/l2tp: initial implementation of decoder

5149224

Issue: 7986

poodle-amazon force-pushed the l2tp-feature branch from 8b1d86e to 5149224 Compare October 14, 2025 06:21

poodle-amazon requested a review from jufajardini as a code owner October 14, 2025 06:21

poodle-amazon added 2 commits October 14, 2025 06:27

Fix up formatting w/git clang-format HEAD~

e944112

Remove some commented out debug code

90ea0fa

Revert clang-format on the JSON Schema

b99c4e9

victorjulien marked this pull request as draft October 14, 2025 07:09

victorjulien added the needs verify Needs (a) Suricata-verify test(s) label Oct 14, 2025

Fix sorting of l2tp in schema.json for properties.stats.properties.de…

0863bea

…coder.properties.event.properties

suricata-qa removed the waiting for qa label Oct 14, 2025

victorjulien reviewed Oct 14, 2025

View reviewed changes

src/decode-l2tp.h Outdated Show resolved Hide resolved

victorjulien reviewed Oct 14, 2025

View reviewed changes

suricata.yaml.in Outdated Show resolved Hide resolved

fix style on the header file / indentation on the yaml

cc0a282

jufajardini reviewed Oct 14, 2025

View reviewed changes

src/decode-l2tp.c Outdated Show resolved Hide resolved

inashivb reviewed Oct 15, 2025

View reviewed changes

victorjulien reviewed Oct 16, 2025

View reviewed changes

decoder/l2tp: initial implementation of decoder #14023

Are you sure you want to change the base?

decoder/l2tp: initial implementation of decoder #14023

Uh oh!

Conversation

poodle-amazon commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution style:

Our Contribution agreements:

Changes (if applicable):

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

Uh oh!

victorjulien left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Oct 14, 2025

Uh oh!

victorjulien commented Oct 14, 2025

Uh oh!

victorjulien commented Oct 14, 2025

Uh oh!

poodle-amazon commented Oct 14, 2025

Uh oh!

github-actions bot commented Oct 14, 2025

Uh oh!

suricata-qa commented Oct 14, 2025

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Oct 14, 2025

Uh oh!

victorjulien commented Oct 14, 2025

Uh oh!

poodle-amazon commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

victorjulien commented Oct 15, 2025

Uh oh!

poodle-amazon commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poodle-amazon Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 16, 2025

Codecov Report

Uh oh!

victorjulien commented Oct 16, 2025

poodle-amazon commented Oct 14, 2025 •

edited

Loading

victorjulien left a comment •

edited

Loading

poodle-amazon commented Oct 14, 2025 •

edited

Loading

poodle-amazon commented Oct 15, 2025 •

edited

Loading

poodle-amazon Oct 15, 2025 •

edited

Loading