From 47a391644113f3dbe4ef13b89b4f4c712fe7fc72 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Thu, 28 May 2015 09:02:17 +0200 Subject: [PATCH 1/2] s3:smb2: add padding to last command in compound requests Following Windows behaviour, the last command in a compound request should be padded to an 8 byte boundary and OS X clients crash badly if we don't pad. [MS-SMB2] 3.3.4.1.3, "Sending Compounded Responses", doesn't make it clear whether the padding requirement governs the last command in a compound response, a future MS-SMB2 update will document Windwows product behaviour in a footnote. Bug: https://bugzilla.samba.org/show_bug.cgi?id=11277 Pair-Programmed-With: Stefan Metzmacher Signed-off-by: Ralph Boehme Signed-off-by: Stefan Metzmacher (cherry picked from commit dfa64b958b201931e0dbe11f153f606f20217594) --- source3/smbd/smb2_server.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c index 53a38f9..2739734 100644 --- a/source3/smbd/smb2_server.c +++ b/source3/smbd/smb2_server.c @@ -2612,8 +2612,13 @@ 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) { + /* + * See if we need to recalculate the offset to the next response + * + * Note that all responses may require padding (including the very last + * one). + */ + if (req->out.vector_count >= (2 * SMBD_SMB2_NUM_IOV_PER_REQ)) { next_command_ofs = SMB2_HDR_BODY; next_command_ofs += SMBD_SMB2_OUT_BODY_LEN(req); next_command_ofs += SMBD_SMB2_OUT_DYN_LEN(req); @@ -2667,8 +2672,11 @@ NTSTATUS smbd_smb2_request_done_ex(struct smbd_smb2_request *req, next_command_ofs += pad_size; } - SIVAL(outhdr, SMB2_HDR_NEXT_COMMAND, next_command_ofs); - + if ((req->current_idx + SMBD_SMB2_NUM_IOV_PER_REQ) >= req->out.vector_count) { + SIVAL(outhdr, SMB2_HDR_NEXT_COMMAND, 0); + } else { + SIVAL(outhdr, SMB2_HDR_NEXT_COMMAND, next_command_ofs); + } return smbd_smb2_request_reply(req); } -- 2.1.0 From ff05f509c7245f80ff738a419515ed82026306f2 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Thu, 14 May 2015 04:27:54 +0200 Subject: [PATCH 2/2] s4:torture:smb2:compound: compound read and padding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Pair-Programmed-With: Stefan Metzmacher Signed-off-by: Ralph Boehme Signed-off-by: Stefan Metzmacher Autobuild-User(master): Ralph Böhme Autobuild-Date(master): Thu May 28 16:50:39 CEST 2015 on sn-devel-104 (cherry picked from commit 2ffa939bbe2c02509e1790c8b3f6f9b6910e3cf6) --- source4/torture/smb2/compound.c | 239 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 239 insertions(+) diff --git a/source4/torture/smb2/compound.c b/source4/torture/smb2/compound.c index 9b3cacc..a502103 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,236 @@ done: return ret; } +static bool test_compound_padding(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); + + h = cr.out.file.handle; + + /* Check 2 compound (unrelateated) reads from existing stream handle */ + smb2_transport_compound_start(tree->session->transport, 2); + + ZERO_STRUCT(r); + r.in.file.handle = h; + r.in.length = 3; + r.in.offset = 0; + r.in.min_count = 1; + req[0] = smb2_read_send(tree, &r); + req[1] = smb2_read_send(tree, &r); + + /* + * 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[0]) || + !smb2_request_is_ok(req[0])) { + torture_fail(tctx, "failed to receive read request"); + } + 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[0]->in.body_size, 24); + CHECK_VALUE(req[1]->in.body_size, 24); + + status = smb2_read_recv(req[0], tree, &r); + CHECK_STATUS(status, NT_STATUS_OK); + status = smb2_read_recv(req[1], tree, &r); + CHECK_STATUS(status, NT_STATUS_OK); + + /* + * now try a single read from the stream and verify there's no padding + */ + ZERO_STRUCT(r); + r.in.file.handle = h; + r.in.length = 3; + r.in.offset = 0; + r.in.min_count = 1; + req[0] = smb2_read_send(tree, &r); + + /* + * 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[0]) || + !smb2_request_is_ok(req[0])) { + torture_fail(tctx, "failed to receive read request"); + } + + /* + * size must be 19: 16 byte read response header plus 3 + * requested bytes without padding. + */ + CHECK_VALUE(req[0]->in.body_size, 19); + + status = smb2_read_recv(req[0], tree, &r); + CHECK_STATUS(status, NT_STATUS_OK); + + smb2_util_close(tree, h); + + 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 +1118,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-padding", test_compound_padding); suite->description = talloc_strdup(suite, "SMB2-COMPOUND tests"); -- 2.1.0