Re: [pacman-dev] [PATCH] Add per-repo PinnedPubKey option

On Mon, Oct 31, 2016 at 04:36:23PM -0400, Travis Burtrum wrote:

From abb057844eec0e5707c31b643d0f2187b4cf0eb6 Mon Sep 17 00:00:00 2001 From: Travis Burtrum Date: Mon, 31 Oct 2016 02:12:31 -0400 Subject: [PATCH] Add per-repo PinnedPubKey option

This sets curl's CURLOPT_PINNEDPUBLICKEY option in the built-in downloader, or replaces %p in XferCommand. This pins public keys to ensure your TLS connection is not man-in-the-middled without relying on CAs etc. Probably most useful currently for very small groups or single servers.

It would obviously be best as a per-mirror option, but such a thing currently does not exist.

But perhaps as part of a larger scope, it could... As mentioned on IRC, I'm not a huge fan of this.

Signed-off-by: Travis Burtrum --- doc/pacman.conf.5.txt | 12 +++++++++++- lib/libalpm/alpm.h | 10 +++++++++- lib/libalpm/be_sync.c | 4 ++++ lib/libalpm/db.c | 12 ++++++++++++ lib/libalpm/db.h | 1 + lib/libalpm/dload.c | 8 +++++++- lib/libalpm/dload.h | 1 + lib/libalpm/sync.c | 7 ++++--- src/pacman/conf.c | 26 ++++++++++++++++++++++++-- src/pacman/conf.h | 1 + 10 files changed, 74 insertions(+), 8 deletions(-)

diff --git a/doc/pacman.conf.5.txt b/doc/pacman.conf.5.txt index c665870..b1859c7 100644 --- a/doc/pacman.conf.5.txt +++ b/doc/pacman.conf.5.txt @@ -126,7 +126,8 @@ Options All instances of `%u` will be replaced with the download URL. If present, instances of `%o` will be replaced with the local filename, plus a ``.part'' extension, which allows programs like wget to do file resumes - properly. + properly. If present, instances of `%p` will be replaced with the value of + the repo specific PinnedPubKey directive, or `` if that is not set. + This option is useful for users who experience problems with built-in HTTP/FTP support, or need the more advanced proxy support that comes with @@ -276,6 +277,15 @@ even be used for different architectures. Note that an enabled repository can be operated on explicitly, regardless of the Usage level set.

+*PinnedPubKey =* pinnedpubkey:: + The string can be the file name of your pinned public key. The file format expected + is "PEM" or "DER". The string can also be any number of base64 encoded sha256 + hashes preceded by "sha256//" and separated by ";" ++ +When negotiating a TLS or SSL connection, the server sends a certificate indicating +its identity. A public key is extracted from this certificate and if it does not +exactly match the public key provided to this option, pacman will abort the +connection before sending or receiving any data.

Package and Database Signature Checking[[SC]] --------------------------------------------- diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index 2d2491d..e942559 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -750,11 +750,12 @@ typedef void (*alpm_cb_totaldl)(off_t total); * @param url the URL of the file to be downloaded * @param localpath the directory to which the file should be downloaded * @param force whether to force an update, even if the file is the same + * @param pinnedpubkey a pinned public key string * @return 0 on success, 1 if the file exists and is identical, -1 on * error. */ typedef int (*alpm_cb_fetch)(const char *url, const char *localpath, - int force); + int force, const char *pinnedpubkey);

If the signature of this must change, I'd strongly suggest we think about some sort of more public payload struct which we can extend, and shipping that across the callback. However, see below where I explain more about why I think this is a Bad Idea.

/** Fetch a remote pkg. * @param handle the context handle @@ -1037,6 +1038,13 @@ alpm_list_t *alpm_db_get_groupcache(alpm_db_t *db); */ alpm_list_t *alpm_db_search(alpm_db_t *db, const alpm_list_t *needles);

+/** Sets the pinned public key of a database. + * @param db pointer to the package database to set the status for + * @param pinnedpubkey a pinned public key string + * @return 0 on success, or -1 on error + */ +int alpm_db_set_pinnedpubkey(alpm_db_t *db, char *pinnedpubkey); + typedef enum _alpm_db_usage_ { ALPM_DB_USAGE_SYNC = 1, ALPM_DB_USAGE_SEARCH = (1 << 1), diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index 7774975..be2cd64 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -242,6 +242,8 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db) payload.handle = handle; payload.force = force; payload.unlink_on_fail = 1; + + payload.pinnedpubkey = db->pinnedpubkey;

ret = _alpm_download(&payload, syncpath, NULL, &final_db_url); _alpm_dload_payload_reset(&payload); @@ -297,6 +299,8 @@ int SYMEXPORT alpm_db_update(int force, alpm_db_t *db) /* set hard upper limit of 16KiB */ payload.max_size = 16 * 1024;

+ payload.pinnedpubkey = db->pinnedpubkey; + sig_ret = _alpm_download(&payload, syncpath, NULL, NULL); /* errors_ok suppresses error messages, but not the return code */ sig_ret = payload.errors_ok ? 0 : sig_ret; diff --git a/lib/libalpm/db.c b/lib/libalpm/db.c index 5d78ee5..0dab1e7 100644 --- a/lib/libalpm/db.c +++ b/lib/libalpm/db.c @@ -306,6 +306,15 @@ alpm_list_t SYMEXPORT *alpm_db_search(alpm_db_t *db, const alpm_list_t *needles) return _alpm_db_search(db, needles); }

+/** Sets the pinned public key for a repo */ +int SYMEXPORT alpm_db_set_pinnedpubkey(alpm_db_t *db, char *pinnedpubkey) +{ + ASSERT(db != NULL, return -1); + ASSERT(pinnedpubkey != NULL, return 0); + db->pinnedpubkey = strdup(pinnedpubkey); + return 0; +} + /** Sets the usage bitmask for a repo */ int SYMEXPORT alpm_db_set_usage(alpm_db_t *db, int usage) { @@ -351,6 +360,9 @@ void _alpm_db_free(alpm_db_t *db) FREELIST(db->servers); FREE(db->_path); FREE(db->treename); + if(db->pinnedpubkey != NULL) { + FREE(db->pinnedpubkey); + } FREE(db);

return; diff --git a/lib/libalpm/db.h b/lib/libalpm/db.h index 8e50196..4ce98c8 100644 --- a/lib/libalpm/db.h +++ b/lib/libalpm/db.h @@ -67,6 +67,7 @@ struct __alpm_db_t { char *treename; /* do not access directly, use _alpm_db_path(db) for lazy access */ char *_path; + char *pinnedpubkey; alpm_pkghash_t *pkgcache; alpm_list_t *grpcache; alpm_list_t *servers; diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c index 9d80358..6d5b371 100644 --- a/lib/libalpm/dload.c +++ b/lib/libalpm/dload.c @@ -327,6 +327,12 @@ static void curl_set_handle_opts(struct dload_payload *payload, curl_easy_setopt(curl, CURLOPT_USERAGENT, useragent); }

+ if(payload->pinnedpubkey != NULL) { + _alpm_log(handle, ALPM_LOG_DEBUG, + "using curl pinnedpubkey: %s\n", payload->pinnedpubkey); + curl_easy_setopt(curl, CURLOPT_PINNEDPUBLICKEY, payload->pinnedpubkey); + } + if(!payload->allow_resume && !payload->force && payload->destfile_name && stat(payload->destfile_name, &st) == 0) { /* start from scratch, but only download if our local is out of date. */ @@ -646,7 +652,7 @@ int _alpm_download(struct dload_payload *payload, const char *localpath, RET_ERR(handle, ALPM_ERR_EXTERNAL_DOWNLOAD, -1); #endif } else { - int ret = handle->fetchcb(payload->fileurl, localpath, payload->force); + int ret = handle->fetchcb(payload->fileurl, localpath, payload->force, payload->pinnedpubkey); if(ret == -1 && !payload->errors_ok) { RET_ERR(handle, ALPM_ERR_EXTERNAL_DOWNLOAD, -1); } diff --git a/lib/libalpm/dload.h b/lib/libalpm/dload.h index 427c486..b938fdc 100644 --- a/lib/libalpm/dload.h +++ b/lib/libalpm/dload.h @@ -41,6 +41,7 @@ struct dload_payload { int errors_ok; int unlink_on_fail; int trust_remote_name; + char *pinnedpubkey; #ifdef HAVE_LIBCURL CURLcode curlerr; /* last error produced by curl */ #endif diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c index 837639d..05e1ffe 100644 --- a/lib/libalpm/sync.c +++ b/lib/libalpm/sync.c @@ -862,7 +862,7 @@ static int validate_deltas(alpm_handle_t *handle, alpm_list_t *deltas) }

static struct dload_payload *build_payload(alpm_handle_t *handle, - const char *filename, size_t size, alpm_list_t *servers) + const char *filename, size_t size, alpm_list_t *servers, char *pinnedpubkey) { struct dload_payload *payload;

@@ -870,6 +870,7 @@ static struct dload_payload *build_payload(alpm_handle_t *handle, STRDUP(payload->remote_name, filename, FREE(payload); RET_ERR(handle, ALPM_ERR_MEMORY, NULL)); payload->max_size = size; payload->servers = servers; + payload->pinnedpubkey = pinnedpubkey; return payload; }

@@ -898,7 +899,7 @@ static int find_dl_candidates(alpm_db_t *repo, alpm_list_t **files, alpm_list_t alpm_delta_t *delta = dlts->data; if(delta->download_size != 0) { struct dload_payload *payload = build_payload( - handle, delta->delta, delta->delta_size, repo->servers); + handle, delta->delta, delta->delta_size, repo->servers, repo->pinnedpubkey); ASSERT(payload, return -1); *files = alpm_list_add(*files, payload); } @@ -909,7 +910,7 @@ static int find_dl_candidates(alpm_db_t *repo, alpm_list_t **files, alpm_list_t } else if(spkg->download_size != 0) { struct dload_payload *payload; ASSERT(spkg->filename != NULL, RET_ERR(handle, ALPM_ERR_PKG_INVALID_NAME, -1)); - payload = build_payload(handle, spkg->filename, spkg->size, repo->servers); + payload = build_payload(handle, spkg->filename, spkg->size, repo->servers, repo->pinnedpubkey); ASSERT(payload, return -1); *files = alpm_list_add(*files, payload); } diff --git a/src/pacman/conf.c b/src/pacman/conf.c index d8d64fb..8dd23e5 100644 --- a/src/pacman/conf.c +++ b/src/pacman/conf.c @@ -163,6 +163,9 @@ void config_repo_free(config_repo_t *repo) if(repo == NULL) { return; } + if(repo->pinnedpubkey != NULL) { + free(repo->pinnedpubkey); + }

No need for the if -- free(NULL) is a no-op.

free(repo->name); FREELIST(repo->servers); free(repo); @@ -204,7 +207,7 @@ static char *get_tempfile(const char *path, const char *filename)

/** External fetch callback */ static int download_with_xfercommand(const char *url, const char *localpath, - int force) + int force, const char *pinnedpubkey) { int ret = 0, retval; int usepart = 0; @@ -232,6 +235,16 @@ static int download_with_xfercommand(const char *url, const char *localpath, }

tempcmd = strdup(config->xfercommand); + /* replace all occurrences of %p with pinnedpubkey */

Why does this need to exist? You suggested on IRC that the pinned key could simply be a list and apply to your semi-trusted set of mirrors. You could already hardcode that set in the XferCommand (you could do that right now, too, without this patch).

+ if(strstr(tempcmd, "%p")) { + if(pinnedpubkey == NULL) { + parsedcmd = strreplace(tempcmd, "%p", "");

This seems like it would lead to badness and cause transfers to fail in bad ways. Something like: XferCommand = /usr/bin/curl -C - -f --pinnedpubkey %p %u > %o Would no longer have a URL, as the --pinnedpubkey would swallow it. Dealing with this gracefully seems quite difficult. Perhaps another reason not to include this new formatter token...

+ } else { + parsedcmd = strreplace(tempcmd, "%p", pinnedpubkey); + } + free(tempcmd); + tempcmd = parsedcmd; + } /* replace all occurrences of %o with fn.part */ if(strstr(tempcmd, "%o")) { usepart = 1; @@ -668,6 +681,8 @@ static int register_repo(config_repo_t *repo) repo->name); alpm_db_set_usage(db, repo->usage == 0 ? ALPM_DB_USAGE_ALL : repo->usage);

+ alpm_db_set_pinnedpubkey(db, repo->pinnedpubkey); + for(i = repo->servers; i; i = alpm_list_next(i)) { char *value = i->data; if(_add_mirror(db, value) != 0) { @@ -915,12 +930,19 @@ static int _parse_repo(const char *key, char *value, const char *file, } FREELIST(values); } + } else if(strcmp(key, "PinnedPubKey") == 0) { + if(!value) { + pm_printf(ALPM_LOG_ERROR, _("config file %s, line %d: directive '%s' needs a value\n"), + file, line, key); + } else { + repo->pinnedpubkey = strdup(value); + pm_printf(ALPM_LOG_DEBUG, "repo config: pinnedpubkey: %s\n", value); + } } else { pm_printf(ALPM_LOG_WARNING, _("config file %s, line %d: directive '%s' in section '%s' not recognized.\n"), file, line, key, repo->name); } - return ret; }

diff --git a/src/pacman/conf.h b/src/pacman/conf.h index 945de7c..7c021bf 100644 --- a/src/pacman/conf.h +++ b/src/pacman/conf.h @@ -40,6 +40,7 @@ typedef struct __config_repo_t { int usage; int siglevel; int siglevel_mask; + char *pinnedpubkey; } config_repo_t;

typedef struct __config_t { -- 2.10.2