[PATCH] Fix panic when running 'smbtorture smb.base'

Volker Lendecke Volker.Lendecke at SerNet.DE
Tue Apr 9 06:58:13 MDT 2013


On Tue, Apr 09, 2013 at 10:41:05AM +0200, Ralph Wuerthner wrote:
> Hello list,
> 
> when running 'smbtorture smb.base' on a recent Samba 4 I noticed the following panic:
> 
> [2013/02/23 19:22:58.524698,  0] lib/util.c:810(smb_panic_s3)
>   PANIC (pid 2109097): sec_len == -1 in pull_ucs2_base_talloc
> [2013/02/23 19:22:58.563334,  0] lib/util.c:921(log_stack_trace)
>   BACKTRACE: 22 stack frames:
>    #0 smbd(log_stack_trace+0x1a) [0x7f3dfe84022a]
>    #1 smbd(smb_panic_s3+0x25) [0x7f3dfe8402f5]
>    #2 smbd(smb_panic+0x1a1) [0x7f3dfe8321a1]
>    #3 smbd(+0x466298) [0x7f3dfe82f298]
>    #4 smbd(srvstr_get_path_wcard+0x42) [0x7f3dfe51a802]
>    #5 smbd(srvstr_get_path_req_wcard+0x3c) [0x7f3dfe51a8cc]
>    #6 smbd(srvstr_get_path_req+0x12) [0x7f3dfe51ba22]
>    #7 smbd(reply_mkdir+0x53) [0x7f3dfe51bdf3]
>    #8 smbd(+0x199a7b) [0x7f3dfe562a7b]
>    #9 smbd(+0x19a864) [0x7f3dfe563864]
>    #10 smbd(+0x19b709) [0x7f3dfe564709]
>    #11 smbd(run_events_poll+0x376) [0x7f3dfe84f0d6]
>    #12 smbd(+0x486580) [0x7f3dfe84f580]
>    #13 smbd(_tevent_loop_once+0x90) [0x7f3dfe84f900]
>    #14 smbd(smbd_process+0xc77) [0x7f3dfe561c07]
>    #15 smbd(+0x7202dc) [0x7f3dfeae92dc]
>    #16 smbd(run_events_poll+0x376) [0x7f3dfe84f0d6]
>    #17 smbd(+0x486580) [0x7f3dfe84f580]
>    #18 smbd(_tevent_loop_once+0x90) [0x7f3dfe84f900]
>    #19 smbd(main+0x1381) [0x7f3dfeaead11]
>    #20 /lib64/libc.so.6(__libc_start_main+0xfd) [0x7f3dfb396cdd]
>    #21 smbd(+0x106a19) [0x7f3dfe4cfa19]
> [2013/02/23 19:22:58.563848,  0] lib/dumpcore.c:317(dump_core)
>   dumping core in /var/log/samba/cores/smbd
> 
> smb.base submits requests without any parameters and for a couple of SMB1 requests (SMBmkdir, SMBrmdir, SMBgetatr, SMBcheckpath, SMBfcloserequests) we try to access data behind req->buf+req->buflen, resulting in above panic.
> 
> Attached patch set adds additional checks to prevent this invalid access.

Attached with my Review. Looks good to me, thanks! Someone else?

Volker

-- 
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de
-------------- next part --------------
From 2b2ea490b7b76b95c88de2dd1279618c502662c0 Mon Sep 17 00:00:00 2001
From: Ralph Wuerthner <ralph.wuerthner at de.ibm.com>
Date: Thu, 4 Apr 2013 12:59:36 +0200
Subject: [PATCH 1/3] s3:smbd: do not access data behind req->buf+req->buflen in srvstr_get_path_req_wcard()

Reviewed-by: Volker Lendecke <vl at samba.org>
---
 source3/smbd/reply.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c
