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>

diff --git a/libcli/smb/smb2cli_ioctl.c b/libcli/smb/smb2cli_ioctl.c
index 42a424e6b9faaad86e4acff12ed3716076a701dc..2b572baeb2381a038a0d1d9a54efdd68ad246bbe 100644 (file)
--- 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;
        }