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 dbd65360eb80dcb46ddefc8938a10c5596083e44..aefe5aae64b9766d6b871c490c000b099bf49472 100644 (file)
--- 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 58b04e983710626ff7f1387d0960348a49379c7c..8a0b072d800d5cb03499be3cb2c8d168848b0c5a 100644 (file)
--- 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 eaeb3b0e0948d197a58ded96e8fcb3adabc36826..031e02a22daf7a53b84ca2c541a7b53b94fad8c3 100644 (file)
--- 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 95adce5a7e3ce7c5b3bd9ac264206b16357bd65c..72c8774ce5c5f69f937bce0df9e4c5caf5dcc946 100644 (file)
--- 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 9b98dd36143bc6c5868d21e6cea4817e4801edd9..c5fa82cdfdef88c599e4058714a86494e5603df2 100644 (file)
--- 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 5927e973c32d6b91b1f10fc0e4cab080a5770933..db992aa9e4729c97a352dfb506d21988c3b84f19 100644 (file)
--- 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 (file)
index 12d5e23..0000000
--- 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