Re: pkgsrc freeze for 2015Q2 branch started [was: Some fixes to GPG signature verification in pkg_install]

[Pierre Pronchery <khorben%defora.org@localhost>]

		Hi tech-pkg@, (pkgsrc-pmc@)

On 06/15/15 05:28, Thomas Klausner wrote:
> The freeze for the pkgsrc-2015Q2 branch has now started and is
> expected to last two weeks.
> 
> As usual, no infrastructure changes or non-leaf updates without prior
> approval.

I would like to request approval for the patch attached here - as
discussed in the past two weeks here, and reviewed by Joerg. From my
earlier post:

« It tries to ensures that there will always be an error message, even
if the path configured to GPG in pkg_install.conf(5) points to a
non-executable file, and that file descriptors and temporary files are
closed and deleted respectively once no longer necessary.

Without this patch, the "/tmp" directory (or temporary directory as
defined in the "TMPDIR" environment variable) keeps getting a new
"pkg_install.XXXXXX" file entry for each signature verification that
failed.

[...]

Among other things this patch improves the interaction of pkg_add(1)
with pkgin(1) when dealing with signed packages (and such errors). It is
not security critical in any way that I can think of. »

Although not being security critical, this patch largely improves user
interaction with handling signed packages. It only affects the handling
of GPG signatures; this includes the verification of the
pkg-vulnerabilities file, but I also checked the patch to be correct
there. I therefore believe the patch should not harm any other
functionality in pkg_install.

The version attached here was generated once applied to pkgsrc's CVS
repository.

HTH,
-- 
khorben

