[pacman-dev,2/2,WIP] run XferCommand via exec

Message ID 20190609171355.5615-2-andrew.gregory.8@gmail.com
State Superseded, archived
Headers show
Series
  • [pacman-dev,1/2,WIP] move wordsplit into ini for sharing
Related show

Commit Message

Andrew Gregory June 9, 2019, 5:13 p.m. UTC
---

systemvp should pretty much be a drop-in replacement for system with
the exception that it takes an argv array and uses exec.  If anybody
wants to play with it to stress test it a little, I have
a self-contained copy and test program at:
https://github.com/andrewgregory/snippets/blob/systemv/c/systemv.c

TODO:
    * update docs
    * fix debug logging
    * should the command be run with PATH lookup (execv vs execvp)?
    * Is the use of mmap with MAP_ANONYMOUS okay?  MAP_ANONYMOUS is
      not POSIX but "most systems also support MAP_ANONYMOUS (or its
      synonym MAP_ANON)" (mmap(2)).
    * should we reset signals prior to exec'ing like we do with
      hooks/scripts?

 src/pacman/conf.c | 95 ++++++++++++++++++++++++++++++++++++++---------
 src/pacman/conf.h |  2 +
 2 files changed, 80 insertions(+), 17 deletions(-)

Comments

Morten Linderud Oct. 17, 2019, 3:01 p.m. UTC | #1
On Sun, Jun 09, 2019 at 10:13:55AM -0700, Andrew Gregory wrote:
> ---
> 
> systemvp should pretty much be a drop-in replacement for system with
> the exception that it takes an argv array and uses exec.  If anybody
> wants to play with it to stress test it a little, I have
> a self-contained copy and test program at:
> https://github.com/andrewgregory/snippets/blob/systemv/c/systemv.c
> 
> TODO:
>     * update docs
>     * fix debug logging
>     * should the command be run with PATH lookup (execv vs execvp)?
>     * Is the use of mmap with MAP_ANONYMOUS okay?  MAP_ANONYMOUS is
>       not POSIX but "most systems also support MAP_ANONYMOUS (or its
>       synonym MAP_ANON)" (mmap(2)).
>     * should we reset signals prior to exec'ing like we do with
>       hooks/scripts?

This issue was assigned CVE-2019-18182.

https://security.archlinux.org/CVE-2019-18182

I'm fixing the AVG whenever pacman 5.2 is released if Xfer isn't included.
Morten Linderud Oct. 17, 2019, 3:04 p.m. UTC | #2
On Thu, Oct 17, 2019 at 05:01:46PM +0200, Morten Linderud wrote:
> On Sun, Jun 09, 2019 at 10:13:55AM -0700, Andrew Gregory wrote:
> > ---
> > 
> > systemvp should pretty much be a drop-in replacement for system with
> > the exception that it takes an argv array and uses exec.  If anybody
> > wants to play with it to stress test it a little, I have
> > a self-contained copy and test program at:
> > https://github.com/andrewgregory/snippets/blob/systemv/c/systemv.c
> > 
> > TODO:
> >     * update docs
> >     * fix debug logging
> >     * should the command be run with PATH lookup (execv vs execvp)?
> >     * Is the use of mmap with MAP_ANONYMOUS okay?  MAP_ANONYMOUS is
> >       not POSIX but "most systems also support MAP_ANONYMOUS (or its
> >       synonym MAP_ANON)" (mmap(2)).
> >     * should we reset signals prior to exec'ing like we do with
> >       hooks/scripts?
> 
> This issue was assigned CVE-2019-18182.
> 
> https://security.archlinux.org/CVE-2019-18182
> 
> I'm fixing the AVG whenever pacman 5.2 is released if Xfer isn't included.
> 

Uh. I might not have paid attention. Eli mentioned on -security Xfer might not
be included in the upcomming release, but then anthraxx pointed out it's in
master :o Whats the status?
Eli Schwartz Oct. 17, 2019, 3:47 p.m. UTC | #3
On 10/17/19 11:04 AM, Morten Linderud wrote:
> On Thu, Oct 17, 2019 at 05:01:46PM +0200, Morten Linderud wrote:
>> On Sun, Jun 09, 2019 at 10:13:55AM -0700, Andrew Gregory wrote:
>>> ---
>>>
>>> systemvp should pretty much be a drop-in replacement for system with
>>> the exception that it takes an argv array and uses exec.  If anybody
>>> wants to play with it to stress test it a little, I have
>>> a self-contained copy and test program at:
>>> https://github.com/andrewgregory/snippets/blob/systemv/c/systemv.c
>>>
>>> TODO:
>>>     * update docs
>>>     * fix debug logging
>>>     * should the command be run with PATH lookup (execv vs execvp)?
>>>     * Is the use of mmap with MAP_ANONYMOUS okay?  MAP_ANONYMOUS is
>>>       not POSIX but "most systems also support MAP_ANONYMOUS (or its
>>>       synonym MAP_ANON)" (mmap(2)).
>>>     * should we reset signals prior to exec'ing like we do with
>>>       hooks/scripts?
>>
>> This issue was assigned CVE-2019-18182.
>>
>> https://security.archlinux.org/CVE-2019-18182
>>
>> I'm fixing the AVG whenever pacman 5.2 is released if Xfer isn't included.
>>
> 
> Uh. I might not have paid attention. Eli mentioned on -security Xfer might not
> be included in the upcomming release, but then anthraxx pointed out it's in
> master :o Whats the status?

