Skip to content
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

SDP: implements sticky buffers #11877

Closed
wants to merge 19 commits into from

Conversation

glongo
Copy link
Contributor

@glongo glongo commented Oct 4, 2024

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

Contribution style:

Our Contribution agreements:

Changes (if applicable):

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

Describe changes:

  • Sticky buffers for SDP have been implemented.
  • 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.

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.

SV_BRANCH=OISF/suricata-verify#2076

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
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
Copy link

codecov bot commented Oct 4, 2024

Codecov Report

Attention: Patch coverage is 92.07921% with 80 lines in your changes missing coverage. Please review.

Project coverage is 82.65%. Comparing base (501f79c) to head (43dafab).
Report is 38 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11877      +/-   ##
==========================================
+ Coverage   82.56%   82.65%   +0.08%     
==========================================
  Files         912      913       +1     
  Lines      249354   250254     +900     
==========================================
+ Hits       205880   206835     +955     
+ Misses      43474    43419      -55     
Flag Coverage Δ
fuzzcorpus 60.44% <34.89%> (-0.03%) ⬇️
livemode 18.85% <31.90%> (+0.12%) ⬆️
pcap 44.04% <35.09%> (-0.05%) ⬇️
suricata-verify 62.18% <91.72%> (+0.18%) ⬆️
unittests 58.83% <35.24%> (-0.11%) ⬇️

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

};
if let Some(sdp) = sdp_message {
let session_name = &sdp.session_name;
if !session_name.is_empty() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we return an empty buffer so we can match on sdp.session_name; bsize:0;?

Copy link
Contributor Author

@glongo glongo Oct 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This field is mandatory, the parser will fail if it's not found. However, this approach might make sense for optional fields, such as session information.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this code should suggest it can be empty then.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should a rule like sdp.session_info; bsize:0 trigger an alert if the session_info field is not present at all?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, cf #11906 which adds absent keyword to match when the field is not present

@glongo
Copy link
Contributor Author

glongo commented Oct 15, 2024

Replaced with #11961

@glongo glongo closed this Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants