[pacman-dev] [PATCH v2] Fallback to detached signatures during keyring check

Andrew Gregory andrew.gregory.8 at gmail.com
Sun May 31 10:23:57 UTC 2020


On 05/31/20 at 01:37am, Anatol Pomozov wrote:
> Hi Andrew, thank you for the quick response
> 
> On Sat, May 30, 2020 at 9:31 PM Andrew Gregory
> <andrew.gregory.8 at gmail.com> wrote:
> >
> > On 05/30/20 at 07:51pm, 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    | 10 ++++++++++
> > >  lib/libalpm/package.c | 31 +++++++++++++++++++++++++++++++
> > >  lib/libalpm/sync.c    | 17 ++++++++---------
> > >  lib/libalpm/util.c    | 35 +++++++++++++++++++++++++++++++++++
> > >  lib/libalpm/util.h    |  1 +
> > >  5 files changed, 85 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
> > > index c6a13273..9c5fff73 100644
> > > --- a/lib/libalpm/alpm.h
> > > +++ b/lib/libalpm/alpm.h
> > > @@ -1403,6 +1403,16 @@ 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
> > > + * buffer needed for the signature data. Caller is responsible for
> > > + * freeing this buffer.
> > > + * @return size of a signature or negative number if error.
> > > + */
> > > +int alpm_pkg_get_sig(alpm_pkg_t *pkg, unsigned char **sig);
> > > +
> > >  /** 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..e0e4d987 100644
> > > --- a/lib/libalpm/package.c
> > > +++ b/lib/libalpm/package.c
> > > @@ -268,6 +268,37 @@ 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)
> >
> > This API is weird, why are you returning an int when neither of the
> > successful values are int and can potentially overflow an int?  You're
> > returning the length of an allocated string, size_t is the appropriate
> > type.
> 
> size_t is unsigned int. But this function returns positive length in
> case of successful signature read or negative value in case of error.
> Thus return value cannot be size_t.

I get that, but you chose that interface.  Why?  Surely it makes more
sense to either return 0 on error, since you treat a missing sig as an
error anyway, or take a size_t* arg like decode_signature.

> >
> > > +{
> > > +     ASSERT(pkg != NULL, return -1);
> > > +     pkg->handle->pm_errno = ALPM_ERR_OK;
> >
> > I don't like clearing pm_errno without a good reason.  It generally
> > doesn't serve any purpose and can get us into trouble if a function
> > gets called during cleanup somewhere.
> 
> I personally do not feel that "pm_errno = ALPM_ERR_OK" is useful here
> and I am more than happy to remove it.
> 
> But I also I see this pattern is used a lot. Only this file
> (lib/libalpm/package.c) has like 20 use-cases of it. e.g.
> 
> off_t SYMEXPORT alpm_pkg_get_isize(alpm_pkg_t *pkg)
> {
>         ASSERT(pkg != NULL, return -1);
>         pkg->handle->pm_errno = ALPM_ERR_OK;
>         return pkg->ops->get_isize(pkg);
> }
> 
> So I was trying to follow a standard practice here. Or are you trying
> to say that lib/libalpm/package.c examples I was looking at are
> incorrect?

I consider this an unfortunate anti-pattern.  It has bitten us in the
past by clearing pm_errno during cleanup just like I said.  I haven't
been motivated enough to go through and remove it so far, but I don't
want to add any more instances of it.

> > > +     if(pkg->base64_sig) {
> > > +             size_t sig_len;
> > > +             int ret = alpm_decode_signature(pkg->base64_sig, sig, &sig_len);
> > > +             return ret == 0 ? (int)sig_len : -1;
> > > +     } else {
> > > +             char *pkgpath, *sigpath;
> > > +             int len;
> > > +
> > > +             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)) {
> > > +                     FREE(pkgpath);
> > > +                     FREE(sigpath);
> > > +                     RET_ERR(pkg->handle, ALPM_ERR_SIG_MISSING, -1);
> > > +             }
> > > +             len = _alpm_read_file(sigpath, sig);
> >
> > You need to check for and handle failure.
> 
> Do you mean to handle a file read failure? But both successful and
> erroneous codepaths are the same - free() the resources and return
> "len".

What is pm_errno if read_file fails?

> > > +             _alpm_log(pkg->handle, ALPM_LOG_DEBUG, "found detached signature %s with size %d\n", sigpath, len);
> > > +             FREE(pkgpath);
> > > +             FREE(sigpath);
> > > +             return len;
> > > +     }
> > > +}
> > > +
> > >  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..0ab0fe26 100644
> > > --- a/lib/libalpm/sync.c
> > > +++ b/lib/libalpm/sync.c
> > > @@ -880,18 +880,17 @@ 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 *signature = NULL;
> > > +                     int sig_len = alpm_pkg_get_sig(pkg, &signature);
> > > +                     if(sig_len > 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, signature,
> > > +                                                     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 +904,8 @@ static int check_keyring(alpm_handle_t *handle)
> > >                                       }
> > >                                       FREELIST(keys);
> > >                               }
> > > -                             free(decoded_sigdata);
> > >                       }
> > > +                     free(signature);
> > >               }
> > >       }
> > >
> > > diff --git a/lib/libalpm/util.c b/lib/libalpm/util.c
> > > index 76728eb4..3d57817b 100644
> > > --- a/lib/libalpm/util.c
> > > +++ b/lib/libalpm/util.c
> > > @@ -1489,3 +1489,38 @@ 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
> > > + * @return size of the data read or negative number in case of error
> > > + */
> > > +int _alpm_read_file(const char *filepath, unsigned char **data)
> >
> > Same as above.
> >
> > > +{
> > > +     struct stat st;
> > > +     FILE *fp;
> > > +
> > > +     if((fp = fopen(filepath, "rb")) == NULL) {
> > > +             return -1;
> > > +     }
> > > +
> > > +     if(fstat(fileno(fp), &st) != 0) {
> > > +             fclose(fp);
> > > +             return -1;
> > > +     }
> > > +
> > > +     MALLOC(*data, st.st_size, fclose(fp); return -1);
> > > +
> > > +     if(fread(*data, st.st_size, 1, fp) != 1) {
> > > +             FREE(*data);
> > > +             fclose(fp);
> > > +             return -1;
> > > +     }
> > > +
> > > +     fclose(fp);
> > > +     return st.st_size;
> > > +}
> > > diff --git a/lib/libalpm/util.h b/lib/libalpm/util.h
> > > index 4fc6e718..50a94489 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);
> > > +int _alpm_read_file(const char *filepath, unsigned char **data);
> > >
> > >  #ifndef HAVE_STRSEP
> > >  char *strsep(char **, const char *);
> > > --
> > > 2.26.2

-- 
apg


More information about the pacman-dev mailing list