SCTP decoder and sticky buffers v5 by glongo · Pull Request #15497 · OISF/suricata

glongo · 2026-06-01T17:13:22Z

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

Describe changes:

sctp.has_init, sctp.has_data, sctp.has_abort keywords removed

Previous PR: #15482

Extend the SCTP decoder to parse chunk headers after the 12-byte common header. Each chunk is validated for minimum header size and length consistency per RFC 4960 sec 3.2. Add SCTPChunkHdr and SCTPVars structs to track per-packet chunk metadata Add five new decoder events for protocol violations: - SCTP_CHUNK_TOO_SMALL: insufficient data for a chunk header - SCTP_CHUNK_LEN_INVALID: chunk length < 4 or exceeds packet - SCTP_INIT_CHUNK_NOT_ALONE: INIT/INIT_ACK bundled (RFC 4960 sec 6.10) - SCTP_INIT_WITH_NON_ZERO_VTAG: INIT with vtag != 0 (RFC 4960 sec 8.5.1) - SCTP_DATA_WITH_ZERO_VTAG: DATA chunk with vtag == 0 Ticket OISF#4251

Implement a sticky buffer to match the raw SCTP header (common header + chunks) Ticket OISF#4251

Add a U8 numeric keyword to match the first SCTP chunk type in a packet with prefilter support. Ticket OISF#4251

Add a U8 numeric keyword to match the number of SCTP chunks parsed in a packet with prefilter support. Ticket OISF#4251

Add a U32 numeric keyword to match the SCTP verification tag from the common header with prefilter support. Ticket OISF#4251

Log SCTP-specific fields in the EVE JSON "sctp" object for alert events. Ticket OISF#4251

Track the first DATA chunk's data offset and length during chunk iteration, then reassign p->payload to point at the user data. When no DATA chunk is present (INIT, SACK, HEARTBEAT, etc.), payload_len is set to 0 since there is no application data. Ticket OISF#4251

Add a sctp.data sticky multi-buffer that allows content matching on the bytes inside any of the SCTP DATA chunks extracted. Ticket OISF#4251

Add documentation for all sctp keywords. Ticket OISF#4251

codecov · 2026-06-01T18:11:47Z

Codecov Report

❌ Patch coverage is 87.18626% with 97 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.76%. Comparing base (736f506) to head (bd1f229).

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #15497    +/-   ##
========================================
  Coverage   82.75%   82.76%            
========================================
  Files         998     1005     +7     
  Lines      272550   273301   +751     
========================================
+ Hits       225562   226195   +633     
- Misses      46988    47106   +118

Flag	Coverage Δ
fuzzcorpus	`61.42% <40.87%> (+<0.01%)`	⬆️
livemode	`18.28% <14.84%> (+<0.01%)`	⬆️
netns	`22.71% <14.84%> (-0.09%)`	⬇️
pcap	`44.91% <35.76%> (-0.08%)`	⬇️
suricata-verify	`66.45% <72.74%> (-0.03%)`	⬇️
unittests	`58.46% <65.25%> (+0.02%)`	⬆️

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.

suricata-qa · 2026-06-01T18:45:04Z

WARNING:

field	baseline	test	%
	SURI_TLPW2_single_stats_chk
.decoder.event.sctp.chunk_too_small	-	0	-
.decoder.event.sctp.chunk_len_invalid	-	0	-
.decoder.event.sctp.init_chunk_not_alone	-	0	-
.decoder.event.sctp.init_with_non_zero_vtag	-	0	-
.decoder.event.sctp.data_with_zero_vtag	-	0	-
.sctp.init	-	0	-
.sctp.init_ack	-	0	-
.sctp.data	-	0	-
.sctp.abort	-	0	-
.sctp.shutdown	-	0	-
	SURI_TLPW2_autofp_stats_chk
