tech-pkg archive
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]
Some fixes to GPG signature verification in pkg_install
Hi tech-pkg@,
I have a patch for pkg_install I'd like to commit. 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. I have attached the patch here, broken down into four parts:
1/4: consistent errors
Just capitalizes the error messages that aren't.
2/4: error codes
Ensures error codes from child processes are below 126 (2 instead
of 255) for failed executions attempts of the GPG binary.
3/4: report errors
Ensures an error is always printed if the execution of the GPG
binary failed.
4/4: cleanup
Close and delete temporary resources as they should be.
I am working on this in the khorben/edgebsd-7/pkg_install branch of the
edgebsd-pkgsrc.git repository:
http://git.edgebsd.org/gitweb/?p=edgebsd-src.git;a=shortlog;h=refs/heads/khorben/edgebsd-7/pkg_install
This was merely for convenience (cross-building etc) but of course this
work really goes into pkgsrc's pkgtools/pkg_install/files. Let me know
if it looks good enough to commit.
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.
HTH,
--
khorben
commit 028cf73cd11508593c3f8d590edbac9cdc83c38a
Author: Pierre Pronchery <khorben%defora.org@localhost>
Date: Wed May 27 23:38:02 2015 +0200
More consistent error messages
diff --git a/external/bsd/pkg_install/dist/lib/gpgsig.c b/external/bsd/pkg_install/dist/lib/gpgsig.c
index 8091d4a..1aaa537 100644
--- a/external/bsd/pkg_install/dist/lib/gpgsig.c
+++ b/external/bsd/pkg_install/dist/lib/gpgsig.c
@@ -61,17 +61,17 @@ verify_signature(const char *input, size_t input_len, const char *keyring,
int fd[2], status;
if (pipe(fd) == -1)
- err(EXIT_FAILURE, "cannot create input pipes");
+ err(EXIT_FAILURE, "Cannot create input pipes");
child = vfork();
if (child == -1)
- err(EXIT_FAILURE, "cannot fork GPG process");
+ err(EXIT_FAILURE, "Cannot fork GPG process");
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";
+ "Cannot redirect stdin of GPG process\n";
write(STDERR_FILENO, err_msg, sizeof(err_msg) - 1);
_exit(255);
}
@@ -168,13 +168,13 @@ detached_gpg_sign(const char *content, size_t len, char **sig, size_t *sig_len,
errx(EXIT_FAILURE, "GPG variable not set");
if (pipe(fd_in) == -1)
- err(EXIT_FAILURE, "cannot create input pipes");
+ err(EXIT_FAILURE, "Cannot create input pipes");
if (pipe(fd_out) == -1)
- err(EXIT_FAILURE, "cannot create output pipes");
+ err(EXIT_FAILURE, "Cannot create output pipes");
child = fork();
if (child == -1)
- err(EXIT_FAILURE, "cannot fork GPG process");
+ err(EXIT_FAILURE, "Cannot fork GPG process");
if (child == 0) {
close(fd_in[1]);
close(STDIN_FILENO);
commit d82dd92b7fddc3d3d6320f731bf25236c5bff298
Author: Pierre Pronchery <khorben%defora.org@localhost>
Date: Wed May 27 22:00:03 2015 +0200
Keep error codes below 126
diff --git a/external/bsd/pkg_install/dist/lib/gpgsig.c b/external/bsd/pkg_install/dist/lib/gpgsig.c
index 1aaa537..645d04c 100644
--- a/external/bsd/pkg_install/dist/lib/gpgsig.c
+++ b/external/bsd/pkg_install/dist/lib/gpgsig.c
@@ -73,7 +73,7 @@ verify_signature(const char *input, size_t input_len, const char *keyring,
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[0]);
argvp = argv;
@@ -92,7 +92,7 @@ verify_signature(const char *input, size_t input_len, const char *keyring,
*argvp = NULL;
execvp(gpg_cmd, __UNCONST(argv));
- _exit(255);
+ _exit(2);
}
close(fd[0]);
if (write(fd[1], input, input_len) != (ssize_t)input_len)
@@ -182,7 +182,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 +192,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,7 +216,7 @@ detached_gpg_sign(const char *content, size_t len, char **sig, size_t *sig_len,
*argvp = NULL;
execvp(gpg_cmd, __UNCONST(argv));
- _exit(255);
+ _exit(2);
}
close(fd_in[0]);
if (write(fd_in[1], content, len) != (ssize_t)len)
commit 9937b83f428dfd77c4431876f8c953c355e6b8f9
Author: Pierre Pronchery <khorben%defora.org@localhost>
Date: Wed May 27 22:00:32 2015 +0200
Report errors when failing to execute GPG
diff --git a/external/bsd/pkg_install/dist/lib/gpgsig.c b/external/bsd/pkg_install/dist/lib/gpgsig.c
index 645d04c..ae21f3f 100644
--- a/external/bsd/pkg_install/dist/lib/gpgsig.c
+++ b/external/bsd/pkg_install/dist/lib/gpgsig.c
@@ -92,6 +92,7 @@ verify_signature(const char *input, size_t input_len, const char *keyring,
*argvp = NULL;
execvp(gpg_cmd, __UNCONST(argv));
+ warn("%s", gpg_cmd);
_exit(2);
}
close(fd[0]);
@@ -216,6 +217,7 @@ detached_gpg_sign(const char *content, size_t len, char **sig, size_t *sig_len,
*argvp = NULL;
execvp(gpg_cmd, __UNCONST(argv));
+ warn("%s", gpg_cmd);
_exit(2);
}
close(fd_in[0]);
commit 30bd12a44796be5a77d34f9c75c38486b91c8cc9
Author: Pierre Pronchery <khorben%defora.org@localhost>
Date: Wed May 27 22:15:53 2015 +0200
Always cleanup temporary files when failing to verify signatures
diff --git a/external/bsd/pkg_install/dist/lib/gpgsig.c b/external/bsd/pkg_install/dist/lib/gpgsig.c
index ae21f3f..1768084 100644
--- a/external/bsd/pkg_install/dist/lib/gpgsig.c
+++ b/external/bsd/pkg_install/dist/lib/gpgsig.c
@@ -52,27 +52,32 @@ __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;
- 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);
+ warn("Cannot redirect stdin of GPG process");
_exit(2);
}
close(fd[0]);
@@ -96,20 +101,31 @@ verify_signature(const char *input, size_t input_len, const char *keyring,
_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
@@ -120,6 +136,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");
@@ -133,26 +150,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
@@ -164,18 +189,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);
@@ -221,8 +261,10 @@ detached_gpg_sign(const char *content, size_t len, char **sig, size_t *sig_len,
_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;
@@ -242,9 +284,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);
}
Home |
Main Index |
Thread Index |
Old Index