Total dcerpc response payload more than 0x400000

[Jeremy Allison]

On Fri, Jun 17, 2016 at 01:11:14PM +1200, Andrew Bartlett wrote:
> 
> I totally agree.  Thanks for filing the bug about this, as this is a
> serious regression in the DRS replication case.
> 
>  https://bugzilla.samba.org/show_bug.cgi?id=11948
> 
> What happens here is that we stop being able to replicate in any domain
> where an object is more than 4MB, or indeed any domains where each
> object is more than 10k on average, which will break on say a domain
> with a jpegPhoto.  Indeed, a jpegPhoto of 4MB is not unreasonable to
> expect, given the default from a camera these days.
> 
> We need to remove this limit, or change it to more like 240MB (talloc
> won't allocate larger the 256MB at the moment). 
> 
> The limit makes sense thought of from the context of simple
> administrative calls, but it is also used for bulk data transport,
> server->client.  I've not changed the client->server direction, but I
> have heard of spoolss calls where the windows client would send over
> 16MB of zeros in a in/out buffer, so we may need to fix that also. 
> 
> Also, at the point when we get this, if the connection is
> authenticated, then the authentication has been checked, and the packet
> is already in memory, so we can be generous here.
> 
> Metze: Can you please ACK this change?

Metze is away on vacation at the moment, but no one person's
absence should prevent bugs getting fixed :-).

Currently this patch checks:

 +     if (req->payload.length + length > DCERPC_NCACN_CLIENT_PAYLOAD_MAX_SIZE) {

then checks:

 +	if (length > DCERPC_NCACN_CLIENT_PAYLOAD_MAX_SIZE) {

I think that should be the other way around - the length check
can never be true if the first check isn't true.

Can you change the patch to check the following 3 things in order:

1). Current packet length check.

 +      if (length > DCERPC_NCACN_CLIENT_PAYLOAD_MAX_SIZE) {

2). Integer wrap check.

 +      if (req->payload.length + length < length) {

3). Combination check.

 +     if (req->payload.length + length > DCERPC_NCACN_CLIENT_PAYLOAD_MAX_SIZE) {

Then I'll re-review. If metze really hates this he can
fix it when he gets back :-) - plus I'll ask dochelp next
week when I'm up at Microsoft about the limits Windows
has on RPC packet reassembly.

Cheers,

	Jeremy.

> -- 
> Andrew Bartlett
> https://samba.org/~abartlet/
> Authentication Developer, Samba Team         https://samba.org
> Samba Development and Support, Catalyst IT   
> https://catalyst.net.nz/services/samba
> 
> 
> 
> 
> 

> From 4409c098cba89bcf8dca74e4c6eb15d3fdaac728 Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <abartlet at samba.org>
> Date: Fri, 17 Jun 2016 12:14:11 +1200
> Subject: [PATCH] s4-librpc: Change the RPC client lib to permit larger RPC
>  replies
> 
> 4MB is too small and is an arbitary limit that prevents DRS replication
> in domains with either large objects, or simply a large number of
> reasonablly sized objects (eg 10kb per object in our default page
> size of 400).  This could be a jpegPhoto for example.
> 
> Signed-off-by: Andrew Bartlett <abartlet at samba.org>
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=11948
> ---
>  librpc/idl/dcerpc.idl       |  1 +
>  source4/librpc/rpc/dcerpc.c | 12 ++++++++++--
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/librpc/idl/dcerpc.idl b/librpc/idl/dcerpc.idl
> index 015eb3d..9b68040 100644
> --- a/librpc/idl/dcerpc.idl
> +++ b/librpc/idl/dcerpc.idl
> @@ -536,6 +536,7 @@ interface dcerpc
>  	const uint8 DCERPC_AUTH_LEN_OFFSET = 10;
>  	const uint8 DCERPC_NCACN_PAYLOAD_OFFSET = 16;
>  	const uint32 DCERPC_NCACN_PAYLOAD_MAX_SIZE = 0x400000; /* 4 MByte */
> +	const uint32 DCERPC_NCACN_CLIENT_PAYLOAD_MAX_SIZE = 0xf000000; /* 240 MByte */
>  
>  	/* little-endian flag */
>  	const uint8 DCERPC_DREP_LE  = 0x10;
> diff --git a/source4/librpc/rpc/dcerpc.c b/source4/librpc/rpc/dcerpc.c
> index 464ae95..bc8a580 100644
> --- a/source4/librpc/rpc/dcerpc.c
> +++ b/source4/librpc/rpc/dcerpc.c
> @@ -1577,10 +1577,18 @@ static void dcerpc_request_recv_data(struct dcecli_connection *c,
>  
>  	length = pkt->u.response.stub_and_verifier.length;
>  
> -	if (req->payload.length + length > DCERPC_NCACN_PAYLOAD_MAX_SIZE) {
> +	if (req->payload.length + length > DCERPC_NCACN_CLIENT_PAYLOAD_MAX_SIZE) {
>  		DEBUG(2,("Unexpected total payload 0x%X > 0x%X dcerpc response\n",
>  			 (unsigned)req->payload.length + length,
> -			 DCERPC_NCACN_PAYLOAD_MAX_SIZE));
> +			 DCERPC_NCACN_CLIENT_PAYLOAD_MAX_SIZE));
> +		dcerpc_connection_dead(c, NT_STATUS_RPC_PROTOCOL_ERROR);
> +		return;
> +	}
> +
> +	if (length > DCERPC_NCACN_CLIENT_PAYLOAD_MAX_SIZE) {
> +		DEBUG(2,("Unexpected stub and verifier 0x%X > 0x%X dcerpc response\n",
> +			 (unsigned)length,
> +			 DCERPC_NCACN_CLIENT_PAYLOAD_MAX_SIZE));
>  		dcerpc_connection_dead(c, NT_STATUS_RPC_PROTOCOL_ERROR);
>  		return;
>  	}
> -- 
> 2.8.0.rc3
>