[PATCH] conntrack: fix nfct_clone with certain attribute data types

[PATCH] conntrack: fix nfct_clone with certain attribute data types

[Florian Westphal]

some attributes are pointers to malloc'd objects.  Simply
copying the pointer results in use-after free
when the original or the clone is destroyed.

Also, add test case for cloned objects to ensure that
ct = nfct_new();
ct2 = nfct_clone(ct);
nfct_destroy(ct);
nfct_destroy(ct2);

won't crash due to double-frees.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 When working on the connlabel stuff I noticed that nfct_clone does a
 plain memcpy.  Afaics nfct_clone has returned a shallow copy for ages.
 But documentation implies that it really should do a deep copy.

 Pablo, could you please double-check?  Thanks!

 qa/test_api.c       |    6 ++++++
 src/conntrack/api.c |    2 +-
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/qa/test_api.c b/qa/test_api.c
index d5f95e9..fa325c8 100644
--- a/qa/test_api.c
+++ b/qa/test_api.c
@@ -2,6 +2,7 @@
  * Run this after adding a new attribute to the nf_conntrack object
  */
 
+#include <assert.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>
@@ -199,6 +200,10 @@ int main(void)
 		eval_sigterm(status);
 	}
 
+	ct2 = nfct_clone(ct);
+	assert(ct2);
+	nfct_destroy(ct2);
+
 	ct2 = nfct_new();
 	if (!ct2) {
 		perror("nfct_new");
@@ -264,6 +269,7 @@ int main(void)
 	}
 
 	nfct_destroy(ct2);
+	printf("== destroy cloned ct entry ==\n");
 	nfct_destroy(ct);
 	nfct_destroy(tmp);
 	nfexp_destroy(exp);
diff --git a/src/conntrack/api.c b/src/conntrack/api.c
index 000571f..ebf0d52 100644
--- a/src/conntrack/api.c
+++ b/src/conntrack/api.c
@@ -147,7 +147,7 @@ struct nf_conntrack *nfct_clone(const struct nf_conntrack *ct)
 
 	if ((clone = nfct_new()) == NULL)
 		return NULL;
-	memcpy(clone, ct, sizeof(*ct));
+	nfct_copy(clone, ct, NFCT_CP_ALL);
 
 	return clone;
 }
-- 
1.7.8.6

Re: [PATCH] conntrack: fix nfct_clone with certain attribute data types

[Pablo Neira Ayuso]

On Mon, Nov 19, 2012 at 10:39:55PM +0100, Florian Westphal wrote:
> some attributes are pointers to malloc'd objects.  Simply
> copying the pointer results in use-after free
> when the original or the clone is destroyed.
> 
> Also, add test case for cloned objects to ensure that
> ct = nfct_new();
> ct2 = nfct_clone(ct);
> nfct_destroy(ct);
> nfct_destroy(ct2);
> 
> won't crash due to double-frees.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  When working on the connlabel stuff I noticed that nfct_clone does a
>  plain memcpy.  Afaics nfct_clone has returned a shallow copy for ages.
>  But documentation implies that it really should do a deep copy.
> 
>  Pablo, could you please double-check?  Thanks!
> 
>  qa/test_api.c       |    6 ++++++
>  src/conntrack/api.c |    2 +-
>  2 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/qa/test_api.c b/qa/test_api.c
> index d5f95e9..fa325c8 100644
> --- a/qa/test_api.c
> +++ b/qa/test_api.c
> @@ -2,6 +2,7 @@
>   * Run this after adding a new attribute to the nf_conntrack object
>   */
>  
> +#include <assert.h>
>  #include <stdio.h>
>  #include <stdlib.h>
>  #include <unistd.h>
> @@ -199,6 +200,10 @@ int main(void)
>  		eval_sigterm(status);
>  	}
>  
> +	ct2 = nfct_clone(ct);
> +	assert(ct2);
> +	nfct_destroy(ct2);
> +
>  	ct2 = nfct_new();
>  	if (!ct2) {
>  		perror("nfct_new");
> @@ -264,6 +269,7 @@ int main(void)
>  	}
>  
>  	nfct_destroy(ct2);
> +	printf("== destroy cloned ct entry ==\n");
>  	nfct_destroy(ct);
>  	nfct_destroy(tmp);
>  	nfexp_destroy(exp);
> diff --git a/src/conntrack/api.c b/src/conntrack/api.c
> index 000571f..ebf0d52 100644
> --- a/src/conntrack/api.c
> +++ b/src/conntrack/api.c
> @@ -147,7 +147,7 @@ struct nf_conntrack *nfct_clone(const struct nf_conntrack *ct)
>  
>  	if ((clone = nfct_new()) == NULL)
>  		return NULL;
> -	memcpy(clone, ct, sizeof(*ct));
> +	nfct_copy(clone, ct, NFCT_CP_ALL);

That seems safe to me.

Still I think NFCT_CP_OVERRIDE is faster. I added that flag way after
to improve a bit the copying time.

It will require also to add the connlabel support to it. See
__copy_fast.

I need to check this again but I think we can remove
src/conntrack/copy.c and make NFCT_CP_ALL behave just like
NFCT_CP_OVERRIDE.

Re: [PATCH] conntrack: fix nfct_clone with certain attribute data types

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Mon, Nov 19, 2012 at 10:39:55PM +0100, Florian Westphal wrote:
> > --- a/src/conntrack/api.c
> > +++ b/src/conntrack/api.c
> > @@ -147,7 +147,7 @@ struct nf_conntrack *nfct_clone(const struct nf_conntrack *ct)
> >  
> >  	if ((clone = nfct_new()) == NULL)
> >  		return NULL;
> > -	memcpy(clone, ct, sizeof(*ct));
> > +	nfct_copy(clone, ct, NFCT_CP_ALL);
> 
> That seems safe to me.
> 
> Still I think NFCT_CP_OVERRIDE is faster. I added that flag way after
> to improve a bit the copying time.

I missed that, thanks.

Suggestion:
I'll re-send with the above changed to
nfct_copy(clone, ct, NFCT_CP_OVERRIDE)

plus adding copy_attr_help_info(ct1, ct2)
to __copy_fast.