[bug][sshd][security] ClientAliveInterval multiplied twice

[Dawid Majchrzak (Nokia)]

Hello,

My team discovered that starting from openssh v9.2 a ClientAliveInterval param from /etc/ssh/sshd_config
is interpreted as multiplied twice value.

----
Component: sshd
Version: 9.2p1 and higher
Hardware: ARM64
OS:Linux
----

sshd_config
----
ClientAliveInterval 5                                                    
ClientAliveCountMax 3
---

Debug logs
----
[2023-05-29 12:00:16] debug1: client_input_channel_req: channel 0 rtype keepalive at openssh.com reply 1
[2023-05-29 12:00:26] debug1: client_input_channel_req: channel 0 rtype keepalive at openssh.com reply 1
[2023-05-29 12:00:36] debug1: client_input_channel_req: channel 0 rtype keepalive at openssh.com reply 1
[2023-05-29 12:00:46] debug1: client_input_channel_req: channel 0 rtype keepalive at openssh.com reply 1
----

Looks like the problem was introduced with commit d478cdc7ad6edd4b1bcd1e86fb2f23194ff33d5a
With a new ptimeout api sshd server can double a client probing interval time.

commit d478cdc7ad6edd4b1bcd1e86fb2f23194ff33d5a
Author: djm at openbsd.org <djm at openbsd.org>
Date:   Fri Jan 6 02:38:23 2023 +0000

    upstream: replace manual poll/ppoll timeout math with ptimeout API
   
    feedback markus / ok markus dtucker
   
    OpenBSD-Commit-ID: c5ec4f2d52684cdb788cd9cbc1bcf89464014be2


@@ -251,19 +237,18 @@ wait_until_can_do_something(struct ssh *ssh,
        *conn_in_readyp = (*pfdp)[0].revents != 0;
        *conn_out_readyp = (*pfdp)[1].revents != 0;
 
+       /* ClientAliveInterval probing */
        if (client_alive_scheduled) {
                time_t now = monotime();
-
-               /*
-                * If the ppoll timed out, or returned for some other reason
-                * but we haven't heard from the client in time, send keepalive.
-                */
-               if (ret == 0 || (last_client_time != 0 && last_client_time +
-                   options.client_alive_interval <= now)) {
+               if (ret == 0 &&
+                   now > last_client_time + options.client_alive_interval) {
+                       /* ppoll timed out and we're due to probe */
                        client_alive_check(ssh);
                        last_client_time = now;
-               } else if (*conn_in_readyp)
+               } else if (ret != 0 && *conn_in_readyp) {
+                       /* Data from peer; reset probe timer. */
                        last_client_time = now;
+               }
        }
 }


This bug can be easily fixed by changing client_alive_check() condition.
Proposed soulution/Fix:

>From 6cf45ca16c51cc99b5210dea708e22e4dec8b2b0 Mon Sep 17 00:00:00 2001
From: Dawid Majchrzak <dawid.majchrzak at nokia.com>
Date: Wed, 31 May 2023 18:51:53 +0200
Subject: [PATCH] serverloop: Fix ClientAliveInterval probing condition

Commit d478cdc7ad6edd4b1bcd1e86fb2f23194ff33d5a introduced
ClientAliveInterval parameter regression that can double a probing
interval time.

Signed-off-by: Dawid Majchrzak <dawid.majchrzak at nokia.com>
---
 serverloop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/serverloop.c b/serverloop.c
index de5fa2e3..f160550e 100644
--- a/serverloop.c
+++ b/serverloop.c
@@ -253,7 +253,7 @@ wait_until_can_do_something(struct ssh *ssh,
 	/* ClientAliveInterval probing */
 	if (client_alive_scheduled) {
 		if (ret == 0 &&
-		    now > last_client_time + options.client_alive_interval) {
+		    now >= last_client_time + options.client_alive_interval) {
 			/* ppoll timed out and we're due to probe */
 			client_alive_check(ssh);
 			last_client_time = now;
-- 
2.25.1


BR, Dawid