.decoder.event.sctp.chunk_too_small	-	0	-
.decoder.event.sctp.chunk_len_invalid	-	0	-
.decoder.event.sctp.init_chunk_not_alone	-	0	-
.decoder.event.sctp.init_with_non_zero_vtag	-	0	-
.decoder.event.sctp.data_with_zero_vtag	-	0	-
.sctp.init	-	0	-
.sctp.init_ack	-	0	-
.sctp.data	-	0	-
.sctp.abort	-	0	-
.sctp.shutdown	-	0	-
	SURI_TLPW1_stats_chk
.decoder.event.sctp.chunk_too_small	-	0	-
.decoder.event.sctp.chunk_len_invalid	-	0	-
.decoder.event.sctp.init_chunk_not_alone	-	0	-
.decoder.event.sctp.init_with_non_zero_vtag	-	0	-
.decoder.event.sctp.data_with_zero_vtag	-	0	-
.sctp.init	-	0	-
.sctp.init_ack	-	0	-
.sctp.data	-	0	-
.sctp.abort	-	0	-
.sctp.shutdown	-	0	-
	SURI_TLPR1_stats_chk
.decoder.event.sctp.chunk_too_small	-	0	-
.decoder.event.sctp.chunk_len_invalid	-	0	-
.decoder.event.sctp.init_chunk_not_alone	-	0	-
.decoder.event.sctp.init_with_non_zero_vtag	-	0	-
.decoder.event.sctp.data_with_zero_vtag	-	0	-
.sctp.init	-	0	-
.sctp.init_ack	-	0	-
.sctp.data	-	0	-
.sctp.abort	-	0	-
.sctp.shutdown	-	0	-
	IPS_AFP_stats_chk
.decoder.event.sctp.chunk_too_small	-	0	-
.decoder.event.sctp.chunk_len_invalid	-	0	-
.decoder.event.sctp.init_chunk_not_alone	-	0	-
.decoder.event.sctp.init_with_non_zero_vtag	-	0	-
.decoder.event.sctp.data_with_zero_vtag	-	0	-
.sctp.init	-	0	-
.sctp.init_ack	-	0	-
.sctp.data	-	0	-
.sctp.abort	-	0	-
.sctp.shutdown	-	0	-
	TREX_GENERIC_stats_chk
.decoder.event.sctp.chunk_too_small	-	0	-
.decoder.event.sctp.chunk_len_invalid	-	0	-
.decoder.event.sctp.init_chunk_not_alone	-	0	-
.decoder.event.sctp.init_with_non_zero_vtag	-	0	-
.decoder.event.sctp.data_with_zero_vtag	-	0	-
.sctp.init	-	0	-
.sctp.init_ack	-	0	-
.sctp.data	-	0	-
.sctp.abort	-	0	-
.sctp.shutdown	-	0	-

Pipeline = 31783

victorjulien

Almost there I think, see inline comments.

victorjulien · 2026-06-02T04:52:27Z

@@ -0,0 +1,8 @@
+# SCTP decoder event rules.
+# SID's fall in the 2239000-2239999 range. See http://doc.emergingthreats.net/bin/view/Main/SidAllocation


nit: we recently updated these for other files

victorjulien · 2026-06-02T04:56:59Z

