From 26b16091f99a5ae17ff53b9fcdc7972a43cbabc1 Mon Sep 17 00:00:00 2001 From: Ralph Boehme 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 --- 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 bce1de9f2915d8043727e4082ff033396cec139c 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 | 170 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 170 insertions(+) diff --git a/source4/torture/smb2/compound.c b/source4/torture/smb2/compound.c index 9b3cacc..c93b8dc 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 received 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 received 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) { @@ -869,6 +1036,9 @@ struct torture_suite *torture_smb2_compound_init(void) { struct torture_suite *suite = torture_suite_create(talloc_autofree_context(), "compound"); + torture_suite_add_1smb2_test(suite, "compound-read-padding", test_compound_read); + suite->description = talloc_strdup(suite, "SMB2-COMPOUND tests"); + return suite; torture_suite_add_1smb2_test(suite, "related1", test_compound_related1); torture_suite_add_1smb2_test(suite, "related2", test_compound_related2); torture_suite_add_1smb2_test(suite, "related3", -- 2.1.0