[PATCH] Remove sock_exec code

Gary Lockyer gary at catalyst.net.nz
Thu Nov 16 18:18:07 UTC 2017


Updated patch attached, have revised the commit message in line with
Andrews comments

Gary

On 15/11/17 00:10, Andrew Bartlett via samba-technical wrote:
> I think you should mention that you are motivated by past exploits of
> Samba that used sock_exec as a proxy for system() matching a talloc
> destructor prototype. 
> 
> See for example
> https://gist.github.com/worawit/051e881fc94fe4a49295
> 
> and referencing the Red Hat post at:
> 
> https://access.redhat.com/blogs/766093/posts/1976553
> 
> The same motivation is for the close-on-exec change, making a repeat of
> this exploit just a little more miserable by not allowing something
> called via system() access to the FD back to the client. 
> 
> Thanks,
> 
> Andrew Bartlett
> 
> On Thu, 2017-11-09 at 15:09 +1300, Gary Lockyer via samba-technical
> wrote:
>> Updated patch attached, removes the man page entry.
>>
>> On 09/11/17 05:11, Andreas Schneider via samba-technical wrote:
>>> On Sunday, 5 November 2017 18:54:32 CET Gary Lockyer via samba-technical 
>>> wrote:
>>>> Patch to remove the sock_exec code, my understanding is that this was
>>>> originally test support code and is no longer used.
>>>>
>>>> Comments and reviews appreciated
>>>
>>> man smbclient -> LIBSMB_PROG
>>>
>>> I think you should remove that too if you want to get rid of it :-)
>>>
>>>
>>> 	andreas
>>>
-------------- next part --------------
From adc924cc00297faddf174b94308e883a1d35c8bb Mon Sep 17 00:00:00 2001
From: Gary Lockyer <gary at catalyst.net.nz>
Date: Fri, 3 Nov 2017 13:35:41 +1300
Subject: [PATCH] source3: remove sock_exec

Remove the sock_exec code which is no longer needed and additionally has been
used by exploit code.

This was originally test support code, the tests relying on the sock_exec
code have been removed.

Past exploits have used sock_exec as a proxy for system() matching a talloc
destructor prototype.

See for example:
Exploit for Samba vulnerabilty (CVE-2015-0240) at
    https://gist.github.com/worawit/051e881fc94fe4a49295
    and the Red Hat post at
    https://access.redhat.com/blogs/766093/posts/1976553

Signed-off-by: Gary Lockyer <gary at catalyst.net.nz>
---
 docs-xml/manpages/smbclient.1.xml |   6 --
 source3/include/proto.h           |   4 --
 source3/lib/sock_exec.c           | 118 --------------------------------------
 source3/libsmb/cliconnect.c       |  14 -----
 source3/wscript_build             |   1 -
 testsuite/build_farm/basicsmb.fns |   2 -
 6 files changed, 145 deletions(-)
 delete mode 100644 source3/lib/sock_exec.c

diff --git a/docs-xml/manpages/smbclient.1.xml b/docs-xml/manpages/smbclient.1.xml
index 0325982..1349a67 100644
--- a/docs-xml/manpages/smbclient.1.xml
+++ b/docs-xml/manpages/smbclient.1.xml
@@ -1216,12 +1216,6 @@
 	the password of the person using the client.  This information is
 	used only if the protocol level is high enough to support
 	session-level passwords. </para>
-
-	<para>The variable <envar>LIBSMB_PROG</envar> may contain
-	the path, executed with system(), which the client should connect
-		to instead of connecting to a server.  This functionality is primarily
-		intended as a development aid, and works best when using a LMHOSTS
-		file</para>
 </refsect1>
 
 
diff --git a/source3/include/proto.h b/source3/include/proto.h
index c86cd44..fa87407 100644
--- a/source3/include/proto.h
+++ b/source3/include/proto.h
@@ -158,10 +158,6 @@ int smbrun_no_sanitize(const char *cmd, int *outfd, char * const *env);
 int smbrun(const char *cmd, int *outfd, char * const *env);
 int smbrunsecret(const char *cmd, const char *secret);
 
-/* The following definitions come from lib/sock_exec.c  */
-
-int sock_exec(const char *prog);
-
 /* The following definitions come from lib/substitute.c  */
 
 void free_local_machine_name(void);
