Skip to content

rust/sip: register parser for tcp v9 #9893

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.

Sign up for GitHub

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

Closed
glongo wants to merge 8 commits into from
Closed

rust/sip: register parser for tcp v9 #9893

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5451fdb
rust/sip: rustfmt sip module
glongo Aug 14, 2023
961a62d
sip/parser: accept valid chars
glongo Aug 1, 2023
add724d
sip/parser: enforce valid chars for sip version
glongo Nov 25, 2023
a4f7db1
rust/sip: register parser for tcp
glongo Aug 1, 2023
c49e208
rust/sip: add direction to transaction
glongo Aug 1, 2023
96b34a7
suricata.yaml: define SIP_PORTS
glongo Aug 1, 2023
fbb6c7a
rust/sip: register pattern matching
glongo Nov 24, 2023
769de02
doc: add upgrade section for 8
glongo Nov 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 59 additions & 0 deletions rust/src/sip/sip.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Contributor

@catenacyber catenacyber Nov 27, 2023

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 ?

Copy link
Contributor Author

@glongo glongo Nov 27, 2023

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.

Copy link
Contributor

@catenacyber catenacyber Nov 27, 2023

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 ?

Copy link
Contributor Author

@glongo glongo Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's right.

Copy link
Contributor Author

@glongo glongo Nov 27, 2023

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");
};

Copy link
Contributor

@catenacyber catenacyber Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://redmine.openinfosecfoundation.org/issues/6583 ;-)

Copy link
Contributor Author

@glongo glongo Nov 27, 2023

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.

"INVITE\0",
"ACK\0",
"BYE\0",
"CANCEL\0",
"UPDATE\0",
"REFER\0",
"PRACK\0",
"SUBSCRIBE\0",
"NOTIFY\0",
"PUBLISH\0",
"MESSAGE\0",
"INFO\0",
"OPTIONS\0",
];
let mut r = 0;
unsafe {
for method in methods {
let depth = (method.len() - 1) as u16;
r |= AppLayerProtoDetectPMRegisterPatternCSwPP(
Copy link
Contributor

@catenacyber catenacyber Nov 27, 2023

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 ?

Copy link
Contributor Author

@glongo glongo Nov 27, 2023

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.)

Copy link
Contributor

@catenacyber catenacyber Nov 27, 2023

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

proto,
ALPROTO_SIP,
method.as_ptr() as *const std::os::raw::c_char,
depth,
0,
core::Direction::ToServer as u8,
rs_sip_probing_parser_tcp_ts,
0,
0,
);
}
r |= AppLayerProtoDetectPMRegisterPatternCSwPP(
proto,
ALPROTO_SIP,
b"SIP/2.0\0".as_ptr() as *const std::os::raw::c_char,
8,
0,
core::Direction::ToClient as u8,
rs_sip_probing_parser_tcp_tc,
0,
0,
);
}

if r == 0 {
return 0;
} else {
return -1;
}
}

export_tx_data_get!(rs_sip_get_tx_data, SIPTransaction);
export_state_data_get!(rs_sip_get_state_data, SIPState);

Expand Down Expand Up @@ -563,6 +616,9 @@ pub unsafe extern "C" fn rs_sip_register_parser() {
if AppLayerProtoDetectConfProtoDetectionEnabled(ip_proto_str.as_ptr(), parser.name) != 0 {
let alproto = AppLayerRegisterProtocolDetection(&parser, 1);
ALPROTO_SIP = alproto;
if register_pattern_probe(core::IPPROTO_UDP) < 0 {
return;
}
if AppLayerParserConfParserEnabled(ip_proto_str.as_ptr(), parser.name) != 0 {
let _ = AppLayerRegisterParser(&parser, alproto);
}
Expand All @@ -581,6 +637,9 @@ pub unsafe extern "C" fn rs_sip_register_parser() {
if AppLayerProtoDetectConfProtoDetectionEnabled(ip_proto_str.as_ptr(), parser.name) != 0 {
let alproto = AppLayerRegisterProtocolDetection(&parser, 1);
ALPROTO_SIP = alproto;
if register_pattern_probe(core::IPPROTO_TCP) < 0 {
return;
}
if AppLayerParserConfParserEnabled(ip_proto_str.as_ptr(), parser.name) != 0 {
let _ = AppLayerRegisterParser(&parser, alproto);
}
Expand Down