Harden iptables memory allocator

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@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) &&
+    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;


:
--
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




[Index of Archives]     [Netfitler Users]     [LARTC]     [Bugtraq]     [Yosemite Forum]

  Powered by Linux