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>

diff --git a/librpc/ndr/ndr_string.c b/librpc/ndr/ndr_string.c
index bddab9edd517890456779468f4e6ebb2227f1363..77efb3e984816a446bbc252742adfcdf3aa98059 100644 (file)
--- a/librpc/ndr/ndr_string.c
+++ b/librpc/ndr/ndr_string.c
@@ -437,8 +437,6 @@ _PUBLIC_ enum ndr_err_code ndr_pull_string_array(struct ndr_pull *ndr, int ndr_f
                                a[count] = s;
                        }
                }
-               a = talloc_realloc(ndr->current_mem_ctx, a, const char *, count + 1);
-               NDR_ERR_HAVE_NO_MEMORY(a);
 
                *_a =a;
                break;