detect/sip: add sticky buffers to match headers v3 by glongo · Pull Request #11004 · OISF/suricata

glongo · 2024-05-01T06:58:56Z

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

I have read the contributing guide lines at
https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html
I have signed the Open Information Security Foundation contribution agreement at
https://suricata.io/about/contribution-agreement/ (note: this is only required once)
I have updated the user guide (in doc/userguide/) to reflect the changes made (if applicable)

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

Describe changes:

Add new keywords in doc/userguide/rules/sip-keywords.rst
Rebase

SV_BRANCH=OISF/suricata-verify#1787

A stub file has been added to implement the sticky buffers for SIP headers. Ticket OISF#6374

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

Ticket OISF#6374

catenacyber

Thanks for the work :-)

Should these fields get logged in addition to have a keyword ?
Would you do a rebase for green CI ?

CI : 🟠 probably just need a rebase
Code : Checking now
Commits segmentation : note sure, feels weird to have a specific commit for registering for instance
Commit messages : ok for me
Git ID set : looks fine for me
CLA : you already contributed
Doc update : will leave some more comments in the code
Redmine ticket : ok
Rustfmt : looks good
Tests : looks good, left a few questions there
Dependencies added: none

catenacyber · 2024-05-14T13:43:22Z

doc/userguide/rules/sip-keywords.rst

+
+  sip.from; content:<from>
+
+Where <from> is the value of the From header.


@jufajardini we should precise this is a sticky buffer, right ?

catenacyber · 2024-05-14T13:43:58Z

doc/userguide/upgrade.rst

+    - sip.content_type
+    - sip.content_length
+
+  Note: Headers expressed in compact form will still be matched.


I think the Note line is too much details for this file

catenacyber · 2024-05-14T13:44:20Z

rust/src/sip/detect.rs

 use std::ptr;

+fn header_compact_name(h: &str) -> Option<String> {
+    let compact = match h {


Would you add a comment to the RFC specifying these ?

Also, would not it be simpler the other way, to associate i to Call-Id?

catenacyber · 2024-05-14T13:46:15Z

doc/userguide/rules/sip-keywords.rst

+sip.from
+--------
+
+This keyword matches on the From field that can be present in SIP headers.


What happens if there are multiple From headers ?

Should it be a multi buffer ?

Yes, it should.

catenacyber · 2024-05-14T13:46:47Z

rust/src/sip/detect.rs

+        "Via" => "v",
+        _ => return None,
+    };
+    Some(compact.to_string())


Why do we need the allocation ?

catenacyber · 2024-05-14T13:47:22Z

rust/src/sip/detect.rs

+        if let Some(headers) = headers {
+            let header_value = headers
+                .get(s)
+                .or_else(|| s2.as_ref().and_then(|s2| headers.get(s2)));


That means if I have the header in long and short form, I only get the long form, right ?

Yes, right.

catenacyber · 2024-05-14T13:48:25Z

rust/src/sip/detect.rs

+                .or_else(|| s2.as_ref().and_then(|s2| headers.get(s2)));
+
+            if let Some(value) = header_value {
+                if !value.is_empty() {


Why this test ? cf #11070 and https://redmine.openinfosecfoundation.org/issues/2224

catenacyber · 2024-05-14T13:49:09Z

src/detect-sip-content-length.h

+ * 02110-1301, USA.
+ */
+
+#ifndef __DETECT_SIP_CONTENT_LENGTH_H__


Old guard style

catenacyber · 2024-05-14T13:50:47Z

src/detect-sip-ua.c

+#define HEADER_NAME      "User-Agent"
+#define KEYWORD_ID       DETECT_AL_SIP_HEADER_UA
+#define KEYWORD_TOSERVER 1
+#define KEYWORD_TOCLIENT 1


User Agent is to client as well ?

Or should we get rid of KEYWORD_TOSERVER as they are always true

Yes, UA is to client as well.

catenacyber · 2024-05-14T13:51:31Z

src/detect-sip-headers-stub.h

+    DetectAppLayerMpmRegister(BUFFER_NAME, SIG_FLAG_TOCLIENT, 2, PrefilterGenericMpmRegister,
+            GetResponseData, ALPROTO_SIP, 1);
+#endif
+#ifdef KEYWORD_TOSERVER


You can group the 2 lines instead of having 2 ifdef KEYWORD_TOSERVER groups

catenacyber · 2024-05-14T13:52:19Z

src/detect-sip-headers-stub.h

+    return 0;
+}
+
+static void DetectSipHeadersRegisterStub(void)


I still think it would be easier and more expressive to have a sip.headers buffer or frame

catenacyber · 2024-05-14T13:52:45Z

src/detect-sip-headers-stub.h

+#include "detect-engine-mpm.h"
+#include "detect-engine-prefilter.h"
+
+#include "util-debug.h"


Why this include ?

glongo · 2024-06-20T04:52:35Z

Replaced with #11330

glongo added 11 commits May 1, 2024 08:25

detect/sip: add stub file for headers keywords

d959bc5

A stub file has been added to implement the sticky buffers for SIP headers. Ticket OISF#6374

rust/sip: store response headers

cc50584

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

rust/sip: match on headers map

6474430

Ticket OISF#6374

detect/sip: add sip.from sticky buffer

0b4f35f

Ticket OISF#6374

detect/sip: add sip.to sticky buffer

7340116

Ticket OISF#6374

detect/sip: add sip.via sticky buffer

6720c69

Ticket OISF#6374

detect/sip: add sip.user_agent sticky buffer

4d414cf

Ticket OISF#6374

detect/sip: add sip.content_type sticky buffer

f9553bc

Ticket OISF#6374

detect/sip: add sip.content_length sticky buffer

e4cc816

Ticket OISF#6374

detect/sip: register headers sticky buffers

b2b5c8a

Ticket OISF#6374

doc: add new sip keywords

a0a400c

glongo requested review from jasonish, jufajardini and victorjulien as code owners May 1, 2024 06:58

This was referenced May 1, 2024

detect/sip: add sticky buffers to match headers v2 #10907

Closed

sip: add tests for headers sticky buffers v2 OISF/suricata-verify#1787

Closed

catenacyber requested changes May 14, 2024

View reviewed changes

catenacyber reviewed May 14, 2024

View reviewed changes

rust/src/sip/detect.rs

"Via" => "v",

_ => return None,

};

Some(compact.to_string())

Copy link

Contributor

catenacyber May 14, 2024

Choose a reason for hiding this comment

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

Why do we need the allocation ?

catenacyber reviewed May 14, 2024

View reviewed changes

src/detect-sip-content-length.h

* 02110-1301, USA.

*/

#ifndef __DETECT_SIP_CONTENT_LENGTH_H__

Copy link

Contributor

catenacyber May 14, 2024

Choose a reason for hiding this comment

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

Old guard style

catenacyber reviewed May 14, 2024

View reviewed changes

inashivb mentioned this pull request May 21, 2024

Dataset string memcap 3910/v5 #11105

Closed

glongo closed this Jun 20, 2024


		sip.from; content:<from>

		Where <from> is the value of the From header.

Comments

Conversation

glongo commented May 1, 2024

Uh oh!

catenacyber 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!

glongo commented Jun 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants