[SCM] Samba Shared Repository - branch v4-17-test updated

Jule Anger janger at samba.org
Wed Jan 4 21:24:01 UTC 2023


The branch, v4-17-test has been updated
       via  bc05daafbc6 s3:client: Fix a use-after-free issue in smbclient
       via  0d2acb2e228 s3:script: Improve test_chdir_cache.sh
       via  72e6fff0e5f s3:params:lp_do_section - protect against NULL deref
       via  4f47415e248 rpc_server:srvsvc - retrieve share ACL via root context
       via  0d89084e044 ctdb: Fix a use-after-free in run_proc
      from  72dcfb4773d VERSION: Bump version up to Samba 4.17.5...

https://git.samba.org/?p=samba.git;a=shortlog;h=v4-17-test


- Log -----------------------------------------------------------------
commit bc05daafbc6ffb972faa58943514ecd9f879cdad
Author: Andreas Schneider <asn at samba.org>
Date:   Thu Dec 22 10:31:11 2022 +0100

    s3:client: Fix a use-after-free issue in smbclient
    
    Detected by
    
        make test TESTS="samba3.blackbox.chdir-cache"
    
    with an optimized build or with AddressSanitizer.
    
    BUG: https://bugzilla.samba.org/show_bug.cgi?id=15268
    
    Signed-off-by: Andreas Schneider <asn at samba.org>
    Reviewed-by: Ralph Boehme <slow at samba.org>
    (cherry picked from commit 9c707b4be27e2a6f79886d3ec8b5066c922b99bd)
    
    Autobuild-User(v4-17-test): Jule Anger <janger at samba.org>
    Autobuild-Date(v4-17-test): Wed Jan  4 21:23:48 UTC 2023 on sn-devel-184

commit 0d2acb2e22846799e90d31b77431dda1dc4ef7de
Author: Andreas Schneider <asn at samba.org>
Date:   Thu Dec 22 10:36:02 2022 +0100

    s3:script: Improve test_chdir_cache.sh
    
    BUG: https://bugzilla.samba.org/show_bug.cgi?id=15268
    
    Signed-off-by: Andreas Schneider <asn at samba.org>
    Reviewed-by: Ralph Boehme <slow at samba.org>
    (cherry picked from commit 0d1961267cd9e8f1158a407c5d135514c363f37e)

commit 72e6fff0e5f61ad4f0d636bcd212120805577032
Author: Andrew Walker <awalker at ixsystems.com>
Date:   Mon Dec 19 08:17:47 2022 -0500

    s3:params:lp_do_section - protect against NULL deref
    
    iServiceIndex may indicate an empty slot in the ServicePtrs
    array. In this case, lpcfg_serivce_ok(ServicePtrs[iServiceIndex])
    may trigger a NULL deref and crash. Skipping the check
    here will cause a scan of the array in add_a_service() and the
    NULL slot will be used safely.
    
    BUG: https://bugzilla.samba.org/show_bug.cgi?id=15267
    
    Signed-off-by: Andrew Walker <awalker at ixsystems.com>
    Reviewed-by: Jeremy Allison <jra at samba.org>
    
    Autobuild-User(master): Jeremy Allison <jra at samba.org>
    Autobuild-Date(master): Tue Dec 20 18:49:54 UTC 2022 on sn-devel-184
    
    (cherry picked from commit 5b19288949e97a5af742ff2719992d56f21e364a)

commit 4f47415e248452dc34b10008474853bbc81a2165
Author: Andrew <awalker at ixsystems.com>
Date:   Fri Dec 16 08:16:10 2022 -0800

    rpc_server:srvsvc - retrieve share ACL via root context
    
    share_info.tdb has permissions of 0o600 and so we need
    to become_root() prior to retrieving the security info.
    
    BUG: https://bugzilla.samba.org/show_bug.cgi?id=15265
    
    Signed-off-by: Andrew Walker <awalker at ixsystems.com>
    Reviewed-by: Jeremy Allison <jra at samba.org>
    
    Autobuild-User(master): Jeremy Allison <jra at samba.org>
    Autobuild-Date(master): Mon Dec 19 20:41:15 UTC 2022 on sn-devel-184
    
    (cherry picked from commit 80c0b416892bfacc0d919fe032461748d7962f05)

commit 0d89084e0443c5cabb3f9cc6633f6b9c6ede29c1
Author: Volker Lendecke <vl at samba.org>
Date:   Fri Sep 30 17:02:41 2022 +0200

    ctdb: Fix a use-after-free in run_proc
    
    If you happen to talloc_free(run_ctx) before all the tevent_req's
    hanging off it, you run into the following:
    
    ==495196== Invalid read of size 8
    ==495196==    at 0x10D757: run_proc_state_destructor (run_proc.c:413)
    ==495196==    by 0x488F736: _tc_free_internal (talloc.c:1158)
    ==495196==    by 0x488FBDD: _talloc_free_internal (talloc.c:1248)
    ==495196==    by 0x4890F41: _talloc_free (talloc.c:1792)
    ==495196==    by 0x48538B1: tevent_req_received (tevent_req.c:293)
    ==495196==    by 0x4853429: tevent_req_destructor (tevent_req.c:129)
    ==495196==    by 0x488F736: _tc_free_internal (talloc.c:1158)
    ==495196==    by 0x4890AF6: _tc_free_children_internal (talloc.c:1669)
    ==495196==    by 0x488F967: _tc_free_internal (talloc.c:1184)
    ==495196==    by 0x488FBDD: _talloc_free_internal (talloc.c:1248)
    ==495196==    by 0x4890F41: _talloc_free (talloc.c:1792)
    ==495196==    by 0x10DE62: main (run_proc_test.c:86)
    ==495196==  Address 0x55b77f8 is 152 bytes inside a block of size 160 free'd
    ==495196==    at 0x48399AB: free (vg_replace_malloc.c:538)
    ==495196==    by 0x488FB25: _tc_free_internal (talloc.c:1222)
    ==495196==    by 0x488FBDD: _talloc_free_internal (talloc.c:1248)
    ==495196==    by 0x4890F41: _talloc_free (talloc.c:1792)
    ==495196==    by 0x10D315: run_proc_context_destructor (run_proc.c:329)
    ==495196==    by 0x488F736: _tc_free_internal (talloc.c:1158)
    ==495196==    by 0x488FBDD: _talloc_free_internal (talloc.c:1248)
    ==495196==    by 0x4890F41: _talloc_free (talloc.c:1792)
    ==495196==    by 0x10DE62: main (run_proc_test.c:86)
    ==495196==  Block was alloc'd at
    ==495196==    at 0x483877F: malloc (vg_replace_malloc.c:307)
    ==495196==    by 0x488EAD9: __talloc_with_prefix (talloc.c:783)
    ==495196==    by 0x488EC73: __talloc (talloc.c:825)
    ==495196==    by 0x488F0FC: _talloc_named_const (talloc.c:982)
    ==495196==    by 0x48925B1: _talloc_zero (talloc.c:2421)
    ==495196==    by 0x10C8F2: proc_new (run_proc.c:61)
    ==495196==    by 0x10D4C9: run_proc_send (run_proc.c:381)
    ==495196==    by 0x10DDF6: main (run_proc_test.c:79)
    
    This happens because run_proc_context_destructor() directly does a
    talloc_free() on the struct proc_context's and not the enclosing
    tevent_req's. run_proc_kill() makes sure that we don't follow
    proc->req, but it forgets the "state->proc", which is free()'ed, but
    later dereferenced in run_proc_state_destructor().
    
    This is an attempt at a quick fix, I believe we should convert
    run_proc_context->plist into an array of tevent_req's, so that we can
    properly TALLOC_FREE() according to the "natural" hierarchy and not
    just pull an arbitrary thread out of that heap.
    
    BUG: https://bugzilla.samba.org/show_bug.cgi?id=15269
    
    Signed-off-by: Volker Lendecke <vl at samba.org>
    Reviewed-by: Martin Schwenke <martin at meltin.net>
    
    Autobuild-User(master): Volker Lendecke <vl at samba.org>
    Autobuild-Date(master): Thu Oct  6 15:10:20 UTC 2022 on sn-devel-184
    
    (cherry picked from commit 688be0177b04d04709813a02ae6da1e983ac25dd)

-----------------------------------------------------------------------

Summary of changes:
 ctdb/common/run_proc.c                    |  5 +++--
 source3/client/client.c                   |  5 +++--
 source3/param/loadparm.c                  |  2 +-
 source3/rpc_server/srvsvc/srv_srvsvc_nt.c | 17 ++++++++++++++---
 source3/script/tests/test_chdir_cache.sh  | 12 ++++++++----
 5 files changed, 29 insertions(+), 12 deletions(-)


Changeset truncated at 500 lines:

