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

2017-11-20 Thread Aaron Conole 
Darrell Ball  writes:

> On 11/20/17, 7:19 AM, "ovs-dev-boun...@openvswitch.org on behalf of
> Aaron Conole"  acon...@redhat.com> wrote:
>
> Hi Darrell,
> 
> Darrell Ball  writes:
> 
> > Presently, alg processing is enabled by default to exercise testing.
> > This is similar to kernels before 4.7.  The recommended default
> > behavior in the kernel 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 
> > ---
> 
> I like having the alg processing off by default.  However, at this point
> some folks may be relying (or not care about) on the default behavior of
> the ct() action (which is to assume
> ALG=whatever-is-appropriate-to-the-port).  I think we probably will need
> to have a knob to enable / disable this.
> 
> This part has me worried.
> The kernel has taken the position that the user should operate such that a 
> helper is 
> needed to be specified. This gives the user total granular control as well.
>
> i.e. the kernel has taken the position that
> net.netfilter.nf_conntrack_helper should be off by default
> in all cases.
>
> Allowing the user to turn this on is a double-edged sword. Yes, it
> allows the user to not have to specify
> alg=blah, but is needing to specify alg= all that onerous?

I'm not sure.  How many scripts and orchestration tools need to be
rewritten to handle it?  Maybe it's early enough that no one will care.
I'm okay not having the knob, but if we get bugs, such a knob will have
to go in and probably need to be backported.  I'm simply trying to think
about that case.

