Harden iptables memory allocator

Harden iptables memory allocator

[Loganaden Velvindron]

Hi All,

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@drijf.net>
+ *
+ * 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) &&
+    nmemb > 0 && SIZE_MAX / nmemb < size) {
+ perror("ip[6]tables: reallocarray failed");
+ exit(1);
+ }
+ return xtables_realloc(optr, size * nmemb);
+}
+
 void *xtables_realloc(void *ptr, size_t size)
 {
  void *p;
@@ -1432,8 +1466,8 @@ void xtables_ipparse_multiple(const char *name,
struct in_addr **addrpp,
  ++loop; /* skip ',' */
  }

- *addrpp = xtables_malloc(sizeof(struct in_addr) * count);
- *maskpp = xtables_malloc(sizeof(struct in_addr) * count);
+ *addrpp = xtables_reallocarray(NULL, count, sizeof(struct in_addr));
+ *maskpp = xtables_reallocarray(NULL, count, sizeof(struct in_addr));

  loop = name;

@@ -1753,8 +1787,8 @@ xtables_ip6parse_multiple(const char *name,
struct in6_addr **addrpp,
  ++loop; /* skip ',' */
  }

- *addrpp = xtables_malloc(sizeof(struct in6_addr) * count);
- *maskpp = xtables_malloc(sizeof(struct in6_addr) * count);
+ *addrpp = xtables_reallocarray(NULL, count, sizeof(struct in6_addr));
+ *maskpp = xtables_reallocarray(NULL, count, sizeof(struct in6_addr));

  loop = name;


:

Re: Harden iptables memory allocator

[Loganaden Velvindron]

On Thu, May 21, 2015 at 06:42:49PM +0000, Loganaden Velvindron wrote:
> Hi All,
> 
> 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 !
> 

Gmail broke the patch. Sorry for that.

Sending it using my other account:

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@drijf.net>
+ *
+ * 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) &&
+	    nmemb > 0 && SIZE_MAX / nmemb < size) {
+		perror("ip[6]tables: reallocarray failed");
+		exit(1);
+	}
+	return xtables_realloc(optr, size * nmemb);
+}
+
 void *xtables_realloc(void *ptr, size_t size)
 {
 	void *p;
@@ -1432,8 +1466,8 @@ void xtables_ipparse_multiple(const char *name, struct in_addr **addrpp,
 		++loop; /* skip ',' */
 	}
 
-	*addrpp = xtables_malloc(sizeof(struct in_addr) * count);
-	*maskpp = xtables_malloc(sizeof(struct in_addr) * count);
+	*addrpp = xtables_reallocarray(NULL, count, sizeof(struct in_addr));
+	*maskpp = xtables_reallocarray(NULL, count, sizeof(struct in_addr));
 
 	loop = name;
 
@@ -1753,8 +1787,8 @@ xtables_ip6parse_multiple(const char *name, struct in6_addr **addrpp,
 		++loop; /* skip ',' */
 	}
 
-	*addrpp = xtables_malloc(sizeof(struct in6_addr) * count);
-	*maskpp = xtables_malloc(sizeof(struct in6_addr) * count);
+	*addrpp = xtables_reallocarray(NULL, count, sizeof(struct in6_addr));
+	*maskpp = xtables_reallocarray(NULL, count, sizeof(struct in6_addr));
 
 	loop = name;
 

Re: Harden iptables memory allocator

[Hannes Frederic Sowa]

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@drijf.net>
> + *
> + * 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

Re: Harden iptables memory allocator

[Jan Engelhardt]

On Friday 2015-05-22 10:50, Hannes Frederic Sowa wrote:
>
>> + if ((nmemb >= MUL_NO_OVERFLOW || size >= MUL_NO_OVERFLOW) &&
>
>if ((nmemb|size) >= MUL_NO_OVERFLOW) && ...

I am sure there are many C tricks one can do, but iptables is
hardly that time-critical to warrant such.
The compiler should perhaps learn to do such an optimization
if it does not already.

Re: Harden iptables memory allocator

[Loganaden Velvindron]

On Fri, May 22, 2015 at 10:50:11AM +0200, Hannes Frederic Sowa wrote:
> 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@drijf.net>
> > + *
> > + * 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.

If the compiler supports it, then we can take advantage of it. 

The advantage of sticking to the portable implementation of reallocarray
is that it's been extensively tested & mature.

> 
> Bye,
> Hannes
> 
> 
> --
> 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

Re: Harden iptables memory allocator

[Loganaden Velvindron]

On Fri, May 22, 2015 at 10:59:44AM +0200, Jan Engelhardt wrote:
> 
> On Friday 2015-05-22 10:50, Hannes Frederic Sowa wrote:
> >
> >> + if ((nmemb >= MUL_NO_OVERFLOW || size >= MUL_NO_OVERFLOW) &&
> >
> >if ((nmemb|size) >= MUL_NO_OVERFLOW) && ...
> 
> I am sure there are many C tricks one can do, but iptables is
> hardly that time-critical to warrant such.

The same can be said of ipset, which uses strlcpy and has strlcat in 
its library. However, those are safer APIs to use.

In this particular case, it's safer to use reallocarray(NULL,x,y) rather than
malloc(x*y).


(Many other less critical software such as m4 have also adopted reallocarray).

> The compiler should perhaps learn to do such an optimization
> if it does not already.
> --
> 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

Re: Harden iptables memory allocator

[Jan Engelhardt]

On Friday 2015-05-22 13:51, Loganaden Velvindron wrote:
>On Fri, May 22, 2015 at 10:59:44AM +0200, Jan Engelhardt wrote:
>> 
>> On Friday 2015-05-22 10:50, Hannes Frederic Sowa wrote:
>> >
>> >> + if ((nmemb >= MUL_NO_OVERFLOW || size >= MUL_NO_OVERFLOW) &&
>> >
>> >if ((nmemb|size) >= MUL_NO_OVERFLOW) && ...
>> 
>> I am sure there are many C tricks one can do, but iptables is
>> hardly that time-critical to warrant such.
>
>The same can be said of ipset, which uses strlcpy and has strlcat in 
>its library. However, those are safer APIs to use.
>
>In this particular case, it's safer to use reallocarray(NULL,x,y) rather than
>malloc(x*y).

My comment was not about reallocarray–malloc, but about the
not-immediately-self-explanatory expression ((a|b) >= x) which to
me sounds like a Google interview question similar to "what would
(x&~(x-1))==x do".
--
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

Re: Harden iptables memory allocator

[Pablo Neira Ayuso]

On Thu, May 21, 2015 at 06:42:49PM +0000, Loganaden Velvindron wrote:
> Hi All,
> 
> 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@drijf.net>
> + *
> + * 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) &&
> +    nmemb > 0 && SIZE_MAX / nmemb < size) {
> + perror("ip[6]tables: reallocarray failed");
> + exit(1);
> + }
> + return xtables_realloc(optr, size * nmemb);
> +}
> +
>  void *xtables_realloc(void *ptr, size_t size)
>  {
>   void *p;
> @@ -1432,8 +1466,8 @@ void xtables_ipparse_multiple(const char *name,
> struct in_addr **addrpp,
>   ++loop; /* skip ',' */
>   }
> 
> - *addrpp = xtables_malloc(sizeof(struct in_addr) * count);
> - *maskpp = xtables_malloc(sizeof(struct in_addr) * count);
> + *addrpp = xtables_reallocarray(NULL, count, sizeof(struct in_addr));
> + *maskpp = xtables_reallocarray(NULL, count, sizeof(struct in_addr));
> 
>   loop = name;
> 
> @@ -1753,8 +1787,8 @@ xtables_ip6parse_multiple(const char *name,
> struct in6_addr **addrpp,
>   ++loop; /* skip ',' */
>   }
> 
> - *addrpp = xtables_malloc(sizeof(struct in6_addr) * count);
> - *maskpp = xtables_malloc(sizeof(struct in6_addr) * count);
> + *addrpp = xtables_reallocarray(NULL, count, sizeof(struct in6_addr));
> + *maskpp = xtables_reallocarray(NULL, count, sizeof(struct in6_addr));

How feasible is to trigger this overflow in iptables? I'm hitting here
argument list too long before I can trigger this.

I'd rather see an evalution on how this integer overflow can affect
us.

Re: Harden iptables memory allocator

[Loganaden Velvindron]

On Mon, May 25, 2015 at 5:56 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Thu, May 21, 2015 at 06:42:49PM +0000, Loganaden Velvindron wrote:
>> Hi All,
>>
>> 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@drijf.net>
>> + *
>> + * 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) &&
>> +    nmemb > 0 && SIZE_MAX / nmemb < size) {
>> + perror("ip[6]tables: reallocarray failed");
>> + exit(1);
>> + }
>> + return xtables_realloc(optr, size * nmemb);
>> +}
>> +
>>  void *xtables_realloc(void *ptr, size_t size)
>>  {
>>   void *p;
>> @@ -1432,8 +1466,8 @@ void xtables_ipparse_multiple(const char *name,
>> struct in_addr **addrpp,
>>   ++loop; /* skip ',' */
>>   }
>>
>> - *addrpp = xtables_malloc(sizeof(struct in_addr) * count);
>> - *maskpp = xtables_malloc(sizeof(struct in_addr) * count);
>> + *addrpp = xtables_reallocarray(NULL, count, sizeof(struct in_addr));
>> + *maskpp = xtables_reallocarray(NULL, count, sizeof(struct in_addr));
>>
>>   loop = name;
>>
>> @@ -1753,8 +1787,8 @@ xtables_ip6parse_multiple(const char *name,
>> struct in6_addr **addrpp,
>>   ++loop; /* skip ',' */
>>   }
>>
>> - *addrpp = xtables_malloc(sizeof(struct in6_addr) * count);
>> - *maskpp = xtables_malloc(sizeof(struct in6_addr) * count);
>> + *addrpp = xtables_reallocarray(NULL, count, sizeof(struct in6_addr));
>> + *maskpp = xtables_reallocarray(NULL, count, sizeof(struct in6_addr));
>
> How feasible is to trigger this overflow in iptables? I'm hitting here
> argument list too long before I can trigger this.
>

Those were conversions that I identified as cases involving
malloc(x*y), which could be readily changed. Rather than having cases
where malloc(x*y) is used, we can switch to reallocarray(x*y).


> I'd rather see an evalution on how this integer overflow can affect
> us.

Well, it's a safe and easy to use API that can be used instead of the
malloc(x*y).


Are they exploitable ? I'm not really into crafting exploits, but I
welcome an easy to use API that prevents that.

At the very least, having it available in the library, would be a good
thing, when there's a case for a dangerous malloc(x*y).

This is what the Xorg project did:
https://www.freetype.org/patch/46133/

They imported reallocarray() and converted cases of malloc(x*y) into
reallocarray(NULL, x, y).

Re: Harden iptables memory allocator

[Pablo Neira Ayuso]

On Mon, May 25, 2015 at 06:59:01PM +0000, Loganaden Velvindron wrote:
[...]
> >> @@ -1753,8 +1787,8 @@ xtables_ip6parse_multiple(const char *name,
> >> struct in6_addr **addrpp,
> >>   ++loop; /* skip ',' */
> >>   }
> >>
> >> - *addrpp = xtables_malloc(sizeof(struct in6_addr) * count);
> >> - *maskpp = xtables_malloc(sizeof(struct in6_addr) * count);
> >> + *addrpp = xtables_reallocarray(NULL, count, sizeof(struct in6_addr));
> >> + *maskpp = xtables_reallocarray(NULL, count, sizeof(struct in6_addr));
> >
> > How feasible is to trigger this overflow in iptables? I'm hitting here
> > argument list too long before I can trigger this.
> >
> 
> Those were conversions that I identified as cases involving
> malloc(x*y), which could be readily changed. Rather than having cases
> where malloc(x*y) is used, we can switch to reallocarray(x*y).
> 
> 
> > I'd rather see an evalution on how this integer overflow can affect
> > us.
> 
> Well, it's a safe and easy to use API that can be used instead of the
> malloc(x*y).
> 
> 
> Are they exploitable ? I'm not really into crafting exploits, but I
> welcome an easy to use API that prevents that.
>
> At the very least, having it available in the library, would be a good
> thing, when there's a case for a dangerous malloc(x*y).

The only client of this library that I know is iptables, which feeds
this function with an input from the command line.

> This is what the Xorg project did:
> https://www.freetype.org/patch/46133/
> 
> They imported reallocarray() and converted cases of malloc(x*y) into
> reallocarray(NULL, x, y).

That seems good for a public library that is used by third parties,
but this is not the case.

Moreover, we also have more spots that were not converted for some
reason in this patch.

Sorry, this doesn't sound very convincing.

Re: Harden iptables memory allocator

[Loganaden Velvindron]

On Mon, May 25, 2015 at 7:28 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Mon, May 25, 2015 at 06:59:01PM +0000, Loganaden Velvindron wrote:
> [...]
>> >> @@ -1753,8 +1787,8 @@ xtables_ip6parse_multiple(const char *name,
>> >> struct in6_addr **addrpp,
>> >>   ++loop; /* skip ',' */
>> >>   }
>> >>
>> >> - *addrpp = xtables_malloc(sizeof(struct in6_addr) * count);
>> >> - *maskpp = xtables_malloc(sizeof(struct in6_addr) * count);
>> >> + *addrpp = xtables_reallocarray(NULL, count, sizeof(struct in6_addr));
>> >> + *maskpp = xtables_reallocarray(NULL, count, sizeof(struct in6_addr));
>> >
>> > How feasible is to trigger this overflow in iptables? I'm hitting here
>> > argument list too long before I can trigger this.
>> >
>>
>> Those were conversions that I identified as cases involving
>> malloc(x*y), which could be readily changed. Rather than having cases
>> where malloc(x*y) is used, we can switch to reallocarray(x*y).
>>
>>
>> > I'd rather see an evalution on how this integer overflow can affect
>> > us.
>>
>> Well, it's a safe and easy to use API that can be used instead of the
>> malloc(x*y).
>>
>>
>> Are they exploitable ? I'm not really into crafting exploits, but I
>> welcome an easy to use API that prevents that.
>>
>> At the very least, having it available in the library, would be a good
>> thing, when there's a case for a dangerous malloc(x*y).
>
> The only client of this library that I know is iptables, which feeds
> this function with an input from the command line.

What if someone wants to have another client tomorrow  that works differently ?

>
>> This is what the Xorg project did:
>> https://www.freetype.org/patch/46133/
>>
>> They imported reallocarray() and converted cases of malloc(x*y) into
>> reallocarray(NULL, x, y).
>
> That seems good for a public library that is used by third parties,
> but this is not the case.
>
> Moreover, we also have more spots that were not converted for some
> reason in this patch.
>
> Sorry, this doesn't sound very convincing.

My point is that reallocarray() is a more robust & safer API than
malloc(x*y).  Having xtables_reallocarray() would have been a nice
addition, but I understand your point.