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

[Roland Hieber <rhi@pengutronix.de>]

On Fri, Sep 03, 2021 at 03:17:13PM +0200, Michael Olbrich wrote:
> On Mon, Aug 09, 2021 at 10:06:08AM +0200, Roland Hieber wrote:
> > Signed-off-by: Roland Hieber <rhi@pengutronix.de>
> > ---
> > PATCH v2:
> >  - cs_check_whitelisted: make "needle"  local variable (feedback by
> >    Michael Olbrich)
> >  - cs_check_whitelisted: error out with ERROR_KEY_NOT_WHITELISTED also
> >    if whitelist does not exist yet (Michael Olbrich)
> >  - rename cs_get_uri to cs_get_uri_unchecked and let cs_get_uri wrap it
> >    instead of setting cs_no_whitelist_check=1 (Michael Olbrich)
> >  - docs: simplify introductory example (Michael Olbrich)
> >  - docs: add short paragraph on how to determine fingerprints of certs
> > 
> > PATCH v1: https://lore.ptxdist.org/ptxdist/
> > ---
> >  doc/dev_code_signing.rst                  | 80 +++++++++++++++++++++++
> >  platforms/code-signing.in                 | 22 +++++++
> >  rules/pre/030-code-signing-consumers.make |  6 ++
> >  scripts/lib/ptxd_lib_code_signing.sh      | 71 ++++++++++++++++++--
> >  4 files changed, 172 insertions(+), 7 deletions(-)
> > 
> > diff --git a/doc/dev_code_signing.rst b/doc/dev_code_signing.rst
> > index 413f694980eb..28ebe055346e 100644
> > --- a/doc/dev_code_signing.rst
> > +++ b/doc/dev_code_signing.rst
> > @@ -172,3 +172,83 @@ 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 "release" code signing provider which you use
> > +to sign bootloaders for production,
> > +and a "development" code signing provider which you use to sign bootloaders
> > +with an extended feature set, e.g. to allow booting arbitrary kernels and
> > +userlands for debugging purposes.
> > +Your production boards are locked down in hardware so the ROM code only
> > +executes bootloaders signed with the "release" key.
> > +Now you don't want any bootloader with debugging features to be signed with a
> > +release key, otherwise someone could boot them on a locked-down production
> > +device, and use the additional debugging features to get extended access to the
> > +production device.
> > +In this case, key whitelisting can help to prevent signing bootloader packages
> > +with the wrong key.
> > +
> > +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 the 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:``,
> > +and the fingerprint refers to the public key of the certificate.
> > +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
> > +
> > +or, in the case of certificates::
> > +
> > +   $ openssl x509 -noout -in cert.pem -pubkey \
> > +     | openssl pkey -pubin -inform pem -pubout -outform der \
> > +     | openssl sha256 \
> > +     | tr 'a-z' 'A-Z'
> > +   (STDIN)= 0034F8FE5ADC3B0DFE642407275D144DE2398C68CC9A86DD6703D7151116B44E
> > +
> > +__ 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/).
> 
> I think, we can just hard-code this name. No need for a config option.
> Or is there a use-case for this?

Hmm yes. The more adequate use case now is probably to add a new layer
on top instead of using different platformconfigs with different key
whitelists for the same platform. So I'd hard-code it too.

> > +
> > +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 909e8ebd6936..bca05854372d 100644
> > --- a/rules/pre/030-code-signing-consumers.make
> > +++ b/rules/pre/030-code-signing-consumers.make
> > @@ -28,6 +28,9 @@ $(strip \
> >  		$(call ptx/cs-consumer-env, $(1))\
> >  			cs_get_uri '$(strip $(2))'\
> >  	)\
> > +	$(if $(filter-out 0,$(.SHELLSTATUS)),\
> > +		$(call ptx/error, cs-get-uri: whitelist check failed – see errors above)\
> > +	)\
> 
> You cannot use ptx/error here. That macro can only be used for stuff that
> is evaluated before the first target is executed. Otherwise the error
> message will get lost.
> You need to use $(error ...) here. Probably with the same $(shell :) hack
> that is used in kernel.make.

Okay.

> >  )
> >  endef
> >  
> > @@ -40,5 +43,8 @@ $(strip \
> >  		$(call ptx/cs-consumer-env, $(1))\
> >  			cs_get_ca '$(strip $(2))'\
> >  	)\
> > +	$(if $(filter-out 0,$(.SHELLSTATUS)),\
> > +		$(call ptx/error, cs-get-ca: whitelist check failed – see errors above)\
> > +	)\
> 
> same here.
> 
> >  )
> >  endef
> > diff --git a/scripts/lib/ptxd_lib_code_signing.sh b/scripts/lib/ptxd_lib_code_signing.sh
> > index 24730d3cf742..c855821bd039 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,42 @@ 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}"
> > +    cs_init_variables
> > +
> > +    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}
> > +    local needle="${pkg_name}\s\+${role}\s\+${hash}"
> > +
> > +    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>
> >  #
> > @@ -171,12 +209,8 @@ cs_set_uri() {
> >  }
> >  export -f cs_set_uri
> >  
> > -#
> > -# cs_get_uri <role>
> > -#
> > -# Get the uri from a role
> > -#
> > -cs_get_uri() {
> > +# internal helper for cs_get_uri
> > +cs_get_uri_unchecked() {
> >      if [ -z "${pkg_name}" ]; then
> >  	    echo ERROR_UNSUPPORTED_CS_API_CALL
> >  	    ptxd_bailout '$(shell cs_get_uri, <role>) is no longer supported in make files.' \
> > @@ -198,8 +232,24 @@ cs_get_uri() {
> >  	    return
> >  	fi
> >      fi
> > +
> >      cat "${keydir}/${role}/uri"
> >  }
> > +export -f cs_get_uri_unchecked
> > +
> > +#
> > +# cs_get_uri <role>
> > +#
> > +# Get the uri from a role
> > +#
> > +cs_get_uri() {
> > +    local role="${1}"
> > +    local uri=$(cs_get_uri_unchecked "$1")
> > +
> > +    if cs_check_whitelisted "${role}" "${uri}"; then
> > +	echo "${uri}"
> > +    fi
> > +}
> >  export -f cs_get_uri
> >  
> >  #
> > @@ -321,6 +371,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 +392,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"
> 
> This should happen first, before the extending ca.pem and make sure to
> abort on error. Use check_pipe_status to catch errors from openssl.

Okay.

> >  }
> >  export -f cs_append_ca_from_pem
> >  
> > @@ -370,7 +426,8 @@ cs_append_ca_from_uri() {
> >      cs_init_variables
> >  
> >      if [ -z "${uri}" ]; then
> > -	uri=$(cs_get_uri "${role}")
> > +	# cs_append_ca_from_der will check this later
> 
> I don't think this comment is correct. There is no checking while adding to
> the CA, right?

Yes, it's outdated. cs_append_ca_from_der calls cs_append_ca_from_pem,
which extracts the fingerprints, but the checking happens in cs_get_ca.

 - Roland

> Michael
> 
> > +	uri=$(cs_get_uri_unchecked "${role}")
> >      fi
> >  
> >      ptxd_exec extract-cert "${uri}" "${tmpdir}/ca.der" &&
> > -- 
> > 2.30.2

-- 
Roland Hieber, Pengutronix e.K.          | r.hieber@pengutronix.de     |
Steuerwalder Str. 21                     | https://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