| Message ID | 1353361195-2345-1-git-send-email-fw@strlen.de |
|---|---|
| State | Changes Requested |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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(-)