Re: [pacman-dev] [PATCH v2] Fallback to detached signatures during keyring check
Hi V2 patch avoids incorrect "int" -> "size_t" conversion in check_keyring() function.
Re: [pacman-dev] [PATCH v2] Fallback to detached signatures during keyring check
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
> ---
> 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.
> +{
> + 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.
> + 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.
> + _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) {
>
Re: [pacman-dev] [PATCH v2] Fallback to detached signatures during keyring check
Hi Andrew, thank you for the quick response
On Sat, May 30, 2020 at 9:31 PM Andrew Gregory
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
> > ---
> > 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.
>
> > +{
> > + 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?
>
> > + 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".
> > + _alpm_log(pkg->handle, ALPM_LOG_DEB
Re: [pacman-dev] [PATCH v2] Fallback to detached signatures during keyring check
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
> 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
> > > ---
> > > 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) {
> > >
Re: [pacman-dev] [PATCH v2] Fallback to detached signatures during keyring check
Hi
On Sun, May 31, 2020 at 3:24 AM Andrew Gregory
wrote:
>
> 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
> > 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
> > > > ---
> > > > 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.
Okay I see what you are suggesting. Sure I can move "length" value to
a function parameter.
> > >
> > > > +{
> > > > + 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.
It would be great to remove this untipattern completely. It will
reduce confu
