SDP: implements sticky buffers v2 #11961

glongo · 2024-10-15T09:11:13Z

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/

Describe changes:

Sticky buffers for SDP have been implemented.
- The empty check within sticky buffer for optional fields has been removed
sdp.media.bandwidth and sdp.media.attribute sticky buffers have not been implemented yet because the media field is a vector of MediaDescription, and both the bandwidth and attribute fields are vectors as well.
I believe the multi-buffer API cannot handle such a situation.
If I'm not mistaken, a simple solution could be to "stringify" these fields like this:

"media_description": {
    "bandwidth": "AS:64,AS:65,AS:66",
    "attribute": "sendonly,recvonly"
}

Media's encryption key is logged now.
Multiple time and repeat_time fields are now parsed correctly

Personal considerations:

The sdp.media.* sticky buffers were initially meant to be named sdp.media_descriptions.*, but I realized the names were too long. I wonder if I should rename the media_description field to media for consistency.
I don't like the name sdp.media.media; I would prefer to rename it to sdp.media.name, or just sdp.media, and adjust the logged field name if necessary.
CI will probably fail due to the changes made to the way the time and repeat_time fields are logged.

SV_BRANCH=OISF/suricata-verify#2092

The encryption key subfield of the media description field is not logged when it should be. Ticket OISF#7305

The current parser implementations take a field, such as connection data, and split it into subfields for a specific structure (e.g., struct ConnectionData). However, following this approach requires several sticky buffers to match the whole field, which can make a rule a bit verbose and doesn't offer any advantage for matching specific parts of a field. With this patch, a single line is still split into pieces if it makes sense for parsing purposes, but these pieces are then reassembled into a single string. This way, only one sticky buffer is needed to match the entire field. Ticket OISF#7291

As defined in RFC4566, the time and repeat_time fields can be present multiple times but they are currently parsed only once. Ticket OISF#7325

This adds a sticky buffer to match the "Session name" field in both requests and responses. Ticket OISF#7291

This adds a sticky buffer to match the "Session information" field in both requests and responses. Ticket OISF#7291

This adds a sticky buffer to match the "Origin" field in both requests and responses. Ticket OISF#7291

This adds a sticky buffer to match the "Uri" field in both requests and responses. Ticket OISF#7291

This adds a sticky buffer to match the "Email" field in both requests and responses. Ticket OISF#7291

This adds a sticky buffer to match the "Phone number" field in both requests and responses. Ticket OISF#7291

This adds a sticky buffer to match the "Connection data" field in both requests and responses. Ticket OISF#7291

This adds a sticky (multi) buffer to match the "Bandwidth" field in both requests and responses. Ticket OISF#7291

This adds a sticky buffer to match the "Time" field in both requests and responses. Ticket OISF#7291

This adds a sticky buffer to match the "Repeat time" field in both requests and responses. Ticket OISF#7291

This adds a sticky bufffer to match the "Timezone" field in both requests and responses. Ticket OISF#7291

This adds a sticky buffer to match the "Encryption key" field in both requests and responses. Ticket OISF#7291

This adds a sticky (multi) buffer to match the "Attribute" field in both requests and responses. Ticket OISF#7291

This adds a sticky (multi) buffer to match the "Media" subfield of the "Media description" field in both requests and responses. Ticket OISF#7291

This adds a stick (multi) buffer to match the "Session information" subfield of the "Media description" field in both requests and responses. Ticket OISF#7291

This adds a sticky (multi) buffer to match the "Connection data" subfield of the "Media description" field in both requests and responses. Ticket OISF#7291

This adds a sticky (multi) buffer to match the "Encryption key" subfield of the "Media description" field in both requests and responses. Ticket OISF#7291

codecov · 2024-10-15T09:31:46Z

Codecov Report

Attention: Patch coverage is 92.09756% with 81 lines in your changes missing coverage. Please review.

Project coverage is 82.80%. Comparing base (378f678) to head (b6fe4e7).
Report is 227 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11961      +/-   ##
==========================================
+ Coverage   82.76%   82.80%   +0.03%     
==========================================
  Files         910      911       +1     
  Lines      249014   249922     +908     
==========================================
+ Hits       206105   206942     +837     
- Misses      42909    42980      +71

Flag	Coverage Δ
fuzzcorpus	`60.59% <35.95%> (-0.18%)`	⬇️
livemode	`18.80% <31.43%> (+0.09%)`	⬆️
pcap	`44.06% <36.14%> (+0.21%)`	⬆️
suricata-verify	`62.35% <91.74%> (+0.18%)`	⬆️
unittests	`58.90% <34.73%> (-0.11%)`	⬇️

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

victorjulien · 2024-10-15T12:52:49Z

Lots of failing jobs. Any idea what is going on?

glongo · 2024-10-15T15:22:03Z

Lots of failing jobs. Any idea what is going on?

