[Merge] ~ubuntu-core-dev/shim/+git/shim-signed:self-signed into ~ubuntu-core-dev/shim/+git/shim-signed:master
Steve Langasek
steve.langasek at canonical.com
Wed Mar 28 21:22:17 UTC 2018
Review: Needs Fixing
Thanks, comments inline. This includes some tweaks to the existing debconf template language - sorry about that - but I think we should get it cleaned up all at the same time while landing this.
Review summary: there are several serious bugs in the current version. Please resubmit for review when fixed.
Diff comments:
> diff --git a/debian/shim-signed.postinst b/debian/shim-signed.postinst
> index 9e082e0..c578445 100644
> --- a/debian/shim-signed.postinst
> +++ b/debian/shim-signed.postinst
> @@ -43,8 +43,6 @@ case $1 in
> find /var/lib/dkms -maxdepth 1 -type d -print \
> | LC_ALL=C sort > /var/lib/shim-signed/dkms-list
> fi
> -
> - SHIM_NOTRIGGER=y update-secureboot-policy
Please remind me why we are no longer calling update-secureboot-policy on configure. From what I can see, if you currently have shim-signed installed, and it is configured to disable secureboot instead of to enroll a MOK, on upgrade you will not be automatically transitioned to MOK because nothing has triggered the package.
*If* a dkms module is upgraded at the same time, that dkms module will trigger update-secureboot-policy; but that is insufficient. Installing shim-signed alone must trigger the policy transition even if there are no dkms module or kernel updates.
> ;;
> esac
>
> diff --git a/debian/templates b/debian/templates
> index 604cda1..ad7a322 100644
> --- a/debian/templates
> +++ b/debian/templates
> @@ -27,20 +19,24 @@ _Description: Enable UEFI Secure Boot?
> Template: shim/secureboot_explanation
> Type: note
> _Description: Your system has UEFI Secure Boot enabled.
> - UEFI Secure Boot is not compatible with the use of third-party drivers.
> + UEFI Secure Boot requires additional configuration to work with third-party
> + drivers.
> .
> - The system will assist you in toggling UEFI Secure Boot. To ensure that this
> - change is being made by you as an authorized user, and not by an attacker,
> - you must choose a password now and then use the same password after reboot
> - to confirm the change.
> + The system will assist you in configuring UEFI Secure Boot. To allow
> + third-party drivers required for your system to work correctly, a new
To permit use of the third-party drivers that are currently installed, a new [...]
> + Machine-Owner Key (MOK) has been created for your system and used to sign
and has been used to sign
> + these drivers. This key now needs to be enrolled in your system's firmware.
> .
> - If you choose to proceed but do not confirm the password upon reboot, Ubuntu
> - will still be able to boot on your system but the Secure Boot state will not
> - be changed.
> + If your system previously had Secure Boot validation disabled, as they new
> + key is enrolled, validation will also be re-enabled.
If Secure Boot validation was previously disabled on your system, validation will also be re-enabled as part of this key enrollment process.
(But also: perhaps this information is superfluous and could be omitted; or is this here to guard against the case where secureboot is disabled in MOK, but for reasons unrelated to dkms?)
> .
> - If Secure Boot remains enabled on your system, your system may still boot but
> - any hardware that requires third-party drivers to work correctly may not be
> - usable.
> + To ensure that this change is being made by you as an authorized user, and
> + not by an attacker, you must choose a password now and then use the same
> + password after reboot to confirm the change.
... and then confirm the change after reboot using the same password.
> + .
> + If you proceed but do not confirm the password upon reboot, Ubuntu
> + will still be able to boot on your system but any hardware that requires
> + third-party drivers to work correctly may not be usable.
>
> Template: shim/secureboot_key
> Type: string
> diff --git a/openssl.cnf b/openssl.cnf
> new file mode 100644
> index 0000000..5a4f734
> --- /dev/null
> +++ b/openssl.cnf
> @@ -0,0 +1,27 @@
> +HOME = /var/lib/shim-signed/mok
> +RANDFILE = /var/lib/shim-signed/mok/.rnd
> +
> +[ req ]
> +distinguished_name = req_distinguished_name
> +x509_extensions = v3_ca
> +string_mask = utf8only
> +
> +[ req_distinguished_name ]
Don't recall if we discussed this, should this section be empty? AFAICS we don't specify DN contents in either the openssl.cnf or in the args when invoking openssl, so I guess that leaves us an unnamed key - is that what we wanted?
> +
> +[ v3_ca ]
> +subjectKeyIdentifier = hash
> +authorityKeyIdentifier = keyid:always,issuer
> +basicConstraints = critical,CA:FALSE
> +
> +# We use extended key usage information to limit what this auto-generated
> +# key can be used for.
> +#
> +# codeSigning: specifies that this key is used to sign code.
> +#
> +# 1.3.6.1.4.1.2312.16.1.2: defines this key as used for module signing
> +# only. See https://lkml.org/lkml/2015/8/26/741.
> +#
> +extendedKeyUsage = codeSigning,1.3.6.1.4.1.2312.16.1.2
> +
> +nsComment = "OpenSSL Generated Certificate"
> +
> diff --git a/update-secureboot-policy b/update-secureboot-policy
> index 6f7b0d4..66447da 100755
> --- a/update-secureboot-policy
> +++ b/update-secureboot-policy
> @@ -1,5 +1,6 @@
> #!/bin/sh
> set -e
> +#set -x
omit
>
> if test $# = 0 \
> && test x"$SHIM_NOTRIGGER" = x \
> @@ -19,153 +20,268 @@ if [ "$(id -u)" -ne 0 ]; then
> exit 1
> fi
>
> +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
> +
> + 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 "Enabling shim validation."
> + printf '%s\n%s\n' "$key" "$again" | mokutil --enable-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/enable_secureboot seen false
> + db_input critical shim/enable_secureboot || true
> db_go
>
> - # Allow the user to skip disabling Secure Boot.
> - db_fset shim/${action}_secureboot seen false
> - db_input critical shim/${action}_secureboot || true
> - ;;
> - 2)
> - db_get shim/${action}_secureboot
> + db_get shim/enable_secureboot
> if [ "$RET" = "false" ]; then
> break
This means that we are now showing the user this question:
Enable UEFI Secure Boot?
If Secure Boot is enabled on your system, your system may still boot but
any hardware that requires third-party drivers to work correctly may not be
usable.
and if the user answers "no", we are breaking out of the entire loop, which means that we do not prompt for the password and we do not enroll the key and we do not upgrade the user's secureboot policy.
The text we're showing the user is no longer accurate. The effect of a 'no' answer to this question is not what is intended. I think this question should just be dropped.
> 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
> + fi
> +
> + if [ $do_toggle -eq 1 ]; then
> +
> + # Make sure we really do need to do something.
> + if [ $moksbstatert -ne 1 ]; then
> + echo "Shim validation is already in the requested state."
> + do_toggle=0
> + return
> + fi
Dead code. The only place we set do_toggle=1 is immediately above this, only on the condition that $moksbstatert -eq 1. So drop the above redundant check.
> +
> + if [ $dkms_modules -le 1 ]; then
> + echo "No DKMS packages installed: not changing Secure Boot validation state."
> + do_toggle=0
> + return
> + fi
This has the effect that if there are no dkms modules installed, and update-secureboot-policy --enroll-key is called (presumably manually?), we /will/ prompt to enroll the key, but we will /not/ re-enable SecureBoot.
Pretty sure that's not the intended behavior.
> +
> + 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
> + fi
> +}
> +
> +create_mok()
> +{
> + if [ -e "$SB_KEY" ]; then
> + return
> fi
> +
> + echo "Generating a new Secure Boot signing key:"
> + openssl req -config /usr/lib/shim/mok/openssl.cnf \
> + -new -x509 -newkey rsa:2048 \
> + -nodes -days 36500 -outform DER \
> + -keyout "$SB_PRIV" \
> + -out "$SB_KEY"
> }
>
> -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
> +update_dkms_list
> +
> +trap finish EXIT
This is wrong (and even if it wasn't wrong, it took me way too much mental effort to prove this to myself, which would point to a design problem). trap EXIT is called on every exit from the script, including non-zero. finish calls save_dkms_list, which stores the current list of installed dkms modules as 'seen'; whether dkms modules are 'seen' or not determines whether a failure to prompt is or is not treated as an error. So if you noninteractively install a dkms module, the flow you get is:
- shim-signed is triggered
- cannot prompt for a password, so update-secureboot-policy exits 1
- /var/lib/shim-signed/dkms-list is updated
- shim-signed trigger is marked failed, so will be retried later
- shim-signed is triggered the next time dpkg is called
- cannot prompt for a password, but there are no "new" dkms modules, so update-secureboot-policy exits *0*
- shim-signed trigger is marked as successful and will therefore never be invoked again
What should happen: the trigger should remain in failed state forever until we get a password.
> +
> +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'|'--disable')
> + echo "Please run mokutil directly to change shim validation behavior."
> + exit 0
> + ;;
> +
> +'--new-key')
> + create_mok
create_mok is the only place we generate the signing key. --new-key is the only place create_mok is called. Nothing in this patch invokes update-secureboot-policy --new-key. So we never actually have our MOK generated?
> + 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."
Surely this should exit non-zero.
> + 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--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