[v2] gpg: add optional gpg signing verification

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                       |   3 +-
 Makefile.flags                |   5 ++
 core/parser.c                 |   5 ++
 core/swupdate.c               |  27 ++++++
 corelib/Makefile              |   1 +
 corelib/swupdate_gpg_verify.c | 164 ++++++++++++++++++++++++++++++++++
 doc/source/signed_images.rst  |  61 +++++++++++--
 include/sslapi.h              |   3 +
 include/swupdate.h            |   2 +
 9 files changed, 265 insertions(+), 6 deletions(-)
 create mode 100644 corelib/swupdate_gpg_verify.c

Hi Amy,

On 22.09.23 17:20, 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                       |   3 +-
>   Makefile.flags                |   5 ++
>   core/parser.c                 |   5 ++
>   core/swupdate.c               |  27 ++++++
>   corelib/Makefile              |   1 +
>   corelib/swupdate_gpg_verify.c | 164 ++++++++++++++++++++++++++++++++++
>   doc/source/signed_images.rst  |  61 +++++++++++--
>   include/sslapi.h              |   3 +
>   include/swupdate.h            |   2 +
>   9 files changed, 265 insertions(+), 6 deletions(-)
>   create mode 100644 corelib/swupdate_gpg_verify.c
> 
> diff --git a/Kconfig b/Kconfig
> index 636c4ac..bd4ce04 100644
> --- a/Kconfig
> +++ b/Kconfig
> @@ -430,7 +430,6 @@ choice
>   	config SSL_IMPL_MBEDTLS
>   		bool "mbedTLS"
>   		depends on HAVE_MBEDTLS
> -
>   endchoice
>   
>   
> @@ -477,6 +476,8 @@ choice
>   		bool "Cryptographic Message Syntax (CMS) / PKCS#7"
>   		depends on SSL_IMPL_OPENSSL || SSL_IMPL_WOLFSSL
>   
> +	config SIGALG_GPG
> +		bool "GPG signing"
>   endchoice
>   
>   menu "CMS / PKCS#7 signature verification options"
> 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/core/parser.c b/core/parser.c
> index 50d9957..bbb7535 100644
> --- a/core/parser.c
> +++ b/core/parser.c
> @@ -143,8 +143,13 @@ int parse(struct swupdate_cfg *sw, const char *descfile)
>   	strcpy(sigfile, descfile);
>   	strcat(sigfile, ".sig");
>   
> +#ifdef CONFIG_SIGALG_GPG
> +	ret = swupdate_verify_file_gpg(sw->gpg_home_directory, sigfile, descfile,
> +				   sw->verbose, sw->gpgme_protocol);
> +#else
>   	ret = swupdate_verify_file(sw->dgst, sigfile, descfile,
>   				   sw->forced_signer_name);
> +#endif


This is quite nasty. This changes the API with the encryption functions 
that is up now consistent. It could be possible that I will introduce in 
future modules for the encryption, just as it is already done for other 
parts like the bootloader. Each crypto interface should then register 
itself with the core, but I need then a consistent API, and this is 
broken above.

The structure sw-> dgst ist thought to make opaque such as difference 
between the libraries. I do not want to change the API. Nevertheless, 
when this is required (never say never..), it should remain consistent 
and should be done for all libraries (mBEDtls, Wolfssl, ...).


>   	free(sigfile);
>   
>   	if (ret)
> diff --git a/core/swupdate.c b/core/swupdate.c
> index 6b536b8..d8a644c 100644
> --- a/core/swupdate.c
> +++ b/core/swupdate.c
> @@ -95,6 +95,7 @@ static struct option long_options[] = {
>   	{"key", required_argument, NULL, 'k'},
>   	{"ca-path", required_argument, NULL, 'k'},
>   	{"cert-purpose", required_argument, NULL, '1'},
> +	{"gpg-home-dir", required_argument, NULL, 'G'},
>   #if defined(CONFIG_SIGALG_CMS) && !defined(CONFIG_SSL_IMPL_WOLFSSL)
>   	{"forced-signer-name", required_argument, NULL, '2'},
>   #endif
> @@ -150,6 +151,7 @@ static void usage(char *programname)
>   		" -l, --loglevel <level>         : logging level\n"
>   		" -L, --syslog                   : enable syslog logger\n"
>   #ifdef CONFIG_SIGNED_IMAGES
> +#ifndef CONFIG_SIGALG_GPG
>   		" -k, --key <public key file>    : file with public key to verify images\n"
>   		"     --cert-purpose <purpose>   : set expected certificate purpose\n"
>   		"                                  [emailProtection|codeSigning] (default: emailProtection)\n"
> @@ -157,6 +159,10 @@ static void usage(char *programname)
>   		"     --forced-signer-name <cn>  : set expected common name of signer certificate\n"
>   #endif
>   		"     --ca-path                  : path to the Certificate Authority (PEM)\n"
> +#else
> +		" -G, --gpg-home-dir\n"
> +		"            <gpg home directory>: GnuPG home directory\n"
> +#endif
>   #endif
>   #ifdef CONFIG_ENCRYPTED_IMAGES
>   		" -K, --key-aes <key file>       : the file contains the symmetric key to be used\n"
> @@ -338,6 +344,10 @@ static int read_globals_settings(void *elem, void *data)
>   
>   	char software_select[SWUPDATE_GENERAL_STRING_SIZE] = "";
>   	GET_FIELD_STRING(LIBCFG_PARSER, elem, "select", software_select);
> +	GET_FIELD_STRING(LIBCFG_PARSER, elem,
> +				"gpg-home-dir", sw->gpg_home_directory);
> +	GET_FIELD_STRING(LIBCFG_PARSER, elem,
> +				"gpgme-protocol", sw->gpgme_protocol);
>   	if (software_select[0] != '\0') {
>   		/* by convention, errors in a configuration section are ignored */
>   		(void)parse_image_selector(software_select, sw);
> @@ -470,8 +480,12 @@ int main(int argc, char **argv)
>   	strcat(main_options, "H:");
>   #endif
>   #ifdef CONFIG_SIGNED_IMAGES
> +#ifndef CONFIG_SIGALG_GPG
>   	strcat(main_options, "k:");
>   	public_key_mandatory = 1;
> +#else
> +	strcat(main_options, "G:");
> +#endif
>   #endif
>   #ifdef CONFIG_ENCRYPTED_IMAGES
>   	strcat(main_options, "K:");
> @@ -620,6 +634,11 @@ int main(int argc, char **argv)
>   				optarg,
>   			       	sizeof(swcfg.publickeyfname));
>   			break;
> +		case 'G':
> +			strlcpy(swcfg.gpg_home_directory,
> +				optarg,
> +				sizeof(swcfg.gpg_home_directory));
> +			break;
>   		case '1':
>   			swcfg.cert_purpose = parse_cert_purpose(optarg);
>   			break;
> @@ -746,6 +765,14 @@ int main(int argc, char **argv)
>   		exit(EXIT_FAILURE);
>   	}
>   
> +#ifdef CONFIG_SIGALG_GPG
> +	if (!strlen(swcfg.gpg_home_directory)) {
> +		fprintf(stderr,
> +			 "Error: SWUpdate is built for signed images, provide a GnuPG home directory.\n");
> +		exit(EXIT_FAILURE);
> +	}
> +#endif
> +
>   	if (opt_c && !opt_i) {
>   		fprintf(stderr,
>   			"Error: Checking local images requires -i <file>.\n");
> 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..24bebbc
> --- /dev/null
> +++ b/corelib/swupdate_gpg_verify.c
> @@ -0,0 +1,164 @@
> +/*
> + * 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;
> +	DEBUG("status_cb: %s %s", keyword, value);
> +	return 0;
> +}
> +
> +#define MSGBUF_LEN 256
> +
> +int swupdate_verify_file_gpg(const char *gpg_home_dir, const char *sigfile,
> +			 const char *file, int verbose, const char *protocol_req)
> +{
> +	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;
> +	int status = 0;
> +	gpgme_protocol_t protocol;
> +	gpgme_verify_result_t result;
> +	char msg[MSGBUF_LEN];
> +	int r;
> +	const char *tmp;
> +
> +	tmp = gpgme_check_version(NULL);
> +	if (tmp == NULL) {
> +		ERROR("Failed to check gpgme library version");
> +		status = -EFAULT;
> +		goto out;
> +	}
> +
> +	err = gpgme_new(&ctx);
> +	if (err) {
> +		ERROR("Failed to create new gpg context");
> +		r = gpgme_strerror_r(err, msg, MSGBUF_LEN);
> +		if (r == 0) {
> +			ERROR("Reason: %s", msg);
> +		}
> +		status = -EFAULT;
> +		goto out;
> +	}
> +
> +	protocol = GPGME_PROTOCOL_OpenPGP;

I see my question is still unaswered, see review of V1.

> +	if (protocol_req != NULL) {
> +		DEBUG("gpg: Enabling protocol %s", protocol_req);
> +		if (!strcmp(protocol_req, "default")) {
> +			TRACE("GPGME: using protocol OpenPGP");
> +			protocol = GPGME_PROTOCOL_OpenPGP;
> +		} else {
> +			ERROR("Unsupported protocol! %s", protocol_req);
> +			status = -EFAULT;
> +			goto out;
> +		}
> +	}
> +	gpgme_set_protocol(ctx, protocol);
> +	gpgme_set_status_cb(ctx, status_cb, NULL);
> +	if (verbose == 1) {
> +		gpgme_set_ctx_flag(ctx, "full-status", "1");
> +	}
> +	gpgme_set_locale(ctx, LC_ALL, setlocale(LC_ALL, ""));
> +
> +	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");
> +		r = gpgme_strerror_r(err, msg, MSGBUF_LEN);
> +		if (r == 0) {
> +			ERROR("Reason: %s", msg);
> +		}
> +		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");
> +		r = gpgme_strerror_r(err, msg, MSGBUF_LEN);
> +		if (r == 0) {
> +			ERROR("Reason: %s", msg);
> +		}
> +		status = -ENOMEM;
> +		goto out;
> +	}
> +
> +	if (gpg_home_dir != NULL) {

Should not checked earlier ? If this is NULL, it makes no sense to 
allocate data or open files.

> +		err = gpgme_ctx_set_engine_info(ctx, protocol, NULL, gpg_home_dir);
> +		if (err) {
> +			ERROR("Something went wrong while setting the engine info");
> +			r = gpgme_strerror_r(err, msg, MSGBUF_LEN);
> +			if (r == 0) {
> +				ERROR("Reason: %s", msg);
> +			}
> +			status = -EFAULT;
> +			goto out;
> +		}
> +	}
> +
> +	err = gpgme_op_verify(ctx, image_sig, image, NULL);
> +	result = gpgme_op_verify_result(ctx);
> +	if (err) {
> +		ERROR("verify failed");
> +		r = gpgme_strerror_r(err, msg, MSGBUF_LEN);
> +		if (r == 0) {
> +			ERROR("Reason: %s", msg);
> +		}
> +		status = -EBADMSG;
> +		goto out;
> +	}
> +
> +	if (result) {
> +		for (sig = result->signatures; sig; sig = sig->next) {
> +			if (sig->status == GPG_ERR_NO_ERROR) {
> +				TRACE("Verified OK");
> +				status = 0;
> +				goto out;
> +			}
> +		}
> +	}
> +	TRACE("Verification failed");
> +	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;
> +}
> diff --git a/doc/source/signed_images.rst b/doc/source/signed_images.rst
> index 581196d..2aa37c5 100644
> --- a/doc/source/signed_images.rst
> +++ b/doc/source/signed_images.rst
> @@ -53,16 +53,20 @@ selected via menuconfig. Currently, the following mechanisms are implemented:
>   - RSA Public / private key. The private key belongs to the build system,
>     while the public key must be installed on the target.
>   - CMS using certificates
> +- GPG key signing
>   
> -Key or certificate is passed to SWUpdate with the `-k` parameter.
> +For RSA and CMS algorithms, key or certificate is passed to SWUpdate
> +with the `-k` parameter.
>   
>   Tool to generate keys / certificates
>   ------------------------------------
>   
> -The `openssl` tool is used to generate the keys. This is part of the
> -OpenSSL project. A complete documentation can be found at
> +For RSA and CMS signing, the `openssl` tool is used to generate the keys.
> +This is part of the OpenSSL project. A complete documentation can be found at
>   the `openSSL Website <https://www.openssl.org/docs/manmaster/man1/openssl.html>`_.
>   
> +For GPG, `gpg` can be used to generate the keys and to sign the images. A complete
> +documentation can be found at the `GnuPG Website <https://www.gnupg.org>`_.
>   
>   Usage with RSA PKCS#1.5 or RSA PSS
>   ----------------------------------
> @@ -158,6 +162,42 @@ Signing the image is simple as in the previous case:
>                   -inkey mycert.key.pem -outform DER -nosmimecap -binary
>   
>   
> +Usage with GNU PG
> +-----------------
> +
> +Generating a new keypair
> +........................
> +
> +
> +First, a primary keypair needs to be generated
> +
> +::
> +	gpg --gen-key
> +
> +The generated keys can be listed as follows
> +
> +::
> +	gpg -k
> +
> +Check the documentation for more information about parameters.
> +
> +How to sign with gpg
> +.....................
> +
> +Signing the image is very simple:
> +
> +::
> +	gpg --batch --output sw-description.sig
> +		--detach-sig sw-description
> +
> +For an alternative GnuPG home directory, and if there are multiple keypairs,
> +the following can be used to specify. In this example, the GnuPG home directory
> +is in GPG_HOMEDIR, while the signing key is found in GPG_KEY.
> +
> +::
> +	gpg --batch --homedir "${GPG_HOMEDIR}" --default-key "${GPG_KEY}" --output sw-description.sig
> +		--detach-sig sw-description
> +

It is quite missing what should be installed on the device. I am 
expecting that it is described how to get the public key (gpg export ...).

And some hints should be done how to install the key on the device. Are 
you running gpg import to put the key in the ring or are you doing in 
another way ? Please describe.

>   Building a signed SWU image
>   ---------------------------
>   
> @@ -187,6 +227,9 @@ A simple script to create a signed image can be:
>   	elif if [ x"$MODE" == "xRSA-PSS" ]; then
>   	    openssl dgst -sha256 -sign priv.pem -sigopt rsa_padding_mode:pss \
>   	        -sigopt rsa_pss_saltlen:-2 sw-description > sw-description.sig
> +	elif if [ x"$MODE" == "xGPG" ]; then
> +            gpg --batch --homedir "${GPG_HOME_DIR}" --default-key "${GPG_KEY}" \
> +                --output sw-description.sig --detach-sig sw-description
>           else
>               openssl cms -sign -in  sw-description -out sw-description.sig -signer mycert.cert.pem \
>                   -inkey mycert.key.pem -outform DER -nosmimecap -binary
> @@ -233,5 +276,13 @@ Running SWUpdate with signed images
>   
>   Verification is activated by setting CONFIG_SIGNED_IMAGES in SWUpdate's configuration.
>   If activated, SWUpdate will always check the compound image. For security reasons,
> -it is not possible to disable the check at runtime. The -k parameter (public key file)
> -is mandatory and the program stops if the public key is not passed.
> +it is not possible to disable the check at runtime.
> +
> +For RSA and CMS signing, the -k parameter (public key file) is mandatory and the program stops
> +if the public key is not passed.
> +
> +For GPG signing, CONFIG_SIGALG_GPG needs to be enabled. The -G parameter
> +(gpg-home-dir in swupdate.cfg) should be used to specify the GnuPG home directory.
> +Currently, the only protocol supported is GPGME_PROTOCOL_OpenPGP, the mechanism for supporting
> +other protocols can be specified by specifying gpgme-protocol in the config file, currently,
> +only "default" is handled.
> diff --git a/include/sslapi.h b/include/sslapi.h
> index a24a152..a04d942 100644
> --- a/include/sslapi.h
> +++ b/include/sslapi.h
> @@ -178,6 +178,8 @@ void swupdate_HASH_cleanup(struct swupdate_digest *dgst);
>   int swupdate_verify_file(struct swupdate_digest *dgst, const char *sigfile,
>   				const char *file, const char *signer_name);
>   int swupdate_HASH_compare(const unsigned char *hash1, const unsigned char *hash2);
> +int swupdate_verify_file_gpg(const char *gpg_home_dir, const char *sigfile,
> +				const char *file, int verbose, const char *gpgme_protocol);
>   
>   
>   #else
> @@ -188,6 +190,7 @@ int swupdate_HASH_compare(const unsigned char *hash1, const unsigned char *hash2
>   #define swupdate_HASH_final(p, result, len)	(-1)
>   #define swupdate_HASH_cleanup(sw)
>   #define swupdate_HASH_compare(hash1,hash2)	(0)
> +#define swupdate_verify_file_gpg(gpg_home_dir, sigfile, file, verbose, gpgme_protocol) ( 0 )
>   #endif
>   
>   #ifdef CONFIG_ENCRYPTED_IMAGES
> diff --git a/include/swupdate.h b/include/swupdate.h
> index e473e41..83d61c6 100644
> --- a/include/swupdate.h
> +++ b/include/swupdate.h
> @@ -93,6 +93,8 @@ struct swupdate_cfg {
>   	void *dgst;	/* Structure for signed images */
>   	struct swupdate_parms parms;
>   	const char *embscript;
> +	char gpg_home_directory[SWUPDATE_GENERAL_STRING_SIZE];
> +	char gpgme_protocol[SWUPDATE_GENERAL_STRING_SIZE];
>   };
>   
>   #define SEARCH_FILE(img, list, found, offs) do { \

Best regards,
Stefano Babic