From 85b00f5bfca993ed8599c9e3b6d557be6c8c689a Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Fri, 27 Nov 2015 17:31:04 +0100 Subject: [PATCH] libcli/smb: make sure we have a body size of 0x31 before dereferencing an ioctl response Found by valgrind, reported by Noel Power : ==7913== Invalid read of size 1 ==7913== at 0xC4F23EE: smb2cli_ioctl_done (smb2cli_ioctl.c:245) ==7913== by 0x747A744: _tevent_req_notify_callback (tevent_req.c:112) ==7913== by 0x747A817: tevent_req_finish (tevent_req.c:149) ==7913== by 0x747A93C: tevent_req_trigger (tevent_req.c:206) ==7913== by 0x7479B2B: tevent_common_loop_immediate (tevent_immediate.c:135) ==7913== by 0xA9CB4BE: run_events_poll (events.c:192) ==7913== by 0xA9CBB32: s3_event_loop_once (events.c:303) ==7913== by 0x7478C72: _tevent_loop_once (tevent.c:533) ==7913== by 0x747AACD: tevent_req_poll (tevent_req.c:256) ==7913== by 0x505315D: tevent_req_poll_ntstatus (tevent_ntstatus.c:109) ==7913== by 0xA7201F2: cli_tree_connect (cliconnect.c:2764) ==7913== by 0x165FF7: cm_prepare_connection (winbindd_cm.c:1276) ==7913== Address 0x16ce24ec is 764 bytes inside a block of size 813 alloc'd ==7913== at 0x4C29110: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so) ==7913== by 0x768A0C1: __talloc_with_prefix (talloc.c:668) ==7913== by 0x768A27E: _talloc_pool (talloc.c:721) ==7913== by 0x768A41E: _talloc_pooled_object (talloc.c:790) ==7913== by 0x747A594: _tevent_req_create (tevent_req.c:66) ==7913== by 0xCF6E2FA: read_packet_send (async_sock.c:414) ==7913== by 0xCF6EB54: read_smb_send (read_smb.c:54) ==7913== by 0xC4DA146: smbXcli_conn_receive_next (smbXcli_base.c:1027) ==7913== by 0xC4DA02D: smbXcli_req_set_pending (smbXcli_base.c:978) ==7913== by 0xC4DF776: smb2cli_req_compound_submit (smbXcli_base.c:3166) ==7913== by 0xC4DFC1D: smb2cli_req_send (smbXcli_base.c:3268) ==7913== by 0xC4F2210: smb2cli_ioctl_send (smb2cli_ioctl.c:149) ==7913== BUG: https://bugzilla.samba.org/show_bug.cgi?id=11622 Signed-off-by: Stefan Metzmacher Reviewed-by: Jeremy Allison (cherry picked from commit 91e12e04fc05a0b09b70ca2986aab9b96a8a035c) --- libcli/smb/smb2cli_ioctl.c | 84 ++++++++++++++++++++++++++-------------------- 1 file changed, 47 insertions(+), 37 deletions(-) diff --git a/libcli/smb/smb2cli_ioctl.c b/libcli/smb/smb2cli_ioctl.c index 42a424e..2b572ba 100644 --- a/libcli/smb/smb2cli_ioctl.c +++ b/libcli/smb/smb2cli_ioctl.c @@ -22,8 +22,6 @@ #include "lib/util/tevent_ntstatus.h" #include "smb_common.h" #include "smbXcli_base.h" -#include "librpc/ndr/libndr.h" -#include "librpc/gen_ndr/ioctl.h" struct smb2cli_ioctl_state { uint8_t fixed[0x38]; @@ -31,6 +29,7 @@ struct smb2cli_ioctl_state { uint32_t max_input_length; uint32_t max_output_length; struct iovec *recv_iov; + bool out_valid; DATA_BLOB out_input_buffer; DATA_BLOB out_output_buffer; uint32_t ctl_code; @@ -161,32 +160,6 @@ struct tevent_req *smb2cli_ioctl_send(TALLOC_CTX *mem_ctx, return req; } -/* - * 3.3.4.4 Sending an Error Response - * An error code other than one of the following indicates a failure: - */ -static bool smb2cli_ioctl_is_failure(uint32_t ctl_code, NTSTATUS status, - size_t data_size) -{ - if (NT_STATUS_IS_OK(status)) { - return false; - } - - /* - * STATUS_INVALID_PARAMETER in a FSCTL_SRV_COPYCHUNK or - * FSCTL_SRV_COPYCHUNK_WRITE Response, when returning an - * SRV_COPYCHUNK_RESPONSE as described in section 2.2.32.1. - */ - if (NT_STATUS_EQUAL(status, NT_STATUS_INVALID_PARAMETER) && - (ctl_code == FSCTL_SRV_COPYCHUNK || - ctl_code == FSCTL_SRV_COPYCHUNK_WRITE) && - data_size == sizeof(struct srv_copychunk_rsp)) { - return false; - } - - return true; -} - static void smb2cli_ioctl_done(struct tevent_req *subreq) { struct tevent_req *req = @@ -225,6 +198,16 @@ static void smb2cli_ioctl_done(struct tevent_req *subreq) .body_size = 0x09, }, { + /* + * a normal error + */ + .status = NT_STATUS_INVALID_PARAMETER, + .body_size = 0x09 + }, + { + /* + * a special case for FSCTL_SRV_COPYCHUNK_* + */ .status = NT_STATUS_INVALID_PARAMETER, .body_size = 0x31 }, @@ -233,10 +216,35 @@ static void smb2cli_ioctl_done(struct tevent_req *subreq) status = smb2cli_req_recv(subreq, state, &iov, expected, ARRAY_SIZE(expected)); TALLOC_FREE(subreq); - if (iov == NULL && tevent_req_nterror(req, status)) { - return; + if (NT_STATUS_EQUAL(status, NT_STATUS_INVALID_PARAMETER)) { + switch (state->ctl_code) { + case FSCTL_SRV_COPYCHUNK: + case FSCTL_SRV_COPYCHUNK_WRITE: + break; + default: + tevent_req_nterror(req, status); + return; + } + + if (iov[1].iov_len != 0x30) { + tevent_req_nterror(req, + NT_STATUS_INVALID_NETWORK_RESPONSE); + return; + } + } else if (NT_STATUS_EQUAL(status, STATUS_BUFFER_OVERFLOW)) { + /* no error */ + } else { + if (tevent_req_nterror(req, status)) { + return; + } } + /* + * At this stage we're sure that got a body size of 0x31, + * either with NT_STATUS_OK, STATUS_BUFFER_OVERFLOW or + * NT_STATUS_INVALID_PARAMETER. + */ + state->recv_iov = iov; fixed = (uint8_t *)iov[1].iov_base; dyn = (uint8_t *)iov[2].iov_base; @@ -247,11 +255,6 @@ static void smb2cli_ioctl_done(struct tevent_req *subreq) output_buffer_offset = IVAL(fixed, 0x20); output_buffer_length = IVAL(fixed, 0x24); - if (smb2cli_ioctl_is_failure(state->ctl_code, status, output_buffer_length) && - tevent_req_nterror(req, status)) { - return; - } - if ((input_buffer_offset > 0) && (input_buffer_length > 0)) { uint32_t ofs; @@ -332,6 +335,8 @@ static void smb2cli_ioctl_done(struct tevent_req *subreq) state->out_output_buffer.length = output_buffer_length; } + state->out_valid = true; + if (tevent_req_nterror(req, status)) { return; } @@ -349,8 +354,13 @@ NTSTATUS smb2cli_ioctl_recv(struct tevent_req *req, struct smb2cli_ioctl_state); NTSTATUS status = NT_STATUS_OK; - if (tevent_req_is_nterror(req, &status) && - smb2cli_ioctl_is_failure(state->ctl_code, status, state->out_output_buffer.length)) { + if (tevent_req_is_nterror(req, &status) && !state->out_valid) { + if (out_input_buffer) { + *out_input_buffer = data_blob_null; + } + if (out_output_buffer) { + *out_output_buffer = data_blob_null; + } tevent_req_received(req); return status; } -- 1.9.1