From 039aa845ccf11c3b50565b5e381ad00df9cebbb8 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 9 Jun 2016 16:46:01 -0700 Subject: [PATCH 1/2] s3: smbd: In reply_read_and_X() SMB1 server is overwriting part of the 'reserved' zero fields with reply data length. This occurred due to old code that used to do: SSVAL(smb_buf(req->outbuf),-2,nread); to set the reply length. This code was not needed, as srv_set_message() was already correctly setting the bcc length and was probably left from much earlier legacy code. However, in commit ddaa65ef6e049a185281c4d5deca4045e3b085e2 this was converted to do: SSVAL(req->outbuf,smb_vwv11,smb_maxcnt); This code actually overwrites the last 'reserved' field in the SMB_COM_READ_ANDX packet reply, but we never noticed as no client (or server code) looks at or checks vwv11 in a SMB_COM_READ_ANDX reply. [MS-SMB] shows for SMB_COM_READ_ANDX reply: SMB_Parameters { UCHAR WordCount; Words { UCHAR AndXCommand; UCHAR AndXReserved; USHORT AndXOffset; USHORT Available; USHORT DataCompactionMode; USHORT Reserved1; USHORT DataLength; USHORT DataOffset; USHORT DataLengthHigh; USHORT Reserved2[4]; } } SMB_Data { USHORT ByteCount; Bytes { UCHAR Pad[] (optional); UCHAR Data[variable]; } } and indeed checking wireshark from Win2012R2 we find that smbd is writing the returned read length into smb_vwv11 and Windows leaves it as zeros (reserved). Also fix the same problem in the named pipes code. Torture test to ensure Reserved2[4] replies are zero to follow. Signed-off-by: Jeremy Allison --- source3/smbd/pipes.c | 1 - source3/smbd/reply.c | 1 - 2 files changed, 2 deletions(-) diff --git a/source3/smbd/pipes.c b/source3/smbd/pipes.c index 2c9516d..bdc5af0 100644 --- a/source3/smbd/pipes.c +++ b/source3/smbd/pipes.c @@ -492,7 +492,6 @@ static void pipe_read_andx_done(struct tevent_req *subreq) + 12 * sizeof(uint16_t) /* vwv */ + 2 /* the buflen field */ + 1); /* padding byte */ - SSVAL(req->outbuf,smb_vwv11,state->smb_maxcnt); DEBUG(3,("readX-IPC min=%d max=%d nread=%d\n", state->smb_mincnt, state->smb_maxcnt, (int)nread)); diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c index e0e55c6..86224e2 100644 --- a/source3/smbd/reply.c +++ b/source3/smbd/reply.c @@ -3942,7 +3942,6 @@ static int setup_readX_header(struct smb_request *req, char *outbuf, + 2 /* the buflen field */ + 1); /* padding byte */ SSVAL(outbuf,smb_vwv7,(smb_maxcnt >> 16)); - SSVAL(outbuf,smb_vwv11,smb_maxcnt); SCVAL(smb_buf(outbuf), 0, 0); /* padding byte */ /* Reset the outgoing length, set_message truncates at 0x1FFFF. */ _smb_setlen_large(outbuf, -- 2.8.0.rc3.226.g39d4020 From e4dc9e6a339577c59ec246c7b8f9f58ff6e55340 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Fri, 10 Jun 2016 09:32:32 -0700 Subject: [PATCH 2/2] s4: torture: Added raw readX test to ensure 'reserved' fields are zero. Passes against Win2k12+, and smbd with the previous patch. Signed-off-by: Jeremy Allison --- source4/torture/raw/read.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/source4/torture/raw/read.c b/source4/torture/raw/read.c index 6abf08b..6160e3e 100644 --- a/source4/torture/raw/read.c +++ b/source4/torture/raw/read.c @@ -19,6 +19,7 @@ #include "includes.h" #include "libcli/raw/libcliraw.h" +#include "libcli/raw/raw_proto.h" #include "system/time.h" #include "system/filesys.h" #include "libcli/libcli.h" @@ -373,6 +374,8 @@ static bool test_readx(struct torture_context *tctx, struct smbcli_state *cli) const char *fname = BASEDIR "\\test.txt"; const char *test_data = "TEST DATA"; unsigned int seed = time(NULL); + struct smbcli_request *smbreq = NULL; + unsigned int i; buf = talloc_zero_array(tctx, uint8_t, maxsize); @@ -422,6 +425,47 @@ static bool test_readx(struct torture_context *tctx, struct smbcli_state *cli) smbcli_write(cli->tree, fnum, 0, test_data, 0, strlen(test_data)); + printf("Checking reserved fields are [0]\n"); + io.readx.in.file.fnum = fnum; + io.readx.in.offset = 0; + io.readx.in.remaining = 0; + io.readx.in.read_for_execute = false; + io.readx.in.mincnt = strlen(test_data); + io.readx.in.maxcnt = strlen(test_data); + smbreq = smb_raw_read_send(cli->tree, &io); + if (smbreq == NULL) { + ret = false; + torture_fail_goto(tctx, done, "smb_raw_read_send failed\n"); + } + if (!smbcli_request_receive(smbreq) || + smbcli_request_is_error(smbreq)) { + status = smbcli_request_destroy(smbreq); + torture_fail_goto(tctx, done, "receive failed\n"); + } + + if (smbreq->in.wct != 12) { + ret = false; + printf("Incorrect wct %u (should be 12)\n", + (unsigned int)smbreq->in.wct); + status = smbcli_request_destroy(smbreq); + torture_fail_goto(tctx, done, "bad wct\n"); + } + + /* Ensure VWV8 - WVW11 are zero. */ + for (i = 8; i < 12; i++) { + uint16_t br = SVAL(smbreq->in.vwv, VWV(i)); + if (br != 0) { + status = smbcli_request_destroy(smbreq); + ret = false; + printf("reserved field %u is %u not zero\n", + i, + (unsigned int)br); + torture_fail_goto(tctx, done, "bad reserved field\n"); + } + } + + smbcli_request_destroy(smbreq); + printf("Trying small read\n"); io.readx.in.file.fnum = fnum; io.readx.in.offset = 0; -- 2.8.0.rc3.226.g39d4020