image_signing: clean up UEFI signing/verifying code

Follow up the code review comments on CL:995175, which was merged as
1493e938e45535f86b7132a83123c6319eacb217
("image_signing: sign UEFI binaries")

BUG=b:62189155
TEST=See CL:*613656
BRANCH=none

Change-Id: Ic01bfbbfe39fbfb85c0f313ab62bbcd3e2fbb9a3
Reviewed-on: https://chromium-review.googlesource.com/1024919
Commit-Ready: Edward Jee <edjee@google.com>
Tested-by: Edward Jee <edjee@google.com>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

5 files changed, 74 insertions, 51 deletions

diff --git a/scripts/image_signing/common_minimal.sh b/scripts/image_signing/common_minimal.sh
index d7ecc10e..3a0dccf9 100644
--- a/scripts/image_signing/common_minimal.sh
+++ b/scripts/image_signing/common_minimal.sh
@@ -254,9 +254,10 @@ mount_image_partition() {
 
 # Mount the image's ESP (EFI System Partition) on a newly created temporary
 # directory.
-# Prints out the newly created temporary directory path if succeeded, prints
-# out nothing if ESP doesn't exist, print out "MOUNT_FAILED" if mount failed.
-# Args: IMAGE ESP_PARTNUM
+# Prints out the newly created temporary directory path if succeeded.
+# If the image doens't have an ESP partition, returns 0 without print anything.
+# Args: IMAGE
+# Returns: 0 if succeeded, 1 otherwise.
 mount_image_esp() {
   local image="$1"
   local ESP_PARTNUM=12
@@ -264,19 +265,19 @@ mount_image_esp() {
   local esp_offset=$(( $(partoffset "${image}" "${ESP_PARTNUM}") ))
   # Check if the image has an ESP partition.
   if [[ "${esp_offset}" == "0" ]]; then
-    return
+    return 0
   fi
 
   local esp_dir="$(make_temp_dir)"
   # We use the 'unsafe' variant because the EFI system partition is vfat type
   # and can be mounted in RW mode.
-  if ! $(_mount_image_partition_retry "${image}" "${ESP_PARTNUM}" \
-                                      "${esp_dir}" > /dev/null); then
-    echo "MOUNT_FAILED"
-    return
+  if ! _mount_image_partition_retry "${image}" "${ESP_PARTNUM}" \
+                                    "${esp_dir}" >/dev/null; then
+    return 1
   fi
 
   echo "${esp_dir}"
+  return 0
 }
 
 # Extract a partition to a file
diff --git a/scripts/image_signing/install_gsetup_certs.sh b/scripts/image_signing/install_gsetup_certs.sh
index d515b790..e51843d8 100755
--- a/scripts/image_signing/install_gsetup_certs.sh
+++ b/scripts/image_signing/install_gsetup_certs.sh
@@ -1,5 +1,4 @@
 #!/bin/bash
-
 # Copyright 2018 The Chromium OS Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
@@ -12,7 +11,7 @@ usage() {
   cat <<EOF
 Usage: $PROG /path/to/esp/dir /path/to/keys/dir
 
-Sign UEFI binaries in ESP.
+Install UEFI certs in GSetup directory in ESP.
 EOF
   if [[ $# -gt 0 ]]; then
     error "$*"
@@ -21,11 +20,13 @@ EOF
   exit 0
 }
 
+# Installs the specified UEFI cert in GSetup directory, if the cert exists.
+# Args: KEY_TYPE CERT GSETUP_DIR
 install_gsetup_cert() {
   local key_type="$1"
   local cert="$2"
   local gsetup_dir="$3"
-  if [[ -f "$cert" ]]; then
+  if [[ -f "${cert}" ]]; then
     info "Putting ${key_type} cert: ${cert}"
     local cert_basename="$(basename "${cert}")"
     local der_filename="${cert_basename%.*}.der"
@@ -62,7 +63,8 @@ main() {
   local kek_cert="${key_dir}/kek/kek.pem"
   install_gsetup_cert kek "${kek_cert}" "${gsetup_dir}"
 
-  for dbx_cert in "${key_dir}/dbx/"*".pem"; do
+  local dbx_cert
+  for dbx_cert in "${key_dir}"/dbx/*.pem; do
     install_gsetup_cert dbx "${dbx_cert}" "${gsetup_dir}"
   done
 }
diff --git a/scripts/image_signing/sign_official_build.sh b/scripts/image_signing/sign_official_build.sh
index 90fbed84..d47ae908 100755
--- a/scripts/image_signing/sign_official_build.sh
+++ b/scripts/image_signing/sign_official_build.sh
@@ -738,6 +738,7 @@ resign_android_image_if_exists() {
 }
 
 # Sign UEFI binaries, if possible.
+# Args: IMAGE
 sign_uefi_binaries() {
   local image="$1"
 
@@ -745,12 +746,12 @@ sign_uefi_binaries() {
     return 0
   fi
 
-  local esp_dir="$(mount_image_esp "${image}")"
-  if [[ -z "${esp_dir}" ]]; then
-    return 0
-  elif [[ "${esp_dir}" == "MOUNT_FAILED" ]]; then
+  local esp_dir
+  if ! esp_dir="$(mount_image_esp "${image}")"; then
     error "Could not mount EFI partition for signing UEFI binaries"
     return 1
+  elif [[ -z "${esp_dir}" ]]; then
+    return 0
   fi
   "${SCRIPT_DIR}/install_gsetup_certs.sh" "${esp_dir}" "${KEY_DIR}/uefi"
   "${SCRIPT_DIR}/sign_uefi.sh" "${esp_dir}" "${KEY_DIR}/uefi"
@@ -765,23 +766,30 @@ sign_uefi_binaries() {
   return 0
 }
 
+# Verify the signatures of UEFI binaries.
+# Args: IMAGE
 verify_uefi_signatures() {
   local image="$1"
   local succeeded=1
 
-  local esp_dir="$(mount_image_esp "${image}")"
-  if [[ -z "${esp_dir}" ]]; then
+  if [[ ! -d "${KEY_DIR}/uefi" ]]; then
     return 0
-  elif [[ "${esp_dir}" == "MOUNT_FAILED" ]]; then
+  fi
+
+  local esp_dir
+  if ! esp_dir="$(mount_image_esp "${image}")"; then
     error "Could not mount EFI partition for verifying UEFI signatures"
     return 1
+  elif [[ -z "${esp_dir}" ]]; then
+    return 0
   fi
-  "${SCRIPT_DIR}/verify_uefi.sh" "${esp_dir}" "${esp_dir}" || succeeded=0
+  "${SCRIPT_DIR}/verify_uefi.sh" "${esp_dir}" "${esp_dir}" \
+      "${KEY_DIR}/uefi" || succeeded=0
 
   local rootfs_dir="$(make_temp_dir)"
   mount_image_partition_ro "${image}" 3 "${rootfs_dir}"
-  "${SCRIPT_DIR}/verify_uefi.sh" "${rootfs_dir}/boot" "${esp_dir}" || \
-      succeeded=0
+  "${SCRIPT_DIR}/verify_uefi.sh" "${rootfs_dir}/boot" "${esp_dir}" \
+      "${KEY_DIR}/uefi" || succeeded=0
   sudo umount "${rootfs_dir}"
 
   sudo umount "${esp_dir}"
@@ -910,13 +918,13 @@ update_legacy_bootloader() {
   local image="$1"
   local loop_kern="$2"
 
-  local esp_dir="$(mount_image_esp "${image}")"
-  if [[ -z "${esp_dir}" ]]; then
-    info "Not updating legacy bootloader configs: ${image}"
-    return 0
-  elif [[ "${esp_dir}" == "MOUNT_FAILED" ]]; then
+  local esp_dir
+  if ! esp_dir="$(mount_image_esp "${image}")"; then
     error "Could not mount EFI partition for updating legacy bootloader cfg."
     return 1
+  elif [[ -z "${esp_dir}" ]]; then
+    info "Not updating legacy bootloader configs: ${image}"
+    return 0
   fi
 
   # If we can't find the dm parameter in the kernel config, bail out now.
diff --git a/scripts/image_signing/sign_uefi.sh b/scripts/image_signing/sign_uefi.sh
index 4cef5a50..6deb2804 100755
--- a/scripts/image_signing/sign_uefi.sh
+++ b/scripts/image_signing/sign_uefi.sh
@@ -1,5 +1,4 @@
 #!/bin/bash
-
 # Copyright 2018 The Chromium OS Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
@@ -10,9 +9,10 @@ set -e
 
 usage() {
   cat <<EOF
-Usage: $PROG /path/to/target/dir /path/to/keys/dir
+Usage: $PROG /path/to/target/dir /path/to/uefi/keys/dir
 
-Sign UEFI binaries in the target directory.
+Sign the UEFI binaries in the target directory.
+The target directory can be either the root of ESP or /boot of root filesystem.
 EOF
   if [[ $# -gt 0 ]]; then
     error "$*"
@@ -21,6 +21,8 @@ EOF
   exit 0
 }
 
+# Signs an EFI binary file, if possible.
+# Args: TARGET_FILE TEMP_DIR PRIVATE_KEY SIGN_CERT VERIFY_CERT
 sign_efi_file() {
   local target="$1"
   local temp_dir="$2"
@@ -51,13 +53,13 @@ main() {
   fi
 
   if ! type -P sbattach &>/dev/null; then
-    die "Skip signing UEFI binaries (sbattach not found)."
+    die "Cannot sign UEFI binaries (sbattach not found)."
   fi
   if ! type -P sbsign &>/dev/null; then
-    die "Skip signing UEFI binaries (sbsign not found)."
+    die "Cannot sign UEFI binaries (sbsign not found)."
   fi
   if ! type -P sbverify &>/dev/null; then
-    die "Skip signing UEFI binaries (sbverify not found)."
+    die "Cannot sign UEFI binaries (sbverify not found)."
   fi
 
   local bootloader_dir="${target_dir}/efi/boot"
@@ -65,7 +67,7 @@ main() {
   local kernel_dir="${target_dir}"
 
   local verify_cert="${key_dir}/db/db.pem"
-  if [[ ! -f "$verify_cert" ]]; then
+  if [[ ! -f "${verify_cert}" ]]; then
     die "No verification cert: ${verify_cert}"
   fi
 
@@ -81,7 +83,8 @@ main() {
 
   local working_dir="$(make_temp_dir)"
 
-  for efi_file in "${bootloader_dir}/"*".efi"; do
+  local efi_file
+  for efi_file in "${bootloader_dir}"/*.efi; do
     if [[ ! -f "${efi_file}" ]]; then
       continue
     fi
@@ -89,7 +92,8 @@ main() {
         "${sign_key}" "${sign_cert}" "${verify_cert}"
   done
 
-  for syslinux_kernel_file in "${syslinux_dir}/vmlinuz."?; do
+  local syslinux_kernel_file
+  for syslinux_kernel_file in "${syslinux_dir}"/vmlinuz.?; do
     if [[ ! -f "${syslinux_kernel_file}" ]]; then
       continue
     fi
diff --git a/scripts/image_signing/verify_uefi.sh b/scripts/image_signing/verify_uefi.sh
index 959b5b8f..0d305117 100755
--- a/scripts/image_signing/verify_uefi.sh
+++ b/scripts/image_signing/verify_uefi.sh
@@ -1,5 +1,4 @@
 #!/bin/bash
-
 # Copyright 2018 The Chromium OS Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
@@ -10,7 +9,7 @@ set -e
 
 usage() {
   cat <<EOF
-Usage: $PROG /path/to/target/dir /path/to/esp/dir
+Usage: $PROG /path/to/target/dir /path/to/esp/dir /path/to/uefi/keys/dir
 
 Verify signatures of UEFI binaries in the target directory.
 EOF
@@ -24,9 +23,10 @@ EOF
 main() {
   local target_dir="$1"
   local esp_dir="$2"
+  local key_dir="$3"
 
-  if [[ $# -ne 2 ]]; then
-    usage "command takes exactly 1 args"
+  if [[ $# -ne 3 ]]; then
+    usage "command takes exactly 3 args"
   fi
 
   if ! type -P sbverify &>/dev/null; then
@@ -39,40 +39,48 @@ main() {
   local gsetup_dir="${esp_dir}/EFI/Google/GSetup"
 
   if [[ ! -f "${gsetup_dir}/pk/pk.der" ]]; then
-    warn "No PK cert"
-    exit 0
+    die "No PK cert"
   fi
 
   local db_cert_der="${gsetup_dir}/db/db.der"
   if [[ ! -f "${db_cert_der}" ]]; then
-    warn "No DB cert"
-    exit 0
+    die "No DB cert"
   fi
 
+  local cert="${key_dir}/db/db.pem"
+
   local working_dir="$(make_temp_dir)"
-  local cert="${working_dir}/cert.pem"
-  openssl x509 -in "${db_cert_der}" -inform DER -out "${cert}" -outform PEM
+  local gsetup_cert="${working_dir}/cert.pem"
+  openssl x509 -in "${db_cert_der}" -inform DER \
+      -out "${gsetup_cert}" -outform PEM
 
-  for efi_file in "${bootloader_dir}/"*".efi"; do
+  for efi_file in "${bootloader_dir}"/*.efi; do
     if [[ ! -f "${efi_file}" ]]; then
       continue
     fi
     sbverify --cert "${cert}" "${efi_file}" ||
-        die "Verification failed: ${efi_file}"
+        die "Verification failed. file:${efi_file} cert:${cert}"
+    sbverify --cert "${gsetup_cert}" "${efi_file}" ||
+        die "Verification failed. file:${efi_file} cert:${gsetup_cert}"
   done
 
-  for syslinux_kernel_file in "${syslinux_dir}/vmlinuz."?; do
+  for syslinux_kernel_file in "${syslinux_dir}"/vmlinuz.?; do
     if [[ ! -f "${syslinux_kernel_file}" ]]; then
       continue
     fi
     sbverify --cert "${cert}" "${syslinux_kernel_file}" ||
-        warn "Verification failed: ${syslinux_kernel_file}"
+        warn "Verification failed. file:${syslinux_kernel_file} cert:${cert}"
+    sbverify --cert "${gsetup_cert}" "${syslinux_kernel_file}" ||
+        warn "Verification failed. file:${syslinux_kernel_file}" \
+            "cert:${gsetup_cert}"
   done
 
   local kernel_file="$(readlink -f "${kernel_dir}/vmlinuz")"
   if [[ -f "${kernel_file}" ]]; then
     sbverify --cert "${cert}" "${kernel_file}" ||
-        warn "Verification failed: ${kernel_file}"
+        warn "Verification failed: file:${kernel_file} cert:${cert}"
+    sbverify --cert "${gsetup_cert}" "${kernel_file}" ||
+        warn "Verification failed: file:${kernel_file} cert:${gsetup_cert}"
   fi
 }