Re: [gentoo-dev] Re: [PATCH] linux-mod.eclass: support module signing

[Michał Górny]

W dniu czw, 19.04.2018 o godzinie 22∶42 -0700, użytkownik Georgy
Yakovlev napisał:
> On Sat, 2018-04-14 at 14:25 -0700, Georgy Yakovlev wrote:
>
> Second version, with safety checks and simplified logic.
> Fixed most issues of the first patch.
>
> Now only use single optional make.conf variable with the path to the
> key.
> Rest of parameters are magically extracted from .config or derived from
> the key itself. So generally it just works.
>
> got rid of STRIP_MASK, all signing happens in pkg_preinst, that way the
> checksum of installed file is calculated with signature appended.
> now works for packages that do not use linux-mod_src_install (zfs & co)
>
>
> Thanks to NP-Hardass for initial review and suggestions.
>
>
> > Hi,
> >
> > There is an old bug[1] to support
> > linux kernel module signing at install.
> >
> > And here is my first attempt to modify an eclass.
> > Need proper input on it and a kick in the right direction.
> >
> > Add 3 variables, settable by users if they keep keys somewhere safe.
> > Otherwise it just works with the auto-generated keys
> > if CONFIG_MODULE_SIG=y and vars are unset.
> >
> > eclass will die if kernel requires a signed module,
> > but signing is not requested.
> >
> >
> > Known problems:
> >
> > Packages that do not use linux-mod_src_install() will not sign
> > the modules,
> > But those packages will still inherit module-sign useflag.
> > It's misleading and I'm not sure how to fix that.
> > Examples : sys-kernel/spl, sys-fs/zfs-kmod
> >
> > May need additional handling of KBUILD_SIGN_PIN variable[2],
> > which can be set to hold the passphrase to the key. But it may end up
> > in vdb environment files, not sure how to handle that or if it worth
> > it
> >
> > not eapi-7 ready because of STRIP_MASK usage.
> > will need to cover this case as well, probably later.
> >
> > older (<4.3.3) kernels use perl to sign modules, not sure if it's
> > worth
> > supporting old kernels, there is no gentoo-sources in the tree old
> > enough, except masked 4.1
> > there are old vanilla-sources that will be affected by this.
> >
> >
> > [1] https://bugs.gentoo.org/447352
> > [2] https://www.kernel.org/doc/html/v4.16/admin-guide/module-signing.html
>
> diff --git a/eclass/linux-mod.eclass b/eclass/linux-mod.eclass
> index bf580cf4cfa9..8197654081cc 100644
> --- a/eclass/linux-mod.eclass
> +++ b/eclass/linux-mod.eclass
> @@ -132,6 +132,16 @@
>  # @DESCRIPTION:
>  # It's a read-only variable. It contains the extension of the kernel modules.
>
> +# @ECLASS-VARIABLE: KERNEL_MODULE_SIG_KEY

Also @USER_VARIABLE since it's supposed to be set in make.conf.

> +# @DEFAULT_UNSET
> +# @DESCRIPTION:
> +# A string, containing absolute path to the private key file.
> +# Defaults to value of CONFIG_MODULE_SIG_KEY extracted from .config
> +# Can be set by user in make.conf
> +# Example:
> +# KERNEL_MODULE_SIG_KEY="/secure/location/keys/kernel.pem"
> +# Assumes that "/secure/location/keys/kernel.x509" is a matching pubkey.
> +
>  inherit eutils linux-info multilib
>  EXPORT_FUNCTIONS pkg_setup pkg_preinst pkg_postinst src_install src_compile pkg_postrm
>
> @@ -144,12 +154,13 @@ esac
>  	0) die "EAPI=${EAPI} is not supported with MODULES_OPTIONAL_USE_IUSE_DEFAULT due to lack of IUSE defaults" ;;
>  esac
>
> -IUSE="kernel_linux ${MODULES_OPTIONAL_USE:+${_modules_optional_use_iuse_default}}${MODULES_OPTIONAL_USE}"
> +IUSE="module-sign kernel_linux ${MODULES_OPTIONAL_USE:+${_modules_optional_use_iuse_default}}${MODULES_OPTIONAL_USE}"
>  SLOT="0"
>  RDEPEND="${MODULES_OPTIONAL_USE}${MODULES_OPTIONAL_USE:+? (} kernel_linux? ( virtual/modutils ) ${MODULES_OPTIONAL_USE:+)}"
>  DEPEND="${RDEPEND}
>      ${MODULES_OPTIONAL_USE}${MODULES_OPTIONAL_USE:+? (}
>  	sys-apps/sed
> +	module-sign? ( || ( dev-libs/openssl dev-libs/libressl ) )
>  	kernel_linux? ( virtual/linux-sources )
>  	${MODULES_OPTIONAL_USE:+)}"
>
> @@ -352,6 +363,93 @@ get-KERNEL_CC() {
>  	echo "${kernel_cc}"
>  }
>
> +# @FUNCTION: check_sig_force

Namespace pollution.  Please prefix it.

> +# @INTERNAL
> +# @DESCRIPTION:
> +# Check if kernel requires module signing and die
> +# if module is not going to be signed.
> +check_sig_force() {
> +	debug-print-function ${FUNCNAME} $*

"${@}"

> +
> +	if linux_chkconfig_present MODULE_SIG_FORCE; then
> +		if use !module-sign; then
> +			ewarn "kernel .config has MODULE_SIG_FORCE=y option set"
> +			ewarn "This means that kernel requires all modules"
> +			ewarn "to be signed and verified before loading"
> +			ewarn "please enable USE=\"module-sign\" or reconfigure your kernel"
> +			ewarn "otherwise loading the module will fail"

Why ewarn if you die?  eerror would be more appropriate.

> +			die "signature required"
> +		fi
> +	fi
> +}
> +
> +# @FUNCTION: sign_module

