rust/sip: register parser for tcp v10 by glongo · Pull Request #9909 · OISF/suricata

glongo · 2023-11-28T13:43:07Z

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/codebase/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/3351

Describe changes:

Documentation updated
SIP version parser fixed
Probing parser functions removed

Provide values to any of the below to override the defaults.

To use a pull request use a branch name like pr/N where N is the
pull request number.

Alternatively, SV_BRANCH may also be a link to an
OISF/suricata-verify pull-request.

SV_REPO=
SV_BRANCH=pr/1499
SU_REPO=
SU_BRANCH=
LIBHTP_REPO=
LIBHTP_BRANCH=

Accepts valid characters as defined in RFC3261.

The `is_version_char` function incorrectly allowed characters that are not part of the valid SIP version "SIP/2.0". For instance, 'HTTP/1.1' was mistakenly accepted as a valid SIP version, although it's not. This commit fixes the issue by updating the condition to strictly check for the correct version string.

This patch lets the parser to work over tcp protocol, taking care of handling data before calling the request/response parsers. Ticket OISF#3351.

This patch permits to set a direction when a new transaction is created in order to avoid 'signature shadowing' as reported by Eric Leblond in commit 5aaf507

This permits to detect the SIP protocol using pattern matching instead of probing parser. Since it is no longer used, the respective probing functions have been removed.

codecov · 2023-11-28T14:00:57Z

Codecov Report

Merging #9909 (3278556) into master (9c3ab36) will decrease coverage by 0.02%.
The diff coverage is 90.49%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9909      +/-   ##
==========================================
- Coverage   82.35%   82.33%   -0.02%     
==========================================
  Files         972      972              
  Lines      273060   273220     +160     
==========================================
+ Hits       224870   224949      +79     
- Misses      48190    48271      +81

Flag	Coverage Δ
fuzzcorpus	`64.13% <69.91%> (-0.02%)`	⬇️
suricata-verify	`61.06% <90.70%> (-0.04%)`	⬇️
unittests	`62.91% <33.05%> (-0.01%)`	⬇️

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

rust/src/sip/sip.rs

catenacyber · 2023-11-29T09:21:01Z