+            break;
+        }
+
+        if (chunk_cnt < SCTP_MAX_TRACKED_CHUNKS) {


should there be an event when there are more chunks than we track?

It would be useful for sure.

victorjulien · 2026-06-02T04:57:58Z

+                has_init_ack = true;
+                break;
+            case SCTP_CHUNK_TYPE_DATA:
+                if (data_chunk_cnt < SCTP_MAX_DATA_CHUNKS && chunk_len >= SCTP_DATA_CHUNK_HDR_LEN) {


same, should there be an event if max is exceeded?

victorjulien · 2026-06-02T05:00:41Z

+#define SCTP_CHUNK_HDR_LEN 4
+
+/** max number of chunks tracked per packet for detection/logging */
+#define SCTP_MAX_TRACKED_CHUNKS 16


how was 16 chosen? Add as a comment in the code

It's a choice I made to keep per-packet overhead low while still allowing some room to track chunks.

SCTP has no hard limit on the number of chunks per packet. A packet can carry as many chunks as fit within the MTU, though in practice most packets contain only a few chunks.

Thinking about the maximum packet size, which is probably close to 64k, how many iterations of the loop will there be at max? So max size / smallest chunk record size I suppose?

victorjulien · 2026-06-02T05:00:48Z

+#define SCTP_MAX_TRACKED_CHUNKS 16
+
+/** max number of DATA chunk payloads tracked per packet */
+#define SCTP_MAX_DATA_CHUNKS 16


victorjulien · 2026-06-02T05:05:46Z

+ */
+static InspectionBuffer *GetBuffer(DetectEngineThreadCtx *det_ctx,
+        const DetectEngineTransforms *transforms, Packet *p, const int list_id,
+        const uint32_t local_id, const uint8_t chunk_idx)


since local_id == chunk_idx every time, could just pass one

victorjulien · 2026-06-02T05:06:34Z

+    }
+
+    const DetectU8Data *data = (const DetectU8Data *)ctx;
+    const uint8_t cnt = MIN(p->l4.vars.sctp.chunk_cnt, SCTP_MAX_TRACKED_CHUNKS);


this has been validated by the decoder already, right?

No, the decoder stores the raw count at line 146:
p->l4.vars.sctp.chunk_cnt = chunk_cnt;

And chunk_cnt is incremented unconditionally for every chunk, even after SCTP_MAX_TRACKED_CHUNKS is reached.

victorjulien · 2026-06-02T06:21:37Z

+    }
+
+    const PrefilterPacketU8HashCtx *h = pectx;
+    const uint8_t cnt = MIN(p->l4.vars.sctp.chunk_cnt, SCTP_MAX_TRACKED_CHUNKS);


victorjulien · 2026-06-02T06:22:28Z

