On Fri, Sep 14, 2018 at 1:46 AM, Zang MingJie <[email protected]> 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 <[email protected]>
> >> >> ---
> >> >> 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
> >> >> [email protected]
> >> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >> >
> >> >
> >
> >
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