index 0d9f415..5fb10d5 100644
--- a/source3/smbd/reply.c
+++ b/source3/smbd/reply.c
@@ -318,9 +318,16 @@ size_t srvstr_get_path_req_wcard(TALLOC_CTX *mem_ctx, struct smb_request *req,
 				 char **pp_dest, const char *src, int flags,
 				 NTSTATUS *err, bool *contains_wcard)
 {
-	return srvstr_get_path_wcard(mem_ctx, (const char *)req->inbuf, req->flags2,
-				     pp_dest, src, smbreq_bufrem(req, src),
-				     flags, err, contains_wcard);
+	ssize_t bufrem = smbreq_bufrem(req, src);
+
+	if (bufrem < 0) {
+		*err = NT_STATUS_INVALID_PARAMETER;
+		return 0;
+	}
+
+	return srvstr_get_path_wcard(mem_ctx, (const char *)req->inbuf,
+				     req->flags2, pp_dest, src, bufrem, flags,
+				     err, contains_wcard);
 }
 
 size_t srvstr_get_path_req(TALLOC_CTX *mem_ctx, struct smb_request *req,
-- 
1.7.0.4


From 18a651503e94b4703b54ca6e6d8e70a03a40468a Mon Sep 17 00:00:00 2001
From: Ralph Wuerthner <ralph.wuerthner at de.ibm.com>
Date: Thu, 4 Apr 2013 13:24:36 +0200
Subject: [PATCH 2/3] s3:smbd: convert srvstr_pull_req_talloc() into a function

Reviewed-by: Volker Lendecke <vl at samba.org>
---
 source3/include/srvstr.h |    9 ---------
 source3/smbd/proto.h     |    2 ++
 source3/smbd/reply.c     |   11 +++++++++++
 3 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/source3/include/srvstr.h b/source3/include/srvstr.h
index 7e7d8a2..2c6e7ef 100644
--- a/source3/include/srvstr.h
+++ b/source3/include/srvstr.h
@@ -19,12 +19,3 @@
 
 #define srvstr_pull_talloc(ctx, base_ptr, smb_flags2, dest, src, src_len, flags) \
     pull_string_talloc(ctx, base_ptr, smb_flags2, dest, src, src_len, flags)
-
-/* pull a string from the smb_buf part of a packet. In this case the
-   string can either be null terminated or it can be terminated by the
-   end of the smbbuf area 
-*/
-
-#define srvstr_pull_req_talloc(ctx, req_, dest, src, flags) \
-    pull_string_talloc(ctx, req_->inbuf, req_->flags2, dest, src, \
-		       smbreq_bufrem(req_, src), flags)
diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
index 7e13049..2be73a2 100644
--- a/source3/smbd/proto.h
+++ b/source3/smbd/proto.h
@@ -841,6 +841,8 @@ size_t srvstr_get_path_req_wcard(TALLOC_CTX *mem_ctx, struct smb_request *req,
 size_t srvstr_get_path_req(TALLOC_CTX *mem_ctx, struct smb_request *req,
 			   char **pp_dest, const char *src, int flags,
 			   NTSTATUS *err);
+size_t srvstr_pull_req_talloc(TALLOC_CTX *ctx, struct smb_request *req,
+			      char **dest, const char *src, int flags);
 bool check_fsp_open(connection_struct *conn, struct smb_request *req,
 		    files_struct *fsp);
 bool check_fsp(connection_struct *conn, struct smb_request *req,
diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c
index 5fb10d5..8c7fc16 100644
--- a/source3/smbd/reply.c
+++ b/source3/smbd/reply.c
@@ -339,6 +339,17 @@ size_t srvstr_get_path_req(TALLOC_CTX *mem_ctx, struct smb_request *req,
 					 flags, err, &ignore);
 }
 
+/* pull a string from the smb_buf part of a packet. In this case the
+   string can either be null terminated or it can be terminated by the
+   end of the smbbuf area
+*/
+size_t srvstr_pull_req_talloc(TALLOC_CTX *ctx, struct smb_request *req,
+			      char **dest, const char *src, int flags)
+{
+	return pull_string_talloc(ctx, req->inbuf, req->flags2, dest, src,
+				  smbreq_bufrem(req, src), flags);
+}
+
 /****************************************************************************
  Check if we have a correct fsp pointing to a file. Basic check for open fsp.
 ****************************************************************************/
-- 
1.7.0.4


From 7cd1acfa3234b464667bb27ac92bf6ad06110bd4 Mon Sep 17 00:00:00 2001
From: Ralph Wuerthner <ralph.wuerthner at de.ibm.com>
Date: Thu, 4 Apr 2013 13:29:01 +0200
Subject: [PATCH 3/3] s3:smbd: do not access data behind req->buf+req->buflen in srvstr_pull_req_talloc()

Reviewed-by: Volker Lendecke <vl at samba.org>
---
 source3/smbd/reply.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/source3/smbd/reply.c b/source3/smbd/reply.c
index 8c7fc16..2f3df3f 100644
--- a/source3/smbd/reply.c
+++ b/source3/smbd/reply.c
@@ -346,8 +346,14 @@ size_t srvstr_get_path_req(TALLOC_CTX *mem_ctx, struct smb_request *req,
 size_t srvstr_pull_req_talloc(TALLOC_CTX *ctx, struct smb_request *req,
 			      char **dest, const char *src, int flags)
 {
+	ssize_t bufrem = smbreq_bufrem(req, src);
+
+	if (bufrem < 0) {
+		return 0;
+	}
+
 	return pull_string_talloc(ctx, req->inbuf, req->flags2, dest, src,
-				  smbreq_bufrem(req, src), flags);
+				  bufrem, flags);
 }
 
 /****************************************************************************
-- 
1.7.0.4



More information about the samba-technical mailing list