Bug 2252 - RekeyLimit breaks ClientAlive
Summary: RekeyLimit breaks ClientAlive
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: sshd (show other bugs)
Version: 6.6p1
Hardware: All All
: P5 normal
Assignee: Damien Miller
URL:
Keywords:
: 2572 (view as bug list)
Depends on:
Blocks: V_7_3
  Show dependency treegraph
 
Reported: 2014-07-06 20:42 AEST by Christian Wittenhorst
Modified: 2023-01-13 13:40 AEDT (History)
5 users (show)

See Also:


Attachments
fix rekey/clientalive interaction (1.16 KB, patch)
2016-02-26 14:04 AEDT, Damien Miller
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christian Wittenhorst 2014-07-06 20:42:35 AEST
If RekeyLimit is enabled, ClientAlive messages will not be sent, ever!

Problem seems to be:

serverloop.c: 
   wait_until_can_do_something(...)

max_time_milliseconds is set to the remaining time to a rekey. 

client_alive_scheduled never gets set, as max_time_milliseconds!=0:

if (compat20 &&
  max_time_milliseconds == 0 && options.client_alive_interval) {
    client_alive_scheduled = 1;
    max_time_milliseconds =
     (u_int64_t)options.client_alive_interval * 1000;
  }

The if clause might need changed to something like this:

if (compat20 &&
  max_time_milliseconds > ((u_int64_t)options.client_alive_interval * 1000)  && 
  options.client_alive_interval) {
 ...
Comment 1 Garrett Lee 2014-09-27 09:00:11 AEST
I am also experiencing this problem and investigating an appropriate fix.  Thank you for point out a possible fix, it saved me the time of having to dig around through the code. 

Looking at your proposed fix, I am wondering about the case where max_time_milliseconds actually is value 0.  I think if the rekey-time is 0 then with your proposed code change it will skip the tcp-keepalive.

Do you suppose the following code change would cover cases when rekey time is zero and cases where it is non-zero?

	if (compat20 &&
            options.client_alive_interval &&
	    ((max_time_milliseconds == 0) || (max_time_milliseconds > ((u_int64_t)options.client_alive_interval * 1000))) ) {
...
Comment 2 Garrett Lee 2014-09-30 03:40:33 AEST
(In reply to Garrett Lee from comment #1)
This bug is about SSH ClientAlive which I erroneously referred to as 'tcp-keepalive' in my previous comment.
Comment 3 Damien Miller 2015-05-01 14:26:45 AEST
It will take a little more than just adjusting the first test in wait_until_can_do_something(). Any timeout return from select() is interpreted as "time to send a keepalive!", so we would need to keep a proper deadline instead
Comment 4 Damien Miller 2015-08-11 22:59:18 AEST
Retarget pending bugs to openssh-7.1
Comment 5 Damien Miller 2016-02-26 14:04:21 AEDT
Created attachment 2793 [details]
fix rekey/clientalive interaction

This fixes the rekey/clientalive interaction, though a little inexactly. 

It will work correctly if clientalivetimeout is less than the timed rekeylimit, but will send extra client alive pings if the rekeylimit timeout is smaller. In practice, I don't think this is a huge problem since rekey timeouts are likely to be long and extra client alive packets are quite harmless.

Fixing it properly would require a more comprehensive timekeeping system to separately manage the rekey and client-alive deadlines.
Comment 6 Damien Miller 2016-03-04 14:47:48 AEDT
Fixed - this will be in openssh-7.3

commit cb34a5a98043bbd7bfb8c88fb0fe0da562de811d
Author: djm@openbsd.org <djm@openbsd.org>
Date:   Fri Mar 4 03:35:44 2016 +0000

    upstream commit
    
    fix ClientAliveInterval when a time-based RekeyLimit is
     set; previously keepalive packets were not being sent. bz#2252 report and
     analysis by Christian Wittenhorst and Garrett Lee feedback and ok dtucker@
    
    Upstream-ID: d48f9deadd35fdacdd5106b41bb07630ddd4aa81
Comment 7 Darren Tucker 2016-07-20 11:03:21 AEST
*** Bug 2572 has been marked as a duplicate of this bug. ***
Comment 8 Damien Miller 2016-08-02 10:42:31 AEST
Close all resolved bugs after 7.3p1 release