Hi, On Do, 2015-05-21 at 18:42 +0000, Loganaden Velvindron wrote: > A number of Open Source projects such as OpenSSH, nsd, unbound, > OpenBSD and FreeBSD have implemented a safe replacement for > malloc(x*y) idiom which may lead to overflows. > > Quoting from man page: > Consider calloc() or the extension reallocarray() when there is > multiplication in the size argument of malloc() or realloc(). For > example, avoid this common idiom as it may lead to integer overflow: > > if ((p = malloc(num * size)) == NULL) > err(1, "malloc"); > > A drop-in replacement is the OpenBSD extension reallocarray(): > > if ((p = reallocarray(NULL, num, size)) == NULL) > err(1, "reallocarray"); > > > Replacing malloc() with reallocarray() has led to one vulnerability > being mitigated: > See here: https://www.kb.cert.org/vuls/id/695940 > > The other advantage is that reallocarray() is faster than calloc(x*y) > as there is no zero'ing overhead. > > Patch below with a few conversions to reallocarray(). -- Feedback welcomed ! > > diff --git a/include/xtables.h b/include/xtables.h > index bad11a8..e18b4cd 100644 > --- a/include/xtables.h > +++ b/include/xtables.h > @@ -418,6 +418,7 @@ extern void xtables_init(void); > extern void xtables_set_nfproto(uint8_t); > extern void *xtables_calloc(size_t, size_t); > extern void *xtables_malloc(size_t); > +extern void *xtables_reallocarray(void *, size_t, size_t); > extern void *xtables_realloc(void *, size_t); > > extern int xtables_insmod(const char *, const char *, bool); > diff --git a/iptables/ip6tables.c b/iptables/ip6tables.c > index 8db13b4..8964c1e 100644 > --- a/iptables/ip6tables.c > +++ b/iptables/ip6tables.c > @@ -858,7 +858,7 @@ for_each_chain6(int (*fn)(const xt_chainlabel, > int, struct xtc_handle *), > chain = ip6tc_next_chain(handle); > } > > - chains = xtables_malloc(sizeof(xt_chainlabel) * chaincount); > + chains = xtables_reallocarray(NULL, chaincount, sizeof(xt_chainlabel)); > i = 0; > chain = ip6tc_first_chain(handle); > while (chain) { > diff --git a/libxtables/xtables.c b/libxtables/xtables.c > index 5357d12..1f62aa3 100644 > --- a/libxtables/xtables.c > +++ b/libxtables/xtables.c > @@ -15,6 +15,23 @@ > * along with this program; if not, write to the Free Software > * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA > 02110-1301, USA. > */ > + > +/* xtables_reallocarray.c is under: > + * Copyright (c) 2008 Otto Moerbeek <otto@xxxxxxxxx> > + * > + * Permission to use, copy, modify, and distribute this software for any > + * purpose with or without fee is hereby granted, provided that the above > + * copyright notice and this permission notice appear in all copies. > + * > + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES > + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF > + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR > + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES > + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN > + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF > + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. > +*/ > + > #include "config.h" > #include <ctype.h> > #include <errno.h> > @@ -308,6 +325,23 @@ void *xtables_malloc(size_t size) > return p; > } > > +/* > + * This is sqrt(SIZE_MAX+1), as s1*s2 <= SIZE_MAX > + * if both s1 < MUL_NO_OVERFLOW and s2 < MUL_NO_OVERFLOW > + */ > +#define MUL_NO_OVERFLOW ((size_t)1 << (sizeof(size_t) * 4)) > + > +void * > +xtables_reallocarray(void *optr, size_t nmemb, size_t size) > +{ > + if ((nmemb >= MUL_NO_OVERFLOW || size >= MUL_NO_OVERFLOW) && if ((nmemb|size) >= MUL_NO_OVERFLOW) && ... > + nmemb > 0 && SIZE_MAX / nmemb < size) { > + perror("ip[6]tables: reallocarray failed"); > + exit(1); > + } Since gcc5.1 you can use __builtin_umul_overflow to let the compiler handle that. Maybe you can abstract that away if needed. Bye, Hannes -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
