On Thu, Sep 20, 2018 at 8:47 AM Darrell Ball <dlu...@gmail.com> wrote:
>
>
> On Fri, Sep 14, 2018 at 1:46 AM, Zang MingJie <zealot0...@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 <zealot0...@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
>> >> >> d...@openvswitch.org
>> >> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>> >> >
>> >> >
>> >
>> >
>>
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
