ssh: remove the banner_wait_eol state

banner_wait_eol only recorded that a banner line of 256 bytes or
more had arrived without EOL; it was not an active phase. The
direction kept reparsing the open line in place, so a rule or
firewall policy pointing at the state acted on a parked, not an
active, direction.

Move the condition to the event channel it already had (the
one-shot ssh.long_banner event) and key the continuation off an
explicit banner_published latch (set at the banner publish
sites): a direction with an open line stays in the banner state,
the line is consumed greedily until the EOL arrives, and the line
completion takes the normal banner-to-kex transition.

The state scale becomes dense: banner(0) kex(1) session(2),
completion done(3). request_banner_wait_eol /
response_banner_wait_eol no longer load: aliasing them to banner
would silently turn a long-banner rule into an any-banner rule.
Firewall rules that acted at the removed state must move to the
ssh.long_banner event or the banner state; and since the progress
gap between banner and kex is gone, the default-policy sweep no
longer drops a clean flow at the server banner.

Ticket: #9046.

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

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--<sw>`) 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";

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<u8>,
     pub swver: Vec<u8>,
     pub error: Option<SSHError>,
@@ -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(),
     }
 }

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