[pacman-dev] [PATCH 0/8] [RFC] Signature checking overhaul
The main change here is that pacman will now check the needed keys are in the
keyring before doing package validation. Example output:
:: Retrieving packages ...
systemd-sysvcompat-... 5.4 KiB 671K/s 00:00 [######################] 100%
(1/1) checking keys in keyring [######################] 100%
:: Import PGP key 2048R/F56C0C53, "Dave Reisner
In preparation for checking key presence and downloading needed keys
before conflict checking.
Signed-off-by: Allan McRae
This will be useful for checking the availablity of all keys before
perfoming validation in sync operations and for downloading a needed
key in upgrade operations.
Signed-off-by: Allan McRae
This does not support all possibilities of RFC4880, but it does
cover every key currently used in Arch Linux.
Signed-off-by: Allan McRae
On Sat, Nov 03, 2012 at 01:28:17AM +1000, Allan McRae wrote:
This does not support all possibilities of RFC4880, but it does cover every key currently used in Arch Linux.
Hmmm, this function doesn't seem to distinguish between "not a key" and "not a supported key". Can we return more useful error codes, like -ENOSYS for unsupported, -EINVAL for not-a-signature packet...
Signed-off-by: Allan McRae
--- lib/libalpm/signing.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++ lib/libalpm/signing.h | 2 + 2 files changed, 102 insertions(+) diff --git a/lib/libalpm/signing.c b/lib/libalpm/signing.c index 9d56aba..6b13522 100644 --- a/lib/libalpm/signing.c +++ b/lib/libalpm/signing.c @@ -948,4 +948,104 @@ int SYMEXPORT alpm_siglist_cleanup(alpm_siglist_t *siglist) return 0; }
+/** + * Extract the Issuer Key ID from a signature + * @param sig PGP signature + * @param len length of signature + * @param keys a pointer to storage for key IDs + * @return 0 on success, -1 on error + */ +int _alpm_extract_keyid(const unsigned char *sig, size_t len, alpm_list_t **keys) +{ + size_t pos, spos, blen, hlen, ulen, slen; + pos = 0; + + while(pos < len) { + if(!(sig[pos] & 0x80)) { + return -1; + } + + if(sig[pos] & 0x40) { + // "new" packet format is not supported + return -1; + } + + if(((sig[pos] & 0x3f) >> 2) != 2) { + // signature is not a "Signature Packet" + return -1; + } + + switch (sig[pos] & 0x03) {
style nit: switch(foo)
+ case 0: + blen = sig[pos + 1]; + pos = pos + 2; + break; + + case 1: + blen = (sig[pos + 1] << 8) | sig[pos + 2]; + pos = pos + 3; + break; + + case 2: + blen = (sig[pos + 1] << 24) | (sig[pos + 2] << 16) | (sig[pos + 3] << 8) | sig[pos + 4]; + pos = pos + 5; + break; + + case 3: + // partial body length not supported + return -1; + } + + if(sig[pos] != 4) { + // only support version 4 signature packet format + return -1; + } + + if(sig[pos + 1] != 0x00) { + // not a signature of a binary document + return -1; + } + + pos = pos + 4; + + hlen = (sig[pos] << 8) | sig[pos + 1]; + pos = pos + hlen + 2; + + ulen = (sig[pos] << 8) | sig[pos + 1]; + pos = pos + 2; + + spos = pos; + + while(spos < pos + ulen) { + if(sig[spos] < 192) { + slen = sig[spos]; + spos = spos + 1; + } else if(sig[spos] < 255) { + slen = (sig[spos] << 8) | sig[spos + 1]; + spos = spos + 2; + } else { + slen = (sig[spos + 1] << 24) | (sig[spos + 2] << 16) | (sig[spos + 3] << 8) | sig[spos + 4]; + spos = spos + 5; + } + + if(sig[spos] == 16) { + // issuer key ID + char key[16]; + size_t i; + for (i = 0; i < 8; i++) { + sprintf(&key[i * 2], "%02hhX", sig[spos + i + 1]); + } + *keys = alpm_list_add(*keys, strdup(key));
Would be nice to be robust against strdup() memory allocation failures here.
+ break; + } + + spos = spos + slen; + } + + pos = pos + (blen - hlen - 8); + } + + return 0; +} + /* vim: set ts=2 sw=2 noet: */ diff --git a/lib/libalpm/signing.h b/lib/libalpm/signing.h index a78e4b7..6537ee3 100644 --- a/lib/libalpm/signing.h +++ b/lib/libalpm/signing.h @@ -34,6 +34,8 @@ int _alpm_process_siglist(alpm_handle_t *handle, const char *identifier, int _alpm_key_in_keychain(alpm_handle_t *handle, const char *fpr); int _alpm_key_import(alpm_handle_t *handle, const char *fpr);
+int _alpm_extract_keyid(const unsigned char *sig, size_t len, alpm_list_t **keys); + #endif /* _ALPM_SIGNING_H */
/* vim: set ts=2 sw=2 noet: */ -- 1.8.0
On 03/11/12 01:40, Dave Reisner wrote:
On Sat, Nov 03, 2012 at 01:28:17AM +1000, Allan McRae wrote:
This does not support all possibilities of RFC4880, but it does cover every key currently used in Arch Linux.
Hmmm, this function doesn't seem to distinguish between "not a key" and "not a supported key". Can we return more useful error codes, like -ENOSYS for unsupported, -EINVAL for not-a-signature packet...
Instead of doing this I have added ALPM_LOG_ERROR messages that output why the key has failed. I think this is a more clear way to allow a user to report and issue allowing us to follow up any failure in the future. Allan
On Fri, Nov 2, 2012 at 10:28 AM, Allan McRae
This does not support all possibilities of RFC4880, but it does cover every key currently used in Arch Linux.
Signed-off-by: Allan McRae
--- lib/libalpm/signing.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++ lib/libalpm/signing.h | 2 + 2 files changed, 102 insertions(+) diff --git a/lib/libalpm/signing.c b/lib/libalpm/signing.c index 9d56aba..6b13522 100644 --- a/lib/libalpm/signing.c +++ b/lib/libalpm/signing.c @@ -948,4 +948,104 @@ int SYMEXPORT alpm_siglist_cleanup(alpm_siglist_t *siglist) return 0; }
+/** + * Extract the Issuer Key ID from a signature + * @param sig PGP signature + * @param len length of signature + * @param keys a pointer to storage for key IDs + * @return 0 on success, -1 on error + */ +int _alpm_extract_keyid(const unsigned char *sig, size_t len, alpm_list_t **keys) +{ + size_t pos, spos, blen, hlen, ulen, slen; + pos = 0; + + while(pos < len) { + if(!(sig[pos] & 0x80)) { + return -1; + } + + if(sig[pos] & 0x40) { + // "new" packet format is not supported Respect the preferred coding style for comments, please. :)
-Dan
Signed-off-by: Allan McRae
Keys used to create signatures are checked for presence in the keyring
before package validation is performed.
Signed-off-by: Allan McRae
Now that the keyring is checked for all needed keys before the
validation, we can not reach a point of a missing key when doing
validity checks for sync operations.
Signed-off-by: Allan McRae
Offer to remove the bad package when a signature fails to validate
as is done for checksum failures.
Signed-off-by: Allan McRae
On Sat, Nov 03, 2012 at 01:28:21AM +1000, Allan McRae wrote:
Offer to remove the bad package when a signature fails to validate as is done for checksum failures.
Signed-off-by: Allan McRae
---
FS#28014 This is totally anti-climactic.
lib/libalpm/sync.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index 031034d..2546579 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -1091,6 +1091,7 @@ static int check_validity(alpm_handle_t *handle, v->level & ALPM_SIG_PACKAGE_OPTIONAL, v->level & ALPM_SIG_PACKAGE_MARGINAL_OK, v->level & ALPM_SIG_PACKAGE_UNKNOWN_OK); + prompt_to_delete(handle, v->path, v->error); } else if(v->error == ALPM_ERR_PKG_INVALID_CHECKSUM) { prompt_to_delete(handle, v->path, v->error); } -- 1.8.0
When installing a package with "pacman -U" that has a detached
signature, check if the needed key is in the keyring and download
if necessary.
Signed-off-by: Allan McRae
On Sat, Nov 03, 2012 at 01:28:22AM +1000, Allan McRae wrote:
When installing a package with "pacman -U" that has a detached signature, check if the needed key is in the keyring and download if necessary.
Signed-off-by: Allan McRae
---
What ever happened to our musings about adding another SigLevel-ish option for handling this sort of thing? Are we satisfied with just letting the global value dictate this behavior?
lib/libalpm/be_package.c | 40 ++++++++++++++++++++++++++++++++++++++++ lib/libalpm/signing.c | 5 +++++ 2 files changed, 45 insertions(+)
diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c index 8c5b2d1..710a063 100644 --- a/lib/libalpm/be_package.c +++ b/lib/libalpm/be_package.c @@ -519,6 +519,46 @@ int SYMEXPORT alpm_pkg_load(alpm_handle_t *handle, const char *filename, int ful CHECK_HANDLE(handle, return -1); ASSERT(pkg != NULL, RET_ERR(handle, ALPM_ERR_WRONG_ARGS, -1));
+ char *sigpath = _alpm_sigpath(handle, filename); + if(sigpath && !_alpm_access(handle, NULL, sigpath, R_OK)) { + if(level & ALPM_SIG_PACKAGE) { + struct stat info;
nitpick: we use 'st' in a lot of places to denote the name of a stat struct. I don't feel incredibly strongly about this, but consistency is nice.
+ alpm_list_t *keys = NULL; + int fail = 0; + size_t len;
If you're going to use this as a quick reference to a stat field, mark it const (or just get rid of it and reference info.st_size).
+ unsigned char *sig; + FILE *fp; + + stat(sigpath, &info);
Check for stat failure?
+ len = info.st_size; + sig = malloc(len);
Check for null return?
+ fp = fopen(sigpath, "rb");
Check for null fp?
+ fread(sig, len, 1, fp); + fclose(fp); + + if(_alpm_extract_keyid(sig, len, &keys) == 0) { + alpm_list_t *k; + for(k = keys; k; k = k->next) { + char *key = k->data; + if(_alpm_key_in_keychain(handle, key) == 0) { + if(_alpm_key_import(handle, key) == -1) { + fail = 1; + } + } + } + FREELIST(keys); + } + + free(sig); + + if(fail) { + _alpm_log(handle, ALPM_LOG_ERROR, _("required key missing from keyring\n")); + return -1; + } + } + } + free(sigpath); + if(_alpm_pkg_validate_internal(handle, filename, NULL, level, NULL, &validation) == -1) { /* pm_errno is set by pkg_validate */ diff --git a/lib/libalpm/signing.c b/lib/libalpm/signing.c index 703e3ea..4f03853 100644 --- a/lib/libalpm/signing.c +++ b/lib/libalpm/signing.c @@ -192,6 +192,11 @@ int _alpm_key_in_keychain(alpm_handle_t *handle, const char *fpr) gpgme_key_t key; int ret = -1;
+ if(init_gpgme(handle)) { + /* pm_errno was set in gpgme_init() */ + goto error; + } + memset(&ctx, 0, sizeof(ctx)); err = gpgme_new(&ctx); CHECK_ERR(); -- 1.8.0
On 03/11/12 01:48, Dave Reisner wrote:
On Sat, Nov 03, 2012 at 01:28:22AM +1000, Allan McRae wrote:
When installing a package with "pacman -U" that has a detached signature, check if the needed key is in the keyring and download if necessary.
Signed-off-by: Allan McRae
--- What ever happened to our musings about adding another SigLevel-ish option for handling this sort of thing? Are we satisfied with just letting the global value dictate this behavior?
For reference: https://mailman.archlinux.org/pipermail/pacman-dev/2012-February/015155.html
participants (3)
-
Allan McRae
-
Dan McGee
-
Dave Reisner