Yes, CI is failing because I changed the json schema for the field time and repeat_time and SV test needs to be updated (which I did in OISF/suricata-verify#2092)

See 2b3e0cb

victorjulien · 2024-10-16T04:32:26Z

Lots of failing jobs. Any idea what is going on?

Yes, CI is failing because I changed the json schema for the field time and repeat_time and SV test needs to be updated (which I did in OISF/suricata-verify#2092)

See 2b3e0cb

I don't understand. The CI for the PR should pass. If the SV PR is needed for it to pass, we should see a green CI. But we don't, so something is off.

victorjulien

CI is very red.

inashivb · 2024-10-16T06:21:51Z

@glongo if you change SV_BRANCH, please tag a team member to rerun the CI checks as they are not auto run.

glongo · 2024-10-16T06:50:48Z

Lots of failing jobs. Any idea what is going on?

Yes, CI is failing because I changed the json schema for the field time and repeat_time and SV test needs to be updated (which I did in OISF/suricata-verify#2092)
See 2b3e0cb

I don't understand. The CI for the PR should pass. If the SV PR is needed for it to pass, we should see a green CI. But we don't, so something is off.

I accidentally used the wrong SV_BRANCH and likely changed it after the CI had already started.

glongo · 2024-10-16T06:51:18Z

CI is very red.

It's green now

glongo · 2024-10-27T13:13:15Z

Any updates on this?

catenacyber · 2024-10-27T19:18:06Z

rust/src/sdp/parser.rs

@@ -40,13 +40,13 @@ use std::str::FromStr;
 #[derive(Debug)]
 pub struct SdpMessage {
    pub version: u32,
-    pub origin: OriginField,
+    pub origin: String,


Why a String and not a Vec ?

Why should I use a vec?

Rust String performs utf8 validation and I think Suricata was the raw bytes even if they are invalid UTF8 ?

Ok for me as all String are generated by format! thanks @glongo

etc/schema.json

glongo · 2024-11-04T09:00:25Z

@victorjulien ?

victorjulien · 2024-11-14T11:19:49Z

@catenacyber can you approve the PR if you think it is good in its current state?

catenacyber · 2024-11-14T11:43:56Z

Looks good, but I did not do a thorough review yet

catenacyber

Thanks for the work Giuseppe

CI : ✅
Code : good
Commits segmentation : ok, great for scoring
Commit messages : good
Git ID set : looks fine for me
CLA : you already contributed
Doc update : nice
Redmine ticket : ok
Rustfmt : looks ok
Tests : nice, thanks
Dependencies added: none

catenacyber · 2024-11-18T15:15:49Z

doc/userguide/upgrade.rst

@@ -79,6 +79,7 @@ Major changes
    - sip.content_length
 - Napatech support has been moved to a capture plugin. See :doc:`Napatech plugin
  <upgrade/8.0-napatech-plugin>`.
+- SDP sticky buffers have been introduced.


nit: I would put in the same line as SDP parsing as this is upgrade notes to 8 ;-)

catenacyber · 2024-11-18T15:16:51Z

rust/src/sdp/detect.rs

@@ -833,6 +834,59 @@ unsafe extern "C" fn sip_media_desc_connection_data_get_data(
    false
 }

+unsafe extern "C" fn sdp_media_desc_encryption_key_setup(


Wondering if a macro would help to keep code more compact

catenacyber · 2024-11-18T15:19:18Z

Had you considered frames for SDP ?

catenacyber · 2024-11-28T08:21:12Z

@glongo I guess it is good to do a new rebased version of this as Victor likely does not see this PR anymore as he had put changes requested ;-)

glongo · 2025-01-16T09:56:01Z

Replaced with #12327

glongo added 20 commits October 15, 2024 09:42

sdp: log media's encryption key

02aebd4

The encryption key subfield of the media description field is not logged when it should be. Ticket OISF#7305

sdp: parse time and repeat_time multiple times

2b3e0cb

As defined in RFC4566, the time and repeat_time fields can be present multiple times but they are currently parsed only once. Ticket OISF#7325

sdp: add sdp.session_name sticky buffer

e29d3b8

This adds a sticky buffer to match the "Session name" field in both requests and responses. Ticket OISF#7291

sdp: add sdp.session_info sticky buffer

96abed7

This adds a sticky buffer to match the "Session information" field in both requests and responses. Ticket OISF#7291

sdp: add sdp.origin sticky buffer

4ec2ef8

This adds a sticky buffer to match the "Origin" field in both requests and responses. Ticket OISF#7291

sdp: add sdp.uri sticky buffer

323b52b

This adds a sticky buffer to match the "Uri" field in both requests and responses. Ticket OISF#7291

sdp: add sdp.email sticky buffer

c6ac35a

This adds a sticky buffer to match the "Email" field in both requests and responses. Ticket OISF#7291

sdp: add sdp.phone_number sticky buffer

25519f5

This adds a sticky buffer to match the "Phone number" field in both requests and responses. Ticket OISF#7291

sdp: add sdp.connection_data sticky buffer

7e94800

This adds a sticky buffer to match the "Connection data" field in both requests and responses. Ticket OISF#7291

sdp: add sdp.bandwidth sticky buffer

ad716e7

This adds a sticky (multi) buffer to match the "Bandwidth" field in both requests and responses. Ticket OISF#7291

sdp: add sdp.time sticky buffer

0a11ba7

This adds a sticky buffer to match the "Time" field in both requests and responses. Ticket OISF#7291

sdp: add sdp.repeat_time sticky buffer

1f78bf3

This adds a sticky buffer to match the "Repeat time" field in both requests and responses. Ticket OISF#7291

sdp: add sdp.timezone sticky buffer

de2c2d7

This adds a sticky bufffer to match the "Timezone" field in both requests and responses. Ticket OISF#7291

sdp: add sdp.encryption_key sticky buffer

adeef95

This adds a sticky buffer to match the "Encryption key" field in both requests and responses. Ticket OISF#7291

sdp: add sdp.attribute sticky buffer

5e1ef90

This adds a sticky (multi) buffer to match the "Attribute" field in both requests and responses. Ticket OISF#7291

sdp: add sdp.media.media sticky buffer

1b17a73

This adds a sticky (multi) buffer to match the "Media" subfield of the "Media description" field in both requests and responses. Ticket OISF#7291

sdp: add sdp.media.media_info sticky buffer

4c45924

This adds a stick (multi) buffer to match the "Session information" subfield of the "Media description" field in both requests and responses. Ticket OISF#7291

sdp: add sdp.media.connection_data sticky buffer

3f82820

This adds a sticky (multi) buffer to match the "Connection data" subfield of the "Media description" field in both requests and responses. Ticket OISF#7291

sdp: add sdp.media.encryption_key sticky buffer

b6fe4e7

This adds a sticky (multi) buffer to match the "Encryption key" subfield of the "Media description" field in both requests and responses. Ticket OISF#7291

glongo requested review from jasonish, jufajardini, victorjulien and a team as code owners October 15, 2024 09:11

This was referenced Oct 15, 2024

SDP: implements sticky buffers #11877

Closed

SDP: add tests for sticky buffers v2 OISF/suricata-verify#2092

Closed

victorjulien requested changes Oct 16, 2024

View reviewed changes

catenacyber reviewed Oct 27, 2024

View reviewed changes

etc/schema.json Show resolved Hide resolved

catenacyber approved these changes Nov 18, 2024

View reviewed changes

catenacyber reviewed Nov 18, 2024

View reviewed changes

glongo closed this Jan 16, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SDP: implements sticky buffers v2 #11961

SDP: implements sticky buffers v2 #11961

glongo commented Oct 15, 2024 •

edited

Loading

codecov bot commented Oct 15, 2024 •

edited

Loading

victorjulien commented Oct 15, 2024

glongo commented Oct 15, 2024

victorjulien commented Oct 16, 2024

victorjulien left a comment

inashivb commented Oct 16, 2024

glongo commented Oct 16, 2024

glongo commented Oct 16, 2024

glongo commented Oct 27, 2024

catenacyber Oct 27, 2024

glongo Oct 29, 2024

catenacyber Oct 29, 2024

catenacyber Nov 13, 2024

glongo commented Nov 4, 2024

victorjulien commented Nov 14, 2024

catenacyber commented Nov 14, 2024

catenacyber left a comment

catenacyber Nov 18, 2024

catenacyber Nov 18, 2024

catenacyber commented Nov 18, 2024

catenacyber commented Nov 28, 2024

glongo commented Jan 16, 2025

SDP: implements sticky buffers v2 #11961

SDP: implements sticky buffers v2 #11961

Conversation

glongo commented Oct 15, 2024 • edited Loading

Contribution style:

Our Contribution agreements:

Changes (if applicable):

codecov bot commented Oct 15, 2024 • edited Loading

Codecov Report

victorjulien commented Oct 15, 2024

glongo commented Oct 15, 2024

victorjulien commented Oct 16, 2024

victorjulien left a comment

Choose a reason for hiding this comment

inashivb commented Oct 16, 2024

glongo commented Oct 16, 2024

glongo commented Oct 16, 2024

glongo commented Oct 27, 2024

catenacyber Oct 27, 2024

Choose a reason for hiding this comment

glongo Oct 29, 2024

Choose a reason for hiding this comment

catenacyber Oct 29, 2024

Choose a reason for hiding this comment

catenacyber Nov 13, 2024

Choose a reason for hiding this comment

glongo commented Nov 4, 2024

victorjulien commented Nov 14, 2024

catenacyber commented Nov 14, 2024

catenacyber left a comment

Choose a reason for hiding this comment

catenacyber Nov 18, 2024

Choose a reason for hiding this comment

catenacyber Nov 18, 2024

Choose a reason for hiding this comment

catenacyber commented Nov 18, 2024

catenacyber commented Nov 28, 2024

glongo commented Jan 16, 2025

glongo commented Oct 15, 2024 •

edited

Loading

codecov bot commented Oct 15, 2024 •

edited

Loading