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

Message ID 20180912091646.24249-1-zealot0630@gmail.com
State New
Headers show
Series
  • [ovs-dev] conntrack: invalid packet should not modify ct state
Related show

Commit Message

Zang MingJie Sept. 12, 2018, 9:16 a.m.
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.

Signed-off-by: Zang MingJie <zealot0630@gmail.com>
---
 lib/conntrack-tcp.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Darrell Ball Sept. 12, 2018, 6:55 p.m. | #1
Thanks for looking MingJie


On Wed, Sep 12, 2018 at 2:16 AM, Zang MingJie <zealot0630@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 */




>
> Signed-off-by: Zang MingJie <zealot0630@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@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Zang MingJie Sept. 13, 2018, 8:55 a.m. | #2
On Thu, Sep 13, 2018 at 2:55 AM Darrell Ball <dlu998@gmail.com> wrote:
>
> Thanks for looking MingJie
>
>
> On Wed, Sep 12, 2018 at 2:16 AM, Zang MingJie <zealot0630@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.

>
>
>
>>
>>
>> Signed-off-by: Zang MingJie <zealot0630@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@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Darrell Ball Sept. 14, 2018, 6:56 a.m. | #3
On Thu, Sep 13, 2018 at 1:55 AM, Zang MingJie <zealot0630@gmail.com> wrote:

> On Thu, Sep 13, 2018 at 2:55 AM Darrell Ball <dlu998@gmail.com> wrote:
> >
> > Thanks for looking MingJie
> >
> >
> > On Wed, Sep 12, 2018 at 2:16 AM, Zang MingJie <zealot0630@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@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@openvswitch.org
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> >
>
Zang MingJie Sept. 14, 2018, 8:46 a.m. | #4
>> > 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@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@openvswitch.org
>> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>> >
>> >
>
>
Darrell Ball Sept. 20, 2018, 12:47 a.m. | #5
On Fri, Sep 14, 2018 at 1:46 AM, Zang MingJie <zealot0630@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
2/ Second packet tcp fields
3/ The tcp lengths if the response is a crafted packet with unexpected data.

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 <zealot0630@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@openvswitch.org
> >> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >> >
> >> >
> >
> >
>

Patch

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