Likewise.

> +# @INTERNAL
> +# @DESCRIPTION:
> +# Sign a kernel module
> +# @USAGE: <filename>

@USAGE goes earlier.

> +sign_module() {
> +	debug-print-function ${FUNCNAME} $*
> +
> +	local dotconfig_sig_hash dotconfig_sig_key
> +	local sign_binary_path sig_key_path sig_x509_path
> +	local module
> +
> +	# extract values from kernel .config
> +	# extracted key path is not full, e.g. "certs/signing_key.pem"
> +	dotconfig_sig_hash="$(linux_chkconfig_string MODULE_SIG_HASH)"
> +	dotconfig_sig_key="$(linux_chkconfig_string MODULE_SIG_KEY)"
> +
> +	# strip out double quotes, sign-file binary chokes on them
> +	dotconfig_sig_hash=${dotconfig_sig_hash//\"/}
> +	dotconfig_sig_key=${dotconfig_sig_key//\"/}
> +
> +	sign_binary_path="${KV_OUT_DIR}/scripts/sign-file"
> +	sig_key_path="${KERNEL_MODULE_SIG_KEY:-${KV_OUT_DIR}/${dotconfig_sig_key}}"
> +	sig_x509_path="${sig_key_path/.pem/.x509}"
> +
> +	module=$(basename "${1%.${KV_OBJ}}")

Don't call external programs when you can do the same in trivial pure
bash, i.e. ${foo##*/}.

> +
> +	# some checks, because sign-file is dumb and produces cryptic errors
> +	[ -w "${1}" ] || die "${1} not found or not writable"

Use [[ ... ]], always.

> +	grep -qFL '~Module signature appended~' "${1}" && die "${module} already signed"
> +	[ -x "${sign_binary_path}" ] || die "${sign_binary_path} not found or not executable"
> +	[ -e "${sig_key_path}" ] || die "Private key ${sig_key_path} not found or not readable"

-e does not test for being readable.  Are you looking for -r?

> +	[ -e "${sig_x509_path}" ] || die "Public key ${sig_x509_path} not found or not readable"
> +
> +	einfo "Signing ${module} using ${sig_key_path}:${dotconfig_sig_hash}"
> +	"${sign_binary_path}" \
> +		"${dotconfig_sig_hash}" "${sig_key_path}" "${sig_x509_path}" \
> +		"${1}" || die "Signing ${module} failed"
> +}
> +
> +# @FUNCTION: sign_all_modules
> +# @INTERNAL
> +# @DESCRIPTION:
> +# Signs all unsigned modules
> +# Must be called in pkg_preinst.
> +sign_all_modules() {
> +	debug-print-function ${FUNCNAME} $*
> +
> +	[ -z "${KV_OBJ}" ] && set_kvobj;

[[ ... ]].  Those semicolons are meaningless here.

> +	require_configured_kernel;
> +	check_kernel_built;
> +
> +	local module
> +	local modules
> +
> +	pushd "${ED}" > /dev/null || die

Why change the directory when you can just pass "${ED}" to find?

> +	modules=$(find "lib/modules/${KV_FULL}" -name "*.${KV_OBJ}" 2>/dev/null)

Use the 'while read -d '' -r ... < <(find ... -print0)' loop to be
on the safe side.  Always.

> +	if [[ -n ${modules} ]]; then
> +		for module in ${modules}; do
> +			sign_module "${module}"
> +		done
> +	else
> +		ewarn 'QA: list of modules to sign is empty, pease report a bug'
> +	fi
> +	popd > /dev/null || die
> +}
> +
>  # internal function
>  #
>  # FUNCTION:
> @@ -583,12 +681,16 @@ linux-mod_pkg_setup() {
>  	# External modules use kernel symbols (bug #591832)
>  	CONFIG_CHECK+=" !TRIM_UNUSED_KSYMS"
>
> +	# if signature is requested, check if kernel actually supports it
> +	use module-sign && CONFIG_CHECK+=" MODULE_SIG"
> +
>  	linux-info_pkg_setup;
>  	require_configured_kernel
>  	check_kernel_built;
>  	strip_modulenames;
>  	[[ -n ${MODULE_NAMES} ]] && check_modules_supported
>  	set_kvobj;
> +	check_sig_force;

Meaningless semicolon.

>  	# Commented out with permission from johnm until a fixed version for arches
>  	# who intentionally use different kernel and userland compilers can be
>  	# introduced - Jason Wever <weeve@g.o>, 23 Oct 2005
> @@ -716,8 +818,8 @@ linux-mod_src_install() {
>
>  		einfo "Installing ${modulename} module"
>  		cd "${objdir}" || die "${objdir} does not exist"
> -		insinto /lib/modules/${KV_FULL}/${libdir}
> -		doins ${modulename}.${KV_OBJ} || die "doins ${modulename}.${KV_OBJ} failed"
> +		insinto /lib/modules/"${KV_FULL}/${libdir}"
> +		doins "${modulename}.${KV_OBJ}" || die "doins ${modulename}.${KV_OBJ} failed"
>  		cd "${OLDPWD}"
>
>  		generate_modulesd "${objdir}/${modulename}"
> @@ -733,6 +835,8 @@ linux-mod_pkg_preinst() {
>
>  	[ -d "${D}lib/modules" ] && UPDATE_DEPMOD=true || UPDATE_DEPMOD=false
>  	[ -d "${D}lib/modules" ] && UPDATE_MODULEDB=true || UPDATE_MODULEDB=false
> +	check_sig_force
> +	use module-sign && sign_all_modules
>  }
>
>  # @FUNCTION: linux-mod_pkg_postinst
>
>

-- 
Best regards,
Michał Górny