Just to clarify, "might not be included in the upcoming release" was
before the v2 patch series posted on Friday. Before then, it was unclear
if the v1 patch series (which was marked as WIP with some TODO items)
would be finished before the upcoming release.

This has landed in master as the following commit:

https://git.archlinux.org/pacman.git/commit/?id=808a4f15ce82d2ed7eeb06de73d0f313620558ee

And is mentioned in the NEWS file which is prepared here:
https://patchwork.archlinux.org/patch/1280/
Morten Linderud Oct. 17, 2019, 3:50 p.m. UTC | #4
On Thu, Oct 17, 2019 at 11:47:58AM -0400, Eli Schwartz wrote:
> On 10/17/19 11:04 AM, Morten Linderud wrote:
> > On Thu, Oct 17, 2019 at 05:01:46PM +0200, Morten Linderud wrote:
> >> On Sun, Jun 09, 2019 at 10:13:55AM -0700, Andrew Gregory wrote:
> >>> ---
> >>>
> >>> systemvp should pretty much be a drop-in replacement for system with
> >>> the exception that it takes an argv array and uses exec.  If anybody
> >>> wants to play with it to stress test it a little, I have
> >>> a self-contained copy and test program at:
> >>> https://github.com/andrewgregory/snippets/blob/systemv/c/systemv.c
> >>>
> >>> TODO:
> >>>     * update docs
> >>>     * fix debug logging
> >>>     * should the command be run with PATH lookup (execv vs execvp)?
> >>>     * Is the use of mmap with MAP_ANONYMOUS okay?  MAP_ANONYMOUS is
> >>>       not POSIX but "most systems also support MAP_ANONYMOUS (or its
> >>>       synonym MAP_ANON)" (mmap(2)).
> >>>     * should we reset signals prior to exec'ing like we do with
> >>>       hooks/scripts?
> >>
> >> This issue was assigned CVE-2019-18182.
> >>
> >> https://security.archlinux.org/CVE-2019-18182
> >>
> >> I'm fixing the AVG whenever pacman 5.2 is released if Xfer isn't included.
> >>
> > 
> > Uh. I might not have paid attention. Eli mentioned on -security Xfer might not
> > be included in the upcomming release, but then anthraxx pointed out it's in
> > master :o Whats the status?
> 
> Just to clarify, "might not be included in the upcoming release" was
> before the v2 patch series posted on Friday. Before then, it was unclear
> if the v1 patch series (which was marked as WIP with some TODO items)
> would be finished before the upcoming release.
> 
> This has landed in master as the following commit:
> 
> https://git.archlinux.org/pacman.git/commit/?id=808a4f15ce82d2ed7eeb06de73d0f313620558ee
> 
> And is mentioned in the NEWS file which is prepared here:
> https://patchwork.archlinux.org/patch/1280/
> 

Ack thanks. That was what anthraxx also wrote to me but the previous mail was
sent a bit too quickly.

Patch

diff --git a/src/pacman/conf.c b/src/pacman/conf.c
index 2d8518c4..faf446dc 100644
--- a/src/pacman/conf.c
+++ b/src/pacman/conf.c
@@ -26,9 +26,11 @@ 
 #include <stdlib.h>
 #include <stdio.h>
 #include <string.h> /* strdup */
+#include <sys/mman.h>
 #include <sys/stat.h>
 #include <sys/types.h>
 #include <sys/utsname.h> /* uname */
+#include <sys/wait.h>
 #include <unistd.h>
 
 /* pacman */
@@ -153,6 +155,7 @@  int config_free(config_t *oldconfig)
 	free(oldconfig->print_format);
 	free(oldconfig->arch);
 	free(oldconfig);
+	_alpm_wordsplit_free(oldconfig->xfercommand_argv);
 
 	return 0;
 }
@@ -201,18 +204,59 @@  static char *get_tempfile(const char *path, const char *filename)
 	return tempfile;
 }
 