> On the flip side, allowing the user to skip specifying “alg=blah”
> opens a security hole that will never be
> noticed until a problem occurs – that is really nasty. Given the
> kernel position, it may be ok to do the
> right thing here and not allow the user to shoot himself in the foot 
> accidently.
>
>
>
>
> More below.
> 
> >  lib/conntrack.c | 35 +--
> >  1 file changed, 29 insertions(+), 6 deletions(-)
> >
> > diff --git a/lib/conntrack.c b/lib/conntrack.c
> > index 7fbcfba..dea2fed 100644
> > --- a/lib/conntrack.c
> > +++ b/lib/conntrack.c
> > @@ -789,13 +789,34 @@ conn_clean(struct conntrack *ct, struct conn 
> *conn,
> >  }
> >  }
> >  
> > +static bool
> > +ct_verify_helper(const char *helper, enum ct_alg_ctl_type ct_alg_ctl)
> > +{
> 
> Since we know the algorithm at this point, doesn't it make sense to
> just associate the alg with the connection?  I say that because if I
> read this correctly (and I haven't had coffee yet - maybe I missed
> something) non-standard ports won't get their associations properly (so
> if I want to run ftp on port 2122 for example, the helper will not get
> associated).
> 
> If we take the approach I mentioned in 2/4, we can just associate the
> function pointer.  After all, the user asked for it - we should do what
> the user wants.
>
> It is orthogonal.
> This of CT_ALG_CTL_FTP and CT_ALG_CTL_TFTP as array indexes (I’ll
> explain this more in my response to patch 2).
> i.e. CT_ALG_CTL_FTP and CT_ALG_CTL_TFTP are not linked to specific L4 port 
> numbers.
>
>
> 
> > +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,
> > struct conn_lookup_ctx *ctx, bool commit, long long now,
> > 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 char *helper, const struct alg_exp_node *alg_exp,
> > +   enum ct_alg_ctl_type ct_alg_ctl)
> >  {
> >  struct conn *nc = NULL;
> >  
> > @@ -819,15 +840,16 @@ conn_not_found(struct conntrack *ct, struct 
> dp_packet *pkt,
> >  return nc;
> >  }
> >  
> > +if (!ct_verify_helper(helper, ct_alg_ctl)) {
> > +return nc;

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

2017-11-20 Thread Darrell Ball 


On 11/20/17, 7:19 AM, "ovs-dev-boun...@openvswitch.org on behalf of Aaron 
Conole"  wrote:

Hi Darrell,

Darrell Ball  writes:

> Presently, alg processing is enabled by default to exercise testing.
> This is similar to kernels before 4.7.  The recommended default
> behavior in the kernel 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 
> ---

I like having the alg processing off by default.  However, at this point
some folks may be relying (or not care about) on the default behavior of
the ct() action (which is to assume
ALG=whatever-is-appropriate-to-the-port).  I think we probably will need
to have a knob to enable / disable this.

This part has me worried.
The kernel has taken the position that the user should operate such that a 
helper is 
needed to be specified. This gives the user total granular control as well.

i.e. the kernel has taken the position that net.netfilter.nf_conntrack_helper 
should be off by default
in all cases.

Allowing the user to turn this on is a double-edged sword. Yes, it allows the 
user to not have to specify
alg=blah, but is needing to specify alg= all that onerous?

On the flip side, allowing the user to skip specifying “alg=blah” opens a 
security hole that will never be
noticed until a problem occurs – that is really nasty. Given the kernel 
position, it may be ok to do the
right thing here and not allow the user to shoot himself in the foot accidently.




More below.

>  lib/conntrack.c | 35 +--
>  1 file changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 7fbcfba..dea2fed 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -789,13 +789,34 @@ conn_clean(struct conntrack *ct, struct conn *conn,
>  }
>  }
>  
> +static bool
> +ct_verify_helper(const char *helper, enum ct_alg_ctl_type ct_alg_ctl)
> +{

Since we know the algorithm at this point, doesn't it make sense to
just associate the alg with the connection?  I say that because if I
read this correctly (and I haven't had coffee yet - maybe I missed
something) non-standard ports won't get their associations properly (so
if I want to run ftp on port 2122 for example, the helper will not get
associated).

If we take the approach I mentioned in 2/4, we can just associate the
function pointer.  After all, the user asked for it - we should do what
the user wants.

It is orthogonal.
This of CT_ALG_CTL_FTP and CT_ALG_CTL_TFTP as array indexes (I’ll explain this 
more in my response to patch 2).
i.e. CT_ALG_CTL_FTP and CT_ALG_CTL_TFTP are not linked to specific L4 port 
numbers.



> +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,
> struct conn_lookup_ctx *ctx, bool commit, long long now,
> 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 char *helper, const struct alg_exp_node *alg_exp,
> +   enum ct_alg_ctl_type ct_alg_ctl)
>  {
>  struct conn *nc = NULL;
>  
> @@ -819,15 +840,16 @@ conn_not_found(struct conntrack *ct, struct 
dp_packet *pkt,
>  return nc;
>  }
>  
> +if (!ct_verify_helper(helper, ct_alg_ctl)) {
> +return nc;
> +}
> +
>  unsigned bucket = hash_to_bucket(ctx->hash);
>  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;
> @@ -1182,7 +1204,8 @@ process_one(struct conntrack *ct, struct

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

2017-11-20 Thread Aaron Conole 
Hi Darrell,

Darrell Ball  writes:

> Presently, alg processing is enabled by default to exercise testing.
> This is similar to kernels before 4.7.  The recommended default
> behavior in the kernel 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 
> ---

I like having the alg processing off by default.  However, at this point
some folks may be relying (or not care about) on the default behavior of
the ct() action (which is to assume
ALG=whatever-is-appropriate-to-the-port).  I think we probably will need
to have a knob to enable / disable this.

More below.

>  lib/conntrack.c | 35 +--
>  1 file changed, 29 insertions(+), 6 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 7fbcfba..dea2fed 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -789,13 +789,34 @@ conn_clean(struct conntrack *ct, struct conn *conn,
>  }
>  }
>  
> +static bool
> +ct_verify_helper(const char *helper, enum ct_alg_ctl_type ct_alg_ctl)
> +{

Since we know the algorithm at this point, doesn't it make sense to
just associate the alg with the connection?  I say that because if I
read this correctly (and I haven't had coffee yet - maybe I missed
something) non-standard ports won't get their associations properly (so
if I want to run ftp on port 2122 for example, the helper will not get
associated).

If we take the approach I mentioned in 2/4, we can just associate the
function pointer.  After all, the user asked for it - we should do what
the user wants.

> +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,
> struct conn_lookup_ctx *ctx, bool commit, long long now,
> 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 char *helper, const struct alg_exp_node *alg_exp,
> +   enum ct_alg_ctl_type ct_alg_ctl)
>  {
>  struct conn *nc = NULL;
>  
> @@ -819,15 +840,16 @@ conn_not_found(struct conntrack *ct, struct dp_packet 
> *pkt,
>  return nc;
>  }
>  
> +if (!ct_verify_helper(helper, ct_alg_ctl)) {
> +return nc;
> +}
> +
>  unsigned bucket = hash_to_bucket(ctx->hash);
>  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;
> @@ -1182,7 +1204,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);
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

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

2017-11-19 Thread Darrell Ball 
Presently, alg processing is enabled by default to exercise testing.
This is similar to kernels before 4.7.  The recommended default
behavior in the kernel 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 7fbcfba..dea2fed 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -789,13 +789,34 @@ 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,
struct conn_lookup_ctx *ctx, bool commit, long long now,
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 char *helper, const struct alg_exp_node *alg_exp,
+   enum ct_alg_ctl_type ct_alg_ctl)
 {
 struct conn *nc = NULL;
 
@@ -819,15 +840,16 @@ conn_not_found(struct conntrack *ct, struct dp_packet 
*pkt,
 return nc;
 }
 
+if (!ct_verify_helper(helper, ct_alg_ctl)) {
+return nc;
+}
+
 unsigned bucket = hash_to_bucket(ctx->hash);
 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;
@@ -1182,7 +1204,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