[ovs-dev] datapath-windows: Block established TCP connections from creating new CT Entry

Message ID 20180308013922.103884-1-vsairam@vmware.com
State Changes Requested
Headers show
Series
  • [ovs-dev] datapath-windows: Block established TCP connections from creating new CT Entry
Related show

Commit Message

Sairam Venugopal March 8, 2018, 1:39 a.m.
Established TCP Connections could potentially form invalid Conntrack
Entries when OVS is getting installed or the Conntrack Flows are applied.
Prevent this from happening by explicitly requiring SYN packets to be
present for creating new Conntrack entries.

Signed-off-by: Sairam Venugopal <vsairam@vmware.com>
---
 datapath-windows/ovsext/Conntrack-tcp.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Ben Pfaff March 8, 2018, 6:43 p.m. | #1
On Wed, Mar 07, 2018 at 05:39:22PM -0800, Sairam Venugopal wrote:
> Established TCP Connections could potentially form invalid Conntrack
> Entries when OVS is getting installed or the Conntrack Flows are applied.
> Prevent this from happening by explicitly requiring SYN packets to be
> present for creating new Conntrack entries.
> 
> Signed-off-by: Sairam Venugopal <vsairam@vmware.com>

I think that this is the opposite of the policy direction that we're
pursuing in the userspace connection tracker.  Darrell, do I understand
that correctly?
Darrell Ball March 9, 2018, 9:41 p.m. | #2
Hi Sai

"loose mode" is the default for Linux/BSD/userpsace DP.

I think this default makes sense because the most common use cases are:
moving a connection from one instance of a firewall (stateless or stateful)
to another.
With loose mode disabled, we would drop the subsequent packets.

Moving from no protection to some form of protection is less common and in
that
case, everything that happened before and the existing endpoint states are
all tainted anyways.

Thanks Darrell




On Thu, Mar 8, 2018 at 10:43 AM, Ben Pfaff <blp@ovn.org> wrote:

> On Wed, Mar 07, 2018 at 05:39:22PM -0800, Sairam Venugopal wrote:
> > Established TCP Connections could potentially form invalid Conntrack
> > Entries when OVS is getting installed or the Conntrack Flows are applied.
> > Prevent this from happening by explicitly requiring SYN packets to be
> > present for creating new Conntrack entries.
> >
> > Signed-off-by: Sairam Venugopal <vsairam@vmware.com>
>
> I think that this is the opposite of the policy direction that we're
> pursuing in the userspace connection tracker.  Darrell, do I understand
> that correctly?
>

Patch

diff --git a/datapath-windows/ovsext/Conntrack-tcp.c b/datapath-windows/ovsext/Conntrack-tcp.c
index 8cbab24..ea43df1 100644
--- a/datapath-windows/ovsext/Conntrack-tcp.c
+++ b/datapath-windows/ovsext/Conntrack-tcp.c
@@ -455,9 +455,15 @@  OvsConntrackValidateTcpPacket(const TCPHdr *tcp)
         return FALSE;
     }
 
+    /* Block pre-established connections from going through */
+    if (!(tcp_flags & TCP_SYN)) {
+        OVS_LOG_TRACE("Pre-established TCP packet detected, non-SYN flags not allowed,"
+                      "tcp_flags %hu", tcp_flags);
+        return FALSE;
+    }
+
     /* A syn+ack is not allowed to create a connection.  We want to allow
-     * totally new connections (syn) or already established, not partially
-     * open (syn+ack). */
+     * totally new connections (syn), not partially open (syn+ack). */
     if ((tcp_flags & TCP_SYN) && (tcp_flags & TCP_ACK)) {
         OVS_LOG_TRACE("Invalid TCP packet detected, SYN+ACK flags not allowed,"
                       "tcp_flags %hu", tcp_flags);