[Merge] ~ubuntu-core-dev/shim/+git/shim-signed:self-signed into ~ubuntu-core-dev/shim/+git/shim-signed:master
Adam Conrad
adconrad at 0c3.net
Wed Feb 14 17:59:54 UTC 2018
Review: Needs Fixing
Some initial thoughts inline in the code. I haven't reviewed the meat of the update-sb-policy code changes yet, as that's an eye-crossing endeavour that requires coffee, but I'm less concerned about finding shell bugs than I am about overall behaviour, design, and upgrade sanity.
Diff comments:
> diff --git a/debian/changelog b/debian/changelog
> index 36c1d71..9d54c98 100644
> --- a/debian/changelog
> +++ b/debian/changelog
> @@ -1,3 +1,21 @@
> +shim-signed (1.34) UNRELEASED; urgency=medium
> +
> + * update-secureboot-policy:
> + - Factor out validate_password() and clear_passwords() for reuse.
> + - Add --new-key option to generate a self-signed MOK.
> + - Add --enroll-key option to allow enrolling a new MOK in shim.
> + - Drop --enable and --disable options; users should call mokutil directly
> + instead.
You dropped the debconf templates, but the enable/disable toggle code is still very much there, which I assume will now just explode in a shiny ball of explodey badness if called. But also, see the comment attached to the Breaks in debian/control.
> + * debian/shim-signed.postinst:
> + - When triggered, explicitly try to enroll the available MOK.
> + * debian/shim-signed.install, openssl.cnf: Install some default configuration
> + for creating our self-signed key.
> + * debian/shim-signed.dirs: make sure we have a directory where to put a MOK.
> + * debian/templates: update templates for update-secureboot-policy changes.
> + * debian/control: add versioned Breaks: for dkms.
> +
> + -- Mathieu Trudel-Lapierre <cyphermox at ubuntu.com> Mon, 12 Feb 2018 10:28:38 -0500
> +
> shim-signed (1.33.1) bionic; urgency=medium
>
> * Update to the signed 13-0ubuntu2 binary from Microsoft. (LP: #1708245)
> diff --git a/debian/control b/debian/control
> index a4b9906..b2b4acc 100644
> --- a/debian/control
> +++ b/debian/control
> @@ -12,6 +12,7 @@ Architecture: amd64
> Depends: ${misc:Depends}, shim (= ${shim:Version}), grub-efi-amd64-bin, grub2-common (>= 2.02~beta2-36ubuntu12), mokutil
> Recommends: secureboot-db
> Built-Using: shim (= ${shim:Version})
> +Breaks: dkms (<< 2.3-3ubuntu5)
As discussed, dkms should break shim-signed, not the other way around, so shim-signed lands on disk before dkms attempts to use the new shiny. Further, the paths that the old dkms would be calling into either need to keep working until post-18.04, or be stubbed out harmlessly, so upgrading *just* shim-signed doesn't break halfway through a partial.
> Description: Secure Boot chain-loading bootloader (Microsoft-signed binary)
> This package provides a minimalist boot loader which allows verifying
> signatures of other UEFI binaries against either the Secure Boot DB/DBX or
> diff --git a/debian/shim-signed.install b/debian/shim-signed.install
> index 5082806..cb65f97 100644
> --- a/debian/shim-signed.install
> +++ b/debian/shim-signed.install
> @@ -1,3 +1,4 @@
> shimx64.efi.signed /usr/lib/shim
> +openssl.cnf /usr/lib/shim/mok
You're installing this to /usr/lib/shim (which feels right and empty-etcish), but then when you call openssl, you reference /etc instead. Maybe what you really want here is for the one in /usr/lib to have a header explaining to copy it to /etc if the user wants to customize, then do a test-and-fallback for /etc, then /usr.
> debian/source_shim-signed.py /usr/share/apport/package-hooks/
> update-secureboot-policy /usr/sbin/
> diff --git a/update-secureboot-policy b/update-secureboot-policy
> index 6f7b0d4..2fee635 100755
> --- a/update-secureboot-policy
> +++ b/update-secureboot-policy
> @@ -1,5 +1,6 @@
> #!/bin/sh
> set -e
> +#set -x
This appears to be leftover cruft from testing.
>
> if test $# = 0 \
> && test x"$SHIM_NOTRIGGER" = x \
> @@ -15,157 +16,288 @@ then
> fi
>
> if [ "$(id -u)" -ne 0 ]; then
> - echo "$0: Permission denied"
> - exit 1
> + echo "$0: Permission denied"
> + exit 1
> fi
Whitespace changes among functional changes make me a sad panda.
>
> +action=mok
> +enable_secureboot=0
> +do_enroll=0
> +do_toggle=0
> +
> +efivars=/sys/firmware/efi/efivars
> +secureboot_var=SecureBoot-8be4df61-93ca-11d2-aa0d-00e098032b8c
> +moksb_var=MokSB-605dab50-e046-4300-abb6-3dd810dd8b23
> +moksbstatert_var=MokSBStateRT-605dab50-e046-4300-abb6-3dd810dd8b23
> +
> +SB_KEY="/var/lib/shim-signed/mok/MOK.der"
> +SB_PRIV="/var/lib/shim-signed/mok/MOK.priv"
> +
> OLD_DKMS_LIST="/var/lib/shim-signed/dkms-list"
> NEW_DKMS_LIST="${OLD_DKMS_LIST}.new"
>
> touch $OLD_DKMS_LIST
>
> -args=$@
> -enable_secureboot=0
> dkms_list=$(find /var/lib/dkms -maxdepth 1 -type d -print 2>/dev/null \
> | LC_ALL=C sort)
> dkms_modules=$(echo "$dkms_list" | wc -l)
>
> . /usr/share/debconf/confmodule
>
> -setup_mok_validation()
> +update_dkms_list()
> {
> - local moksbstatert
> - local efivars secureboot_var moksb_var moksbstatert_var
> - local enable_sb action
> - enable_sb=$1
> - efivars=/sys/firmware/efi/efivars
> - secureboot_var=SecureBoot-8be4df61-93ca-11d2-aa0d-00e098032b8c
> - moksb_var=MokSB-605dab50-e046-4300-abb6-3dd810dd8b23
> - moksbstatert_var=MokSBStateRT-605dab50-e046-4300-abb6-3dd810dd8b23
> - action=disable
> -
> - if [ $enable_sb -eq 1 ]; then
> - action=enable
> - fi
> + echo "$dkms_list" > $NEW_DKMS_LIST
> +}
>
> - if ! [ -f $efivars/$secureboot_var ] \
> - || [ "$(od -An -t u1 $efivars/$secureboot_var | awk '{ print $NF }')" -ne 1 ]
> - then
> - echo "Secure Boot not enabled on this system." >&2
> - return 0
> - fi
> - moksbstatert=0
> - if [ -f $efivars/$moksb_var ]; then
> - # if MokSB exists we've likely already run mokutil since last boot
> - echo "The Secure Boot policy was already changed since last reboot; nothing to do." >&2
> - return 0
> - 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; }')
> +save_dkms_list()
> +{
> + mv "$NEW_DKMS_LIST" "$OLD_DKMS_LIST"
> +}
> +
> +finish() {
> + save_dkms_list
> +}
> +
> +validate_password()
> +{
> + db_capb
> + if [ "$key" != "$again" ]; then
> + db_fset shim/error/secureboot_key_mismatch seen false
> + db_input critical shim/error/secureboot_key_mismatch || true
> + STATE=$(($STATE - 2))
> + else
> + length=$((`echo "$key" | wc -c` - 1))
> + if [ $length -lt 8 ] || [ $length -gt 16 ]; then
> + db_fset shim/error/bad_secureboot_key seen false
> + db_input critical shim/error/bad_secureboot_key || true
> + STATE=$(($STATE - 2))
> + elif [ $length -ne 0 ]; then
> + return 0
> + fi
> fi
> - # poor man's xor
> - if [ $(($moksbstatert+$enable_sb)) -ne 1 ]; then
> - STATE=1
> - db_settitle shim/title/secureboot
> - while true; do
> - case "$STATE" in
> - 1)
> - db_capb
> - db_fset shim/secureboot_explanation seen false
> - db_input critical shim/secureboot_explanation || true
> - db_go
>
> - # Allow the user to skip disabling Secure Boot.
> + return 1
> +}
> +
> +clear_passwords()
> +{
> + # Always clear secureboot key.
> + db_set shim/secureboot_key ''
> + db_fset shim/secureboot_key seen false
> + db_set shim/secureboot_key_again ''
> + db_fset shim/secureboot_key_again seen false
> +}
> +
> +toggle_validation()
> +{
> + local key=$1
> + local again=$2
> +
> + echo "Changing shim validation state to '${action}d'."
> + printf '%s\n%s\n' "$key" "$again" | mokutil --${action}-validation >/dev/null || true
> +}
> +
> +enroll_mok()
> +{
> + local key=$1
> + local again=$2
> +
> + echo "Adding '$SB_KEY' to shim:"
> + printf '%s\n%s\n' "$key" "$again" | mokutil --import "$SB_KEY" >/dev/null || true
> +}
> +
> +do_it()
> +{
> + STATE=1
> + db_settitle shim/title/secureboot
> + while true; do
> + case "$STATE" in
> + 1)
> + db_capb
> + db_fset shim/secureboot_explanation seen false
> + db_input critical shim/secureboot_explanation || true
> + ;;
> + 2)
> + if [ "$do_toggle" -eq 1 ]; then
> + # Allow the user to skip toggling Secure Boot.
> db_fset shim/${action}_secureboot seen false
> db_input critical shim/${action}_secureboot || true
> - ;;
> - 2)
> + db_go
> +
> db_get shim/${action}_secureboot
> if [ "$RET" = "false" ]; then
> break
> fi
> + fi
> + ;;
> + 3)
>
> - db_input critical shim/secureboot_key || true
> - seen_key=$RET
> - db_input critical shim/secureboot_key_again || true
> - ;;
> - 3)
> - db_get shim/secureboot_key
> - key="$RET"
> - db_get shim/secureboot_key_again
> - again="$RET"
> -
> - if [ -z "$key$again" ] && echo "$seen_key" | grep -q ^30; then
> - echo "Running in non-interactive mode, doing nothing." >&2
> -
> - if ! diff -u $OLD_DKMS_LIST $NEW_DKMS_LIST; then
> - exit 1
> - else
> - exit 0
> - fi
> - fi
> + db_input critical shim/secureboot_key || true
> + seen_key=$RET
> + db_input critical shim/secureboot_key_again || true
> + ;;
> + 4)
> + db_get shim/secureboot_key
> + key="$RET"
> + db_get shim/secureboot_key_again
> + again="$RET"
> +
> + if [ -z "$key$again" ] && echo "$seen_key" | grep -q ^30; then
> + echo "Running in non-interactive mode, doing nothing." >&2
>
> - db_capb
> - if [ "$key" != "$again" ]; then
> - db_fset shim/error/secureboot_key_mismatch seen false
> - db_input critical shim/error/secureboot_key_mismatch || true
> - STATE=$(($STATE - 2))
> + if ! diff -u $OLD_DKMS_LIST $NEW_DKMS_LIST; then
> + exit 1
> else
> - length=$((`echo "$key" | wc -c` - 1))
> - if [ $length -lt 8 ] || [ $length -gt 16 ]; then
> - db_fset shim/error/bad_secureboot_key seen false
> - db_input critical shim/error/bad_secureboot_key || true
> - STATE=$(($STATE - 2))
> - elif [ $length -ne 0 ]; then
> - printf '%s\n%s\n' "$key" "$again" | mokutil --${action}-validation >/dev/null || true
> - fi
> + exit 0
> fi
> + fi
>
> - # Always clear secureboot key.
> - db_set shim/secureboot_key ''
> - db_fset shim/secureboot_key seen false
> - db_set shim/secureboot_key_again ''
> - db_fset shim/secureboot_key_again seen false
> - ;;
> - *)
> - break
> - ;;
> - esac
> -
> - if db_go; then
> - STATE=$(($STATE + 1))
> - else
> - STATE=$(($STATE - 1))
> + if validate_password; then
> + if [ $do_toggle -eq 1 ]; then
> + toggle_validation "$key" "$again"
> + fi
> + if [ $do_enroll -eq 1 ]; then
> + enroll_mok "$key" "$again"
> + fi
> fi
> - db_capb backup
> - done
> - db_capb
> +
> + clear_passwords
> + ;;
> + *)
> + break
> + ;;
> + esac
> +
> + if db_go; then
> + STATE=$(($STATE + 1))
> + else
> + STATE=$(($STATE - 1))
> + fi
> + db_capb backup
> + done
> + db_capb
> +}
> +
> +validate_actions() {
> + # Validate any queued actions before we go try to do them.
> + local moksbstatert=0
> +
> + 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
> +
> + # We were asked to enroll a key. This only makes sense if validation
> + # is enabled.
> + if [ $do_enroll -eq 1 ] && [ $moksbstatert -eq 1 ]; then
> + do_toggle=1
> + enable_secureboot=1
> + fi
> +
> + if [ $do_toggle -eq 1 ]; then
> +
> + # Make sure we really do need to do something.
> + if [ $moksbstatert -ne $enable_secureboot ]; then
> + echo "Shim validation is already in the requested state."
> + do_toggle=0
> + return
> + fi
> +
> + if [ $enable_secureboot -eq 0 ] && [ $dkms_modules -le 1 ]; then
> + echo "No DKMS packages installed: not changing Secure Boot validation state."
> + do_toggle=0
> + return
> + fi
> +
> + if [ -f $efivars/$moksb_var ]; then
> + # if MokSB exists we've likely already run mokutil since last boot
> + echo "The Secure Boot policy was already changed since last reboot." >&2
> + do_toggle=0
> + return
> + fi
> +
> + if [ $enable_secureboot -eq 1 ]; then
> + action=enable
> + else
> + action=disable
> + fi
> fi
> }
>
> -if echo "$args" | grep -qc -- '--enable'; then
> - enable_secureboot=1
> -elif echo "$args" | grep -qc -- '--disable'; then
> - enable_secureboot=0
> -elif echo "$args" | grep -qc -- '--help'; then
> - echo "update-secureboot-policy: toggle UEFI Secure Boot in shim"
> - echo
> - echo "\t--enable\tPrompt to enable Secure Boot validation."
> - echo "\t--disable\tPrompt to disable Secure Boot validation (default)."
> - echo "\t--help\t\tThis help text."
> - exit 0
> +create_mok()
> +{
> + if [ -e "$SB_KEY" ]; then
> + return
> + fi
> +
> + echo "Generating a new Secure Boot signing key:"
> + openssl req -config /etc/shim/mok/openssl.cnf \
> + -new -x509 -newkey rsa:2048 \
> + -nodes -days 36500 -outform DER \
> + -keyout "$SB_PRIV" \
> + -out "$SB_KEY"
> +}
> +
> +update_dkms_list
> +
> +trap finish EXIT
> +
> +if ! [ -f $efivars/$secureboot_var ] \
> + || [ "$(od -An -t u1 $efivars/$secureboot_var | awk '{ print $NF }')" -ne 1 ]
> +then
> + echo "Secure Boot not enabled on this system." >&2
> + exit 0
> fi
>
> -if [ $dkms_modules -gt 1 ] || [ $enable_secureboot -eq 1 ]; then
> - echo "$dkms_list" > $NEW_DKMS_LIST
> - setup_mok_validation $enable_secureboot
> - mv "$NEW_DKMS_LIST" "$OLD_DKMS_LIST"
> -else
> - echo "No DKMS packages installed: not changing Secure Boot validation state."
> +case "$1" in
> +'--enable')
> + enable_secureboot=1
> + do_toggle=1
> + ;;
> +
> +'--disable')
> + enable_secureboot=0
> + do_toggle=1
> + ;;
> +
> +'--new-key')
> + create_mok
> + exit 0
> + ;;
> +
> +'--enroll-key')
> + if [ -e "$SB_KEY" ]; then
> + if mokutil --test-key "$SB_KEY" | \
> + grep -qc 'is not'; then
> + do_enroll=1
> + fi
> + else
> + echo "No MOK found."
> + fi
> + ;;
> +
> +*)
> + echo "update-secureboot-policy: toggle UEFI Secure Boot in shim"
> + echo
> + echo "\t--new-key\tCreate a new MOK."
> + echo "\t--enroll-key\tEnroll the new MOK for this system in shim."
> + echo "\t--enable\tPrompt to enable Secure Boot validation."
> + echo "\t--disable\tPrompt to disable Secure Boot validation (default)."
> + echo "\t--help\t\tThis help text."
> + exit 0
> +
> +esac
> +
> +validate_actions
> +
> +if [ $(($do_toggle + $do_enroll)) -lt 1 ]; then
> + echo "Nothing to do."
> + exit 0
> fi
>
> +do_it
> +
> exit 0
--
https://code.launchpad.net/~ubuntu-core-dev/shim/+git/shim-signed/+merge/337571
Your team Ubuntu Core Development Team is subscribed to branch ~ubuntu-core-dev/shim/+git/shim-signed:master.
More information about the Ubuntu-reviews
mailing list