From: Andrew Bartlett <abartlet@samba.org>
Date: Sat, 22 May 2021 06:40:13 +0000 (+1200)
Subject: pidl: Avoid leaving array_size NDR tokens around
X-Git-Tag: tevent-0.11.0~688
X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=3bc680c1e38bef75d5b212992e15f094c523923b;p=thirdparty%2Fsamba.git

pidl: Avoid leaving array_size NDR tokens around

In many cases these can and should be consumed as soon as
they are used.

This is not a complete fix, we don't clean up the array_size
token after using it split between an NDR_SCALARS and
an NDR_BUFFERS pass, but it is much better than it was
and helps the winbind case with a large number of groups
(eg 100,000) as otherwise we hit the 65535 NDR token limit.

(This is an arbitary Samba-only limit to avoid DoS conditions)

BUG: https://bugzilla.samba.org/show_bug.cgi?id=14710

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

diff --git a/librpc/ABI/ndr-2.0.0.sigs b/librpc/ABI/ndr-2.0.0.sigs
index dbd65360eb8..aefe5aae64b 100644
--- a/librpc/ABI/ndr-2.0.0.sigs
+++ b/librpc/ABI/ndr-2.0.0.sigs
@@ -20,6 +20,7 @@ ndr_check_array_size: enum ndr_err_code (struct ndr_pull *, const void *, uint32
 ndr_check_padding: void (struct ndr_pull *, size_t)
 ndr_check_pipe_chunk_trailer: enum ndr_err_code (struct ndr_pull *, int, uint32_t)
 ndr_check_steal_array_length: enum ndr_err_code (struct ndr_pull *, const void *, uint32_t)
+ndr_check_steal_array_size: enum ndr_err_code (struct ndr_pull *, const void *, uint32_t)
 ndr_check_string_terminator: enum ndr_err_code (struct ndr_pull *, uint32_t, uint32_t)
 ndr_get_array_length: enum ndr_err_code (struct ndr_pull *, const void *, uint32_t *)
 ndr_get_array_size: enum ndr_err_code (struct ndr_pull *, const void *, uint32_t *)
@@ -250,6 +251,7 @@ ndr_size_struct: size_t (const void *, int, ndr_push_flags_fn_t)
 ndr_size_union: size_t (const void *, int, uint32_t, ndr_push_flags_fn_t)
 ndr_size_winreg_Data_GPO: size_t (const union winreg_Data_GPO *, uint32_t, int)
 ndr_steal_array_length: enum ndr_err_code (struct ndr_pull *, const void *, uint32_t *)
+ndr_steal_array_size: enum ndr_err_code (struct ndr_pull *, const void *, uint32_t *)
 ndr_string_array_size: size_t (struct ndr_push *, const char *)
 ndr_string_length: uint32_t (const void *, uint32_t)
 ndr_syntax_id_buf_string: char *(const struct ndr_syntax_id *, struct ndr_syntax_id_buf *)
diff --git a/librpc/ndr/libndr.h b/librpc/ndr/libndr.h
index 58b04e98371..8a0b072d800 100644
--- a/librpc/ndr/libndr.h
+++ b/librpc/ndr/libndr.h
@@ -656,7 +656,9 @@ enum ndr_err_code ndr_token_retrieve(struct ndr_token_list *list, const void *ke
 enum ndr_err_code ndr_token_peek(struct ndr_token_list *list, const void *key, uint32_t *v);
 enum ndr_err_code ndr_pull_array_size(struct ndr_pull *ndr, const void *p);
 enum ndr_err_code ndr_get_array_size(struct ndr_pull *ndr, const void *p, uint32_t *size);
+enum ndr_err_code ndr_steal_array_size(struct ndr_pull *ndr, const void *p, uint32_t *size);
 enum ndr_err_code ndr_check_array_size(struct ndr_pull *ndr, const void *p, uint32_t size);
+enum ndr_err_code ndr_check_steal_array_size(struct ndr_pull *ndr, const void *p, uint32_t size);
 enum ndr_err_code ndr_pull_array_length(struct ndr_pull *ndr, const void *p);
 enum ndr_err_code ndr_get_array_length(struct ndr_pull *ndr, const void *p, uint32_t *length);
 enum ndr_err_code ndr_steal_array_length(struct ndr_pull *ndr, const void *p, uint32_t *length);
diff --git a/librpc/ndr/ndr.c b/librpc/ndr/ndr.c
index eaeb3b0e094..031e02a22da 100644
--- a/librpc/ndr/ndr.c
+++ b/librpc/ndr/ndr.c
@@ -1098,8 +1098,34 @@ _PUBLIC_ enum ndr_err_code ndr_get_array_size(struct ndr_pull *ndr, const void *
 }
 
 /*
-  check the stored array size field
+  get and remove from the stored list the stored array size field
 */
+_PUBLIC_ enum ndr_err_code ndr_steal_array_size(struct ndr_pull *ndr, const void *p, uint32_t *size)
+{
+	return ndr_token_retrieve(&ndr->array_size_list, p, size);
+}
+
+/*
+ * check the stored array size field and remove from the stored list
+ * (the array_size NDR token list).  We try to remove when possible to
+ * avoid the list growing towards the bounds check
+ */
+_PUBLIC_ enum ndr_err_code ndr_check_steal_array_size(struct ndr_pull *ndr, const void *p, uint32_t size)
+{
+	uint32_t stored;
+	NDR_CHECK(ndr_steal_array_size(ndr, p, &stored));
+	if (stored != size) {
+		return ndr_pull_error(ndr, NDR_ERR_ARRAY_SIZE,
+				      "Bad array size - got %u expected %u\n",
+				      stored, size);
+	}
+	return NDR_ERR_SUCCESS;
+}
+
+/*
+ * check the stored array size field (leaving it on the array_size
+ * token list)
+ */
 _PUBLIC_ enum ndr_err_code ndr_check_array_size(struct ndr_pull *ndr, const void *p, uint32_t size)
 {
 	uint32_t stored;
diff --git a/librpc/ndr/ndr_negoex.c b/librpc/ndr/ndr_negoex.c
index 95adce5a7e3..72c8774ce5c 100644
--- a/librpc/ndr/ndr_negoex.c
+++ b/librpc/ndr/ndr_negoex.c
@@ -99,7 +99,7 @@ enum ndr_err_code ndr_pull_negoex_BYTE_VECTOR(struct ndr_pull *ndr, int ndr_flag
 		}
 #if 0
 		if (r->blob.data) {
-			NDR_CHECK(ndr_check_array_size(ndr, (void*)&r->blob.data, r->blob.length));
+			NDR_CHECK(ndr_check_steal_array_size(ndr, (void*)&r->blob.data, r->blob.length));
 		}
 #endif
 	}
@@ -179,7 +179,7 @@ enum ndr_err_code ndr_pull_negoex_AUTH_SCHEME_VECTOR(struct ndr_pull *ndr, int n
 		}
 #if 0
 		if (r->array) {
-			NDR_CHECK(ndr_check_array_size(ndr, (void*)&r->array, r->count));
+			NDR_CHECK(ndr_check_steal_array_size(ndr, (void*)&r->array, r->count));
 		}
 #endif
 	}
@@ -265,7 +265,7 @@ enum ndr_err_code ndr_pull_negoex_EXTENSION_VECTOR(struct ndr_pull *ndr, int ndr
 		}
 #if 0
 		if (r->array) {
-			NDR_CHECK(ndr_check_array_size(ndr, (void*)&r->array, r->count));
+			NDR_CHECK(ndr_check_steal_array_size(ndr, (void*)&r->array, r->count));
 		}
 #endif
 	}
