Total dcerpc response payload more than 0x400000

[Andrew Bartlett]

On Fri, 2016-06-17 at 13:32 -0700, Jeremy Allison wrote:
> 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.

Thanks.  There are some more clues and details from metze in the bug,
which I was finally able to access late yesterday, including where he
got the 4MB number from.
I'll tidy this up and get it back to you however, as 'clearly' it could
cause nasty regressions for some, and we and re-instant the clamp once
we figure out how to do safely.  
If we have to, then 16MB may be a compromise later on, once we add code
to retry DRS replication in this case.  I expect we will need to add a
large-object replication test.
Andrew Bartlett
-- 
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba