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

Darrell Ball dlu998 at gmail.com
Fri Sep 14 06:56:10 UTC 2018


On Thu, Sep 13, 2018 at 1:55 AM, Zang MingJie <zealot0630 at gmail.com> wrote:

> On Thu, Sep 13, 2018 at 2:55 AM Darrell Ball <dlu998 at gmail.com> wrote:
> >
> > Thanks for looking MingJie
> >
> >
> > On Wed, Sep 12, 2018 at 2:16 AM, Zang MingJie <zealot0630 at gmail.com>
> wrote:
> >>
> >> When encounter an invalid packet, all changes made by the packet should
> >> be reverted. Currently an invalid packet can change the seq number while
> >> the connection is in SEQ_RECV state.
> >
> >
> >
> > 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 ?


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