Total dcerpc response payload more than 0x400000

[Andrew Bartlett]

On Mon, 2016-06-20 at 22:15 +0200, Stefan Metzmacher wrote:
> Am 17.06.2016 um 23:32 schrieb 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 think using 16 MB should be a good enough fix for now.
> 
> > 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.
> 
> https://msdn.microsoft.com/en-us/library/ms954378.aspx
> indicates the following:
> 
>   5. Attribute Values Must Not Exceed 500KB in Length,
>      and the Total Object Size Must Not Exceed 1MB
> 
>   Active Directory does not support streaming of attribute values
>   and is not appropriate for very large data. For this reason,
>   the maximum length of an attribute value stored in
>   Active Directory must not exceed 500KB in size, and the
>   total object size must not exceed 1MB.

Except that Samba has never honoured such a restriction, nor has any
code to prevent the server-side sending large responses, nor the client
side to retry asking for smaller responses. 

A 4MB and even a 16MB limit is still small enough to break networks in
frustrating, incredibly difficult to debug ways that *will* only happen
at large, complex installations, when it breaks replication.  

Given all that, and that even if we fix all the above we will have old
versions as replication partners, I strongly request we remove the
restriction for DRS replication. 

Thanks,

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