-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
rust/sip: register parser for tcp v5 #9321
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.
editted PR description to use new SV PR picking style, as I wasn't sure the old style still works. Re-triggered build tests, too. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #9321 +/- ##
==========================================
- Coverage 82.41% 82.41% -0.01%
==========================================
Files 968 968
Lines 274055 274198 +143
==========================================
+ Hits 225874 225971 +97
- Misses 48181 48227 +46
Flags with carried forward coverage won't be shown. Click here to find out more. |
Yeah, that style doesn't work anymore. |
if let Ok((_, resp_line)) = sip_take_line(input) { | ||
tx.response_line = resp_line; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this isn't new, just a refactor. But should we be creating an empty transaction of sip_take_line
fails?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If sip_parse_request
is succeeded, sip_take_line
shouldn't fail because the whole request is parsed before.
@@ -50,7 +50,7 @@ vars: | |||
GENEVE_PORTS: 6081 | |||
VXLAN_PORTS: 4789 | |||
TEREDO_PORTS: 3544 | |||
|
|||
SIP_PORTS: "[5060, 5061]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty line is lost here, can you please add back.
pub unsafe extern "C" fn rs_sip_probing_parser_tcp_ts( | ||
_flow: *const Flow, | ||
_direction: u8, | ||
input: *const u8, | ||
input_len: u32, | ||
_rdir: *mut u8, | ||
) -> AppProto { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't our formatting for new rust code. Would you be willing to rustfmt
the whole SIP module, commit, then apply your changes letting rustfmt do its thing? I think the module is small enough to do a one-shot rustfmt on it, then apply your changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pull request @glongo. Could you please address a few minor things:
- rebase
- formatting
The S-V test appears to be a crafted pcap right? Once rebased, I can test on some live TCP SIP traffic.
Replaced by #9387 |
Make sure these boxes are signed before submitting your Pull Request -- thank you.
Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/3351
Previous PR: #8897
Describe changes:
suricata-verify-pr: 1341
SV_BRANCH=OISF/suricata-verify#1341