[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
> >> >
> >> >
> >
> >
>
Zang MingJie Sept. 25, 2018, 8:43 a.m. | #6
On Thu, Sep 20, 2018 at 8:47 AM Darrell Ball <dlu998@gmail.com> wrote:

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

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 <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 Oct. 5, 2018, 12:28 a.m. | #7
On Tue, Sep 25, 2018 at 1:43 AM Zang MingJie <zealot0630@gmail.com> wrote:

>
>
> On Thu, Sep 20, 2018 at 8:47 AM Darrell Ball <dlu998@gmail.com> wrote:
>
>>
>>
>> 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
>>
>
> first packet is normal tcp syn packet, nothing special
>

I really would like to see the packet; pls send it.
You could send just the TCP fields and change the port numbers to
fictitious ones if you are
worried about providing proprietary information; alternatively, you can
supply it to me offline.



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

I really would like to see the packet; pls send it.
As mentioned above, you could send just the TCP fields and change the port
numbers to fictitious ones if you are
worried about providing proprietary information; alternatively, you can
supply it to me offline.



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

I already understood your perspective on the issue.

Thanks Darrell



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