Re: tcp keep-alives sent without timestamps

2015-05-07 Thread Mike Belopuhov 
On 6 May 2015 at 13:05, Martin Pieuchot m...@openbsd.org wrote:
 On 20/04/15(Mon) 18:37, Mike Belopuhov wrote:
 On Tue, Apr 14, 2015 at 22:08 +0300, Lauri Tirkkonen wrote:
  On Tue, Apr 14 2015 20:40:58 +0200, Mike Belopuhov wrote:
   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.
 
  Thank you, I somehow missed the existence of this RFC.
 

 Does anyone else want to comment on this?

 Diff looks good to me.  Since you mentioned other *BSD in your previous
 post, I look at what Linux and Solaris do and they both seem to set
 timestamps on keep alive packets.

Thanks.

 As for MD5 signatures Linux also include it.


Can't fix every problem with one patch...

 ok mpi@


Committed! Thanks for the review!

Re: tcp keep-alives sent without timestamps

2015-05-07 Thread Mike Belopuhov 
On 14 April 2015 at 21:08, Lauri Tirkkonen loth...@iki.fi wrote:
 On Tue, Apr 14 2015 20:40:58 +0200, Mike Belopuhov wrote:
 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.

 Thank you, I somehow missed the existence of this RFC.

 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.

 With your patch, I confirm that timestamps are present on keep-alive
 messages.


Hi Lauri,

I've committed the fix.  Thanks a lot for bringing this up and testing.

Re: tcp keep-alives sent without timestamps

2015-05-06 Thread Martin Pieuchot 
On 20/04/15(Mon) 18:37, Mike Belopuhov wrote:
 On Tue, Apr 14, 2015 at 22:08 +0300, Lauri Tirkkonen wrote:
  On Tue, Apr 14 2015 20:40:58 +0200, Mike Belopuhov wrote:
   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.
  
  Thank you, I somehow missed the existence of this RFC.
 
 
 Does anyone else want to comment on this?

Diff looks good to me.  Since you mentioned other *BSD in your previous
post, I look at what Linux and Solaris do and they both seem to set
timestamps on keep alive packets.  As for MD5 signatures Linux also
include it.

ok mpi@

   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.
  
  With your patch, I confirm that timestamps are present on keep-alive
  messages.
  
 
 The patch needs a small (but crucial) amendment: tcp pcb can be NULL...
 
 diff --git sys/netinet/tcp_subr.c sys/netinet/tcp_subr.c
 index c8c8e77..6f17af0 100644
 --- sys/netinet/tcp_subr.c
 +++ sys/netinet/tcp_subr.c
 @@ -295,13 +295,10 @@ tcp_template(tp)
   * attached mbufs.
   *
   * In any case the ack and sequence number of the transmitted
   * segment are as specified by the parameters.
   */
 -#ifdef INET6
 -/* This function looks hairy, because it was so IPv4-dependent. */
 -#endif /* INET6 */
  void
  tcp_respond(struct tcpcb *tp, caddr_t template, struct tcphdr *th0,
  tcp_seq ack, tcp_seq seq, int flags, u_int rtableid)
  {
   int tlen;
 @@ -370,14 +367,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;
   th-th_ack = htonl(ack);
   th-th_x2 = 0;
   th-th_off = sizeof (struct tcphdr)  2;
   th-th_flags = flags;
 @@ -386,10 +379,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  (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;

Re: tcp keep-alives sent without timestamps

2015-04-20 Thread Mike Belopuhov 
On Tue, Apr 14, 2015 at 22:08 +0300, Lauri Tirkkonen wrote:
 On Tue, Apr 14 2015 20:40:58 +0200, Mike Belopuhov wrote:
  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.
 
 Thank you, I somehow missed the existence of this RFC.


Does anyone else want to comment on this?

  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.
 
 With your patch, I confirm that timestamps are present on keep-alive
 messages.
 

The patch needs a small (but crucial) amendment: tcp pcb can be NULL...

diff --git sys/netinet/tcp_subr.c sys/netinet/tcp_subr.c
index c8c8e77..6f17af0 100644
--- sys/netinet/tcp_subr.c
+++ sys/netinet/tcp_subr.c
@@ -295,13 +295,10 @@ tcp_template(tp)
  * attached mbufs.
  *
  * In any case the ack and sequence number of the transmitted
  * segment are as specified by the parameters.
  */
-#ifdef INET6
-/* This function looks hairy, because it was so IPv4-dependent. */
-#endif /* INET6 */
 void
 tcp_respond(struct tcpcb *tp, caddr_t template, struct tcphdr *th0,
 tcp_seq ack, tcp_seq seq, int flags, u_int rtableid)
 {
int tlen;
@@ -370,14 +367,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 +379,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  (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;

Re: tcp keep-alives sent without timestamps

2015-04-14 Thread Mike Belopuhov 
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;

Re: tcp keep-alives sent without timestamps

2015-04-14 Thread Lauri Tirkkonen 
On Tue, Apr 14 2015 20:40:58 +0200, Mike Belopuhov wrote:
 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.

Thank you, I somehow missed the existence of this RFC.

 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.

With your patch, I confirm that timestamps are present on keep-alive
messages.

-- 
Lauri Tirkkonen | lotheac @ IRCnet