diff --git a/ctdb/common/run_proc.c b/ctdb/common/run_proc.c
index d55af6c3a1e..84bc343ba1f 100644
--- a/ctdb/common/run_proc.c
+++ b/ctdb/common/run_proc.c
@@ -408,10 +408,10 @@ struct tevent_req *run_proc_send(TALLOC_CTX *mem_ctx,
 static int run_proc_state_destructor(struct run_proc_state *state)
 {
 	/* Do not get rid of the child process if timeout has occurred */
-	if (state->proc->req != NULL) {
+	if ((state->proc != NULL) && (state->proc->req != NULL)) {
 		state->proc->req = NULL;
 		DLIST_REMOVE(state->run_ctx->plist, state->proc);
-		talloc_free(state->proc);
+		TALLOC_FREE(state->proc);
 	}
 
 	return 0;
@@ -439,6 +439,7 @@ static void run_proc_kill(struct tevent_req *req)
 		req, struct run_proc_state);
 
 	state->proc->req = NULL;
+	state->proc = NULL;
 
 	state->result.sig = SIGKILL;
 
diff --git a/source3/client/client.c b/source3/client/client.c
index 651da5fbf7a..613fa4fd1f6 100644
--- a/source3/client/client.c
+++ b/source3/client/client.c
@@ -5126,10 +5126,11 @@ static int cmd_tcon(void)
 		return -1;
 	}
 
-	talloc_free(sharename);
-
 	d_printf("tcon to %s successful, tid: %u\n", sharename,
 		 cli_state_get_tid(cli));
+
+	talloc_free(sharename);
+
 	return 0;
 }
 
diff --git a/source3/param/loadparm.c b/source3/param/loadparm.c
index 7e20acbf8b9..0ebdd315bd1 100644
--- a/source3/param/loadparm.c
+++ b/source3/param/loadparm.c
@@ -2887,7 +2887,7 @@ bool lp_do_section(const char *pszSectionName, void *userdata)
 	/* if we have a current service, tidy it up before moving on */
 	bRetval = true;
 
-	if (iServiceIndex >= 0)
+	if ((iServiceIndex >= 0) && (ServicePtrs[iServiceIndex] != NULL))
 		bRetval = lpcfg_service_ok(ServicePtrs[iServiceIndex]);
 
 	/* if all is still well, move to the next record in the services array */
diff --git a/source3/rpc_server/srvsvc/srv_srvsvc_nt.c b/source3/rpc_server/srvsvc/srv_srvsvc_nt.c
index 233718ff310..fbc617c3ac1 100644
--- a/source3/rpc_server/srvsvc/srv_srvsvc_nt.c
+++ b/source3/rpc_server/srvsvc/srv_srvsvc_nt.c
@@ -540,6 +540,7 @@ static bool is_hidden_share(int snum)
 static bool is_enumeration_allowed(struct pipes_struct *p,
                                    int snum)
 {
+	bool allowed;
 	struct dcesrv_call_state *dce_call = p->dce_call;
 	struct auth_session_info *session_info =
 		dcesrv_call_session_info(dce_call);
@@ -556,9 +557,19 @@ static bool is_enumeration_allowed(struct pipes_struct *p,
 		return false;
 	}
 
-	return share_access_check(session_info->security_token,
-				  lp_servicename(talloc_tos(), lp_sub, snum),
-				  FILE_READ_DATA, NULL);
+
+	/*
+	 * share_access_check() must be opened as root
+	 * because it ultimately gets a R/W db handle on share_info.tdb
+	 * which has 0o600 permissions
+	 */
+	become_root();
+	allowed = share_access_check(session_info->security_token,
+				     lp_servicename(talloc_tos(), lp_sub, snum),
+				     FILE_READ_DATA, NULL);
+	unbecome_root();
+
+	return allowed;
 }
 
 /****************************************************************************
diff --git a/source3/script/tests/test_chdir_cache.sh b/source3/script/tests/test_chdir_cache.sh
index ae244acdd58..c649d2b07b3 100755
--- a/source3/script/tests/test_chdir_cache.sh
+++ b/source3/script/tests/test_chdir_cache.sh
@@ -33,7 +33,7 @@ conf_dir=$(dirname ${SERVERCONFFILE})
 log_file=${conf_dir}/../smbd_test.log
 
 error_inject_conf=${conf_dir}/error_inject.conf
->${error_inject_conf}
+rm -f ${error_inject_conf}
 
 incdir=$(dirname $0)/../../../testprogs/blackbox
 . $incdir/subunit.sh
@@ -80,7 +80,9 @@ head -n 4 <&101
 
 # Ensure any chdir will give EACCESS.
 echo "error_inject:chdir = EACCES" >${error_inject_conf}
-${SMBCONTROL} ${CONF} 0 reload-config
+testit "reload config 1" \
+	"${SMBCONTROL}" "${CONF}" smbd reload-config ||
+	failed=$((failed + 1))
 
 sleep 1
 
@@ -94,8 +96,10 @@ kill ${CLIENT_PID}
 rm -f smbclient-stdin smbclient-stdout smbclient-stderr
 
 # Remove the chdir inject.
->${error_inject_conf}
-${SMBCONTROL} ${CONF} 0 reload-config
+rm -f ${error_inject_conf}
+testit "reload config 2" \
+	"${SMBCONTROL}" "${CONF}" smbd reload-config ||
+	failed=$((failed + 1))
 
 # Now look for chdir_current_service: vfs_ChDir.*failed: Permission denied
 # in the smb log. There should be one more than before.


-- 
Samba Shared Repository



More information about the samba-cvs mailing list