ndr: fix ndr_pull_string_array() off by one alloc
authorDouglas Bagnall <douglas.bagnall@catalyst.net.nz>
Tue, 4 Aug 2020 23:04:11 +0000 (11:04 +1200)
committerDouglas Bagnall <dbagnall@samba.org>
Fri, 7 Aug 2020 03:23:44 +0000 (03:23 +0000)
commitbf16cd72b285662bcb4367cd2bdd6eac7655254f
tree96a8333919ab1ddf2c73eeef5a42740f8f678892
parent889c461c009e242fd3ce86b08b87192f21852569
ndr: fix ndr_pull_string_array() off by one alloc

The correct line should have been

       talloc_realloc(ndr->current_mem_ctx, a, const char *, count + 2);

because if the loop does not increment count on exit (it exits via
break), so count is left pointing at the thing that just got put in.
i.e., if there was one item it is at a[0], count is 0, but we also
need the trailing NULL byte at a[1] and the length is 2. Thus + 2, not
+ 1.

This will not affect ordinary (that is, non-malicious) traffic,
because talloc_realloc will not actually realloc unless it is saving a
kilobyte. Since the allocation grows slowly with the exponent ~1.25,
the actual reallocs will start happening at some point between 512 and
1024 items.

In the example we have, there were 666 pointers, and space for 824 was
allocated.

Rather than doing the +2 realloc, it is simpler to leave it off
altogether; in the common case (<512 items) it is a no-op anyway, and
in the best possible case it reduces the temporary array by 20%.

Credit to OSS-Fuzz.

REF: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=24646

Signed-off-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
librpc/ndr/ndr_string.c