| Message ID | 20180912091646.24249-1-zealot0630@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [ovs-dev] conntrack: invalid packet should not modify ct state | expand |
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 >
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 > >
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 > > > > >
>> > 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 >> > >> > > >
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 > >> > > >> > > > > > >
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 >> >> > >> >> > >> > >> > >> > >
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 >>> >> > >>> >> > >>> > >>> > >>> >> >>
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; }
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(-)