[Merge] ~juliank/grub/+git/ubuntu:juliank/check-signed-kernels into ~ubuntu-core-dev/grub/+git/ubuntu:ubuntu

Steve Langasek steve.langasek at canonical.com
Thu Jul 12 21:07:59 UTC 2018


A few final comments inline, nothing that should block landing this.  Please resolve these comments as you see fit and land this change.

Diff comments:

> diff --git a/debian/grub-check-signatures b/debian/grub-check-signatures
> new file mode 100755
> index 0000000..633ad2d
> --- /dev/null
> +++ b/debian/grub-check-signatures
> @@ -0,0 +1,90 @@
> +#!/bin/sh
> +
> +set -e
> +
> +. /usr/share/debconf/confmodule
> +
> +# Check if we are on an EFI system
> +efivars=/sys/firmware/efi/efivars
> +secureboot_var=SecureBoot-8be4df61-93ca-11d2-aa0d-00e098032b8c
> +moksbstatert_var=MokSBStateRT-605dab50-e046-4300-abb6-3dd810dd8b23
> +
> +on_secure_boot() {
> +	# Validate any queued actions before we go try to do them.
> +	local moksbstatert=0
> +
> +	if ! [ -d $efivars ]; then
> +		return 1
> +	fi
> +
> +	if ! [ -f $efivars/$secureboot_var ] \
> +		|| [ "$(od -An -t u1 $efivars/$secureboot_var | awk '{ print $NF }')" -ne 1 ]
> +	then
> +		return 1
> +	fi
> +
> +	if [ -f /proc/sys/kernel/moksbstate_disabled ]; then
> +		moksbstatert=$(cat /proc/sys/kernel/moksbstate_disabled 2>/dev/null || echo 0)
> +	elif [ -f $efivars/$moksbstatert_var ]; then
> +		# MokSBStateRT set to 1 means validation is disabled
> +		moksbstatert=$(od -An -t u1 $efivars/$moksbstatert_var | \
> +					   awk '{ print $NF; }')
> +	fi
> +
> +	if [ $moksbstatert -eq 1 ]; then
> +		return 1
> +	fi
> +
> +	return 0
> +}
> +
> +# Check if a given kernel image is sized

s/sized/signed/

> +is_signed() {
> +	tmp=$(mktemp)
> +	sbattach --detach $tmp $1 >/dev/null  	# that's ugly...
> +	test "$(wc -c < $tmp)" -ge 16	# Just _some_ minimum size
> +	result=$?
> +	rm $tmp
> +	return $result
> +}
> +
> +# Check that our current kernel and every newer one is signed
> +find_unsigned() {
> +	uname_r="$(uname -r)"
> +	for kernel in $(ls -1 /boot/vmlinuz-* | sort -V -r); do
> +		this_uname_r="$(echo "$kernel" | sed -nr 's#^/boot/vmlinuz-(.*)#\1#p' | sed 's#\.efi\.signed$##')"

micro-optimization, I think?
		this_uname_r="$(echo "$kernel" | sed -r 's#^/boot/vmlinuz-(.*)#\1#; s#\.efi\.signed$##')"

since there should be no cases where $kernel doesn't match, therefore the -n + s///p is superfluous.

Have you checked that this code DTRT in the case that /boot/vmlinuz-* matches nothing?

> +		if dpkg --compare-versions "$this_uname_r" lt "$uname_r"; then
> +			continue
> +		fi
> +		if ! is_signed $kernel; then
> +			echo "$this_uname_r"
> +		fi
> +	done
> +}
> +
> +# Only reached from show_warning
> +error() {
> +	echo "E: Your kernels are unsigned. This system will fail to boot in a secure boot environment." >&2
> +	exit 1
> +}
> +
> +# Either shows a debconf note or prints an error with error() above if
> +# that fails
> +show_warning() {
> +	# kernels should be an indented list of one version per line
> +	escaped="$(printf "%s" "$unsigned" | sed "s#^#     #" | debconf-escape -e )"
> +	db_capb escape
> +	db_settitle grub2/unsigned_kernels_title || error
> +	db_fset grub2/unsigned_kernels seen 0 || error
> +	db_subst grub2/unsigned_kernels unsigned_versions "$escaped" || error
> +	db_input critical grub2/unsigned_kernels || error
> +	db_go || error
> +	error
> +}
> +
> +if on_secure_boot; then
> +	unsigned="$(find_unsigned)"
> +	if [ "$unsigned" ]; then

style nit, I'd prefer if [ -n "$unsigned" ] there

> +		show_warning "$unsigned"
> +	fi
> +fi


-- 
https://code.launchpad.net/~juliank/grub/+git/ubuntu/+merge/345403
Your team Ubuntu Core Development Team is subscribed to branch ~ubuntu-core-dev/grub/+git/ubuntu:ubuntu.



More information about the Ubuntu-reviews mailing list