[ovs-dev] [PATCH] conntrack: invalid packet should not modify ct state

Zang MingJie zealot0630 at gmail.com
Fri Sep 14 08:46:47 UTC 2018


>> > 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, 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

malformed packet will set wrong seq_lo and seq_hi to the state, preventing
following packets pass ct.

>
>>
>>
>> >
>> >
>> >
>> >>
>> >>
>> >> Signed-off-by: Zang MingJie <zealot0630 at 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
>> >> dev at openvswitch.org
>> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>> >
>> >
>
>


More information about the dev mailing list