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

detect/sip: add sticky buffers to match headers v5 #11620

Closed
wants to merge 10 commits into from

Conversation

glongo
Copy link
Contributor

@glongo glongo commented Aug 10, 2024

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

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

Describe changes:

  • New sticky buffers have been rewritten in rust
  • Existing sticky buffers have been rewritten in rust, except for sip.method and sip.uri

Note:
sip.method and sip.uri require other C functions to be exposed in rust in order to be rewritten.
I can address this in a follow-up PR, I just need some time to review it.

SV_BRANCH=OISF/suricata-verify#2005

According to RFC 3261, a single header can be repeated one or more times,
and its name can also be specified using the 'compact form.'

This patch updates the hashmap used for storing headers to accommodate multiple
values instead of just one.

Additionally, if a header name is defined in the compact form, it is expanded
into its long form (i.e., the standard name).

This conversion simplifies the logic for matching a given header
and ensures 1:1 parity with keywords.

Ticket OISF#6374
To match on response SIP headers, those headers must be stored.

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

Ticket OISF#6374
This adds a sticky (multi) buffer to match the 'To' header field in
both requests and responses.

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

Ticket OISF#6374
This adds a sticky (multi) buffer to match the "User-Agent" header field in
both requests and responses.

Ticket OISF#6374
This adds a sticky (multi) buffer to match the "Content-Type" header field in
both requests and responses.

Ticket OISF#6374
This adds a sticky (multi) buffer to match the "Content-Length" header field in
both requests and responses.

Ticket OISF#6374
@catenacyber
Copy link
Contributor

CI is red because of #11622 for your info

@catenacyber
Copy link
Contributor

New sticky buffers have been rewritten in rust

Thanks :-)

Existing sticky buffers have been rewritten in rust, except for sip.method and sip.uri

Why these 2 exceptions ?

@glongo
Copy link
Contributor Author

glongo commented Aug 12, 2024

New sticky buffers have been rewritten in rust

Thanks :-)

Existing sticky buffers have been rewritten in rust, except for sip.method and sip.uri

Why these 2 exceptions ?

They require other C functions to be exposed in rust, such as DetectBufferTypeRegisterValidateCallback, DetectBufferTypeRegisterValidateCallback and DetectUrilenValidateContent.

flow_flags,
buffer,
buffer_len,
"From\0".as_ptr() as *const c_char,
Copy link
Contributor

Choose a reason for hiding this comment

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

Cannot we use a rust string or slice instead of going to a c_char pointer ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The compiler complains about that:
extern fn uses type `str`, which is not FFI-safe

Same for string.

@@ -292,7 +311,7 @@ mod tests {
assert_eq!(req.method, "REGISTER");
assert_eq!(req.path, "sip:sip.cybercity.dk");
assert_eq!(req.version, "SIP/2.0");
assert_eq!(req.headers["Content-Length"], "0");
assert_eq!(req.headers["Content-Length"].first().unwrap(), "0");
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we have a test with a header having multiple values ?

@catenacyber
Copy link
Contributor

Looks really nice :-)

Could you rebase it to get green CI ?

Copy link
Contributor

@catenacyber catenacyber left a comment

Choose a reason for hiding this comment

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

I love this.

Just some nits, and mostly a rebase to get green CI

@glongo
Copy link
Contributor Author

glongo commented Aug 28, 2024

Replaced with #11672

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.

2 participants