rdr pass for proto tcp sometimes creates states with expire time zero and so breaking connections

[Kristof Provost]

On 15 Oct 2018, at 15:26, Andreas Longwitz wrote:
> On two of my FreeBSD 10 (r338093) firewall servers some incoming ssh
> connections stopped to work because pf started to create states with
> expire time zero (instead of 86400 sec) for translation statements 
> like
>
> rdr pass on em0 proto tcp from any to myip port 8022 --> 10.0.0.254
>
> Therefore a command given on a remote server like
>
>            ssh -p 8022 myip sleep 15
>
> did not return, because the created state for the connection was 
> purged
> by pf (interval 10 sec) before 15 seconds. If I replace the rdr pass
> rule with a rdr rule and a separate filter pass rule then the created
> state always has expire time 24h and everything is ok.
>
> I have tried to analyze the bug in the rdr pass rule. Immediately 
> after
> starting the above ssh command on the remote sever I see with pfctl 
> -vss
> the sreated state on my firewall machine:
>
> all tcp 10.0.0.254:8022 (myip:8022) <- remoteip:59233
> ESTABLISHED:ESTABLISHED
>    [1443872812 + 66608] wscale 6  [1365307391 + 66560] wscale 3
>    age 00:00:00, expires in 00:00:00, 13:12 pkts, 2955:3306 bytes
>
> and a DTrace script running at the same time gives
>
>   3  19323        pfsync_state_export:entry
>             creation=4491391. expire=4491391, state_timeout=2
>   3  16459           pf_state_expires:entry         
> state_timeout=86400,
>             start_timeout=6000, end_timeout=12000 states=882
>   3  17624          counter_u64_fetch:entry
>   3  17625         counter_u64_fetch:return
>             returncode (states_cur)=4294967248 = 0xffffffd0
>   3  16460          pf_state_expires:return
>             returncode=4491391
>   3  19324       pfsync_state_export:return
>             creation=0. expire=0, syncstate_timeout=2
>   0  12730                   pfioctl:return
>             returncode=0, time_uptime=4491391
>
> So pf_state_expires() returns the value time_update and therefore
> pfsync_state_export() gives expire zero. Reason for this is the very 
> big
> (means negative) value returned by the function counter_u64_fetch() 
> for
> states_cur. This variable is of type uint64_t and only incremented and
> decremented by the macros STATE_INC_COUNTERS and STATE_DEC_COUNTERS.
>
I wonder if there’s an integer overflow in the of_state_expires() 
calculation.
The OpenBSD people have a cast to u_int64_t in their version:

	timeout = (u_int64_t)timeout * (end - states) / (end - start);

Perhaps this would fix your problem? (Untested, not even compiled)

	        if (end && states > start && start < end) {
	                if (states < end) {
						timeout = (uint64_t)timeout * (end - states) / (end - start);
	                        return (state->expire + timeout;
					}
	                else
	                        return (time_uptime);
	        }
	        return (state->expire + timeout);
	}

Best regards,
Kristof