+    uint32_t seen[8] = { 0 };
+    for (uint8_t i = 0; i < cnt; i++) {
+        const uint8_t val = p->l4.vars.sctp.chunk_types[i];
+        if (seen[val >> 5] & (1U << (val & 0x1F))) {


can you explain the shift & mask logic here?

The purpose is to skip calling PrefilterAddSids for duplicate chunk types within the same packet.
If two chunks both have type DATA, the sigs for that type are only added once.

So, line 119 is used to check if val has been seen.

victorjulien · 2026-06-02T06:24:00Z

+                SCJbOpenObject(js, "sctp");
+                SCJbSetUint(js, "vtag", SCTP_GET_RAW_VTAG(PacketGetSCTP(p)));
+                SCJbSetUint(js, "chunk_cnt", p->l4.vars.sctp.chunk_cnt);
+                const uint8_t cnt = MIN(p->l4.vars.sctp.chunk_cnt, SCTP_MAX_TRACKED_CHUNKS);


same, I think we should be able to trust the decoder. To double check we can add debug validate bug on's

victorjulien · 2026-06-02T08:11:15Z

+            break;
+        }
+
+        const SCTPChunkHdr *chunk = (const SCTPChunkHdr *)(pkt + offset);


can this be an unaligned access? Should probably be a memcpy.

Isn't this line following the same pattern as, for instance, PacketSetTCP?
buf is cast directly to a TCPHdr *:

p->l4.hdrs.tcph = (TCPHdr *)buf;

Yes but we're slowing going to update these.

victorjulien · 2026-06-02T08:13:28Z

    p->proto = IPPROTO_SCTP;
+
+    if (p->payload_len > 0) {
+        if (DecodeSCTPChunks(p, p->payload, p->payload_len) < 0) {


we pass in payload_len, which is uint16_t

victorjulien · 2026-06-02T08:14:13Z

+ * \retval 0 on success (even if some events were set)
+ * \retval -1 on fatal error (packet should be rejected)
+ */
+static int DecodeSCTPChunks(Packet *p, const uint8_t *pkt, uint32_t len)


do we need to use uint32_t here? We pass in uint16_t

victorjulien · 2026-06-02T08:15:57Z

+        }
+
+        /* chunk must not extend beyond available data */
+        if (chunk_len > (len - offset)) {


can we also have an absolute max value?

I've tried to use UINT16_MAX as absolute value but, as expected, the compiler is complaining:

decode-sctp.c:91:23: warning: comparison is always false due to limited range of data type [-Wtype-limits] 91 | if (chunk_len > UINT16_MAX || chunk_len > (len - offset)) {

I wonder if I should use a different value or I can remove this check.

victorjulien · 2026-06-02T08:19:27Z

+                    p->l4.vars.sctp.data_lens[data_chunk_cnt] = chunk_len - SCTP_DATA_CHUNK_HDR_LEN;
+                    data_chunk_cnt++;
+                }
+                has_data = true;


is there a scenario where we set this but didn't track the chunks? If so, what would happen?

There are two cases:

chunk_len < SCTP_DATA_CHUNK_HDR_LEN (Malformed data chunk)

data_chunk_cnt >= SCTP_MAX_DATA_CHUNKS) (more DATA chunks than the array can hold)

The only effect is that counter_sctp_data is incremented, see DecodeSCTP line 210:

if (p->l4.vars.sctp.has_data) { StatsCounterIncr(&tv->stats, dtv->counter_sctp_data); }

victorjulien · 2026-06-02T08:20:51Z

+        if (padded_len < SCTP_CHUNK_HDR_LEN) {
+            padded_len = SCTP_CHUNK_HDR_LEN;
+        }
+        offset += padded_len;


can we add a DEBUG_VALIDATE_BUG_ON(offset > len) here, as we shouldn't advance offset beyond len

Yes but note that offset can legitimately exceed len after padding.
The padding rounds up to a 4-byte boundary, so if the last chunk ends at len but isn't 4-byte aligned, padded_len > chunk_len and offset overshoots len.

We'd need to check only for the case where offset exceeds len by more than 3 bytes (the max padding).

glongo · 2026-06-03T16:08:09Z

Replaced with #15534

glongo added 9 commits June 1, 2026 17:49

detect/sctp: add sctp.hdr sticky buffer

7e8ee23

Implement a sticky buffer to match the raw SCTP header (common header + chunks) Ticket OISF#4251

detect/sctp: add sctp.chunk_type keyword

341a9a6

Add a U8 numeric keyword to match the first SCTP chunk type in a packet with prefilter support. Ticket OISF#4251

detect/sctp: add sctp.chunk_cnt keyword

56317b5

Add a U8 numeric keyword to match the number of SCTP chunks parsed in a packet with prefilter support. Ticket OISF#4251

detect/sctp: add sctp.vtag keyword

7fbdfd7

Add a U32 numeric keyword to match the SCTP verification tag from the common header with prefilter support. Ticket OISF#4251

output/json: add sctp metadata to alerts

03e1dbb

Log SCTP-specific fields in the EVE JSON "sctp" object for alert events. Ticket OISF#4251

detect/sctp: add sctp.chunk_data sticky buffer

38b6634

Add a sctp.data sticky multi-buffer that allows content matching on the bytes inside any of the SCTP DATA chunks extracted. Ticket OISF#4251

doc/sctp: add sctp keywords

bd1f229

Add documentation for all sctp keywords. Ticket OISF#4251

glongo requested review from a team, jasonish, jufajardini and victorjulien as code owners June 1, 2026 17:13

glongo mentioned this pull request Jun 1, 2026

SCTP decoder and sticky buffers v4 #15482

Closed

victorjulien requested changes Jun 2, 2026

View reviewed changes

victorjulien reviewed Jun 2, 2026

View reviewed changes

glongo mentioned this pull request Jun 3, 2026

SCTP decoder and sticky buffers v6 #15534

Closed

glongo closed this Jun 3, 2026

		@@ -0,0 +1,8 @@
		# SCTP decoder event rules.
		# SID's fall in the 2239000-2239999 range. See http://doc.emergingthreats.net/bin/view/Main/SidAllocation

Uh oh!

Conversation

glongo commented Jun 1, 2026

Uh oh!

codecov Bot commented Jun 1, 2026

Codecov Report

Uh oh!

suricata-qa commented Jun 1, 2026

Uh oh!

victorjulien left a comment

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

glongo commented Jun 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants