Re: [ptxdist] [PATCH v1 5/5] ptxd_lib_code_signing: add key whitelist checks

[Michael Olbrich <m.olbrich@pengutronix.de>]

On Wed, Aug 04, 2021 at 04:23:30PM +0200, Roland Hieber wrote:
> Signed-off-by: Roland Hieber <rhi@pengutronix.de>
> ---
>  doc/dev_code_signing.rst                  | 68 +++++++++++++++++++++++
>  platforms/code-signing.in                 | 22 ++++++++
>  rules/pre/030-code-signing-consumers.make |  6 ++
>  scripts/lib/ptxd_lib_code_signing.sh      | 52 ++++++++++++++++-
>  4 files changed, 147 insertions(+), 1 deletion(-)
> 
> diff --git a/doc/dev_code_signing.rst b/doc/dev_code_signing.rst
> index 413f694980eb..aaaaae0d9107 100644
> --- a/doc/dev_code_signing.rst
> +++ b/doc/dev_code_signing.rst
> @@ -172,3 +172,71 @@ also via an environment variable.
>    (``=``, not ``:=``).
>    Otherwise the variable is expanded before a code signing provider can perform
>    its setup.
> +
> +Key Whitelisting
> +~~~~~~~~~~~~~~~~
> +
> +In some use cases, it may be feasible to do additional checks to make sure that
> +a package uses the correct key material.
> +For example, suppose you have a "development" code signing provider (e.g. with a
> +SoftHSM workflow) which you use to sign your development kernels,
> +and a "release" provider (e.g. on a HSM) which you use to sign your release kernels,
> +and a "development" and "release" variant of the bootloader which only boots
> +kernels that have been signed with the respective development or release key.
> +Now you might want to prevent the "release" bootloader from accidentally being
> +provided with the key for the "development" kernel –
> +otherwise someone could boot a development kernel on an actual production
> +board, and use the additional debugging features of the kernel to do things
> +with it that it was not meant to do.

I don't think this is a good example and somewhat confusing. It's just fine
to add development (public) keys to a bootloader with a 'release'
configuration, as long as the bootloader is also signed with a development
key.
And validation if release and development keys are mixed is not possible
with this mechanism. But that should not be a problem, because release and
development keys should not be available at the same time. Maybe the public
keys of the release keys could be added when otherwise using development
keys, but that must be handled in the provider.

A better example would be a bootloader with a development configuration
that should not be signed with a release key. But the reverse is just fine.

And please don't just use "release bootloader", that's ambiguous. There is
the feature set or configuration, and there is the key. And we can gave
'development' and 'release' for both.
And 3 of the 4 possible combinations are ok and one is not.

