[PATCH] Stop smbd from writing into 'reserved' fields on a SMB_COM_READ_ANDX reply.
Jeremy Allison
jra at samba.org
Mon Jun 13 16:40:15 UTC 2016
On Fri, Jun 10, 2016 at 10:33:56AM -0700, Jeremy Allison wrote:
> Took me a while to track down, and strictly
> doesn't fix any breakage, but it's untidy
> of smbd to write things it shouldn't into
> reserved fields on such an important packet
> (readX) reply.
>
> Complete with torture test that passes against
> Win2K12 and (with patch applied) against smbd.
>
> Please review and push !
Updated version - now addresses the root
cause of:
https://bugzilla.samba.org/show_bug.cgi?id=11845
(so it does fix a bug now :-) which can cause the
byte count to be wrong on an SMB1 aio read return.
Fixed by making all readX return paths use
one common header setup function.
Please review and push !
Jeremy.
-------------- next part --------------
From 25b74b9f8f28f60c70b4cf7f7e9b45b1245453cd Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 13 Jun 2016 09:20:43 -0700
Subject: [PATCH 1/5] s3: smbd: Remove unused 'req' argument from
setup_readX_header()
https://bugzilla.samba.org/show_bug.cgi?id=11845
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/reply.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c
index e0e55c6..0328ae9 100644
--- a/source3/smbd/reply.c
+++ b/source3/smbd/reply.c
@@ -3922,8 +3922,7 @@ strict_unlock:
Setup readX header.
****************************************************************************/
-static int setup_readX_header(struct smb_request *req, char *outbuf,
- size_t smb_maxcnt)
+static int setup_readX_header(char *outbuf, size_t smb_maxcnt)
{
int outsize;
@@ -4010,7 +4009,7 @@ static void send_file_readX(connection_struct *conn, struct smb_request *req,
header = data_blob_const(headerbuf, sizeof(headerbuf));
construct_reply_common_req(req, (char *)headerbuf);
- setup_readX_header(req, (char *)headerbuf, smb_maxcnt);
+ setup_readX_header((char *)headerbuf, smb_maxcnt);
nread = SMB_VFS_SENDFILE(xconn->transport.sock, fsp, &header,
startpos, smb_maxcnt);
@@ -4111,7 +4110,7 @@ normal_read:
}
construct_reply_common_req(req, (char *)headerbuf);
- setup_readX_header(req, (char *)headerbuf, smb_maxcnt);
+ setup_readX_header((char *)headerbuf, smb_maxcnt);
/* Send out the header. */
ret = write_data(xconn->transport.sock, (char *)headerbuf,
@@ -4161,7 +4160,7 @@ nosendfile_read:
return;
}
- setup_readX_header(req, (char *)req->outbuf, nread);
+ setup_readX_header((char *)req->outbuf, nread);
DEBUG(3, ("send_file_readX %s max=%d nread=%d\n",
fsp_fnum_dbg(fsp), (int)smb_maxcnt, (int)nread));
--
2.8.0.rc3.226.g39d4020
From 98fb2cfaed50e96330a3095936b99c4bfbecea83 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 13 Jun 2016 09:22:56 -0700
Subject: [PATCH 2/5] s3: smbd: Make setup_readX_header() externally accessible
https://bugzilla.samba.org/show_bug.cgi?id=11845
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/proto.h | 1 +
source3/smbd/reply.c | 2 +-
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
index 3612034..81bdc87 100644
--- a/source3/smbd/proto.h
+++ b/source3/smbd/proto.h
@@ -932,6 +932,7 @@ ssize_t sendfile_short_send(struct smbXsrv_connection *xconn,
size_t smb_maxcnt);
void reply_readbraw(struct smb_request *req);
void reply_lockread(struct smb_request *req);
+int setup_readX_header(char *outbuf, size_t smb_maxcnt);
void reply_read(struct smb_request *req);
void reply_read_and_X(struct smb_request *req);
void error_to_writebrawerr(struct smb_request *req);
diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c
index 0328ae9..559aab0 100644
--- a/source3/smbd/reply.c
+++ b/source3/smbd/reply.c
@@ -3922,7 +3922,7 @@ strict_unlock:
Setup readX header.
****************************************************************************/
-static int setup_readX_header(char *outbuf, size_t smb_maxcnt)
+int setup_readX_header(char *outbuf, size_t smb_maxcnt)
{
int outsize;
--
2.8.0.rc3.226.g39d4020
From 59cac8fd68ac241edb7d6846257fee89347c506f Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 13 Jun 2016 09:25:02 -0700
Subject: [PATCH 3/5] s3: smbd: Use common function setup_readX_header() in aio
read code.
https://bugzilla.samba.org/show_bug.cgi?id=11845
Signed-off-by: Jeremy Allison <jra at samba.org>
---
source3/smbd/aio.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/source3/smbd/aio.c b/source3/smbd/aio.c
index 2958ac3..ff1be13 100644
--- a/source3/smbd/aio.c
+++ b/source3/smbd/aio.c
@@ -272,7 +272,6 @@ static void aio_pread_smb1_done(struct tevent_req *req)
files_struct *fsp = aio_ex->fsp;
int outsize;
char *outbuf = (char *)aio_ex->outbuf.data;
- char *data = smb_buf(outbuf) + 1 /* padding byte */;
ssize_t nread;
struct vfs_aio_state vfs_aio_state;
@@ -301,13 +300,7 @@ static void aio_pread_smb1_done(struct tevent_req *req)
ERROR_NT(map_nt_error_from_unix(vfs_aio_state.error));
outsize = srv_set_message(outbuf,0,0,true);
} else {
- outsize = srv_set_message(outbuf, 12,
- nread + 1 /* padding byte */, false);
- SSVAL(outbuf,smb_vwv2, 0xFFFF); /* Remaining - must be * -1. */
- SSVAL(outbuf,smb_vwv5, nread);
- SSVAL(outbuf,smb_vwv6, smb_offset(data,outbuf));
- SSVAL(outbuf,smb_vwv7, ((nread >> 16) & 1));
- SSVAL(smb_buf(outbuf), -2, nread);
+ outsize = setup_readX_header(outbuf, nread);
aio_ex->fsp->fh->pos = aio_ex->offset + nread;
aio_ex->fsp->fh->position_information = aio_ex->fsp->fh->pos;
--
2.8.0.rc3.226.g39d4020
From 704c9e0cb90de645481183b30b73c1fc920681b9 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Mon, 13 Jun 2016 09:30:25 -0700
Subject: [PATCH 4/5] 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.
https://bugzilla.samba.org/show_bug.cgi?id=11845
Signed-off-by: Jeremy Allison <jra at samba.org>
---
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 559aab0..0b7a4fb 100644
--- a/source3/smbd/reply.c
+++ b/source3/smbd/reply.c
@@ -3941,7 +3941,6 @@ int setup_readX_header(char *outbuf, size_t smb_maxcnt)
+ 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 b277e7670e6095df3e94692cb23feae6d2aa48e3 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Fri, 10 Jun 2016 09:32:32 -0700
Subject: [PATCH 5/5] s4: torture: Added raw readX test to ensure 'reserved'
fields are zero.
Passes against Win2k12+, and smbd with the previous patch.
https://bugzilla.samba.org/show_bug.cgi?id=11845
Signed-off-by: Jeremy Allison <jra at samba.org>
---
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
More information about the samba-technical
mailing list