Conversation
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
Add a boolean NOOPT keyword to match SCTP packets containing an INIT or INIT_ACK chunk. Ticket OISF#4251
Add a boolean NOOPT keyword to match SCTP packets containing a DATA chunk. Ticket OISF#4251
Add a boolean NOOPT keyword to match SCTP packets containing an ABORT chunk. 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 buffer that allows content matching on the bytes inside the first SCTP DATA chunk. Ticket OISF#4251
Add documentation for all sctp keywords. Ticket OISF#4251
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #15226 +/- ##
========================================
Coverage 82.68% 82.69%
========================================
Files 993 1010 +17
Lines 271880 272682 +802
========================================
+ Hits 224807 225493 +686
- Misses 47073 47189 +116
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| sctp.chunk_type | ||
| --------------- | ||
|
|
||
| Match on the type of the first SCTP chunk in the packet. |
There was a problem hiding this comment.
forgot to update this doc
|
|
|
Information: QA ran without warnings. Pipeline = 30979 |
|
Array like this can be wrapped in |
| void DetectSCTPDataRegister(void) | ||
| { | ||
| sigmatch_table[DETECT_SCTP_DATA].name = "sctp.data"; | ||
| sigmatch_table[DETECT_SCTP_DATA].desc = "sticky buffer to match on the SCTP DATA chunk payload"; |
There was a problem hiding this comment.
I think this should be sctp.chunk_data to clearly indicate it's not reassembled.
And it also should be a multibuffer so we can inspect each chunk that may be in the packet, not just the first.
jufajardini
left a comment
There was a problem hiding this comment.
Inline, a few things docs / schema related.
I tried to check whether the progress-tracking for buffer keywords work (8f82445) would be relevant here, but it doesn't seem so. (In any case, highlighting, in case I'm wrong and in the chance it's unknown)
| Match if the SCTP packet contains an INIT or INIT_ACK chunk. This is a | ||
| boolean keyword that takes no arguments. |
There was a problem hiding this comment.
Is this piece also missing an update (based on previous PR's review comments)?
| "properties": { | ||
| "chunk_cnt": { | ||
| "type": "integer", | ||
| "suricata": { |
There was a problem hiding this comment.
This is probably the best time to also add a description to these, isn't it? :P
| "init": { | ||
| "type": "integer", | ||
| "description": "Number of SCTP packets with INIT or INIT_ACK chunk" | ||
| }, |
There was a problem hiding this comment.
Has this been decoupled? (as per the counter below and https://github.com/OISF/suricata/pull/15129/changes#r3021634492)
| @@ -0,0 +1,8 @@ | |||
| # SCTP decoder event rules. | |||
| # SID's fall in the 2200000+ range. See http://doc.emergingthreats.net/bin/view/Main/SidAllocation | |||
There was a problem hiding this comment.
2200000+ seems too generic, looking at examples from some other app-proto event rules (ldap, pgsql, ftp...) and the 2200130.. 2200135 sids present here.
catenacyber
left a comment
There was a problem hiding this comment.
Looks like there are remarks to be taken care of like the clang-format off
|
|
||
| Syntax:: | ||
|
|
||
| sctp.chunk_type:[op]<number> |
There was a problem hiding this comment.
Do these chunk-types have a stringer ?
| sctp.has_init | ||
| ------------- | ||
|
|
||
| Match if the SCTP packet contains an INIT or INIT_ACK chunk. This is a |
There was a problem hiding this comment.
Why a dedicated keyword instead of sctp.chunk_type:INIT; ?
|
|
||
| alert ipv4 any any -> any any (msg:"SURICATA IPv4 unknown protocol"; decode-event:ipv4.unknown_protocol; threshold: type limit, track by_src, seconds 60, count 1;classtype:protocol-command-decode; sid:2200125;) | ||
| # next sid is 2200130 | ||
| # next sid is 2200136 |
| "(msg:\"test\"; sctp.chunk_cnt:>5; sid:2;)")); | ||
|
|
||
| FAIL_IF_NOT_NULL(DetectEngineAppendSig(de_ctx, "alert sctp any any -> any any " | ||
| "(msg:\"test\"; sctp.chunk_cnt:bar; sid:3;)")); |
There was a problem hiding this comment.
Is it not better to do SV tests than unit tests ?
| /** \test match on a non-first chunk: bundled [DATA, SACK], match sctp.chunk_type:3 */ | ||
| static int DetectSCTPChunkTypeMatchNonFirstTest(void) | ||
| { | ||
| uint8_t raw_sctp[] = { |
There was a problem hiding this comment.
This looks even more like it should be a SV test
| #define SCTP_DATA_CHUNK_HDR_LEN 16 | ||
|
|
||
| /* SCTP chunk types (RFC 4960 sec 3.2) */ | ||
| #define SCTP_CHUNK_TYPE_DATA 0x00 |
There was a problem hiding this comment.
Looks like we have a stringer :-)
Did you think about trying rust here ?
| sigmatch_table[transform_id].Transform = (void (*)(DetectEngineThreadCtx * det_ctx, | ||
| InspectionBuffer * buffer, const void *options)) kw->Transform; | ||
| sigmatch_table[transform_id].Transform = (void (*)(DetectEngineThreadCtx *det_ctx, | ||
| InspectionBuffer *buffer, const void *options))kw->Transform; |
There was a problem hiding this comment.
You have a too recent clang-format ;-)
|
|
||
| const DetectU8Data *data = (const DetectU8Data *)ctx; | ||
| const uint8_t cnt = MIN(p->l4.vars.sctp.chunk_cnt, SCTP_MAX_TRACKED_CHUNKS); | ||
| for (uint8_t i = 0; i < cnt; i++) { |
There was a problem hiding this comment.
Looks like you could try to implement SIGMATCH_INFO_MULTI_UINT ;-)
|
|
||
| static int DetectSCTPChunkTypeSetup(DetectEngineCtx *de_ctx, Signature *s, const char *str) | ||
| { | ||
| if (!(DetectProtoContainsProto(s->proto, IPPROTO_SCTP))) |
There was a problem hiding this comment.
Could I not write alert ip any any -> any any (sctp.chunk_type: 1;) ?
Link to ticket: https://redmine.openinfosecfoundation.org/issues/4251
Describe changes:
decoder-sctp.c#L166withconst SCTPHdr *sctph = PacketSetSCTP(p, pkt);decoder-sctp.c#L145withhlen = SCTP_HEADER_LEN + MIN(offset, len);pkthdrwithsctpinrules/sctp-events.rulessctp.has_initmatches only onINIT(INIT_ACKremoved)Previous PR: #15129
SV_BRANCH=OISF/suricata-verify#2999