rust/src/sip/sip.rs


 #[no_mangle]
 pub unsafe extern "C" fn rs_sip_register_parser() {
-    let default_port = CString::new("[5060,5061]").unwrap();


What do you think about 55216df ?

catenacyber · 2023-11-29T09:22:52Z

rust/src/sip/parser.rs

-fn parse_version(i: &[u8]) -> IResult<&[u8], &str> {
-    map_res(take_while1(is_version_char), std::str::from_utf8)(i)
+fn parse_version(i: &[u8]) -> IResult<&[u8], String> {
+    let (i, prefix) = map_res(tag("SIP/"), std::str::from_utf8)(i)?;


Do we really need the utf8 here ?

jufajardini

Documentation changes look good to me :)

suricata-qa · 2023-11-30T13:55:04Z

ERROR:

ERROR: QA failed on ASAN_TLPR1_suri.

Pipeline 16804

catenacyber · 2023-11-30T14:24:54Z

ERROR:

ERROR: QA failed on ASAN_TLPR1_suri.

Pipeline 16804

QA failure is triggered assertion
app-layer-detect-proto.c:1434: AppProto AppLayerProtoDetectGetProto(AppLayerProtoDetectThreadCtx *, Flow *, const uint8_t *, uint32_t, uint8_t, uint8_t, _Bool *): Assertion !((pm_matches > 1))' failed.`

Can you investigate if one pattern registered for protocol detection is a duplicate ? (or a prefix maybe )

glongo · 2023-11-30T16:10:24Z

ERROR:
ERROR: QA failed on ASAN_TLPR1_suri.
Pipeline 16804

QA failure is triggered assertion app-layer-detect-proto.c:1434: AppProto AppLayerProtoDetectGetProto(AppLayerProtoDetectThreadCtx *, Flow *, const uint8_t *, uint32_t, uint8_t, uint8_t, _Bool *): Assertion !((pm_matches > 1))' failed.`

Can you investigate if one pattern registered for protocol detection is a duplicate ? (or a prefix maybe )

Where is this error coming from? Is it reproducible?

catenacyber · 2023-11-30T16:27:11Z

Yes but not sure we can share the pcap

catenacyber · 2023-11-30T16:29:38Z

OPTIONS is also a HTTP method...

glongo · 2023-11-30T16:49:01Z

OPTIONS is also a HTTP method...

This is exactly what I was fearing. Initially, I thought that using the same method between SIP and HTTP could lead to conflicts.
Please share the pcap file if possible, so I can confirm this.
Otherwise, feel free to debug it yourself if you are unable to share it (and if you want, obviously).

catenacyber · 2023-12-01T21:27:48Z

Please share the pcap file if possible, so I can confirm this.
Otherwise, feel free to debug it yourself if you are unable to share it (and if you want, obviously).

TL;DR let's remove the OPTIONS method for SIP recognition

If you want a pcap, you can try to create one by sending a OPTIONS request to a HTTP server (like curl -X OPTIONS 127.0.0.1:8080/

glongo · 2023-12-05T14:45:58Z

Replaced by #9967

victorjulien · 2023-12-05T14:54:17Z

So OPTIONS could be a nice candidate for a PM+PP setup both in HTTP and SIP, where the PP would do the additional validation of the protocol?

catenacyber · 2023-12-05T15:05:20Z

So OPTIONS could be a nice candidate for a PM+PP setup both in HTTP and SIP, where the PP would do the additional validation of the protocol?

Indeed.

The current behavior is to recognize it as HTTP.
So, I would have it as a separate issue.

glongo added 8 commits November 28, 2023 09:35

rust/sip: rustfmt sip module

bb9cffd

sip/parser: accept valid chars

f6e7521

Accepts valid characters as defined in RFC3261.

rust/sip: register parser for tcp

7837884

This patch lets the parser to work over tcp protocol, taking care of handling data before calling the request/response parsers. Ticket OISF#3351.

rust/sip: add direction to transaction

8f92f8e

This patch permits to set a direction when a new transaction is created in order to avoid 'signature shadowing' as reported by Eric Leblond in commit 5aaf507

suricata.yaml: define SIP_PORTS

0ddd17c

rust/sip: register pattern matching

80fe03a

This permits to detect the SIP protocol using pattern matching instead of probing parser. Since it is no longer used, the respective probing functions have been removed.

doc: add upgrade section for 8

3278556

glongo requested review from a team, jasonish, jufajardini and victorjulien as code owners November 28, 2023 13:43

glongo changed the title ~~rust/sip: register parser for tcp v9~~ rust/sip: register parser for tcp v10 Nov 28, 2023

This was referenced Nov 28, 2023

rust/sip: register parser for tcp v9 #9893

Closed

sip: add tests for sip over tcp v4 OISF/suricata-verify#1499

Closed

catenacyber reviewed Nov 29, 2023

View reviewed changes

rust/src/sip/sip.rs Show resolved Hide resolved

catenacyber reviewed Nov 29, 2023

View reviewed changes

jufajardini reviewed Nov 29, 2023

View reviewed changes

ct0br0 added the waiting for qa label Nov 30, 2023

suricata-qa removed the waiting for qa label Nov 30, 2023

glongo closed this Dec 5, 2023

glongo deleted the dev-sip-tcp-v10 branch February 28, 2024 14:11

Comments

Conversation

glongo commented Nov 28, 2023

Provide values to any of the below to override the defaults.

Uh oh!

codecov bot commented Nov 28, 2023

Codecov Report

Uh oh!

Uh oh!

catenacyber Nov 29, 2023

Choose a reason for hiding this comment

Uh oh!

catenacyber Nov 29, 2023

Choose a reason for hiding this comment

Uh oh!

jufajardini left a comment

Choose a reason for hiding this comment

Uh oh!

suricata-qa commented Nov 30, 2023

Uh oh!

catenacyber commented Nov 30, 2023

Uh oh!

glongo commented Nov 30, 2023

Uh oh!

catenacyber commented Nov 30, 2023

Uh oh!

catenacyber commented Nov 30, 2023

Uh oh!

glongo commented Nov 30, 2023

Uh oh!

catenacyber commented Dec 1, 2023

Uh oh!

glongo commented Dec 5, 2023

Uh oh!

victorjulien commented Dec 5, 2023

Uh oh!

catenacyber commented Dec 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants