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.
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 log sip messages over tcp.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #9387 +/- ##
==========================================
- Coverage 82.17% 78.35% -3.83%
==========================================
Files 968 968
Lines 274198 274139 -59
==========================================
- Hits 225331 214790 -10541
- Misses 48867 59349 +10482
Flags with carried forward coverage won't be shown. Click here to find out more. |
| @@ -388,4 +569,22 @@ pub unsafe extern "C" fn rs_sip_register_parser() { | |||
| } else { | |||
| SCLogDebug!("Protocol detecter and parser disabled for SIP/UDP."); | |||
There was a problem hiding this comment.
The only thing I'd change now I think is this line and the one further below.
Protocol detection and parsing disabled for UDP SIP
and likewise for the TCP. Or just bring correct spelling to both. Easy to do with in a staging commit though.
jasonish
left a comment
jasonish
left a comment
There was a problem hiding this comment.
I think this is OK. I'm not sure if its suitable for 7.0.x as it is much like adding a new parser, but it is a reasonable enhancement to 7.0 as well. Will need to be discussed.
|
Why is the CI red ? |
Maybe you need to rebase the S-V PR ?.. |
| if AppLayerParserStateIssetFlag(pstate, APP_LAYER_PARSER_EOF_TS) > 0 { | ||
| return AppLayerResult::ok(); | ||
| } else { | ||
| return AppLayerResult::err(); |
There was a problem hiding this comment.
How can we end up here ?
|
Commit |
|
I think commits should be squashed into |
| GENEVE_PORTS: 6081 | ||
| VXLAN_PORTS: 4789 | ||
| TEREDO_PORTS: 3544 | ||
| SIP_PORTS: "[5060, 5061]" |
There was a problem hiding this comment.
This is meant to be used in signatures, right ?
catenacyber
left a comment
catenacyber
left a comment
There was a problem hiding this comment.
Thanks again, looking good.
Here is my quick feedback :
CI : can you make it green ? (rebasing the S-V PR I guess)
Code : looks good
Commits segmentation : I think some commits can be squashed together
Commit messages :Could you put the redmine ticket reference in the main commit ?
Git ID set : looks fine for me
CLA : I do not have access, but you have already contributed
Doc update : Where should we document this change @jufajardini ? upgrade section ?
Redmine ticket : ok
Rustfmt : thanks 🙇
Tests : Looks noce, will comment on it
Dependencies added: none
|
Replaced by #9483 right ? |
4 participants
Make sure these boxes are signed before submitting your Pull Request -- thank you.
Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/3351
Describe changes:
SV_BRANCH=OISF/suricata-verify#1341