rust/sip: register parser for tcp v8 #9880
Conversation
Accepts valid characters as defined in RFC3261.
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
glongo
requested review from
jasonish,
jufajardini,
victorjulien and
a team
as code owners
| @@ -34,6 +34,10 @@ also check all the new features that have been added but are not covered by | |||
| this guide. Those features are either not enabled by default or require | |||
| dedicated new configuration. | |||
|
|
|||
| Upgrading 7.0 to 8.0 | |||
| -------------------- | |||
| - SIP over TCP is detected | |||
There was a problem hiding this comment.
"detected" seems to miss represent the features. SIP/TCP is detected, tracked, logged and available to rules through the sip keywords?
| @@ -3800,7 +3800,10 @@ | |||
| "rfb": { | |||
| "$ref": "#/$defs/stats_applayer_error" | |||
| }, | |||
| "sip": { | |||
| "sip_udp": { | |||
There was a problem hiding this comment.
this is a breaking change that should be mentioned in the upgrade guide
| @@ -112,6 +109,15 @@ impl SIPState { | |||
| } | |||
| } | |||
|
|
|||
| fn append_request(&mut self, input: &[u8], request: Request) { | |||
There was a problem hiding this comment.
naming is a bit weird. I would expect something to be appended to an existing request, but we're actually setting up a TX. Please reword
There was a problem hiding this comment.
renamed append_request and append_response to build_tx_request and build_tx_response.
| pub unsafe extern "C" fn rs_sip_probing_parser_tcp_tc( | ||
| _flow: *const Flow, _direction: u8, input: *const u8, input_len: u32, _rdir: *mut u8, | ||
| ) -> AppProto { | ||
| if input_len >= 3 && !input.is_null() { |
There was a problem hiding this comment.
does it make sense to already send 3 bytes of data? It seems the parser will need quite a few more to be able to succeed
There was a problem hiding this comment.
agree, it doesn't make much sense. Removed it.
| } | ||
|
|
||
| // register TCP parser | ||
| parser.ipproto = core::IPPROTO_TCP; |
There was a problem hiding this comment.
I think we should use pattern based detection too, esp since this is a http like proto. We can use AppLayerProtoDetectPMRegisterPatternCSwPP to register patterns with a probing parser to validate it. See smb.rs.
There was a problem hiding this comment.
Giuseppe already answered me in a previous PR that this was tracked as a separate redmine ticket https://redmine.openinfosecfoundation.org/issues/5047
|
Replaced by #9893 |
jufajardini
changed the title
Make sure these boxes are signed before submitting your Pull Request -- thank you.
https://docs.suricata.io/en/latest/devguide/codebase/contributing/contribution-process.html
https://suricata.io/about/contribution-agreement/ (note: this is only required once)
Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/3351
Describe changes:
Provide values to any of the below to override the defaults.
To use a pull request use a branch name like
pr/NwhereNis thepull request number.
Alternatively,
SV_BRANCHmay also be a link to anOISF/suricata-verify pull-request.