+static int systemvp(const char *file, char *const argv[])
+{
+	int pid, *err;
+
+	err = mmap(NULL, sizeof(*err), PROT_READ | PROT_WRITE,
+			MAP_SHARED | MAP_ANONYMOUS, -1, 0);
+	if(err == NULL) {
+		return -1;
+	}
+
+	pid = fork();
+	if(pid == -1) {
+		/* error */
+		munmap(err, sizeof(*err));
+		return -1;
+	} else if(pid == 0) {
+		/* child */
+		*err = 0;
+		execvp(file, argv);
+		*err = errno;
+		munmap(err, sizeof(*err));
+		_Exit(1);
+	} else {
+		/* parent */
+		int status;
+		while(waitpid(pid, &status, 0) == -1) {
+			if(errno != EINTR) {
+				munmap(err, sizeof(*err));
+				return -1;
+			}
+		}
+		if(*err != 0) {
+			errno = *err;
+			status = -1;
+		}
+		munmap(err, sizeof(*err));
+		return status;
+	}
+}
+
 /** External fetch callback */
 static int download_with_xfercommand(const char *url, const char *localpath,
 		int force)
 {
 	int ret = 0, retval;
 	int usepart = 0;
-	int cwdfd;
+	int cwdfd = -1;
 	struct stat st;
-	char *parsedcmd, *tempcmd;
 	char *destfile, *tempfile, *filename;
+	const char **argv;
+	size_t i;
 
-	if(!config->xfercommand) {
+	if(!config->xfercommand_argv) {
 		return -1;
 	}
 
@@ -230,17 +274,20 @@  static int download_with_xfercommand(const char *url, const char *localpath,
 		unlink(destfile);
 	}
 
-	tempcmd = strdup(config->xfercommand);
-	/* replace all occurrences of %o with fn.part */
-	if(strstr(tempcmd, "%o")) {
-		usepart = 1;
-		parsedcmd = strreplace(tempcmd, "%o", tempfile);
-		free(tempcmd);
-		tempcmd = parsedcmd;
+	if((argv = calloc(config->xfercommand_argc + 1, sizeof(char*))) == NULL) {
+		pm_printf(ALPM_LOG_ERROR, _("could not get current working directory\n"));
+		goto cleanup;
+	}
+
+	for(i = 0; i <= config->xfercommand_argc; i++) {
+		const char *val = config->xfercommand_argv[i];
+		if(val && strcmp(val, "%o") == 0) {
+			val = tempfile;
+		} else if(val && strcmp(val, "%u") == 0) {
+			val = url;
+		}
+		argv[i] = val;
 	}
-	/* replace all occurrences of %u with the download URL */
-	parsedcmd = strreplace(tempcmd, "%u", url);
-	free(tempcmd);
 
 	/* save the cwd so we can restore it later */
 	do {
@@ -256,9 +303,13 @@  static int download_with_xfercommand(const char *url, const char *localpath,
 		ret = -1;
 		goto cleanup;
 	}
-	/* execute the parsed command via /bin/sh -c */
-	pm_printf(ALPM_LOG_DEBUG, "running command: %s\n", parsedcmd);
-	retval = system(parsedcmd);
+
+	printf("running command: %s", config->xfercommand_argv[0]);
+	for(i = 1; argv[i]; i++) {
+		printf(" '%s'", argv[i]);
+	}
+	printf("\n");
+	retval = systemvp(argv[0], (char**)argv);
 
 	if(retval == -1) {
 		pm_printf(ALPM_LOG_WARNING, _("running XferCommand: fork failed!\n"));
@@ -296,7 +347,6 @@  cleanup:
 	}
 	free(destfile);
 	free(tempfile);
-	free(parsedcmd);
 
 	return ret;
 }
@@ -544,6 +594,17 @@  static int _parse_options(const char *key, char *value,
 				pm_printf(ALPM_LOG_DEBUG, "config: logfile: %s\n", value);
 			}
 		} else if(strcmp(key, "XferCommand") == 0) {
+			char **c;
+			if((config->xfercommand_argv = _alpm_wordsplit(value)) == NULL) {
+				pm_printf(ALPM_LOG_WARNING,
+						_("config file %s, line %d: invalid value for '%s' : '%s'\n"),
+						file, linenum, "XferCommand", value);
+				return 1;
+			}
+			config->xfercommand_argc = 0;
+			for(c = config->xfercommand_argv; *c; c++) {
+				config->xfercommand_argc++;
+			}
 			config->xfercommand = strdup(value);
 			pm_printf(ALPM_LOG_DEBUG, "config: xfercommand: %s\n", value);
 		} else if(strcmp(key, "CleanMethod") == 0) {
diff --git a/src/pacman/conf.h b/src/pacman/conf.h
index f45ed436..47df979d 100644
--- a/src/pacman/conf.h
+++ b/src/pacman/conf.h
@@ -125,6 +125,8 @@  typedef struct __config_t {
 	alpm_list_t *noextract;
 	alpm_list_t *overwrite_files;
 	char *xfercommand;
+	char **xfercommand_argv;
+	size_t xfercommand_argc;
 
 	/* our connection to libalpm */
 	alpm_handle_t *handle;