Hi Logan, Thanks for looking at that - certainly something to be wary of. I've actually removed netio.c's malloc altogether a couple of weeks ago though had neglected to push it until now https://secure.ucc.asn.au/hg/dropbear/rev/cc6116cc0b5c (my github mirror isn't updated, the machine that does that is offline, should be back next week).
I think netio.c was OK because *ret_iov_count was limited to IOV_MAX which is 1024 or less on most platforms. Cheers, Matt On Sun, Apr 12, 2015 at 08:19:00AM +0000, Loganaden Velvindron wrote: > Hi All, > > 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 > > dropbear is one of the most popular ssh servers for home routers. > > I have conducted a analysis of dropbear's multiple m_malloc instances, > and came up with the following diff: > > http://elandsys.com/~logan/dropbear_reallocarray_c.diff > > Feedback welcomed > //Logan > C-x-C-c