diff --git a/source3/lib/sock_exec.c b/source3/lib/sock_exec.c
deleted file mode 100644
index 3d2ace9..0000000
--- a/source3/lib/sock_exec.c
+++ /dev/null
@@ -1,118 +0,0 @@
-/* 
-   Unix SMB/CIFS implementation.
-   Samba utility functions
-   Copyright (C) Andrew Tridgell 1992-1998
-   Copyright (C) Tim Potter      2000-2001
-   
-   This program is free software; you can redistribute it and/or modify
-   it under the terms of the GNU General Public License as published by
-   the Free Software Foundation; either version 3 of the License, or
-   (at your option) any later version.
-   
-   This program is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-   GNU General Public License for more details.
-   
-   You should have received a copy of the GNU General Public License
-   along with this program.  If not, see <http://www.gnu.org/licenses/>.
-*/
-
-#include "includes.h"
-
-/*******************************************************************
-this is like socketpair but uses tcp. It is used by the Samba
-regression test code
-The function guarantees that nobody else can attach to the socket,
-or if they do that this function fails and the socket gets closed
-returns 0 on success, -1 on failure
-the resulting file descriptors are symmetrical
- ******************************************************************/
-static int socketpair_tcp(int fd[2])
-{
-	int listener;
-	struct sockaddr_in sock;
-	struct sockaddr_in sock2;
-	socklen_t socklen = sizeof(sock);
-	int connect_done = 0;
-	
-	fd[0] = fd[1] = listener = -1;
-
-	memset(&sock, 0, sizeof(sock));
-	
-	if ((listener = socket(PF_INET, SOCK_STREAM, 0)) == -1) goto failed;
-
-        memset(&sock2, 0, sizeof(sock2));
-#ifdef HAVE_SOCK_SIN_LEN
-        sock2.sin_len = sizeof(sock2);
-#endif
-        sock2.sin_family = PF_INET;
-
-        if (bind(listener, (struct sockaddr *)&sock2, sizeof(sock2)) != 0) goto failed;
-
-	if (listen(listener, 1) != 0) goto failed;
-
-	if (getsockname(listener, (struct sockaddr *)&sock, &socklen) != 0) goto failed;
-
-	if ((fd[1] = socket(PF_INET, SOCK_STREAM, 0)) == -1) goto failed;
-
-	set_blocking(fd[1], 0);
-
-	sock.sin_addr.s_addr = htonl(INADDR_LOOPBACK);
-
-	if (connect(fd[1], (struct sockaddr *)&sock, socklen) == -1) {
-		if (errno != EINPROGRESS) goto failed;
-	} else {
-		connect_done = 1;
-	}
-
-	if ((fd[0] = accept(listener, (struct sockaddr *)&sock, &socklen)) == -1) goto failed;
-
-	if (connect_done == 0) {
-		if (connect(fd[1], (struct sockaddr *)&sock, socklen) != 0
-		    && errno != EISCONN) goto failed;
-	}
-	close(listener);
-
-	set_blocking(fd[1], 1);
-
-	/* all OK! */
-	return 0;
-
- failed:
-	if (fd[0] != -1) close(fd[0]);
-	if (fd[1] != -1) close(fd[1]);
-	if (listener != -1) close(listener);
-	return -1;
-}
-
-
-/*******************************************************************
-run a program on a local tcp socket, this is used to launch smbd
-when regression testing
-the return value is a socket which is attached to a subprocess
-running "prog". stdin and stdout are attached. stderr is left
-attached to the original stderr
- ******************************************************************/
-int sock_exec(const char *prog)
-{
-	int fd[2];
-	if (socketpair_tcp(fd) != 0) {
-		DEBUG(0,("socketpair_tcp failed (%s)\n", strerror(errno)));
-		return -1;
-	}
-	if (fork() == 0) {
-		close(fd[0]);
-		close(0);
-		close(1);
-		if (dup(fd[1]) == -1) {
-			exit(1);
-		}
-		if (dup(fd[1]) == -1) {
-			exit(1);
-		}
-		exit(system(prog));
-	}
-	close(fd[1]);
-	return fd[0];
-}
diff --git a/source3/libsmb/cliconnect.c b/source3/libsmb/cliconnect.c
index 70bcead..26bf569 100644
--- a/source3/libsmb/cliconnect.c
+++ b/source3/libsmb/cliconnect.c
@@ -2505,7 +2505,6 @@ static struct tevent_req *cli_connect_sock_send(
 {
 	struct tevent_req *req, *subreq;
 	struct cli_connect_sock_state *state;
-	const char *prog;
 	struct sockaddr_storage *addrs;
 	unsigned i, num_addrs;
 	NTSTATUS status;
@@ -2516,19 +2515,6 @@ static struct tevent_req *cli_connect_sock_send(
 		return NULL;
 	}
 
-	prog = getenv("LIBSMB_PROG");
-	if (prog != NULL) {
-		state->fd = sock_exec(prog);
-		if (state->fd == -1) {
-			status = map_nt_error_from_unix(errno);
-			tevent_req_nterror(req, status);
-		} else {
-			state->port = 0;
-			tevent_req_done(req);
-		}
-		return tevent_req_post(req, ev);
-	}
-
 	if ((pss == NULL) || is_zero_addr(pss)) {
 
 		/*
diff --git a/source3/wscript_build b/source3/wscript_build
index dc7a51c..f4c3eb5 100644
--- a/source3/wscript_build
+++ b/source3/wscript_build
@@ -304,7 +304,6 @@ bld.SAMBA3_SUBSYSTEM('samba3util',
                           lib/util_tsock.c
                           lib/util_transfer_file.c
                           lib/sys_popen.c
-                          lib/sock_exec.c
                           ''',
                    deps='''
                         CHARSET3
diff --git a/testsuite/build_farm/basicsmb.fns b/testsuite/build_farm/basicsmb.fns
index 3a9080f..d1127e8 100644
--- a/testsuite/build_farm/basicsmb.fns
+++ b/testsuite/build_farm/basicsmb.fns
@@ -177,8 +177,6 @@ test_listfilesauth_should_deny() {
 	return 0
 }
 
-echo "LIBSMB_PROG=$LIBSMB_PROG" >&2
-
 
 
 # Give sensible defaults to some variables.  
-- 
2.7.4

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20171117/2adc148a/signature.sig>


More information about the samba-technical mailing list