From: Douglas Bagnall Date: Tue, 4 Aug 2020 23:04:11 +0000 (+1200) Subject: ndr: fix ndr_pull_string_array() off by one alloc X-Git-Tag: talloc-2.3.2~902 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=bf16cd72b285662bcb4367cd2bdd6eac7655254f;p=thirdparty%2Fsamba.git 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 Reviewed-by: Andrew Bartlett --- diff --git a/librpc/ndr/ndr_string.c b/librpc/ndr/ndr_string.c index bddab9edd51..77efb3e9848 100644 --- 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;