> +
> +When the ``CODE_SIGNING_REQUIRE_WHITELIST`` kconfig symbol is enabled,
> +the consumer functions :ref:`ptx/cs-get-ca` and :ref:`ptx/cs-get-uri`
> +look up the triplet of package name, role name, and the pubkey's SHA256
> +fingerprint in a whitelist, whose file name is determined by the
> +``CODE_SIGNING_WHITELIST_FILENAME`` kconfig symbol (the default path is
> +``configs/platform-<name>/code-signing-whitelist``).
> +If a key or a CA is not whitelisted for the package in which it is to be used,
> +the functions will exit with an error message on the terminal::
> +
> +   $ ptxdist -v print KERNEL_MAKE_OPT
> +   ptxdist: error: SPKI whitelist record 'kernel kernel-modules
> +   69C9BBB8BB4DFAE74AB21D06DFB5F2C67066373AE545453276847340822CDF04' not found in
> +   distrokit/configs/platform-v7a/code-signing-whitelist
> +
> +   …/ptxdist/rules/kernel.make:196: *** cs-get-uri: whitelist check failed, see errors above.  Stop.
> +
> +
> +If ``CODE_SIGNING_REQUIRE_WHITELIST`` is disabled (the default),
> +all keys and CAs are provided to all packages without further checks.
> +
> +The format of the code signing whitelist consists of one triplet per line, in
> +which the elements of the triplet are separated by whitespace.
> +If a CA is to be checked, the role name is prefixed with a literal ``ca:``.
> +All other unmatched lines in the file are ignored, but we suggest to use ``#``
> +to start a line comment so as not to add a whitelist record accidentally.
> +
> +For example, here is a whitelist for use with the *devel* code provider which
> +allows all provided keys to be used by their respective consumers::
> +
> +   # format: package-name role-name sha256-pubkey-fingerprint
> +   kernel      kernel-modules 69C9BBB8BB4DFAE74AB21D06DFB5F2C67066373AE545453276847340822CDF04
> +   image-rauc  update         0034F8FE5ADC3B0DFE642407275D144DE2398C68CC9A86DD6703D7151116B44E
> +   image-rauc  ca:update      0034F8FE5ADC3B0DFE642407275D144DE2398C68CC9A86DD6703D7151116B44E
> +   rauc        ca:update      0034F8FE5ADC3B0DFE642407275D144DE2398C68CC9A86DD6703D7151116B44E
> +
> +.. note:: The match is case-sensitive, and the fingerprints are expected
> +   in uppercase.
> +
> +   If a CA consists of more than one certificate, all of their fingerprints
> +   must be whitelisted.
> +
> +You can determine the key fingerprints by copying it from the error message,
> +or with the `spki-hash`__ tool from the ``host-extract-cert`` package,
> +or with openssl::
> +
> +   $ openssl pkey -in keyfile.pem -pubout -outform der \
> +     | openssl sha256 \
> +     | tr 'a-z' 'A-Z'
> +   (STDIN)= 69C9BBB8BB4DFAE74AB21D06DFB5F2C67066373AE545453276847340822CDF04
> +
> +__ https://git.pengutronix.de/cgit/extract-cert/tree/spki-hash.c
> diff --git a/platforms/code-signing.in b/platforms/code-signing.in
> index 81f9ef6f3c9e..92f555c7e965 100644
> --- a/platforms/code-signing.in
> +++ b/platforms/code-signing.in
> @@ -20,4 +20,26 @@ source "generated/code_signing_provider.in"
>  
>  endchoice
>  
> +config CODE_SIGNING_REQUIRE_WHITELISTED_KEYS
> +	bool
> +	prompt "require whitelisted keys"
> +	help
> +	  Every time a key from the code provider is used, check if the consumer
> +	  is allowed to use it.
> +
> +	  Code signing consumers can depend on this option if they want to force
> +	  the key whitelist check.
> +
> +if CODE_SIGNING_REQUIRE_WHITELISTED_KEYS
> +
> +config CODE_SIGNING_WHITELIST_FILENAME
> +	string
> +	prompt "whitelist file name"
> +	default "code-signing-whitelist"
> +	help
> +	  This file name is used for the key whitelist. The path is relative to
> +	  PTXDIST_PLATFORMCONFIGDIR (e.g. configs/platform-nnn/).
> +
> +endif # CODE_SIGNING_REQUIRE_WHITELISTED_KEYS
> +
>  endif
> diff --git a/rules/pre/030-code-signing-consumers.make b/rules/pre/030-code-signing-consumers.make
> index 72ee8e63b7b9..439db626dd14 100644
> --- a/rules/pre/030-code-signing-consumers.make
> +++ b/rules/pre/030-code-signing-consumers.make
> @@ -27,6 +27,9 @@ ptx/cs-get-uri = \
>  			$(call ptx/cs-consumer-env, $(1))\
>  				cs_get_uri '$(strip $(2))'\
>  		)\
> +		$(if $(filter-out 0,$(.SHELLSTATUS)),\
> +			$(error cs-get-uri: whitelist check failed, see errors above)\

Use ptx/error. There are cases where the output of $(error ...) gets lost.

> +		)\
>  	)
>  
>  #
> @@ -38,4 +41,7 @@ ptx/cs-get-ca = \
>  			$(call ptx/cs-consumer-env, $(1))\
>  				cs_get_ca '$(strip $(2))'\
>  		)\
> +		$(if $(filter-out 0,$(.SHELLSTATUS)),\
> +			$(error cs-get-ca: whitelist check failed, see errors above)\
> +		)\
>  	)
> diff --git a/scripts/lib/ptxd_lib_code_signing.sh b/scripts/lib/ptxd_lib_code_signing.sh
> index 24730d3cf742..332bab22e7c8 100644
> --- a/scripts/lib/ptxd_lib_code_signing.sh
> +++ b/scripts/lib/ptxd_lib_code_signing.sh
> @@ -1,6 +1,7 @@
>  #!/bin/bash
>  #
>  # Copyright (C) 2019 Sascha Hauer <s.hauer@pengutronix.de>
> +# Copyright (C) 2020 Jan Luebbe <j.luebbe@pengutronix.de>
>  # Copyright (C) 2021 Roland Hieber, Pengutronix <rhi@pengutronix.de>
>  #
>  # For further information about the PTXdist project and license conditions
> @@ -70,6 +71,7 @@ cs_init_variables() {
>      sysroot="$(ptxd_get_ptxconf PTXCONF_SYSROOT_HOST)"
>      keyprovider="$(ptxd_get_ptxconf PTXCONF_CODE_SIGNING_PROVIDER)"
>      keydir="${sysroot}/var/lib/keys/${keyprovider}"
> +    whitelist="$(ptxd_get_ptxconf PTXCONF_CODE_SIGNING_WHITELIST_FILENAME || true)"
>  }
>  export -f cs_init_variables
>  
> @@ -157,6 +159,40 @@ cs_group_get_roles() {
>  }
>  export -f cs_group_get_roles
>  
> +#
> +# cs_check_whitelisted <role> <uri/pem or fingerprint prefixed with "sha256:">
> +#
> +# Checks if the SPKI (Subject Public Key Info) Hash is in the whitelist
> +#
> +cs_check_whitelisted() {
> +    local role="${1:-ERROR_ROLE_IS_EMPTY}"
> +    local src="${2}"
> +
> +    if [ "$(ptxd_get_ptxconf PTXCONF_CODE_SIGNING_REQUIRE_WHITELISTED_KEYS)" != "y" ]; then
> +	return
> +    fi
> +
> +    if ! ptxd_in_path PTXDIST_PATH_PLATFORMCONFIGDIR "${whitelist}"; then

	echo ERROR_KEY_NOT_WHITELISTED

> +	ptxd_bailout "${pkg_name}: SPKI hash whitelist check for ${role} (${src}) is required, but configs/<platform>/${whitelist} is missing."
> +    fi
> +    local whitelist_path="${ptxd_reply}"
> +
> +    local hash
> +    if [ "${src#sha256:}" != "${src}" ]; then
> +	hash="${src#sha256:}"
> +    else
> +	hash=$(ptxd_exec_silent_stderr spki-hash "${src}")
> +    fi
> +    hash=${hash:-ERROR_HASH_IS_EMPTY}
> +    needle="${pkg_name}\s\+${role}\s\+${hash}"

needle should be local.

> +
> +    if ! grep -q --line-regexp "${needle}" "${whitelist_path}"; then
> +	echo ERROR_KEY_NOT_WHITELISTED
> +	ptxd_bailout "SPKI whitelist record '${pkg_name} ${role} ${hash}' not found in $(ptxd_print_path "${whitelist_path}")"
> +    fi
> +}
> +export -f cs_check_whitelisted
> +
>  #
>  # cs_set_uri <role> <uri>
>  #
> @@ -198,7 +234,12 @@ cs_get_uri() {
>  	    return
>  	fi
>      fi
> -    cat "${keydir}/${role}/uri"
> +
> +    local uri=$(cat "${keydir}/${role}/uri")
> +    if [ -z "${cs_no_whitelist_check}" ]; then
> +	cs_check_whitelisted "${role}" "${uri}"
> +    fi
> +    echo "${uri}"
>  }
>  export -f cs_get_uri
>  
> @@ -321,6 +362,9 @@ cs_get_ca() {
>      fi
>  
>      if [ -e "${ca}" ]; then
> +	while read fp; do
> +	    cs_check_whitelisted "ca:${role}" "sha256:${fp}"
> +	done < "${keydir}/${role}/ca.fingerprints"
>  	echo "${ca}"
>      fi
>  }
> @@ -339,6 +383,9 @@ cs_append_ca_from_pem() {
>      cat "${pem}" >> "${keydir}/${role}/ca.pem"
>      # add new line in case ${pem} does not end with an EOL
>      echo >> "${keydir}/${role}/ca.pem"
> +
> +    openssl x509 -in "${pem}" -inform pem -noout -pubkey | \
> +	spki-hash /dev/stdin >> "${keydir}/${role}/ca.fingerprints"
>  }
>  export -f cs_append_ca_from_pem
>  
> @@ -370,7 +417,10 @@ cs_append_ca_from_uri() {
>      cs_init_variables
>  
>      if [ -z "${uri}" ]; then
> +	# cs_append_ca_from_der will check this later
> +	local cs_no_whitelist_check=1
>  	uri=$(cs_get_uri "${role}")
> +	unset cs_no_whitelist_check

I prefer a cs_get_uri_unchecked() function (or something like that) and
call that on here and in cs_get_uri().

Michael

>      fi
>  
>      ptxd_exec extract-cert "${uri}" "${tmpdir}/ca.der" &&
> -- 
> 2.30.2
> 
> 
> _______________________________________________
> ptxdist mailing list
> ptxdist@pengutronix.de
> To unsubscribe, send a mail with subject "unsubscribe" to ptxdist-request@pengutronix.de

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
ptxdist mailing list
ptxdist@pengutronix.de
To unsubscribe, send a mail with subject "unsubscribe" to ptxdist-request@pengutronix.de