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

[Steve Langasek]

Review: Needs Fixing



Diff comments:

> diff --git a/debian/grub-check-signatures b/debian/grub-check-signatures
> new file mode 100755
> index 0000000..6443c8c
> --- /dev/null
> +++ b/debian/grub-check-signatures
> @@ -0,0 +1,55 @@
> +#!/bin/sh
> +
> +set -e
> +
> +. /usr/share/debconf/confmodule
> +
> +# Check if we are on an EFI system
> +are_efi() {

system_is_efi(), please.

However, we should be checking not only that the system is efi, but that secureboot is enabled.  At minimum, that should mean checking the output of mokutil --sb-state, or if that's not an appropriate dependency, checking the content of /sys/firmware/efi/efivars/SecureBoot-8be4df61-93ca-11d2-aa0d-00e098032b8c directly.

> +	test -d /sys/firmware/efi/
> +	return $?
> +}
> +
> +# Check if a given kernel image is sized
> +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
> +have_signed() {
> +	uname_r="$(uname -r)"
> +	for kernel in $(grep '^\s*linux' /boot/grub/grub.cfg  | awk '{print $2}' | uniq); do
> +		this_uname_r="$(echo "$kernel" | sed -nr 's#^/vmlinuz-(.*)#\1#p' | sed 's#\.efi\.signed$##')"
> +		if test  -e /boot/$kernel && ! is_signed /boot/$kernel; then
> +			return 1
> +		fi
> +		if [ "$uname_r" = "$this_uname_r" ]; then
> +			return 0
> +		fi
> +	done
> +	return 0
> +}
> +
> +# 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() {
> +	db_title "Unsigned kernel" || error
> +	db_fset grub2/unsigned_kernels seen 0 || error
> +	db_input critical grub2/unsigned_kernels || error
> +	db_go || error
> +}
> +
> +if are_efi && ! have_signed; then
> +	show_warning
> +fi
> diff --git a/debian/update-grub b/debian/update-grub
> index 0c43327..5b57e5e 100644
> --- a/debian/update-grub
> +++ b/debian/update-grub
> @@ -1,3 +1,4 @@
>  #!/bin/sh
>  set -e
> -exec grub-mkconfig -o /boot/grub/grub.cfg "$@"
> +grub-mkconfig -o /boot/grub/grub.cfg "$@" 3>&-
> +/usr/share/grub/grub-check-signatures

Is it really necessary to make this part of update-grub, and have it run each time grub or a kernel is upgraded?  I would expect us to only run this check at the point in time that we are switching the grub policy to enforce kernel signatures, because this is a behavior change in grub that carries significant risk.  I don't think it's the responsibility of the grub package to check on every run that a new kernel the user has installed is secureboot-compatible.

Also, I expect us to *block* the upgrade of the grub package based on the result of this check, which this does not do.  (We should avoid calling grub-install --target=x86_64-efi if doing so will render the system unbootable; and the only reasonable way to bubble this up is as a package installation failure.)



-- 
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.