Index: lib/gpgsig.c
===================================================================
RCS file: /cvsroot/pkgsrc/pkgtools/pkg_install/files/lib/gpgsig.c,v
retrieving revision 1.3
diff -p -u -r1.3 gpgsig.c
--- lib/gpgsig.c	2 Aug 2009 17:56:45 -0000	1.3
+++ lib/gpgsig.c	15 Jun 2015 18:06:01 -0000
@@ -52,28 +52,35 @@ __RCSID("$NetBSD: gpgsig.c,v 1.3 2009/08
 
 #include "lib.h"
 
-static void
+static int
 verify_signature(const char *input, size_t input_len, const char *keyring,
     const char *detached_signature)
 {
 	const char *argv[8], **argvp;
 	pid_t child;
 	int fd[2], status;
+	int error = 0;
+	char const * err_msg;
 
-	if (pipe(fd) == -1)
-		err(EXIT_FAILURE, "cannot create input pipes");
+	if (pipe(fd) == -1) {
+		warn("Cannot create input pipes");
+		return -1;
+	}
 
 	child = vfork();
-	if (child == -1)
-		err(EXIT_FAILURE, "cannot fork GPG process");
+	if (child == -1) {
+		warn("Cannot fork GPG process");
+		close(fd[0]);
+		close(fd[1]);
+		return -1;
+	}
 	if (child == 0) {
 		close(fd[1]);
 		close(STDIN_FILENO);
 		if (dup2(fd[0], STDIN_FILENO) == -1) {
-			static const char err_msg[] =
-			    "cannot redirect stdin of GPG process\n";
-			write(STDERR_FILENO, err_msg, sizeof(err_msg) - 1);
-			_exit(255);
+			err_msg = "Cannot redirect stdin of GPG process\n";
+			write(STDERR_FILENO, err_msg, strlen(err_msg));
+			_exit(2);
 		}
 		close(fd[0]);
 		argvp = argv;
@@ -92,23 +99,38 @@ verify_signature(const char *input, size
 		*argvp = NULL;
 
 		execvp(gpg_cmd, __UNCONST(argv));
-		_exit(255);
+		/* we would call warn(gpg_cmd) but stdio is not allowed */
+		write(STDERR_FILENO, gpg_cmd, strlen(gpg_cmd));
+		err_msg = ": Could not execute GPG\n";
+		write(STDERR_FILENO, err_msg, strlen(err_msg));
+		_exit(2);
 	}
 	close(fd[0]);
-	if (write(fd[1], input, input_len) != (ssize_t)input_len)
-		errx(EXIT_FAILURE, "Short read from GPG");
+	if (waitpid(child, &status, WNOHANG) != 0) {
+		warnx("GPG could not verify the signature");
+		close(fd[0]);
+		close(fd[1]);
+		return 1;
+	}
+	/* XXX dies on SIGPIPE if the child exited in the meantime (race) */
+	if (write(fd[1], input, input_len) != (ssize_t)input_len) {
+		warnx("Short read from GPG");
+		error = 1;
+	}
 	close(fd[1]);
-	waitpid(child, &status, 0);
-	if (status)
-		errx(EXIT_FAILURE, "GPG could not verify the signature");
+	if (waitpid(child, &status, 0) == -1
+			|| !WIFEXITED(status)
+			|| WEXITSTATUS(status)) {
+		warnx("GPG could not verify the signature");
+		error = 1;
+	}
+	return error;
 }
 
 int
 inline_gpg_verify(const char *content, size_t len, const char *keyring)
 {
-	verify_signature(content, len, keyring, NULL);
-
-	return 0;
+	return verify_signature(content, len, keyring, NULL);
 }
 
 int
@@ -119,6 +141,7 @@ detached_gpg_verify(const char *content,
 	const char *tmpdir;
 	char *tempsig;
 	ssize_t ret;
+	int error = 0;
 
 	if (gpg_cmd == NULL) {
 		warnx("GPG variable not set, failing signature check");
@@ -132,26 +155,34 @@ detached_gpg_verify(const char *content,
 	fd = mkstemp(tempsig);
 	if (fd == -1) {
 		warnx("Creating temporary file for GPG signature failed");
+		free(tempsig);
 		return -1;
 	}
 
 	while (signature_len) {
 		ret = write(fd, signature, signature_len);
-		if (ret == -1)
-			err(EXIT_FAILURE, "Write to GPG failed");
-		if (ret == 0)
-			errx(EXIT_FAILURE, "Short write to GPG");
+		if (ret == -1) {
+			warn("Write to GPG failed");
+			error = 1;
+			break;
+		}
+		if (ret == 0) {
+			warnx("Short write to GPG");
+			error = 1;
+			break;
+		}
 		signature_len -= ret;
 		signature += ret;
 	}
 
-	verify_signature(content, len, keyring, tempsig);
+	if (error == 0)
+		error = verify_signature(content, len, keyring, tempsig);
 
 	unlink(tempsig);
 	close(fd);
 	free(tempsig);
 
-	return 0;
+	return error;
 }
 
 int
@@ -163,18 +194,33 @@ detached_gpg_sign(const char *content, s
 	int fd_in[2], fd_out[2], status;
 	size_t allocated;
 	ssize_t ret;
+	int error = 0;
 
-	if (gpg_cmd == NULL)
-		errx(EXIT_FAILURE, "GPG variable not set");
+	if (gpg_cmd == NULL) {
+		warnx("GPG variable not set");
+		return 1;
+	}
 
-	if (pipe(fd_in) == -1)
-		err(EXIT_FAILURE, "cannot create input pipes");
-	if (pipe(fd_out) == -1)
-		err(EXIT_FAILURE, "cannot create output pipes");
+	if (pipe(fd_in) == -1) {
+		warn("Cannot create input pipes");
+		return 1;
+	}
+	if (pipe(fd_out) == -1) {
+		warn("Cannot create output pipes");
+		close(fd_in[0]);
+		close(fd_in[1]);
+		return 1;
+	}
 
 	child = fork();
-	if (child == -1)
-		err(EXIT_FAILURE, "cannot fork GPG process");
+	if (child == -1) {
+		warn("Cannot fork GPG process");
+		close(fd_in[0]);
+		close(fd_in[1]);
+		close(fd_out[0]);
+		close(fd_out[1]);
+		return 1;
+	}
 	if (child == 0) {
 		close(fd_in[1]);
 		close(STDIN_FILENO);
@@ -182,7 +228,7 @@ detached_gpg_sign(const char *content, s
 			static const char err_msg[] =
 			    "cannot redirect stdin of GPG process\n";
 			write(STDERR_FILENO, err_msg, sizeof(err_msg) - 1);
-			_exit(255);
+			_exit(2);
 		}
 		close(fd_in[0]);
 
@@ -192,7 +238,7 @@ detached_gpg_sign(const char *content, s
 			static const char err_msg[] =
 			    "cannot redirect stdout of GPG process\n";
 			write(STDERR_FILENO, err_msg, sizeof(err_msg) - 1);
-			_exit(255);
+			_exit(2);
 		}
 		close(fd_out[1]);
 
@@ -216,11 +262,14 @@ detached_gpg_sign(const char *content, s
 		*argvp = NULL;
 
 		execvp(gpg_cmd, __UNCONST(argv));
-		_exit(255);
+		warn("%s", gpg_cmd);
+		_exit(2);
 	}
 	close(fd_in[0]);
-	if (write(fd_in[1], content, len) != (ssize_t)len)
-		errx(EXIT_FAILURE, "Short read from GPG");
+	if (write(fd_in[1], content, len) != (ssize_t)len) {
+		warnx("Short read from GPG");
+		error = 1;
+	}
 	close(fd_in[1]);
 
 	allocated = 1024;
@@ -240,9 +289,12 @@ detached_gpg_sign(const char *content, s
 
 	close(fd_out[0]);
 
-	waitpid(child, &status, 0);
-	if (status)
-		errx(EXIT_FAILURE, "GPG could not create signature");
+	if (waitpid(child, &status, 0) == -1
+			|| !WIFEXITED(status)
+			|| WEXITSTATUS(status)) {
+		warnx("GPG could not create signature");
+		error = 1;
+	}
 
-	return 0;
+	return error;
 }
Index: lib/vulnerabilities-file.c
===================================================================
RCS file: /cvsroot/pkgsrc/pkgtools/pkg_install/files/lib/vulnerabilities-file.c,v
retrieving revision 1.7
diff -p -u -r1.7 vulnerabilities-file.c
--- lib/vulnerabilities-file.c	16 Jun 2010 23:02:49 -0000	1.7
+++ lib/vulnerabilities-file.c	15 Jun 2015 18:06:01 -0000
@@ -115,7 +115,8 @@ verify_signature(const char *input, size
 		    "At least GPG or CERTIFICATE_ANCHOR_PKGVULN "
 		    "must be configured");
 	if (gpg_cmd != NULL)
-		inline_gpg_verify(input, input_len, gpg_keyring_pkgvuln);
+		if (inline_gpg_verify(input, input_len, gpg_keyring_pkgvuln))
+			exit(EXIT_FAILURE);
 	if (certs_pkg_vulnerabilities != NULL)
 		verify_signature_pkcs7(input);
 }