[Dnsmasq-discuss] Hardenining dnsmasq memory allocator against overflows

Simon Kelley simon at thekelleys.org.uk
Sun Apr 12 22:22:45 BST 2015


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 12/04/15 06:57, Loganaden Velvindron wrote:
> Dear dnsmasq developers,
> 
> OpenBSD introduced a new API known as reallocarray():
> 
> If malloc() must be used with multiplication, be sure to test for
> overflow:
> 
> size_t num, size; ...
> 
> /* Check for size_t overflow */ if (size && num > SIZE_MAX / size) 
> errc(1, EOVERFLOW, "overflow");
> 
> if ((p = malloc(size * num)) == NULL) err(1, "malloc");
> 
> The above test is not sufficient in all cases. For example, 
> multiplying ints requires a different set of checks:
> 
> int num, size; ...
> 
> /* Avoid invalid requests */ if (size < 0 || num < 0) errc(1,
> EOVERFLOW, "overflow");
> 
> /* Check for signed int overflow */ if (size && num > INT_MAX /
> size) errc(1, EOVERFLOW, "overflow");
> 
> if ((p = malloc(size * num)) == NULL) err(1, "malloc");
> 
> Assuming the implementation checks for integer overflow as OpenBSD 
> does, it is much easier to use calloc() or reallocarray().
> 
> 
> The dangers of multiplication integer overflows was what caused
> the one of the vulnerabilities in OpenSSH: 
> https://www.owasp.org/index.php/Integer_overflow
> 
> DNSmasq is one of the most popular resolvers, which is often 
> misconfigured to answer requests from the internet: 
> http://openresolverproject.org/version.bind.report.txt (Taken drom 
> Dave Taht's talk at RIPE)
> 
> I have conducted a analysis of dnsmasq's multiple malloc
> instances, and came up with the following diff:
> 
> http://elandsys.com/~logan/dnsmasq_reallocarray.diff
> 
> Feedback welcomed


The principle is a good one, but looking through the patch, on none of
the code that's changed, does either of the multiplication factors
come from an untrusted source, and it's quite easy to demonstrate the
it can never be big enough to overflow, except possibly if
daemon->cachesize is very large. Since daemon->cachesize is a
configuration item, any attacker who can control it has much more
profitable attacks involving other configuration.

Another example:

static void blockdata_expand(int n)

is only called with n as a function of daemon->cachesize or a constant.


The closest to using untrusted data is

static int expand_workspace(unsigned char ***wkspc, int *sz, int new)

but in this case the size is the number of resource records in an
RRset of an incoming (and therefore untrusted) DNS answer. But since a
minimum-size RR is of the order of 10 bytes, and the maximum size of
an answer, even over TCP, is limited to 2^16 bytes, that's not going
to be overflow-able. there's also an explicit, hard coded, limit of
100 for the RRset size, to avoid just such attacks.

The dnsmasq code tries hard to avoid mallocing during execution, and
where it does, it's generally to expand a pool so that no malloc is
needed next time. That means that the sort of code that feeds
untrusted data to malloc doesn't really exist within the codebase.


Cheers,


Simon.




> //Logan C-x-C-c
> 
> _______________________________________________ Dnsmasq-discuss
> mailing list Dnsmasq-discuss at lists.thekelleys.org.uk 
> http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
> 

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)

iEYEARECAAYFAlUq4iUACgkQKPyGmiibgrcbMQCgolFlgTHjjLn54TaCBqPxAhCj
RvcAn2lfDpEbdeQxwOwHtXubfLYPOlrw
=wva8
-----END PGP SIGNATURE-----



More information about the Dnsmasq-discuss mailing list