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

Darrell Ball dlu998 at gmail.com
Fri Oct 5 00:28:28 UTC 2018


On Tue, Sep 25, 2018 at 1:43 AM Zang MingJie <zealot0630 at gmail.com> wrote:

>
>
> On Thu, Sep 20, 2018 at 8:47 AM Darrell Ball <dlu998 at gmail.com> wrote:
>
>>
>>
>> On Fri, Sep 14, 2018 at 1:46 AM, Zang MingJie <zealot0630 at 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 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