On Tue, Apr 14, 2015 at 19:40 +0300, Lauri Tirkkonen wrote:
> >Synopsis: tcp keep-alives sent without timestamps
> >Category: kernel
> >Environment:
> System : OpenBSD 5.7
> Details : OpenBSD 5.7-current (GENERIC) #860: Mon Apr 13 20:58:42
> MDT 2015
>
> dera...@amd64.openbsd.org:/usr/src/sys/arch/amd64/compile/GENERIC
>
> Architecture: OpenBSD.amd64
> Machine : amd64
> >Description:
> TCP keep-alive messages sent by OpenBSD do not include timestamp
> options. When using pf tcp normalisation, this breaks eg. ssh(1)
> from OpenBSD to illumos after transmission of a keep-alive.
>
> On (at least) illumos, receiving an empty ACK like this in a
> connection which was initiated using timestamps in the SYN, the
> following data packets sent by the illumos host will not include
> timestamps either (I'm discussing on their mailing lists [0]
> whether that makes sense). This is a problem if those data
> packets are scrubbed with reassemble tcp when received by
> OpenBSD; they will get dropped, because previous data packets
> *did* include timestamps [pf_norm.c:1252 onwards].
> >How-To-Repeat:
> - set sysctl net.inet.tcp.keepidle to a low value
> - open a tcp connection with SO_KEEPALIVE to an illumos host,
> eg. using ssh (TCPKeepAlive=yes is the default)
> - let the connection idle for half the amount of
> net.inet.tcp.keepidle
> - observe that data packets get delivered to the illumos host,
> but no data packets make it back. With 'pfctl -x notice',
> observe that pf_norm.c:1283 is reached.
> >Fix:
> Include timestamp options in TCP keep-alive ACKs when the
> connection uses them for other packets.
>
> [0]:
> https://www.listbox.com/member/archive/182193/2015/04/sort/time_rev/page/1/entry/0:1/20150414115040:F678B734-E2BD-11E4-A441-A07D3EA1AED1/
>
> --
> Lauri Tirkkonen | lotheac @ IRCnet
>
According to 3.2 in RFC 7323:
Once TSopt has been successfully negotiated, that is both <SYN> and
<SYN,ACK> contain TSopt, the TSopt MUST be sent in every non-<RST>
segment for the duration of the connection, and SHOULD be sent in an
<RST> segment (see Section 5.2 for details). The TCP SHOULD remember
this state by setting a flag, referred to as Snd.TS.OK, to one. If a
non-<RST> segment is received without a TSopt, a TCP SHOULD silently
drop the segment. A TCP MUST NOT abort a TCP connection because any
segment lacks an expected TSopt.
Indeed, we must set timestamps on keep alive packets, except that keep
alive is a bit of a hack and hence specs don't make any mention of it.
FreeBSD and NetBSD both should behave the same way. Which seems to
contradict RFC IMO. But then it begs the question should other options
be also included, e.g. TCP MD5 signatures?
Should in fact tcp_timer_keep call syn_cache_respond (with a patched up
sequence number)?
I had a stab at adding timestamp support to tcp_respond but couldn't
test yet. If you feel like giving it a try, please be my guest.
diff --git sys/netinet/tcp_subr.c sys/netinet/tcp_subr.c
index c8c8e77..9f83bca 100644
--- sys/netinet/tcp_subr.c
+++ sys/netinet/tcp_subr.c
@@ -370,14 +370,10 @@ tcp_respond(struct tcpcb *tp, caddr_t template, struct
tcphdr *th0,
xchg(th->th_dport, th->th_sport, u_int16_t);
else
flags = TH_ACK;
#undef xchg
- m->m_len = tlen;
- m->m_pkthdr.len = tlen;
- m->m_pkthdr.rcvif = (struct ifnet *) 0;
- m->m_pkthdr.csum_flags |= M_TCP_CSUM_OUT;
th->th_seq = htonl(seq);
th->th_ack = htonl(ack);
th->th_x2 = 0;
th->th_off = sizeof (struct tcphdr) >> 2;
th->th_flags = flags;
@@ -386,10 +382,26 @@ tcp_respond(struct tcpcb *tp, caddr_t template, struct
tcphdr *th0,
if (win > TCP_MAXWIN)
win = TCP_MAXWIN;
th->th_win = htons((u_int16_t)win);
th->th_urp = 0;
+ if ((tp->t_flags & (TF_REQ_TSTMP|TF_NOOPT)) == TF_REQ_TSTMP &&
+ (flags & TH_RST) == 0 && (tp->t_flags & TF_RCVD_TSTMP)) {
+ u_int32_t *lp = (u_int32_t *)(th + 1);
+ /* Form timestamp option as shown in appendix A of RFC 1323. */
+ *lp++ = htonl(TCPOPT_TSTAMP_HDR);
+ *lp++ = htonl(tcp_now + tp->ts_modulate);
+ *lp = htonl(tp->ts_recent);
+ tlen += TCPOLEN_TSTAMP_APPA;
+ th->th_off = (sizeof(struct tcphdr) + TCPOLEN_TSTAMP_APPA) >> 2;
+ }
+
+ m->m_len = tlen;
+ m->m_pkthdr.len = tlen;
+ m->m_pkthdr.rcvif = (struct ifnet *) 0;
+ m->m_pkthdr.csum_flags |= M_TCP_CSUM_OUT;
+
/* force routing table */
if (tp)
m->m_pkthdr.ph_rtableid = tp->t_inpcb->inp_rtableid;
else
m->m_pkthdr.ph_rtableid = rtableid;
