Total dcerpc response payload more than 0x400000
Stefan Metzmacher
metze at samba.org
Wed Jun 22 18:37:30 UTC 2016
Am 22.06.2016 um 00:49 schrieb 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.
What about the attached patches?
I'll do an autobuild with this later.
metze
metze
-------------- next part --------------
From 98be260b376e08462d4d0dccb58212ee783a63cb Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Wed, 22 Jun 2016 16:58:03 +0200
Subject: [PATCH 1/3] dcerpc.idl: add
DCERPC_NCACN_{REQUEST,RESPONSE}_DEFAULT_MAX_SIZE
This will replace DCERPC_NCACN_PAYLOAD_MAX_SIZE (4 MByte),
this limit is too strict for some workloads, e.g. DRSUAPI replication
with large objects.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=11948
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
librpc/idl/dcerpc.idl | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/librpc/idl/dcerpc.idl b/librpc/idl/dcerpc.idl
index 015eb3d..5e0f919 100644
--- a/librpc/idl/dcerpc.idl
+++ b/librpc/idl/dcerpc.idl
@@ -537,6 +537,23 @@ interface dcerpc
const uint8 DCERPC_NCACN_PAYLOAD_OFFSET = 16;
const uint32 DCERPC_NCACN_PAYLOAD_MAX_SIZE = 0x400000; /* 4 MByte */
+ /*
+ * See [MS-RPCE] 3.3.3.5.4 Maximum Server Input Data Size
+ * 4 MByte is the default limit of reassembled request payload
+ */
+ const uint32 DCERPC_NCACN_REQUEST_DEFAULT_MAX_SIZE = 0x400000;
+
+ /*
+ * See [MS-RPCE] 3.3.2.5.2 Handling Responses
+ *
+ * Indicates that Windows accepts up to 0x7FFFFFFF ~2 GByte
+ *
+ * talloc has a limit of 256 MByte, so we need to use something smaller.
+ *
+ * For now we try our luck with 240 MByte.
+ */
+ const uint32 DCERPC_NCACN_RESPONSE_DEFAULT_MAX_SIZE = 0xf000000; /* 240 MByte */
+
/* little-endian flag */
const uint8 DCERPC_DREP_LE = 0x10;
--
1.9.1
From 0180642abb1b8cd0bc03078b69ccd11ae0919802 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Wed, 22 Jun 2016 17:18:28 +0200
Subject: [PATCH 2/3] s4:librpc/rpc: allow a total reassembled response payload
of 240 MBytes
This will replace DCERPC_NCACN_PAYLOAD_MAX_SIZE (4 MByte),
The limit of DCERPC_NCACN_PAYLOAD_MAX_SIZE (4 MByte) was too
strict for some workloads, e.g. DRSUAPI replication with large objects.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=11948
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
source4/librpc/rpc/dcerpc.c | 5 +++--
source4/librpc/rpc/dcerpc.h | 3 +++
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/source4/librpc/rpc/dcerpc.c b/source4/librpc/rpc/dcerpc.c
index 464ae95..55b4385 100644
--- a/source4/librpc/rpc/dcerpc.c
+++ b/source4/librpc/rpc/dcerpc.c
@@ -155,6 +155,7 @@ static struct dcecli_connection *dcerpc_connection_init(TALLOC_CTX *mem_ctx,
*/
c->srv_max_xmit_frag = 5840;
c->srv_max_recv_frag = 5840;
+ c->max_total_response_size = DCERPC_NCACN_RESPONSE_DEFAULT_MAX_SIZE;
c->pending = NULL;
c->io_trigger = tevent_create_immediate(c);
@@ -1577,10 +1578,10 @@ 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 > c->max_total_response_size) {
DEBUG(2,("Unexpected total payload 0x%X > 0x%X dcerpc response\n",
(unsigned)req->payload.length + length,
- DCERPC_NCACN_PAYLOAD_MAX_SIZE));
+ (unsigned)c->max_total_response_size));
dcerpc_connection_dead(c, NT_STATUS_RPC_PROTOCOL_ERROR);
return;
}
diff --git a/source4/librpc/rpc/dcerpc.h b/source4/librpc/rpc/dcerpc.h
index 39d28a6..24c7948 100644
--- a/source4/librpc/rpc/dcerpc.h
+++ b/source4/librpc/rpc/dcerpc.h
@@ -107,6 +107,9 @@ struct dcecli_connection {
/* the next context_id to be assigned */
uint32_t next_context_id;
+
+ /* The maximum total payload of reassembled response pdus */
+ size_t max_total_response_size;
};
/*
--
1.9.1
From 87930f2b89c1c3c19c1a6fc4e0157f832741d630 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Wed, 22 Jun 2016 17:18:28 +0200
Subject: [PATCH 3/3] s4:rpc_server: use a variable for the max total
reassembled request payload
We still use the same limit of 4 MByte (DCERPC_NCACN_REQUEST_DEFAULT_MAX_SIZE)
by default.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=11948
Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
source4/rpc_server/dcerpc_server.c | 5 +++--
source4/rpc_server/dcerpc_server.h | 3 +++
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/source4/rpc_server/dcerpc_server.c b/source4/rpc_server/dcerpc_server.c
index 36b3fd2..025cb20 100644
--- a/source4/rpc_server/dcerpc_server.c
+++ b/source4/rpc_server/dcerpc_server.c
@@ -408,6 +408,7 @@ _PUBLIC_ NTSTATUS dcesrv_endpoint_connect(struct dcesrv_context *dce_ctx,
p->allow_bind = true;
p->max_recv_frag = 5840;
p->max_xmit_frag = 5840;
+ p->max_total_request_size = DCERPC_NCACN_REQUEST_DEFAULT_MAX_SIZE;
*_p = p;
return NT_STATUS_OK;
@@ -1532,7 +1533,7 @@ static NTSTATUS dcesrv_process_ncacn_packet(struct dcesrv_connection *dce_conn,
/*
* Up to 4 MByte are allowed by all fragments
*/
- available = DCERPC_NCACN_PAYLOAD_MAX_SIZE;
+ available = dce_conn->max_total_request_size;
if (er->stub_and_verifier.length > available) {
dcesrv_call_disconnect_after(existing,
"dcesrv_auth_request - existing payload too large");
@@ -1585,7 +1586,7 @@ static NTSTATUS dcesrv_process_ncacn_packet(struct dcesrv_connection *dce_conn,
/*
* Up to 4 MByte are allowed by all fragments
*/
- if (call->pkt.u.request.alloc_hint > DCERPC_NCACN_PAYLOAD_MAX_SIZE) {
+ if (call->pkt.u.request.alloc_hint > dce_conn->max_total_request_size) {
dcesrv_call_disconnect_after(call,
"dcesrv_auth_request - initial alloc hint too large");
return dcesrv_fault(call, DCERPC_FAULT_ACCESS_DENIED);
diff --git a/source4/rpc_server/dcerpc_server.h b/source4/rpc_server/dcerpc_server.h
index aead405e..54187ee 100644
--- a/source4/rpc_server/dcerpc_server.h
+++ b/source4/rpc_server/dcerpc_server.h
@@ -278,6 +278,9 @@ struct dcesrv_connection {
/* the association group the connection belongs to */
struct dcesrv_assoc_group *assoc_group;
+
+ /* The maximum total payload of reassembled request pdus */
+ size_t max_total_request_size;
};
--
1.9.1
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20160622/a25e22cc/signature.sig>
More information about the samba-technical
mailing list