On Mon, May 25, 2015 at 7:28 PM, Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> 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. -- 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
