[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