Attachment 11058 Details for Bug 11277 – Patch and torture test

[patch] Patch and torture test

compound-response.patch (text/plain), 10.17 KB, created by Ralph Böhme on 2015-05-17 16:49:19 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Ralph Böhme

Created: 2015-05-17 16:49:19 UTC

Size: 10.17 KB

patch

obsolete

>From 26b16091f99a5ae17ff53b9fcdc7972a43cbabc1 Mon Sep 17 00:00:00 2001
>From: Ralph Boehme <slow@samba.org>
>Date: Wed, 13 May 2015 21:01:08 +0200
>Subject: [PATCH 1/2] s3:smb2: add padding to last command in compound requests
>
>The last command in a compound request must be padded to an 8 byte
>boundary. According to [MS-SMB2] 3.3.4.1.3, "Sending Compounded
>Responses", this shouldn't be necessary, but OS X clients crash hard if
>we don't and this is also what Windows 2012 server does.
>
>Bug: https://bugzilla.samba.org/show_bug.cgi?id=11277
>
>Signed-off-by: Ralph Boehme <slow@samba.org>
>---
> source3/smbd/smb2_server.c | 57 ++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 47 insertions(+), 10 deletions(-)
>
>diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c
>index 9e5eff7..371294c 100644
>--- a/source3/smbd/smb2_server.c
>+++ b/source3/smbd/smb2_server.c
>@@ -2619,7 +2619,9 @@ NTSTATUS smbd_smb2_request_done_ex(struct smbd_smb2_request *req,
> 	uint8_t *outhdr;
> 	struct iovec *outbody_v;
> 	struct iovec *outdyn_v;
>-	uint32_t next_command_ofs;
>+	uint32_t next_command_body_ofs, tmp_offset, next_command_ofs = 0;
>+	uint16_t flags;
>+	size_t pad_size = 0;
> 
> 	DEBUG(10,("smbd_smb2_request_done_ex: "
> 		  "idx[%d] status[%s] body[%u] dyn[%s:%u] at %s\n",
>@@ -2640,7 +2642,8 @@ NTSTATUS smbd_smb2_request_done_ex(struct smbd_smb2_request *req,
> 	outbody_v = SMBD_SMB2_OUT_BODY_IOV(req);
> 	outdyn_v = SMBD_SMB2_OUT_DYN_IOV(req);
> 
>-	next_command_ofs = IVAL(outhdr, SMB2_HDR_NEXT_COMMAND);
>+	flags = SVAL(outhdr, SMB2_HDR_FLAGS);
>+	next_command_body_ofs = IVAL(outhdr, SMB2_HDR_NEXT_COMMAND);
> 	SIVAL(outhdr, SMB2_HDR_STATUS, NT_STATUS_V(status));
> 
> 	outbody_v->iov_base = (void *)body.data;
>@@ -2654,15 +2657,41 @@ NTSTATUS smbd_smb2_request_done_ex(struct smbd_smb2_request *req,
> 		outdyn_v->iov_len = 0;
> 	}
> 
>-	/* see if we need to recalculate the offset to the next response */
>-	if (next_command_ofs > 0) {
>-		next_command_ofs  = SMB2_HDR_BODY;
>-		next_command_ofs += SMBD_SMB2_OUT_BODY_LEN(req);
>-		next_command_ofs += SMBD_SMB2_OUT_DYN_LEN(req);
>+	/*
>+	 * See if we need to recalculate the offset to the next
>+	 * response, or add padding for the last request in a compound
>+	 * chain.
>+	 */
>+
>+	if ((next_command_body_ofs > 0) ||
>+	    ((next_command_body_ofs == 0) && (flags & SMB2_HDR_FLAG_CHAINED))) {
>+		tmp_offset  = SMB2_HDR_BODY;
>+		tmp_offset += SMBD_SMB2_OUT_BODY_LEN(req);
>+		tmp_offset += SMBD_SMB2_OUT_DYN_LEN(req);
>+
>+		if (next_command_body_ofs > 0) {
>+			/*
>+			 * We have a next compound, use calculated
>+			 * offset and add padding.
>+			 */
>+			next_command_ofs = tmp_offset;
>+			pad_size = 8 - (tmp_offset % 8);
>+		} else if (flags & SMB2_HDR_FLAG_CHAINED) {
>+			/*
>+			 * Last command (next_command_body_ofs == 0)
>+			 * in a compound request, add padding to 8
>+			 * byte boundary if necessary.  According to
>+			 * [MS-SMB2] 3.3.4.1.3, Sending Compounded
>+			 * Responses this, this shouldn't be
>+			 * necessary, but OS X client crash hard if we
>+			 * don't and this is also what Windows 2012
>+			 * server does.
>+			 */
>+			pad_size = 8 - (tmp_offset % 8);
>+		}
> 	}
> 
>-	if ((next_command_ofs % 8) != 0) {
>-		size_t pad_size = 8 - (next_command_ofs % 8);
>+	if (pad_size > 0) {
> 		if (SMBD_SMB2_OUT_DYN_LEN(req) == 0) {
> 			/*
> 			 * if the dyn buffer is empty
>@@ -2706,7 +2735,15 @@ NTSTATUS smbd_smb2_request_done_ex(struct smbd_smb2_request *req,
> 			outdyn_v->iov_base = (void *)new_dyn;
> 			outdyn_v->iov_len = new_size;
> 		}
>-		next_command_ofs += pad_size;
>+		if (next_command_body_ofs > 0) {
>+			/*
>+			 * The last command in a compound chain will
>+			 * have a next_command_body_ofs of 0, we don't
>+			 * want to add the padding in that case, we
>+			 * want to return 0 as next command offset.
>+			 */
>+			next_command_ofs += pad_size;
>+		}
> 	}
> 
> 	SIVAL(outhdr, SMB2_HDR_NEXT_COMMAND, next_command_ofs);
>-- 
>2.1.0
>
>
>From 82a214f105dacc117d6180beaec670e624541142 Mon Sep 17 00:00:00 2001
>From: Ralph Boehme <slow@samba.org>
>Date: Thu, 14 May 2015 04:27:54 +0200
>Subject: [PATCH 2/2] s4:torture:smb2:compound: compound read and padding
>
>Add test to check that compound read responses are padded to an 8 byte
>boundary.
>
>Bug: https://bugzilla.samba.org/show_bug.cgi?id=11277
>
>Signed-off-by: Ralph Boehme <slow@samba.org>
>---
> source4/torture/smb2/compound.c | 168 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 168 insertions(+)
>
>diff --git a/source4/torture/smb2/compound.c b/source4/torture/smb2/compound.c
>index 9b3cacc..692af09 100644
>--- a/source4/torture/smb2/compound.c
>+++ b/source4/torture/smb2/compound.c
>@@ -34,6 +34,14 @@
> 		goto done; \
> 	}} while (0)
> 
>+#define CHECK_VALUE(v, correct) do { \
>+	if ((v) != (correct)) { \
>+		torture_result(tctx, TORTURE_FAIL, \
>+		    "(%s) Incorrect value %s=%d - should be %d\n", \
>+		    __location__, #v, (int)v, (int)correct); \
>+		ret = false; \
>+	}} while (0)
>+
> static struct {
> 	struct smb2_handle handle;
> 	uint8_t level;
>@@ -433,6 +441,165 @@ done:
> 	return ret;
> }
> 
>+static bool test_compound_read(struct torture_context *tctx,
>+			       struct smb2_tree *tree)
>+{
>+	struct smb2_handle h;
>+	struct smb2_create cr;
>+	struct smb2_read r;
>+	const char *fname = "compound_read.dat";
>+	const char *sname = "compound_read.dat:foo";
>+	struct smb2_request *req[3];
>+	NTSTATUS status;
>+	bool ret = false;
>+
>+	smb2_util_unlink(tree, fname);
>+
>+	/* Write file */
>+	ZERO_STRUCT(cr);
>+	cr.in.desired_access = SEC_FILE_WRITE_DATA;
>+	cr.in.file_attributes = FILE_ATTRIBUTE_NORMAL;
>+	cr.in.create_disposition = NTCREATEX_DISP_CREATE;
>+	cr.in.impersonation_level = SMB2_IMPERSONATION_ANONYMOUS;
>+	cr.in.fname = fname;
>+	cr.in.share_access = NTCREATEX_SHARE_ACCESS_READ|
>+		NTCREATEX_SHARE_ACCESS_WRITE|
>+		NTCREATEX_SHARE_ACCESS_DELETE;
>+	status = smb2_create(tree, tctx, &cr);
>+	CHECK_STATUS(status, NT_STATUS_OK);
>+	h = cr.out.file.handle;
>+
>+	status = smb2_util_write(tree, h, "123", 0, 3);
>+	CHECK_STATUS(status, NT_STATUS_OK);
>+
>+	smb2_util_close(tree, h);
>+
>+	/* Write stream */
>+	ZERO_STRUCT(cr);
>+	cr.in.desired_access = SEC_FILE_WRITE_DATA;
>+	cr.in.file_attributes = FILE_ATTRIBUTE_NORMAL;
>+	cr.in.create_disposition = NTCREATEX_DISP_CREATE;
>+	cr.in.impersonation_level = SMB2_IMPERSONATION_ANONYMOUS;
>+	cr.in.fname = sname;
>+	cr.in.share_access = NTCREATEX_SHARE_ACCESS_READ|
>+		NTCREATEX_SHARE_ACCESS_WRITE|
>+		NTCREATEX_SHARE_ACCESS_DELETE;
>+	status = smb2_create(tree, tctx, &cr);
>+	CHECK_STATUS(status, NT_STATUS_OK);
>+	h = cr.out.file.handle;
>+
>+	status = smb2_util_write(tree, h, "456", 0, 3);
>+	CHECK_STATUS(status, NT_STATUS_OK);
>+
>+	smb2_util_close(tree, h);
>+
>+	/* Check compound read from basefile */
>+	smb2_transport_compound_start(tree->session->transport, 2);
>+
>+	ZERO_STRUCT(cr);
>+	cr.in.impersonation_level = SMB2_IMPERSONATION_ANONYMOUS;
>+	cr.in.desired_access	= SEC_FILE_READ_DATA;
>+	cr.in.file_attributes	= FILE_ATTRIBUTE_NORMAL;
>+	cr.in.create_disposition = NTCREATEX_DISP_OPEN;
>+	cr.in.fname		= fname;
>+	cr.in.share_access = NTCREATEX_SHARE_ACCESS_READ|
>+		NTCREATEX_SHARE_ACCESS_WRITE|
>+		NTCREATEX_SHARE_ACCESS_DELETE;
>+	req[0] = smb2_create_send(tree, &cr);
>+
>+	smb2_transport_compound_set_related(tree->session->transport, true);
>+
>+	ZERO_STRUCT(r);
>+	h.data[0] = UINT64_MAX;
>+	h.data[1] = UINT64_MAX;
>+	r.in.file.handle = h;
>+	r.in.length      = 3;
>+	r.in.offset      = 0;
>+	r.in.min_count      = 1;
>+	req[1] = smb2_read_send(tree, &r);
>+
>+	status = smb2_create_recv(req[0], tree, &cr);
>+	CHECK_STATUS(status, NT_STATUS_OK);
>+
>+	/*
>+	 * We must do a manual smb2_request_receive() in order to be
>+	 * able to check the transport layer info, as smb2_read_recv()
>+	 * will destroy the req. smb2_read_recv() will call
>+	 * smb2_request_receive() again, but that's ok.
>+	 */
>+	if (!smb2_request_receive(req[1]) ||
>+	    !smb2_request_is_ok(req[1])) {
>+		torture_fail(tctx, "failed to receive read request");
>+	}
>+
>+	/*
>+	 * size must be 24: 16 byte read response header plus 3
>+	 * requested bytes padded to an 8 byte boundary.
>+	 */
>+	CHECK_VALUE(req[1]->in.body_size, 24);
>+
>+	status = smb2_read_recv(req[1], tree, &r);
>+	CHECK_STATUS(status, NT_STATUS_OK);
>+
>+	smb2_util_close(tree, cr.out.file.handle);
>+
>+	/* Check compound read from stream */
>+	smb2_transport_compound_start(tree->session->transport, 2);
>+
>+	ZERO_STRUCT(cr);
>+	cr.in.impersonation_level = SMB2_IMPERSONATION_ANONYMOUS;
>+	cr.in.desired_access	= SEC_FILE_READ_DATA;
>+	cr.in.file_attributes	= FILE_ATTRIBUTE_NORMAL;
>+	cr.in.create_disposition = NTCREATEX_DISP_OPEN;
>+	cr.in.fname		= sname;
>+	cr.in.share_access = NTCREATEX_SHARE_ACCESS_READ|
>+		NTCREATEX_SHARE_ACCESS_WRITE|
>+		NTCREATEX_SHARE_ACCESS_DELETE;
>+	req[0] = smb2_create_send(tree, &cr);
>+
>+	smb2_transport_compound_set_related(tree->session->transport, true);
>+
>+	ZERO_STRUCT(r);
>+	h.data[0] = UINT64_MAX;
>+	h.data[1] = UINT64_MAX;
>+	r.in.file.handle = h;
>+	r.in.length      = 3;
>+	r.in.offset      = 0;
>+	r.in.min_count   = 1;
>+	req[1] = smb2_read_send(tree, &r);
>+
>+	status = smb2_create_recv(req[0], tree, &cr);
>+	CHECK_STATUS(status, NT_STATUS_OK);
>+
>+	/*
>+	 * We must do a manual smb2_request_receive() in order to be
>+	 * able to check the transport layer info, as smb2_read_recv()
>+	 * will destroy the req. smb2_read_recv() will call
>+	 * smb2_request_receive() again, but that's ok.
>+	 */
>+	if (!smb2_request_receive(req[1]) ||
>+	    !smb2_request_is_ok(req[1])) {
>+		torture_fail(tctx, "failed to receive read request");
>+	}
>+
>+	/*
>+	 * size must be 24: 16 byte read response header plus 3
>+	 * requested bytes padded to an 8 byte boundary.
>+	 */
>+	CHECK_VALUE(req[1]->in.body_size, 24);
>+
>+	status = smb2_read_recv(req[1], tree, &r);
>+	CHECK_STATUS(status, NT_STATUS_OK);
>+
>+	smb2_util_close(tree, cr.out.file.handle);
>+	status = smb2_util_unlink(tree, fname);
>+	CHECK_STATUS(status, NT_STATUS_OK);
>+
>+	ret = true;
>+done:
>+	return ret;
>+}
>+
> static bool test_compound_unrelated1(struct torture_context *tctx,
> 				     struct smb2_tree *tree)
> {
>@@ -880,6 +1047,7 @@ struct torture_suite *torture_smb2_compound_init(void)
> 	torture_suite_add_1smb2_test(suite, "interim1",  test_compound_interim1);
> 	torture_suite_add_1smb2_test(suite, "interim2",  test_compound_interim2);
> 	torture_suite_add_1smb2_test(suite, "compound-break", test_compound_break);
>+	torture_suite_add_1smb2_test(suite, "compound-read-padding", test_compound_read);
> 
> 	suite->description = talloc_strdup(suite, "SMB2-COMPOUND tests");
> 
>-- 
>2.1.0
>

Actions: View

Attachments on bug 11277: 11057 | 11058 | 11063 | 11099 | 11103 | 11104