-
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 v9 #9893
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 SIP protocol using pattern matching as well.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #9893 +/- ##
==========================================
- Coverage 82.45% 82.35% -0.11%
==========================================
Files 972 972
Lines 273057 273260 +203
==========================================
- Hits 225156 225047 -109
- Misses 47901 48213 +312
Flags with carried forward coverage won't be shown. Click here to find out more. |
Why so ? |
@@ -78,7 +78,7 @@ fn is_request_uri_char(b: u8) -> bool { | |||
|
|||
#[inline] | |||
fn is_version_char(b: u8) -> bool { | |||
is_alphanumeric(b) || b"./".contains(&b) | |||
b"SIP/2.0".contains(&b) |
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 think we rather want the caller of is_version_char
begin by tag("SIP/")
, what do you think ?
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 will also match 0022.PSI
for instance
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 agree with you, it's much better tagging SIP/
string first and then taking the chars until \r\n
is found.
@@ -518,6 +518,59 @@ pub unsafe extern "C" fn rs_sip_parse_response_tcp( | |||
state.parse_response_tcp(flow, stream_slice) | |||
} | |||
|
|||
fn register_pattern_probe(proto: u8) -> i8 { | |||
let methods: Vec<&str> = vec![ | |||
"REGISTER\0", |
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 do not think you need the final 0 in rust, do you ?
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 you look at register_pattern_probe
in rust/src/smb/smb.rs
you'll see that the patterns contains \0
.
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.
Looks that SMB can be optimized as well then. We just use a buffer and its length, and you specify the - 1
for the length to get rid of this 0, right ?
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.
Yes, it's right.
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.
It seems that the same logic is applied in bittorrent_dht.rs
const BITTORRENT_DHT_PAYLOAD_PREFIX: &[u8] = b"d1:ad2:id20:\0";
if AppLayerProtoDetectPMRegisterPatternCS(
IPPROTO_UDP,
ALPROTO_BITTORRENT_DHT,
BITTORRENT_DHT_PAYLOAD_PREFIX.as_ptr() as *const c_char,
BITTORRENT_DHT_PAYLOAD_PREFIX.len() as u16 - 1,
0,
crate::core::Direction::ToServer.into(),
) < 0
{
SCLogDebug!("Failed to register protocol detection pattern for direction TOSERVER");
};
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.
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've made some tests, and it looks like the null character is mandatory; otherwise, the pattern matching won't work.
Patterns (such as BITTORRENT_DHT_PAYLOAD_PREFIX
) are used in C functions, like DetectContentDataParse
, that takes a string as input.
I believe it's not worth making changes around the code just to remove the null byte from the patterns.
unsafe { | ||
for method in methods { | ||
let depth = (method.len() - 1) as u16; | ||
r |= AppLayerProtoDetectPMRegisterPatternCSwPP( |
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.
do we really need probing after this pattern ?
Plus this commit should reference https://redmine.openinfosecfoundation.org/issues/5047 right ?
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.
Well, I think that probing it's not needed at this point and yes, the commit should point to the redmine ticket. (missed it.)
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.
So, I think it is rather AppLayerProtoDetectPMRegisterPatternCS
to be used
Also, SIP protocol is detected using pattern matching and not only | ||
probing parser. | ||
- ``SIP_PORTS`` variable has been introduced in suricata.yaml | ||
- Application layer's ``sip`` counter has been split into ``sip_tcp`` and ``sip_udp``. |
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 should mention that this is for the stats
events.
cc @jufajardini
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.
Agreed!
IIRC |
Replaced by #9909 |
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:
append_request
andappend_response
tobuild_tx_request
andbuild_tx_response
input_len >= 3
conditionSIP/2.0
Provide values to any of the below to override the defaults.
To use a pull request use a branch name like
pr/N
whereN
is thepull request number.
Alternatively,
SV_BRANCH
may also be a link to anOISF/suricata-verify pull-request.