Re: [ovs-dev] [patch v2 3/3] conntrack: Disable algs by default.

[Darrell Ball]

On 11/30/17, 8:02 AM, "Aaron Conole"  wrote:

Darrell Ball  writes:

> There is a bug here in that the control connection should still be 
created even though it is likely an unused control connection.
> The following incremental fixes it.

Good catch.  While thinking about it, I was wondering if it would be
possible to reject flows that attempt to add insane alg= helpers.  Such
a check currently exists, but only when building ofp messages (at least
as far as I can tell).  Anyway, that path lies outside the scope of this
patch but it's something to consider.

There is protection at various layers, including common checks in ofproto xlate.
Common checks would change if/when userspace and kernel support diverge.

Please keep my ack if you repost with this incremental.

I added a test to the patch as I earlier mentioned I would, so I reverted the 
ack.

Thanks, Darrell!

> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index fed9f61..18016c1 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -505,7 +505,7 @@ handle_alg_ctl(struct conntrack *ct, const struct 
conn_lookup_ctx *ctx,
> const struct conn *conn_for_expectation)
>  {
>  /* ALG control packet handling with expectation creation. */
> -if (OVS_UNLIKELY(alg_helpers[ct_alg_ctl] && conn)) {
> +if (OVS_UNLIKELY(alg_helpers[ct_alg_ctl] && conn && conn->alg)) {
>  alg_helpers[ct_alg_ctl](ct, ctx, pkt, conn_for_expectation, now,
>  CT_FTP_CTL_INTEREST, nat);
>  }
> @@ -871,15 +871,14 @@ conn_not_found(struct conntrack *ct, struct 
dp_packet *pkt,
>  return nc;
>  }
>  
> -if (!ct_verify_helper(helper, ct_alg_ctl)) {
> -return nc;
> -}
> -
>  nc = new_conn(>buckets[bucket], pkt, >key, now);
>  ctx->conn = nc;
>  nc->rev_key = nc->key;
>  conn_key_reverse(>rev_key);
> -nc->alg = nullable_xstrdup(helper);
> +
> +if (ct_verify_helper(helper, ct_alg_ctl)) {
> +nc->alg = nullable_xstrdup(helper);
> +}
>  
>  if (alg_exp) {
>  nc->alg_related = true;
>
>
> This deserves a test case and I created one, which I will submit with 
this patch
>
> Darrell
>
>
> On 11/22/17, 12:56 AM, "ovs-dev-boun...@openvswitch.org on behalf of 
Darrell Ball"  
wrote:
>
> Presently, alg processing is enabled by default to better exercise 
code.
> This is similar to kernels before 4.7 as well.  The recommended 
default
> behavior in the newer kernels is to only process algs if a helper is
> supplied in a conntrack rule.  The behavior is changed to match the
> later kernels.
> 
> Signed-off-by: Darrell Ball 
> ---
>  lib/conntrack.c | 35 +--
>  1 file changed, 29 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 1364d05..fed9f61 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -819,6 +819,26 @@ conn_clean(struct conntrack *ct, struct conn 
*conn,
>  }
>  }
>  
> +static bool
> +ct_verify_helper(const char *helper, enum ct_alg_ctl_type ct_alg_ctl)
> +{
> +if (ct_alg_ctl == CT_ALG_CTL_NONE) {
> +return true;
> +} else if (helper) {
> +if ((ct_alg_ctl == CT_ALG_CTL_FTP) &&
> + !strncmp(helper, "ftp", strlen("ftp"))) {
> +return true;
> +} else if ((ct_alg_ctl == CT_ALG_CTL_TFTP) &&
> +   !strncmp(helper, "tftp", strlen("tftp"))) {
> +return true;
> +} else {
> +return false;
> +}
> +} else {
> +return false;
> +}
> +}
> +
>  /* This function is called with the bucket lock held. */
>  static struct conn *
>  conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
> @@ -826,7 +846,8 @@ conn_not_found(struct conntrack *ct, struct 
dp_packet *pkt,
> const struct nat_action_info_t *nat_action_info,
> struct conn *conn_for_un_nat_copy,
> const char *helper,
> -   const struct alg_exp_node *alg_exp)
> +   const struct alg_exp_node *alg_exp,
> +   enum ct_alg_ctl_type ct_alg_ctl)
>  {
>  unsigned bucket = hash_to_bucket(ctx->hash);
>  

Re: [ovs-dev] [patch v2 3/3] conntrack: Disable algs by default.

[Aaron Conole]

Darrell Ball  writes:

> There is a bug here in that the control connection should still be created 
> even though it is likely an unused control connection.
> The following incremental fixes it.

Good catch.  While thinking about it, I was wondering if it would be
possible to reject flows that attempt to add insane alg= helpers.  Such
a check currently exists, but only when building ofp messages (at least
as far as I can tell).  Anyway, that path lies outside the scope of this
patch but it's something to consider.

Please keep my ack if you repost with this incremental.

Thanks, Darrell!

> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index fed9f61..18016c1 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -505,7 +505,7 @@ handle_alg_ctl(struct conntrack *ct, const struct 
> conn_lookup_ctx *ctx,
> const struct conn *conn_for_expectation)
>  {
>  /* ALG control packet handling with expectation creation. */
> -if (OVS_UNLIKELY(alg_helpers[ct_alg_ctl] && conn)) {
> +if (OVS_UNLIKELY(alg_helpers[ct_alg_ctl] && conn && conn->alg)) {
>  alg_helpers[ct_alg_ctl](ct, ctx, pkt, conn_for_expectation, now,
>  CT_FTP_CTL_INTEREST, nat);
>  }
> @@ -871,15 +871,14 @@ conn_not_found(struct conntrack *ct, struct dp_packet 
> *pkt,
>  return nc;
>  }
>  
> -if (!ct_verify_helper(helper, ct_alg_ctl)) {
> -return nc;
> -}
> -
>  nc = new_conn(>buckets[bucket], pkt, >key, now);
>  ctx->conn = nc;
>  nc->rev_key = nc->key;
>  conn_key_reverse(>rev_key);
> -nc->alg = nullable_xstrdup(helper);
> +
> +if (ct_verify_helper(helper, ct_alg_ctl)) {
> +nc->alg = nullable_xstrdup(helper);
> +}
>  
>  if (alg_exp) {
>  nc->alg_related = true;
>
>
> This deserves a test case and I created one, which I will submit with this 
> patch
>
> Darrell
>
>
> On 11/22/17, 12:56 AM, "ovs-dev-boun...@openvswitch.org on behalf of Darrell 
> Ball"  wrote:
>
> Presently, alg processing is enabled by default to better exercise code.
> This is similar to kernels before 4.7 as well.  The recommended default
> behavior in the newer kernels is to only process algs if a helper is
> supplied in a conntrack rule.  The behavior is changed to match the
> later kernels.
> 
> Signed-off-by: Darrell Ball 
> ---
>  lib/conntrack.c | 35 +--
>  1 file changed, 29 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 1364d05..fed9f61 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -819,6 +819,26 @@ conn_clean(struct conntrack *ct, struct conn *conn,
>  }
>  }
>  
> +static bool
> +ct_verify_helper(const char *helper, enum ct_alg_ctl_type ct_alg_ctl)
> +{
> +if (ct_alg_ctl == CT_ALG_CTL_NONE) {
> +return true;
> +} else if (helper) {
> +if ((ct_alg_ctl == CT_ALG_CTL_FTP) &&
> + !strncmp(helper, "ftp", strlen("ftp"))) {
> +return true;
> +} else if ((ct_alg_ctl == CT_ALG_CTL_TFTP) &&
> +   !strncmp(helper, "tftp", strlen("tftp"))) {
> +return true;
> +} else {
> +return false;
> +}
> +} else {
> +return false;
> +}
> +}
> +
>  /* This function is called with the bucket lock held. */
>  static struct conn *
>  conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
> @@ -826,7 +846,8 @@ conn_not_found(struct conntrack *ct, struct dp_packet 
> *pkt,
> const struct nat_action_info_t *nat_action_info,
> struct conn *conn_for_un_nat_copy,
> const char *helper,
> -   const struct alg_exp_node *alg_exp)
> +   const struct alg_exp_node *alg_exp,
> +   enum ct_alg_ctl_type ct_alg_ctl)
>  {
>  unsigned bucket = hash_to_bucket(ctx->hash);
>  struct conn *nc = NULL;
> @@ -850,14 +871,15 @@ conn_not_found(struct conntrack *ct, struct 
> dp_packet *pkt,
>  return nc;
>  }
>  
> +if (!ct_verify_helper(helper, ct_alg_ctl)) {
> +return nc;
> +}
> +
>  nc = new_conn(>buckets[bucket], pkt, >key, now);
>  ctx->conn = nc;
>  nc->rev_key = nc->key;
>  conn_key_reverse(>rev_key);
> -
> -if (helper) {
> -nc->alg = xstrdup(helper);
> -}
> +nc->alg = nullable_xstrdup(helper);
>  
>  if (alg_exp) {
>  nc->alg_related = true;
> @@ -1222,7 +1244,8 @@ 

Re: [ovs-dev] [patch v2 3/3] conntrack: Disable algs by default.

[Darrell Ball]

There is a bug here in that the control connection should still be created even 
though it is likely an unused control connection.
The following incremental fixes it.

diff --git a/lib/conntrack.c b/lib/conntrack.c
index fed9f61..18016c1 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -505,7 +505,7 @@ handle_alg_ctl(struct conntrack *ct, const struct 
conn_lookup_ctx *ctx,
const struct conn *conn_for_expectation)
 {
 /* ALG control packet handling with expectation creation. */
-if (OVS_UNLIKELY(alg_helpers[ct_alg_ctl] && conn)) {
+if (OVS_UNLIKELY(alg_helpers[ct_alg_ctl] && conn && conn->alg)) {
 alg_helpers[ct_alg_ctl](ct, ctx, pkt, conn_for_expectation, now,
 CT_FTP_CTL_INTEREST, nat);
 }
@@ -871,15 +871,14 @@ conn_not_found(struct conntrack *ct, struct dp_packet 
*pkt,
 return nc;
 }
 
-if (!ct_verify_helper(helper, ct_alg_ctl)) {
-return nc;
-}
-
 nc = new_conn(>buckets[bucket], pkt, >key, now);
 ctx->conn = nc;
 nc->rev_key = nc->key;
 conn_key_reverse(>rev_key);
-nc->alg = nullable_xstrdup(helper);
+
+if (ct_verify_helper(helper, ct_alg_ctl)) {
+nc->alg = nullable_xstrdup(helper);
+}
 
 if (alg_exp) {
 nc->alg_related = true;


This deserves a test case and I created one, which I will submit with this patch

Darrell


On 11/22/17, 12:56 AM, "ovs-dev-boun...@openvswitch.org on behalf of Darrell 
Ball"  wrote:

Presently, alg processing is enabled by default to better exercise code.
This is similar to kernels before 4.7 as well.  The recommended default
behavior in the newer kernels is to only process algs if a helper is
supplied in a conntrack rule.  The behavior is changed to match the
later kernels.

Signed-off-by: Darrell Ball 
---
 lib/conntrack.c | 35 +--
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 1364d05..fed9f61 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -819,6 +819,26 @@ conn_clean(struct conntrack *ct, struct conn *conn,
 }
 }
 
+static bool
+ct_verify_helper(const char *helper, enum ct_alg_ctl_type ct_alg_ctl)
+{
+if (ct_alg_ctl == CT_ALG_CTL_NONE) {
+return true;
+} else if (helper) {
+if ((ct_alg_ctl == CT_ALG_CTL_FTP) &&
+ !strncmp(helper, "ftp", strlen("ftp"))) {
+return true;
+} else if ((ct_alg_ctl == CT_ALG_CTL_TFTP) &&
+   !strncmp(helper, "tftp", strlen("tftp"))) {
+return true;
+} else {
+return false;
+}
+} else {
+return false;
+}
+}
+
 /* This function is called with the bucket lock held. */
 static struct conn *
 conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
@@ -826,7 +846,8 @@ conn_not_found(struct conntrack *ct, struct dp_packet 
*pkt,
const struct nat_action_info_t *nat_action_info,
struct conn *conn_for_un_nat_copy,
const char *helper,
-   const struct alg_exp_node *alg_exp)
+   const struct alg_exp_node *alg_exp,
+   enum ct_alg_ctl_type ct_alg_ctl)
 {
 unsigned bucket = hash_to_bucket(ctx->hash);
 struct conn *nc = NULL;
@@ -850,14 +871,15 @@ conn_not_found(struct conntrack *ct, struct dp_packet 
*pkt,
 return nc;
 }
 
+if (!ct_verify_helper(helper, ct_alg_ctl)) {
+return nc;
+}
+
 nc = new_conn(>buckets[bucket], pkt, >key, now);
 ctx->conn = nc;
 nc->rev_key = nc->key;
 conn_key_reverse(>rev_key);
-
-if (helper) {
-nc->alg = xstrdup(helper);
-}
+nc->alg = nullable_xstrdup(helper);
 
 if (alg_exp) {
 nc->alg_related = true;
@@ -1222,7 +1244,8 @@ process_one(struct conntrack *ct, struct dp_packet 
*pkt,
 ct_rwlock_unlock(>resources_lock);
 
 conn = conn_not_found(ct, pkt, ctx, commit, now, nat_action_info,
-  _for_un_nat_copy, helper, alg_exp);
+  _for_un_nat_copy, helper, alg_exp,
+  ct_alg_ctl);
 }
 
 write_ct_md(pkt, zone, conn, >key, alg_exp);
-- 
1.9.1

___
dev mailing list
d...@openvswitch.org

[ovs-dev] [patch v2 3/3] conntrack: Disable algs by default.

[Darrell Ball]

Presently, alg processing is enabled by default to better exercise code.
This is similar to kernels before 4.7 as well.  The recommended default
behavior in the newer kernels is to only process algs if a helper is
supplied in a conntrack rule.  The behavior is changed to match the
later kernels.

Signed-off-by: Darrell Ball 
---
 lib/conntrack.c | 35 +--
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 1364d05..fed9f61 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -819,6 +819,26 @@ conn_clean(struct conntrack *ct, struct conn *conn,
 }
 }
 
+static bool
+ct_verify_helper(const char *helper, enum ct_alg_ctl_type ct_alg_ctl)
+{
+if (ct_alg_ctl == CT_ALG_CTL_NONE) {
+return true;
+} else if (helper) {
+if ((ct_alg_ctl == CT_ALG_CTL_FTP) &&
+ !strncmp(helper, "ftp", strlen("ftp"))) {
+return true;
+} else if ((ct_alg_ctl == CT_ALG_CTL_TFTP) &&
+   !strncmp(helper, "tftp", strlen("tftp"))) {
+return true;
+} else {
+return false;
+}
+} else {
+return false;
+}
+}
+
 /* This function is called with the bucket lock held. */
 static struct conn *
 conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
@@ -826,7 +846,8 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
const struct nat_action_info_t *nat_action_info,
struct conn *conn_for_un_nat_copy,
const char *helper,
-   const struct alg_exp_node *alg_exp)
+   const struct alg_exp_node *alg_exp,
+   enum ct_alg_ctl_type ct_alg_ctl)
 {
 unsigned bucket = hash_to_bucket(ctx->hash);
 struct conn *nc = NULL;
@@ -850,14 +871,15 @@ conn_not_found(struct conntrack *ct, struct dp_packet 
*pkt,
 return nc;
 }
 
+if (!ct_verify_helper(helper, ct_alg_ctl)) {
+return nc;
+}
+
 nc = new_conn(>buckets[bucket], pkt, >key, now);
 ctx->conn = nc;
 nc->rev_key = nc->key;
 conn_key_reverse(>rev_key);
-
-if (helper) {
-nc->alg = xstrdup(helper);
-}
+nc->alg = nullable_xstrdup(helper);
 
 if (alg_exp) {
 nc->alg_related = true;
@@ -1222,7 +1244,8 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
 ct_rwlock_unlock(>resources_lock);
 
 conn = conn_not_found(ct, pkt, ctx, commit, now, nat_action_info,
-  _for_un_nat_copy, helper, alg_exp);
+  _for_un_nat_copy, helper, alg_exp,
+  ct_alg_ctl);
 }
 
 write_ct_md(pkt, zone, conn, >key, alg_exp);
-- 
1.9.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev