Re: Some fixes to GPG signature verification in pkg_install

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

			Hi there,

On 05/30/15 16:10, Pierre Pronchery wrote:
> [...]

I have updated the patch again (attached here). It does not use
strerror(3) in vfork(2) context anymore either.

HTH,
-- 
khorben

diff --git a/external/bsd/pkg_install/dist/lib/gpgsig.c b/external/bsd/pkg_install/dist/lib/gpgsig.c
index 8091d4a..9c60e10 100644
--- a/external/bsd/pkg_install/dist/lib/gpgsig.c
+++ b/external/bsd/pkg_install/dist/lib/gpgsig.c
@@ -52,28 +52,35 @@ __RCSID("$NetBSD: gpgsig.c,v 1.1.1.2 2009/08/06 16:55:27 joerg Exp $");
 
 #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_t input_len, const char *keyring,
 		*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, size_t len,
 	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, size_t len,
 	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, size_t len, char **sig, size_t *sig_len,
 	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, size_t len, char **sig, size_t *sig_len,
 			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, size_t len, char **sig, size_t *sig_len,
 			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, size_t len, char **sig, size_t *sig_len,
 		*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, size_t len, char **sig, size_t *sig_len,
 
 	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;
 }
diff --git a/external/bsd/pkg_install/dist/lib/vulnerabilities-file.c b/external/bsd/pkg_install/dist/lib/vulnerabilities-file.c
index e183689..ddc55e1 100644
--- a/external/bsd/pkg_install/dist/lib/vulnerabilities-file.c
+++ b/external/bsd/pkg_install/dist/lib/vulnerabilities-file.c
@@ -115,7 +115,8 @@ verify_signature(const char *input, size_t input_len)
 		    "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);
 }