| Message ID | 20230920182251.1156432-1-amy.fong@siemens.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | gpg: add optional gpg signing verification | expand |
Hi Amy, first some general remarks: - documentation is missing. This adds a further way to verify the SWU and it should be documented. Add a patch for that. See also current documentation at : http://sbabic.github.io/swupdate/signed_images.html#choice-of-algorithm Documentation how to sign the SWU should be added, too (see openssl commands for current cases). - GPG_HOMEDIR to replace ~/.gnupg is quite forced. This adds an additional channel (ENV) to set up the tool and it is quite confusing. Currently, the only env variable that is evaluated by SWUpdate is TMPDIR, because it should be done before parsing the configuration to create sockets, etc. After that, configuration is done via swupdate.cfg, and overridded viy command line parameter. So homedir for gnupg should be configured via swupdate.cfg, and not via env. - the protocol is hard-coded to GPGME_PROTOCOL_OpenPGP. Can you explain this choice ? Shouldn't be open for other ones, like CMS ? On 20.09.23 20:22, amy.fong.3142@gmail.com wrote: > From: Amy Fong <amy.fong@siemens.com> > > This change introduces a Kconfig parameter allowing gpg verification. > The environment variable GPG_HOMEDIR, if set, is used to specify the > home directory. > > Signed-off-by: Amy Fong <amy.fong@siemens.com> > --- > Kconfig | 2 + > Makefile.flags | 5 ++ > corelib/Makefile | 1 + > corelib/swupdate_gpg_verify.c | 126 ++++++++++++++++++++++++++++++++++ > 4 files changed, 134 insertions(+) > create mode 100644 corelib/swupdate_gpg_verify.c > > diff --git a/Kconfig b/Kconfig > index 636c4ac..187a656 100644 > --- a/Kconfig > +++ b/Kconfig > @@ -431,6 +431,8 @@ choice > bool "mbedTLS" > depends on HAVE_MBEDTLS > > + config SIGALG_GPG > + bool "GPG signing" > endchoice > > > diff --git a/Makefile.flags b/Makefile.flags > index 2d27a8f..5046a69 100644 > --- a/Makefile.flags > +++ b/Makefile.flags > @@ -305,3 +305,8 @@ endif > # (we stole scripts/checkstack.pl from the kernel... thanks guys!) > # Reduced from 20k to 16k in 1.9.0. > FLTFLAGS += -s 16000 > + > +ifeq ($(CONFIG_SIGALG_GPG),y) > +LDLIBS += gpgme > +endif > + > diff --git a/corelib/Makefile b/corelib/Makefile > index 5f6f8e9..f5dda73 100644 > --- a/corelib/Makefile > +++ b/corelib/Makefile > @@ -32,6 +32,7 @@ endif > lib-$(CONFIG_SIGALG_RAWRSA) += swupdate_rsa_verify_mbedtls.o > lib-$(CONFIG_SIGALG_RSAPSS) += swupdate_rsa_verify_mbedtls.o > endif > +lib-$(CONFIG_SIGALG_GPG) += swupdate_gpg_verify.o > lib-$(CONFIG_LIBCONFIG) += swupdate_settings.o \ > parsing_library_libconfig.o > lib-$(CONFIG_JSON) += parsing_library_libjson.o server_utils.o > diff --git a/corelib/swupdate_gpg_verify.c b/corelib/swupdate_gpg_verify.c > new file mode 100644 > index 0000000..781c3bc > --- /dev/null > +++ b/corelib/swupdate_gpg_verify.c > @@ -0,0 +1,126 @@ > +/* > + * Author: Amy Fong > + * Copyright (C) 2023, Siemens AG > + * > + * SPDX-License-Identifier: GPL-2.0-only > + */ > +#include <stdio.h> > +#include <stdlib.h> > +#include <string.h> > +#include <stdbool.h> > +#include "swupdate.h" > +#include "sslapi.h" > +#include "util.h" > + > +#include <errno.h> > +#include <locale.h> > +#include <gpgme.h> > + > +static gpg_error_t > +status_cb(void *opaque, const char *keyword, const char *value) > +{ > + (void)opaque; > + TRACE("status_cb: %s %s\n", keyword, value); Is TRACE ok or should be DEBUG ? > + return 0; > +} > + > +int swupdate_verify_file(struct swupdate_digest *dgst, const char *sigfile, > + const char *file, const char *signer_name) > +{ > + gpgme_ctx_t ctx; > + gpgme_error_t err; > + gpgme_data_t image_sig, image; > + FILE *fp_sig = NULL; > + FILE *fp = NULL; > + gpgme_signature_t sig; > + char *gpg_home_dir = NULL; > + int status = 0; > + gpgme_protocol_t protocol = GPGME_PROTOCOL_OpenPGP; > + gpgme_verify_result_t result; > + > + TRACE("Enter swupdate_verify_file: gpg verify"); This seems to be copied from something else, and it does not print any information, it is useless. > + /* Initialize the locale environment. */ > + setlocale(LC_ALL, ""); This is global, and conflicts with other setup, for example with archive handler. You should use newlocale() and restore locale at the end of the function. Which is the reason for this ? Shouln't gpgme functions be called, ike gpgme_set_locale() ? > + (void)gpgme_check_version(NULL); Which is the use case ? The return value with the version string is ignored. > + > + err = gpgme_new(&ctx); > + if (err) { > + ERROR("Failed to create new gpg context: %s", > + gpgme_strerror(err)); > + status = -EFAULT; > + goto out; > + } > + > + gpgme_set_protocol(ctx, protocol); See comment above. > + gpgme_set_status_cb(ctx, status_cb, NULL); > + gpgme_set_ctx_flag(ctx, "full-status", "1"); This controls if status is called or not. Maybe should be also a configuration, it sets verbosity by gpgme. > + > + fp_sig = fopen(sigfile, "rb"); > + if (!fp_sig) { > + ERROR("Failed to open %s", sigfile); > + status = -EBADF; > + goto out; > + } > + err = gpgme_data_new_from_stream(&image_sig, fp_sig); > + if (err) { > + ERROR("error allocating data object: %s", gpgme_strerror(err)); > + status = -ENOMEM; > + goto out; > + } > + > + fp = fopen(file, "rb"); > + if (!fp) { > + ERROR("Failed to open %s", file); > + status = -EBADF; > + goto out; > + } > + err = gpgme_data_new_from_stream(&image, fp); > + if (err) { > + ERROR("error allocating data object: %s", gpgme_strerror(err)); > + status = -ENOMEM; > + goto out; > + } > + > + gpg_home_dir = getenv("GPG_HOMEDIR"); > + if (gpg_home_dir != NULL) { > + err = gpgme_ctx_set_engine_info(ctx, protocol, NULL, gpg_home_dir); See above. > + if (err) { > + ERROR("Something went wrong while setting the engine info: %s", > + gpgme_strerror(err)); Probably better gpgme_strerror_r, at least if new features will come. > + status = -EFAULT; > + goto out; > + } > + } > + > + err = gpgme_op_verify(ctx, image_sig, image, NULL); > + result = gpgme_op_verify_result(ctx); > + if (err) { > + ERROR("verify failed: %s\n", gpgme_strerror(err)); > + status = -EBADMSG; > + goto out; > + } > + > + if (result) { > + for (sig = result->signatures; sig; sig = sig->next) { > + if (sig->status == GPG_ERR_NO_ERROR) { > + TRACE("Verified OK\n"); > + status = 0; > + goto out; > + } > + } > + } > + TRACE(" Verification failed\n"); > + status = -EBADMSG; > + > + out: > + gpgme_data_release(image); > + gpgme_data_release(image_sig); > + gpgme_release(ctx); > + > + if (fp) > + fclose(fp); > + if (fp_sig) > + fclose(fp_sig); > + > + return status; > +} Best regards, Stefano Babic
diff --git a/Kconfig b/Kconfig index 636c4ac..187a656 100644 --- a/Kconfig +++ b/Kconfig @@ -431,6 +431,8 @@ choice bool "mbedTLS" depends on HAVE_MBEDTLS + config SIGALG_GPG + bool "GPG signing" endchoice diff --git a/Makefile.flags b/Makefile.flags index 2d27a8f..5046a69 100644 --- a/Makefile.flags +++ b/Makefile.flags @@ -305,3 +305,8 @@ endif # (we stole scripts/checkstack.pl from the kernel... thanks guys!) # Reduced from 20k to 16k in 1.9.0. FLTFLAGS += -s 16000 + +ifeq ($(CONFIG_SIGALG_GPG),y) +LDLIBS += gpgme +endif + diff --git a/corelib/Makefile b/corelib/Makefile index 5f6f8e9..f5dda73 100644 --- a/corelib/Makefile +++ b/corelib/Makefile @@ -32,6 +32,7 @@ endif lib-$(CONFIG_SIGALG_RAWRSA) += swupdate_rsa_verify_mbedtls.o lib-$(CONFIG_SIGALG_RSAPSS) += swupdate_rsa_verify_mbedtls.o endif +lib-$(CONFIG_SIGALG_GPG) += swupdate_gpg_verify.o lib-$(CONFIG_LIBCONFIG) += swupdate_settings.o \ parsing_library_libconfig.o lib-$(CONFIG_JSON) += parsing_library_libjson.o server_utils.o diff --git a/corelib/swupdate_gpg_verify.c b/corelib/swupdate_gpg_verify.c new file mode 100644 index 0000000..781c3bc --- /dev/null +++ b/corelib/swupdate_gpg_verify.c @@ -0,0 +1,126 @@ +/* + * Author: Amy Fong + * Copyright (C) 2023, Siemens AG + * + * SPDX-License-Identifier: GPL-2.0-only + */ +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <stdbool.h> +#include "swupdate.h" +#include "sslapi.h" +#include "util.h" + +#include <errno.h> +#include <locale.h> +#include <gpgme.h> + +static gpg_error_t +status_cb(void *opaque, const char *keyword, const char *value) +{ + (void)opaque; + TRACE("status_cb: %s %s\n", keyword, value); + return 0; +} + +int swupdate_verify_file(struct swupdate_digest *dgst, const char *sigfile, + const char *file, const char *signer_name) +{ + gpgme_ctx_t ctx; + gpgme_error_t err; + gpgme_data_t image_sig, image; + FILE *fp_sig = NULL; + FILE *fp = NULL; + gpgme_signature_t sig; + char *gpg_home_dir = NULL; + int status = 0; + gpgme_protocol_t protocol = GPGME_PROTOCOL_OpenPGP; + gpgme_verify_result_t result; + + TRACE("Enter swupdate_verify_file: gpg verify"); + /* Initialize the locale environment. */ + setlocale(LC_ALL, ""); + (void)gpgme_check_version(NULL); + + err = gpgme_new(&ctx); + if (err) { + ERROR("Failed to create new gpg context: %s", + gpgme_strerror(err)); + status = -EFAULT; + goto out; + } + + gpgme_set_protocol(ctx, protocol); + gpgme_set_status_cb(ctx, status_cb, NULL); + gpgme_set_ctx_flag(ctx, "full-status", "1"); + + fp_sig = fopen(sigfile, "rb"); + if (!fp_sig) { + ERROR("Failed to open %s", sigfile); + status = -EBADF; + goto out; + } + err = gpgme_data_new_from_stream(&image_sig, fp_sig); + if (err) { + ERROR("error allocating data object: %s", gpgme_strerror(err)); + status = -ENOMEM; + goto out; + } + + fp = fopen(file, "rb"); + if (!fp) { + ERROR("Failed to open %s", file); + status = -EBADF; + goto out; + } + err = gpgme_data_new_from_stream(&image, fp); + if (err) { + ERROR("error allocating data object: %s", gpgme_strerror(err)); + status = -ENOMEM; + goto out; + } + + gpg_home_dir = getenv("GPG_HOMEDIR"); + if (gpg_home_dir != NULL) { + err = gpgme_ctx_set_engine_info(ctx, protocol, NULL, gpg_home_dir); + if (err) { + ERROR("Something went wrong while setting the engine info: %s", + gpgme_strerror(err)); + status = -EFAULT; + goto out; + } + } + + err = gpgme_op_verify(ctx, image_sig, image, NULL); + result = gpgme_op_verify_result(ctx); + if (err) { + ERROR("verify failed: %s\n", gpgme_strerror(err)); + status = -EBADMSG; + goto out; + } + + if (result) { + for (sig = result->signatures; sig; sig = sig->next) { + if (sig->status == GPG_ERR_NO_ERROR) { + TRACE("Verified OK\n"); + status = 0; + goto out; + } + } + } + TRACE(" Verification failed\n"); + status = -EBADMSG; + + out: + gpgme_data_release(image); + gpgme_data_release(image_sig); + gpgme_release(ctx); + + if (fp) + fclose(fp); + if (fp_sig) + fclose(fp_sig); + + return status; +}