[PATCH] Fix undefined behavior in NDR

[Stefan Metzmacher]

Hi Andreas,

> From 1dfc0522ee8bf173c6d02625cac7e0a5297b18f6 Mon Sep 17 00:00:00 2001
> From: Andreas Schneider <asn at samba.org>
> Date: Thu, 22 Nov 2018 15:15:03 +0100
> Subject: [PATCH] librpc:ndr: Fix undefined behavior in ndr_basic
> 
> librpc/ndr/ndr_basic.c:723:2: runtime error: null pointer passed as
> argument 2, which is declared to never be null
> 
> The following triggered the undefined behavior:
> 
> (gdb) bt
>     at librpc/gen_ndr/ndr_drsuapi.c:2318
>     fn=0x7ffff6e72983 <ndr_push_drsuapi_DsReplicaObjectIdentifier3Binary>) at ../../librpc/ndr/ndr.c:1337
>     at ../../source4/dsdb/schema/schema_syntax.c:2136
>     drs_str=<optimized out>) at ../../source4/dsdb/schema/tests/schema_syntax.c:122
>     already_setup=<optimized out>, restricted=restricted at entry=0x0) at ../../lib/torture/torture.c:442
>     at ../../lib/torture/torture.c:507
>     suite=0x5555563d9490, matched=0x7fffffffcef7) at ../../source4/torture/smbtorture.c:93
>     matched=0x7fffffffcef7) at ../../source4/torture/smbtorture.c:95
>     at ../../source4/torture/smbtorture.c:143
> (gdb) f 1
> 1335            NDR_CHECK(ndr_push_bytes(ndr, blob.data, blob.length));
> (gdb) p blob
> $2 = {data = 0x0, length = 0}
> 
> Signed-off-by: Andreas Schneider <asn at samba.org>
> ---
>  librpc/ndr/ndr_basic.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/librpc/ndr/ndr_basic.c b/librpc/ndr/ndr_basic.c
> index c874f340388..b488c7c22d9 100644
> --- a/librpc/ndr/ndr_basic.c
> +++ b/librpc/ndr/ndr_basic.c
> @@ -719,6 +719,12 @@ _PUBLIC_ enum ndr_err_code ndr_pull_trailer_align(struct ndr_pull *ndr, size_t s
>  */
>  _PUBLIC_ enum ndr_err_code ndr_push_bytes(struct ndr_push *ndr, const uint8_t *data, uint32_t n)
>  {
> +	if (n == 0) {
> +		return NDR_ERR_SUCCESS;
> +	}
> +	if (data == NULL) {
> +		return NDR_ERR_INVALID_POINTER;
> +	}
>
>  	NDR_PUSH_NEED_BYTES(ndr, n);
>  	memcpy(ndr->data + ndr->offset, data, n);
>  	ndr->offset += n;
> @@ -1329,6 +1335,10 @@ _PUBLIC_ enum ndr_err_code ndr_push_DATA_BLOB(struct ndr_push *ndr, int ndr_flag
>  	} else {
>  		NDR_CHECK(ndr_push_uint3264(ndr, NDR_SCALARS, blob.length));
>  	}
> +	if (blob.length == 0) {
> +		return NDR_ERR_SUCCESS;
> +	}
> +
>  	NDR_CHECK(ndr_push_bytes(ndr, blob.data, blob.length));
>  	return NDR_ERR_SUCCESS;
>  }

Why do we need this if we have checks in ndr_push_bytes()?

Adding the above two checks in ndr_push_bytes() might also be very
costly. Isn't the first check enough? It should also use unlikely()

metze

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20181205/9fadd311/signature.sig>