On Thu, Sep 20, 2018 at 8:47 AM Darrell Ball <dlu...@gmail.com> wrote:
> > > On Fri, Sep 14, 2018 at 1:46 AM, Zang MingJie <zealot0...@gmail.com> > wrote: > >> >> > Did you notice this check ? >> >> > >> >> > if (src->state < CT_DPIF_TCPS_SYN_SENT) { >> >> > /* First packet from this end. Set its state */ >> >> >> >> Yes, this is exactly where we found the problem. If first reply packet >> >> is invalid, it masses all following packets. >> > >> > >> > >> > Based on your description in the commit message: >> > " Currently an invalid packet can change the seq number while the >> connection is in SEQ_RECV state." >> > we don't fall into this condition since SEQ_RECV > >> CT_DPIF_TCPS_SYN_SENT, right ? >> > >> > If you did not mean "SEQ_RECV", maybe you meant something else ? >> > What the value of "src->state" where you saw an issue ? >> >> SYN_RECV is our server side tcp state, >> > > Thanks for clarifying what you referring to. > > > >> ct table track either side tcp state separately, the problem happens in >> this situation: >> >> client | ct.src ct.dst | server >> packet: syn -> >> state : SYN_SEND | SYN_SEND CLOSED | SYN_RECV >> packet: <- malformed packet >> state: SYN_SEND | SYN_SEND SYN_SEND | SYN_RECV <-updated to wrong >> state >> packet: <- syn+ack <-invalid >> > > That's all fine, but details are needed. > > Is the second packet crafted to be invalid ? > For that matter, is the first packet crafted to be bogus as well ? > > Pls send along: > > 1/ First packet tcp fields > first packet is normal tcp syn packet, nothing special > 2/ Second packet tcp fields > second invalid packet is triggered due to a previous kernel bug, in general it is packet of previous connection with same 5-tuple, everything is wrong including flags, seq and ack number. the actual value doesn't matter as long as it is invalid, as I have already shown, the CT state of this peer will go to SYN_SEND even that no syn flag was set, and the wrong seq number in the invalid packet will be tracked. > 3/ The tcp lengths if the response is a crafted packet with unexpected > data. > it doesn't matter at all. In the situation where we found the bug, it's a fin packet of the last connection, no data. The major problem is following checking you have posted: if (src->state < CT_DPIF_TCPS_SYN_SENT) { It is too loose, and if meet, the peer state will be changed, it is unexpected. > > Thanks Darrell > > >> >> malformed packet will set wrong seq_lo and seq_hi to the state, >> preventing following packets pass ct. >> >> > >> >> >> >> >> >> > >> >> > >> >> > >> >> >> >> >> >> >> >> >> Signed-off-by: Zang MingJie <zealot0...@gmail.com> >> >> >> --- >> >> >> lib/conntrack-tcp.c | 10 ++++++++-- >> >> >> 1 file changed, 8 insertions(+), 2 deletions(-) >> >> >> >> >> >> diff --git a/lib/conntrack-tcp.c b/lib/conntrack-tcp.c >> >> >> index 86d313d28..7443a3293 100644 >> >> >> --- a/lib/conntrack-tcp.c >> >> >> +++ b/lib/conntrack-tcp.c >> >> >> @@ -151,9 +151,9 @@ tcp_conn_update(struct conn *conn_, struct >> conntrack_bucket *ctb, >> >> >> struct conn_tcp *conn = conn_tcp_cast(conn_); >> >> >> struct tcp_header *tcp = dp_packet_l4(pkt); >> >> >> /* The peer that sent 'pkt' */ >> >> >> - struct tcp_peer *src = &conn->peer[reply ? 1 : 0]; >> >> >> + struct tcp_peer orig_src, *src = &conn->peer[reply ? 1 : 0]; >> >> >> /* The peer that should receive 'pkt' */ >> >> >> - struct tcp_peer *dst = &conn->peer[reply ? 0 : 1]; >> >> >> + struct tcp_peer orig_dst, *dst = &conn->peer[reply ? 0 : 1]; >> >> >> uint8_t sws = 0, dws = 0; >> >> >> uint16_t tcp_flags = TCP_FLAGS(tcp->tcp_ctl); >> >> >> >> >> >> @@ -187,6 +187,10 @@ tcp_conn_update(struct conn *conn_, struct >> conntrack_bucket *ctb, >> >> >> dws = TCP_MAX_WSCALE; >> >> >> } >> >> >> >> >> >> + >> >> >> + orig_src = *src; >> >> >> + orig_dst = *dst; >> >> >> + >> >> >> /* >> >> >> * Sequence tracking algorithm from Guido van Rooij's paper: >> >> >> * http://www.madison-gurkha.com/publications/tcp_filtering/ >> >> >> @@ -385,6 +389,8 @@ tcp_conn_update(struct conn *conn_, struct >> conntrack_bucket *ctb, >> >> >> src->state = dst->state = CT_DPIF_TCPS_TIME_WAIT; >> >> >> } >> >> >> } else { >> >> >> + *src = orig_src; >> >> >> + *dst = orig_dst; >> >> >> return CT_UPDATE_INVALID; >> >> >> } >> >> >> >> >> >> -- >> >> >> 2.19.0.rc1 >> >> >> >> >> >> _______________________________________________ >> >> >> dev mailing list >> >> >> d...@openvswitch.org >> >> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > >> >> > >> > >> > >> > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev