[pacman-dev] [PATCH v3] Fallback to detached signatures during keyring check
Anatol Pomozov
anatol.pomozov at gmail.com
Thu Jun 4 17:34:01 UTC 2020
Hi
On Wed, Jun 3, 2020 at 6:40 PM Andrew Gregory
<andrew.gregory.8 at gmail.com> wrote:
>
> On 06/01/20 at 11:45pm, Anatol Pomozov wrote:
> > Pacman has a 'key in keyring' verification step that makes sure the signatures
> > have a valid keyid. Currently pacman parses embedded package signatures only.
> >
> > Add a fallback to detached signatures. If embedded signature is missing then it
> > tries to read corresponding *.sig file and get keyid from there.
> >
> > Verification:
> > debug: found cached pkg: /var/cache/pacman/pkg/glib-networking-2.64.3-1-x86_64.pkg.tar.zst
> > debug: found detached signature /var/cache/pacman/pkg/glib-networking-2.64.3-1-x86_64.pkg.tar.zst.sig with size 310
> > debug: found signature key: A5E9288C4FA415FA
> > debug: looking up key A5E9288C4FA415FA locally
> > debug: key lookup success, key exists
> >
> > Signed-off-by: Anatol Pomozov <anatol.pomozov at gmail.com>
> > ---
> > lib/libalpm/alpm.h | 11 +++++++++++
> > lib/libalpm/package.c | 34 ++++++++++++++++++++++++++++++++++
> > lib/libalpm/sync.c | 18 +++++++++---------
> > lib/libalpm/util.c | 37 +++++++++++++++++++++++++++++++++++++
> > lib/libalpm/util.h | 1 +
> > 5 files changed, 92 insertions(+), 9 deletions(-)
> >
> > diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
> > index c6a13273..0ba57109 100644
> > --- a/lib/libalpm/alpm.h
> > +++ b/lib/libalpm/alpm.h
> > @@ -1403,6 +1403,17 @@ alpm_db_t *alpm_pkg_get_db(alpm_pkg_t *pkg);
> > */
> > const char *alpm_pkg_get_base64_sig(alpm_pkg_t *pkg);
> >
> > +/** Extracts package signature either from embedded package signature
> > + * or if it is absent then reads data from detached signature file.
> > + * @param pkg a pointer to package.
> > + * @param sig output parameter for signature data. Callee function allocates
> > + * a buffer needed for the signature data. Caller is responsible for
> > + * freeing this buffer.
> > + * @param sig_len output parameter for the signature data length.
> > + * @return 0 on success, negative number on error.
>
> alpm_errno_t values are positive.
This function returns -1 int on error. So the description is correct.
>
> > + */
> > +int alpm_pkg_get_sig(alpm_pkg_t *pkg, unsigned char **sig, size_t *sig_len);
> > +
> > /** Returns the method used to validate a package during install.
> > * @param pkg a pointer to package
> > * @return an enum member giving the validation method
> > diff --git a/lib/libalpm/package.c b/lib/libalpm/package.c
> > index 5c5fa073..1335b70e 100644
> > --- a/lib/libalpm/package.c
> > +++ b/lib/libalpm/package.c
> > @@ -268,6 +268,40 @@ const char SYMEXPORT *alpm_pkg_get_base64_sig(alpm_pkg_t *pkg)
> > return pkg->base64_sig;
> > }
> >
> > +int SYMEXPORT alpm_pkg_get_sig(alpm_pkg_t *pkg, unsigned char **sig, size_t *sig_len)
> > +{
> > + ASSERT(pkg != NULL, return -1);
> > +
> > + if(pkg->base64_sig) {
> > + return alpm_decode_signature(pkg->base64_sig, sig, sig_len);
>
> You're returning an error without setting pm_errno.
Done.
And while we are here I have a question.
What is the rule on "setting pm_error on function error"?
I see that some utility functions set pm_error while others just
return error code to the caller.
> > + } else {
> > + char *pkgpath, *sigpath;
> > + alpm_errno_t err;
> > + int ret = -1;
> > +
> > + pkgpath = _alpm_filecache_find(pkg->handle, pkg->filename);
> > + if(!pkgpath) {
> > + RET_ERR(pkg->handle, ALPM_ERR_PKG_NOT_FOUND, -1);
> > + }
> > + sigpath = _alpm_sigpath(pkg->handle, pkgpath);
> > + if(!sigpath || _alpm_access(pkg->handle, NULL, sigpath, R_OK)) {
> > + GOTO_ERR(pkg->handle, ALPM_ERR_SIG_MISSING, exit);
> > + }
> > + err = _alpm_read_file(sigpath, sig, sig_len);
> > + if(err == ALPM_ERR_OK) {
> > + _alpm_log(pkg->handle, ALPM_LOG_DEBUG, "found detached signature %s with size %ld\n",
> > + sigpath, *sig_len);
> > + } else {
> > + GOTO_ERR(pkg->handle, err, exit);
> > + }
> > + ret = 0;
> > +exit:
>
> Change exit to cleanup; exit makes it look like you are terminating
> the program.
done
>
> > + FREE(pkgpath);
> > + FREE(sigpath);
> > + return ret;
> > + }
> > +}
> > +
> > const char SYMEXPORT *alpm_pkg_get_arch(alpm_pkg_t *pkg)
> > {
> > ASSERT(pkg != NULL, return NULL);
> > diff --git a/lib/libalpm/sync.c b/lib/libalpm/sync.c
> > index 8c01ad95..9350793a 100644
> > --- a/lib/libalpm/sync.c
> > +++ b/lib/libalpm/sync.c
> > @@ -880,18 +880,18 @@ static int check_keyring(alpm_handle_t *handle)
> > }
> >
> > level = alpm_db_get_siglevel(alpm_pkg_get_db(pkg));
> > - if((level & ALPM_SIG_PACKAGE) && pkg->base64_sig) {
> > - unsigned char *decoded_sigdata = NULL;
> > - size_t data_len;
> > - int decode_ret = alpm_decode_signature(pkg->base64_sig,
> > - &decoded_sigdata, &data_len);
> > - if(decode_ret == 0) {
> > + if((level & ALPM_SIG_PACKAGE)) {
> > + unsigned char *sig = NULL;
> > + size_t sig_len;
> > + int ret = alpm_pkg_get_sig(pkg, &sig, &sig_len);
> > + if(ret == 0) {
> > alpm_list_t *keys = NULL;
> > - if(alpm_extract_keyid(handle, pkg->name, decoded_sigdata,
> > - data_len, &keys) == 0) {
> > + if(alpm_extract_keyid(handle, pkg->name, sig,
> > + sig_len, &keys) == 0) {
> > alpm_list_t *k;
> > for(k = keys; k; k = k->next) {
> > char *key = k->data;
> > + _alpm_log(handle, ALPM_LOG_DEBUG, "found signature key: %s\n", key);
> > if(!alpm_list_find(errors, key, key_cmp) &&
> > _alpm_key_in_keychain(handle, key) == 0) {
> > keyinfo = malloc(sizeof(struct keyinfo_t));
> > @@ -905,8 +905,8 @@ static int check_keyring(alpm_handle_t *handle)
> > }
> > FREELIST(keys);
> > }
> > - free(decoded_sigdata);
> > }
> > + free(sig);
> > }
> > }
> >
> > diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c
> > index 76728eb4..65a6f3df 100644
> > --- a/lib/libalpm/util.c
> > +++ b/lib/libalpm/util.c
> > @@ -1489,3 +1489,40 @@ void _alpm_alloc_fail(size_t size)
> > {
> > fprintf(stderr, "alloc failure: could not allocate %zu bytes\n", size);
> > }
> > +
> > +/** This functions reads file content.
> > + *
> > + * Memory buffer is allocated by the callee function. It is responsibility
> > + * of the caller to free the buffer.
> > + *
> > + * @param filepath filepath to read
> > + * @param data pointer to output buffer
> > + * @param data_len size of the output buffer
> > + * @return error code for the operation
> > + */
> > +alpm_errno_t _alpm_read_file(const char *filepath, unsigned char **data, size_t *data_len)
> > +{
> > + struct stat st;
> > + FILE *fp;
> > +
> > + if((fp = fopen(filepath, "rb")) == NULL) {
> > + return -1;
>
> -1 is not an alpm_errno_t.
fixed
>
> > + }
> > +
> > + if(fstat(fileno(fp), &st) != 0) {
> > + fclose(fp);
> > + return ALPM_ERR_NOT_A_FILE;
> > + }
> > + *data_len = st.st_size;
> > +
> > + MALLOC(*data, *data_len, fclose(fp); return ALPM_ERR_MEMORY);
> > +
> > + if(fread(*data, *data_len, 1, fp) != 1) {
> > + FREE(*data);
> > + fclose(fp);
> > + return ALPM_ERR_SYSTEM;
> > + }
> > +
> > + fclose(fp);
> > + return ALPM_ERR_OK;
> > +}
> > diff --git a/lib/libalpm/util.h b/lib/libalpm/util.h
> > index 4fc6e718..03c8ed44 100644
> > --- a/lib/libalpm/util.h
> > +++ b/lib/libalpm/util.h
> > @@ -155,6 +155,7 @@ int _alpm_fnmatch_patterns(alpm_list_t *patterns, const char *string);
> > int _alpm_fnmatch(const void *pattern, const void *string);
> > void *_alpm_realloc(void **data, size_t *current, const size_t required);
> > void *_alpm_greedy_grow(void **data, size_t *current, const size_t required);
> > +alpm_errno_t _alpm_read_file(const char *filepath, unsigned char **data, size_t *data_len);
> >
> > #ifndef HAVE_STRSEP
> > char *strsep(char **, const char *);
> > --
> > 2.27.0
More information about the pacman-dev
mailing list