rust/sip: register parser for tcp v13#10058
Conversation
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.
This change involves creating the SIP PDU after the payload has been parsed correctly. Additionally, when a packet contains multiple SIP messages, a PDU is now created for each of them.
glongo
requested review from
a team,
jasonish,
jufajardini and
victorjulien
as code owners
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #10058 +/- ##
==========================================
- Coverage 82.45% 82.27% -0.18%
==========================================
Files 972 972
Lines 271461 271622 +161
==========================================
- Hits 223822 223474 -348
- Misses 47639 48148 +509
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Information: QA ran without warnings. Pipeline 17109 |
| match sip_parse_request(start) { | ||
| Ok((rem, request)) => { | ||
| let pdu_len = (start.len() - rem.len()) as i64; | ||
| let _f = Frame::new( |
There was a problem hiding this comment.
the frame should be created earlier, and not depend on the parser returning ok. It should be created as soon as we start parsing the data, and then be updated with the length when it's complete
There was a problem hiding this comment.
Sorry for the confusion, I was the one who asked this.
What should the length be updated to when the parser errors (incomplete or not) ?
Why does it have to be so ?
On the S-V test, a frame was created before returning incomplete, and suricata did not match on it...
So, why should we create a frame if we cannot match on it ? (or how can we match on it)
There was a problem hiding this comment.
As it is done for telnet? https://github.com/OISF/suricata/blob/master/rust/src/telnet/telnet.rs#L201-L204
There was a problem hiding this comment.
Indeed, the telnet code seems to do it well.
We need this so we can start matching immediately for large frames. Also if we have malformed data we can still do frame based content matching.
There was a problem hiding this comment.
We need this so we can start matching immediately for large frames. Also if we have malformed data we can still do frame based content matching.
This does not seem to work cf #10037 CI is green with SV OISF/suricata-verify#1538 where there is only one alert for https://github.com/OISF/suricata-verify/pull/1538/files#diff-efc7fbd99eb9389e40b2f9f16aa9eca8d58a80ea88324756b8a5d4b476cf69cfR18 alert sip any any -> any any (flow:to_server; frame:pdu; content:"REGISTER"; startswith; sid:2;) after the frame is created on packet 6 (and no alert for the frame created on packet 4, as can be checked adding a bsize keyword to the rule)
|
|
||
| match sip_parse_request(input) { | ||
| Ok((_, request)) => { | ||
| let _pdu = Frame::new( |
There was a problem hiding this comment.
frame should not depend on the parser returning ok
|
Replaced by #10098 |
5 participants
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.