From 314e0031a76ee1f169d2788cb97fe2452fd0f012 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 Signed-off-by: Ralph Boehme --- source3/smbd/smb2_server.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c index 9e5eff7..1ef3034 100644 --- a/source3/smbd/smb2_server.c +++ b/source3/smbd/smb2_server.c @@ -2655,7 +2655,7 @@ NTSTATUS smbd_smb2_request_done_ex(struct smbd_smb2_request *req, } /* see if we need to recalculate the offset to the next response */ - if (next_command_ofs > 0) { + 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); @@ -2709,8 +2709,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 260c6b5c4b636342bff10929f9f30b029d87458b 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 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 --- 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..7eb1ed2 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_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); + + 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-padding", test_compound_padding); suite->description = talloc_strdup(suite, "SMB2-COMPOUND tests"); -- 2.1.0