diff --git a/rust/src/ssh/logger.rs b/rust/src/ssh/logger.rs index 118462b3b3..36217e005a 100644 --- a/rust/src/ssh/logger.rs +++ b/rust/src/ssh/logger.rs @@ -42,7 +42,6 @@ fn log_ssh_direction(js: &mut JsonBuilder, name: &str, hdr: &SshHeader) -> Resul // must not be mislabelled let st_name = match hdr.state { SSHConnectionState::SshStateBanner => "banner", - SSHConnectionState::SshStateBannerWaitEol => "banner_wait_eol", SSHConnectionState::SshStateKex => "kex", // a direction that reached session can still fail on a // later record (the encryption bypass has not kicked in diff --git a/rust/src/ssh/parser.rs b/rust/src/ssh/parser.rs index 6e46933438..cd14d5b294 100644 --- a/rust/src/ssh/parser.rs +++ b/rust/src/ssh/parser.rs @@ -273,6 +273,30 @@ mod tests { } } + /// The protover is 1+ non-dash bytes: nom 8's is_not requires at + /// least one byte, so an empty-protover banner (`SSH--`) is + /// rejected by the grammar, not published with an empty protover. + /// The parse continuation latch keys off the banner_published + /// marker (the publish event) rather than the protover field for + /// this reason as well. + #[test] + fn test_ssh_parse_banner_empty_protover() { + for buf in [b"SSH--" as &[u8], b"SSH--1.99-OpenSSH_fake"] { + match ssh_parse_banner(buf) { + Ok(_) => panic!("empty protover banner should be rejected: {:?}", buf), + Err(Err::Error(_)) => { + // expected: the grammar rejects an empty protover + } + Err(Err::Failure(_)) => { + panic!("unexpected failure: {:?}", buf); + } + Err(Err::Incomplete(_)) => { + panic!("unexpected incomplete: {:?}", buf); + } + } + } + } + #[test] fn test_parse_line() { let buf = b"SSH-Single\n"; diff --git a/rust/src/ssh/ssh.rs b/rust/src/ssh/ssh.rs index 4ade1c45aa..d22df444f7 100644 --- a/rust/src/ssh/ssh.rs +++ b/rust/src/ssh/ssh.rs @@ -113,10 +113,9 @@ pub enum SSHError { #[suricata(alstate_strip_prefix = "SshState")] pub enum SSHConnectionState { SshStateBanner = 0, - SshStateBannerWaitEol = 1, - SshStateKex = 2, - SshStateSession = 3, - SshStateDone = 4, + SshStateKex = 1, + SshStateSession = 2, + SshStateDone = 3, } pub const SSH_MAX_BANNER_LEN: usize = 256; @@ -128,6 +127,10 @@ pub struct SshHeader { record_left_msg: parser::MessageCode, pub state: SSHConnectionState, + // explicit latch for the continuation path: set the moment the + // banner first parses; the latch keys off the publish event + // rather than a data field + banner_published: bool, pub protover: Vec, pub swver: Vec, pub error: Option, @@ -149,6 +152,7 @@ impl SshHeader { record_left_msg: parser::MessageCode::Undefined(0), state: SSHConnectionState::SshStateBanner, + banner_published: false, protover: Vec::new(), swver: Vec::new(), error: None, @@ -381,10 +385,13 @@ impl SSHState { } else { &mut self.transaction.srv_hdr }; - if hdr.state == SSHConnectionState::SshStateBannerWaitEol { + // A banner already parsed with the line still open: the + // published latch keeps the continuation from re-parsing the + // banner. + if hdr.banner_published { match parser::ssh_parse_line(input) { Ok((rem, _)) => { - // line complete: the banner data was parsed at + // line complete: the banner data was published at // entry, this only takes the direction to kex hdr.state = SSHConnectionState::SshStateKex; let mut r = self.parse_record(rem, resp, pstate, flow, stream_slice); @@ -420,6 +427,7 @@ impl SSHState { if !banner.swver.is_empty() { hdr.swver.extend(banner.swver); } + hdr.banner_published = true; hdr.state = SSHConnectionState::SshStateKex; } else { SCLogDebug!("SSH invalid banner"); @@ -465,7 +473,7 @@ impl SSHState { if !banner.swver.is_empty() { hdr.swver.extend(banner.swver); } - hdr.state = SSHConnectionState::SshStateBannerWaitEol; + hdr.banner_published = true; self.set_event(SSHEvent::LongBanner); return AppLayerResult::ok(); } else { @@ -643,7 +651,6 @@ unsafe extern "C" fn ssh_state_id_by_name( }; match s2 { "banner" => SSHConnectionState::SshStateBanner as i32, - "banner_wait_eol" => SSHConnectionState::SshStateBannerWaitEol as i32, "kex" => SSHConnectionState::SshStateKex as i32, "session" => SSHConnectionState::SshStateSession as i32, _ => -1, @@ -654,15 +661,9 @@ extern "C" fn ssh_state_name_by_id( id: std::os::raw::c_int, dir: u8, ) -> *const std::os::raw::c_char { // process-lifetime statics - static NAMES_TS: [&[u8]; 4] = [ - b"request_banner\0", - b"request_banner_wait_eol\0", - b"request_kex\0", - b"request_session\0", - ]; - static NAMES_TC: [&[u8]; 4] = [ + static NAMES_TS: [&[u8]; 3] = [b"request_banner\0", b"request_kex\0", b"request_session\0"]; + static NAMES_TC: [&[u8]; 3] = [ b"response_banner\0", - b"response_banner_wait_eol\0", b"response_kex\0", b"response_session\0", ]; @@ -675,7 +676,7 @@ extern "C" fn ssh_state_name_by_id( // so the hook listing and the firewall policy key walk see the // same surface as the name-to-id table match id { - 0..=3 => names[id as usize].as_ptr() as *const std::os::raw::c_char, + 0..=2 => names[id as usize].as_ptr() as *const std::os::raw::c_char, _ => std::ptr::null(), } } diff --git a/src/app-layer-ssh.c b/src/app-layer-ssh.c index 690707af39..a7130602fb 100644 --- a/src/app-layer-ssh.c +++ b/src/app-layer-ssh.c @@ -1576,15 +1576,14 @@ static int SSHParserTest25(void) PASS; } -/** \test State name table: new names, legacy aliases, the unhookable - * completion state and the id-to-name mapping. */ +/** \test State name table: the four phase names, the rejected legacy + * names (no backward-compat aliases), the unhookable completion + * state, unknown names and the id-to-name mapping. */ static int SSHParserTest26(void) { /* new names */ FAIL_IF(AppLayerParserGetStateIdByName( IPPROTO_TCP, ALPROTO_SSH, "request_banner", STREAM_TOSERVER) != SshStateBanner); - FAIL_IF(AppLayerParserGetStateIdByName(IPPROTO_TCP, ALPROTO_SSH, "request_banner_wait_eol", - STREAM_TOSERVER) != SshStateBannerWaitEol); FAIL_IF(AppLayerParserGetStateIdByName( IPPROTO_TCP, ALPROTO_SSH, "request_kex", STREAM_TOSERVER) != SshStateKex); FAIL_IF(AppLayerParserGetStateIdByName(IPPROTO_TCP, ALPROTO_SSH, "request_session", @@ -1611,9 +1610,12 @@ static int SSHParserTest26(void) FAIL_IF(AppLayerParserGetStateIdByName( IPPROTO_TCP, ALPROTO_SSH, "response_done", STREAM_TOCLIENT) != -1); - /* unknown name, and wrong direction prefix */ + /* unknown names (including banner_wait_eol) and wrong direction + * prefix */ FAIL_IF(AppLayerParserGetStateIdByName( IPPROTO_TCP, ALPROTO_SSH, "request_nosuchstate", STREAM_TOSERVER) != -1); + FAIL_IF(AppLayerParserGetStateIdByName( + IPPROTO_TCP, ALPROTO_SSH, "request_banner_wait_eol", STREAM_TOSERVER) != -1); FAIL_IF(AppLayerParserGetStateIdByName( IPPROTO_TCP, ALPROTO_SSH, "response_banner", STREAM_TOSERVER) != -1); @@ -1846,10 +1848,12 @@ static int SSHParserTest29(void) PASS; } -/** \test A direction parked in banner_wait_eol that then fails a - * \test record freezes it at kex; a >=256B banner without EOL that - * \test does not parse as a banner freezes the direction at - * \test banner at once. +/** \test A long banner (>=256B without EOL) keeps the direction in + * \test the banner phase with the line consumed greedily; the line + * \test completion moves it to kex, and a record that then fails + * \test freezes the direction at kex. A >=256B line without EOL that + * \test does not parse as a banner freezes the direction at banner + * \test at once. */ static int SSHParserTest30(void) { @@ -1858,7 +1862,6 @@ static int SSHParserTest30(void) AppLayerParserThreadCtx *alp_tctx = NULL; uint8_t longbanner[300]; uint8_t junk[300]; - uint8_t contbuf[8 + 2]; uint8_t badrecord[] = { 0x00, 0x00, 0x00, 0x00, 0x08, 0x21, 0x00, 0x00 }; void *tx; @@ -1873,9 +1876,10 @@ static int SSHParserTest30(void) alp_tctx = AppLayerParserThreadCtxAlloc(); FAIL_IF_NULL(alp_tctx); - // phase 1: 300B banner without EOL -> banner_wait_eol; the line - // completes and the following record is invalid -> the direction - // freezes at kex + // phase 1: 300B banner without EOL -> the direction stays in the + // banner phase (the line is still open); the line completion + // moves it to kex; the following record is invalid -> the + // direction freezes at kex memset(longbanner, 'A', sizeof(longbanner)); memcpy(longbanner, "SSH-2.0-", 8); int r = AppLayerParserParse( @@ -1883,12 +1887,16 @@ static int SSHParserTest30(void) FAIL_IF(r != 0); FAIL_IF_NULL(f.alstate); tx = SCSshStateGetTx(f.alstate, 0); - FAIL_IF(SCSshTxGetAlStateProgress(tx, STREAM_TOSERVER) != SshStateBannerWaitEol); + FAIL_IF(SCSshTxGetAlStateProgress(tx, STREAM_TOSERVER) != SshStateBanner); - memcpy(contbuf, "\r\n", 2); - memcpy(contbuf + 2, badrecord, sizeof(badrecord)); + // line completion -> kex r = AppLayerParserParse( - NULL, alp_tctx, &f, ALPROTO_SSH, STREAM_TOSERVER, contbuf, sizeof(contbuf)); + NULL, alp_tctx, &f, ALPROTO_SSH, STREAM_TOSERVER, (const uint8_t *)"\r\n", 2); + FAIL_IF(r != 0); + FAIL_IF(SCSshTxGetAlStateProgress(tx, STREAM_TOSERVER) != SshStateKex); + + r = AppLayerParserParse( + NULL, alp_tctx, &f, ALPROTO_SSH, STREAM_TOSERVER, badrecord, sizeof(badrecord)); FAIL_IF(r != -1); FAIL_IF(SCSshTxGetAlStateProgress(tx, STREAM_TOSERVER) != SshStateKex); FAIL_IF(SCSshTxGetAlStateProgress(tx, STREAM_TOCLIENT) != SshStateBanner); @@ -2356,7 +2364,8 @@ void SSHParserRegisterTests(void) SSHParserTest28); UtRegisterTest( "SSHParserTest29 - stash and fragment deliveries mark the tx updated", SSHParserTest29); - UtRegisterTest("SSHParserTest30 - wait_eol failure freezes at kex", SSHParserTest30); + UtRegisterTest( + "SSHParserTest30 - long-banner continuation failure freezes at kex", SSHParserTest30); UtRegisterTest("SSHParserTest31 - failed flow admits the eve log condition", SSHParserTest31); UtRegisterTest("SSHParserTest32 - banner continuation marks the tx updated at parse entry", SSHParserTest32);