@@ -351,7 +351,7 @@ enum ndr_err_code ndr_pull_negoex_ALERT_VECTOR(struct ndr_pull *ndr, int ndr_fla
 		}
 #if 0
 		if (r->array) {
-			NDR_CHECK(ndr_check_array_size(ndr, (void*)&r->array, r->count));
+			NDR_CHECK(ndr_check_steal_array_size(ndr, (void*)&r->array, r->count));
 		}
 #endif
 	}
diff --git a/librpc/ndr/ndr_spoolss_buf.c b/librpc/ndr/ndr_spoolss_buf.c
index 9b98dd36143..c5fa82cdfde 100644
--- a/librpc/ndr/ndr_spoolss_buf.c
+++ b/librpc/ndr/ndr_spoolss_buf.c
@@ -1227,7 +1227,7 @@ _PUBLIC_ enum ndr_err_code ndr_pull_spoolss_DriverInfo101(struct ndr_pull *ndr,
 			ndr->flags = _flags_save_string;
 		}
 		if (r->file_info) {
-			NDR_CHECK(ndr_check_array_size(ndr, (void*)&r->file_info, r->file_count));
+			NDR_CHECK(ndr_check_steal_array_size(ndr, (void*)&r->file_info, r->file_count));
 		}
 	}
 	return NDR_ERR_SUCCESS;
