Attachment 11989 Details for Bug 11622 – Patch for v4-3-test

[patch] Patch for v4-3-test

tmp43.diff.txt (text/plain), 6.33 KB, created by Stefan Metzmacher on 2016-04-13 17:22:58 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Stefan Metzmacher

Created: 2016-04-13 17:22:58 UTC

Size: 6.33 KB

patch

obsolete

>From 85b00f5bfca993ed8599c9e3b6d557be6c8c689a Mon Sep 17 00:00:00 2001
>From: Stefan Metzmacher <metze@samba.org>
>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 <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>
>(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
>

Flags:

jra: review+

Actions: View

Attachments on bug 11622: 11989