libcli/smb: make sure we have a body size of 0x31 before dereferencing an ioctl response

Found by valgrind, reported by Noel Power <nopower@suse.com>:

==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 <metze@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>

1 files changed, 47 insertions, 37 deletions

diff --git a/libcli/smb/smb2cli_ioctl.c b/libcli/smb/smb2cli_ioctl.c
index 42a424e6b9f..2b572baeb23 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;
 	}