diff --git a/pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm b/pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm
index 5927e973c32..db992aa9e47 100644
--- a/pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm
+++ b/pidl/lib/Parse/Pidl/Samba4/NDR/Parser.pm
@@ -342,11 +342,17 @@ sub ParseArrayPullGetSize($$$$$$)
 
 	my $array_size = "size_$e->{NAME}_$l->{LEVEL_INDEX}";
 
-	if ($l->{IS_CONFORMANT}) {
+	if ($l->{IS_CONFORMANT} and (defined($l->{SIZE_IS}) or not $l->{IS_ZERO_TERMINATED})) {
 		$self->pidl("NDR_CHECK(ndr_get_array_size($ndr, (void*)" . get_pointer_to($var_name) . ", &$array_size));");
+
+	} elsif ($l->{IS_CONFORMANT}) {
+		# This will be the last use of the array_size token
+		$self->pidl("NDR_CHECK(ndr_steal_array_size($ndr, (void*)" . get_pointer_to($var_name) . ", &$array_size));");
+
 	} elsif ($l->{IS_ZERO_TERMINATED} and $l->{SIZE_IS} == 0 and $l->{LENGTH_IS} == 0) { # Noheader arrays
 		$size = "ndr_get_string_size($ndr, sizeof(*$var_name))";
 		$self->pidl("$array_size = $size;");
+
 	} else {
 		$size = ParseExprExt($l->{SIZE_IS}, $env, $e->{ORIGINAL},
 			check_null_pointer($e, $env, sub { $self->pidl(shift); },
@@ -443,7 +449,14 @@ sub ParseArrayPullHeader($$$$$$)
 			check_null_pointer($e, $env, sub { $self->defer(shift); },
 					   "return ndr_pull_error($ndr, NDR_ERR_INVALID_POINTER, \"NULL Pointer for size_is()\");"),
 			check_fully_dereferenced($e, $env));
-		$self->defer("NDR_CHECK(ndr_check_array_size($ndr, (void*)" . get_pointer_to($var_name) . ", $size));");
+		if (ContainsDeferred($e, $l)) {
+			# We will be needing the array_size token in
+			# the NDR_BUFFERS call, so don't steal it now
+			$self->defer("NDR_CHECK(ndr_check_array_size($ndr, (void*)" . get_pointer_to($var_name) . ", $size));");
+		} else {
+			# This will be deferred until after the last ndr_get_array_size()
+			$self->defer("NDR_CHECK(ndr_check_steal_array_size($ndr, (void*)" . get_pointer_to($var_name) . ", $size));");
+		}
 		$self->defer_deindent;
 		$self->defer("}");
 	}
diff --git a/selftest/knownfail.d/ndr b/selftest/knownfail.d/ndr
deleted file mode 100644
index 12d5e23c57e..00000000000
--- a/selftest/knownfail.d/ndr
+++ /dev/null
@@ -1 +0,0 @@
-^samba.tests.ndr.samba.tests.ndr.NdrTestCase.test_wbint_max_token_Principals
\ No newline at end of file