Changes to allow user-signed kernels to be generated.

Make vbutil_keyblock handle unsigned blocks. Also enable --unpack option and
add tests for it.

Modify vbutil_kernel to allow unsigned keyblocks, correct usage message,
and fix the --debug option which was somehow disabled.

Update load_kernel_test to accept /dev/null for the public key, to test
non-signed kernel keyblocks.

Review URL: http://codereview.chromium.org/3124004

10 files changed, 284 insertions, 85 deletions

diff --git a/firmware/lib/vboot_common.c b/firmware/lib/vboot_common.c
index a2a5d9f5..2635fe64 100644
--- a/firmware/lib/vboot_common.c
+++ b/firmware/lib/vboot_common.c
@@ -181,7 +181,11 @@ int KeyBlockVerify(const VbKeyBlockHeader* block, uint64_t size,
     return VBOOT_KEY_BLOCK_INVALID;
   }
 
-  /* Check signature or hash, depending on whether we have a key. */
+  /* Check signature or hash, depending on whether we provide a key. Note that
+   * we don't require a key even if the keyblock has a signature, because the
+   * caller may not care if the keyblock itself is signed (for example, booting
+   * a Google-signed kernel in developer mode).
+   */
   if (key) {
     /* Check signature */
     RSAPublicKey* rsa;
@@ -205,10 +209,13 @@ int KeyBlockVerify(const VbKeyBlockHeader* block, uint64_t size,
       VBDEBUG(("Signature calculated past end of the block\n"));
       return VBOOT_KEY_BLOCK_INVALID;
     }
+    VBDEBUG(("Checking key block signature...\n"));
     rv = VerifyData((const uint8_t*)block, size, sig, rsa);
     RSAPublicKeyFree(rsa);
-    if (rv)
+    if (rv) {
+      VBDEBUG(("Invalid key block signature.\n"));
       return VBOOT_KEY_BLOCK_SIGNATURE;
+    }
   } else {
     /* Check hash */
     uint8_t* header_checksum = NULL;
@@ -225,6 +232,7 @@ int KeyBlockVerify(const VbKeyBlockHeader* block, uint64_t size,
       return VBOOT_KEY_BLOCK_INVALID;
     }
 
+    VBDEBUG(("Checking key block hash only...\n"));
     header_checksum = DigestBuf((const uint8_t*)block, sig->data_size,
                                 SHA512_DIGEST_ALGORITHM);
     rv = SafeMemcmp(header_checksum, GetSignatureDataC(sig),
diff --git a/firmware/version.c b/firmware/version.c
index 96f6fb6a..5cf92d5b 100644
--- a/firmware/version.c
+++ b/firmware/version.c
@@ -1 +1 @@
-char* VbootVersion = "VBOOv=0b67eb7f";
+char* VbootVersion = "VBOOv=1361f907";
diff --git a/host/lib/host_keyblock.c b/host/lib/host_keyblock.c
index f86f35bd..1c1fa127 100644
--- a/host/lib/host_keyblock.c
+++ b/host/lib/host_keyblock.c
@@ -22,7 +22,7 @@ VbKeyBlockHeader* KeyBlockCreate(const VbPublicKey* data_key,
   VbKeyBlockHeader* h;
   uint64_t signed_size = sizeof(VbKeyBlockHeader) + data_key->key_size;
   uint64_t block_size = (signed_size + SHA512_DIGEST_SIZE +
-                         siglen_map[signing_key->algorithm]);
+                         (signing_key ? siglen_map[signing_key->algorithm] : 0));
   uint8_t* data_key_dest;
   uint8_t* block_sig_dest;
   uint8_t* block_chk_dest;
@@ -49,8 +49,11 @@ VbKeyBlockHeader* KeyBlockCreate(const VbPublicKey* data_key,
   /* Set up signature structs so we can calculate the signatures */
   SignatureInit(&h->key_block_checksum, block_chk_dest,
                 SHA512_DIGEST_SIZE, signed_size);
-  SignatureInit(&h->key_block_signature, block_sig_dest,
-                siglen_map[signing_key->algorithm], signed_size);
+  if (signing_key)
+    SignatureInit(&h->key_block_signature, block_sig_dest,
+                  siglen_map[signing_key->algorithm], signed_size);
+  else
+    Memset(&h->key_block_signature, 0, sizeof(VbSignature));
 
   /* Calculate checksum */
   sigtmp = CalculateChecksum((uint8_t*)h, signed_size);
@@ -58,9 +61,11 @@ VbKeyBlockHeader* KeyBlockCreate(const VbPublicKey* data_key,
   Free(sigtmp);
 
   /* Calculate signature */
-  sigtmp = CalculateSignature((uint8_t*)h, signed_size, signing_key);
-  SignatureCopy(&h->key_block_signature, sigtmp);
-  Free(sigtmp);
+  if (signing_key) {
+    sigtmp = CalculateSignature((uint8_t*)h, signed_size, signing_key);
+    SignatureCopy(&h->key_block_signature, sigtmp);
+    Free(sigtmp);
+  }
 
   /* Return the header */
   return h;
diff --git a/tests/devkeys/create_new_keys.sh b/tests/devkeys/create_new_keys.sh
index 904c1ceb..29611900 100755
--- a/tests/devkeys/create_new_keys.sh
+++ b/tests/devkeys/create_new_keys.sh
@@ -24,6 +24,11 @@ function alg_to_keylen {
 }
 
 # Emit .vbpubk and .vbprivk using given basename and algorithm
+# NOTE: This function also appears in ../../utility/dev_make_keypair. Making
+# the two implementations the same would require some common.sh, which is more
+# likely to cause problems than just keeping an eye out for any differences. If
+# you feel the need to change this file, check the history of that other file
+# to see what may need updating here too.
 function make_pair {
   local base=$1
   local alg=$2
@@ -101,10 +106,10 @@ make_pair recovery_kernel_data_key 11
 # since it's never even checked during Recovery mode.
 make_keyblock firmware 7 firmware_data_key root_key
 
-# Create the recovery kernel keyblock for use only in Recovery mode. 
+# Create the recovery kernel keyblock for use only in Recovery mode.
 make_keyblock recovery_kernel 11 recovery_kernel_data_key recovery_key
 
-# Create the normal kernel keyblock for use only in Normal mode. 
+# Create the normal kernel keyblock for use only in Normal mode.
 make_keyblock kernel 7 kernel_data_key kernel_subkey
 
 # Create the installer keyblock for use in Developer + Recovery mode
diff --git a/tests/run_vbutil_tests.sh b/tests/run_vbutil_tests.sh
index abbd17b7..b8811305 100755
--- a/tests/run_vbutil_tests.sh
+++ b/tests/run_vbutil_tests.sh
@@ -68,13 +68,25 @@ ${datahashalgo}${COL_STOP}"
           keyblockfile+="${data_algorithmcounter}.keyblock"
           rm -f ${keyblockfile}
 
-          # Wrap
+          # Wrap private key
           ${UTIL_DIR}/vbutil_key \
             --pack ${TESTKEY_SCRATCH_DIR}/key_alg${algorithmcounter}.vbprivk \
             --key ${TESTKEY_DIR}/key_rsa${signing_keylen}.pem \
             --algorithm $signing_algorithmcounter
           if [ $? -ne 0 ]
           then
+            echo -e "${COL_RED}Wrap vbprivk${COL_STOP}"
+            return_code=255
+          fi
+
+          # Wrap public key
+          ${UTIL_DIR}/vbutil_key \
+            --pack ${TESTKEY_SCRATCH_DIR}/key_alg${algorithmcounter}.vbpubk \
+            --key ${TESTKEY_DIR}/key_rsa${signing_keylen}.keyb \
+            --algorithm $signing_algorithmcounter
+          if [ $? -ne 0 ]
+          then
+            echo -e "${COL_RED}Wrap vbpubk${COL_STOP}"
             return_code=255
           fi
 
@@ -86,17 +98,30 @@ ${datahashalgo}${COL_STOP}"
               ${TESTKEY_SCRATCH_DIR}/key_alg${algorithmcounter}.vbprivk
           if [ $? -ne 0 ]
           then
+            echo -e "${COL_RED}Pack${COL_STOP}"
             return_code=255
           fi
 
           # Unpack
           ${UTIL_DIR}/vbutil_keyblock --unpack ${keyblockfile} \
+            --datapubkey \
+            ${TESTKEY_SCRATCH_DIR}/key_alg${data_algorithmcounter}.vbpubk2 \
             --signpubkey \
-            ${TESTKEY_SCRATCH_DIR}/key_alg${signing_algorithmcounter}.vbpubk
-          # TODO: check data key against the packed one?
+            ${TESTKEY_SCRATCH_DIR}/key_alg${algorithmcounter}.vbpubk
           if [ $? -ne 0 ]
           then
+            echo -e "${COL_RED}Unpack${COL_STOP}"
+            return_code=255
+          fi
+
+          # Check
+          if ! cmp -s \
+            ${TESTKEY_SCRATCH_DIR}/key_alg${data_algorithmcounter}.vbpubk \
+            ${TESTKEY_SCRATCH_DIR}/key_alg${data_algorithmcounter}.vbpubk2
+          then
+            echo -e "${COL_RED}Check${COL_STOP}"
             return_code=255
+            exit 1
           fi
 
           let data_algorithmcounter=data_algorithmcounter+1
diff --git a/utility/Makefile b/utility/Makefile
index ff0ab102..f45603bb 100644
--- a/utility/Makefile
+++ b/utility/Makefile
@@ -27,7 +27,8 @@ TARGET_NAMES = dumpRSAPublicKey \
 		vbutil_kernel \
 		vbutil_key \
 		vbutil_keyblock \
-		verify_data
+		verify_data \
+		dev_make_keypair
 
 TARGET_BINS = $(addprefix ${BUILD_ROOT}/,$(TARGET_NAMES))
 ALL_DEPS = $(addsuffix .d,${TARGET_BINS})
@@ -73,6 +74,9 @@ ${BUILD_ROOT}/sign_image: sign_image.c $(LIBS)
 ${BUILD_ROOT}/tpm_init_temp_fix: tpm_init_temp_fix.c $(LIBS)
 	$(CC) $(CFLAGS) $(INCLUDES) $< -o $@ $(LIBS)
 
+${BUILD_ROOT}/dev_make_keypair: dev_make_keypair
+	cp -f $< $@
+
 install: $(TARGET_BINS)
 	mkdir -p $(DESTDIR)
 	cp -f $(TARGET_BINS) $(DESTDIR)
diff --git a/utility/dev_make_keypair b/utility/dev_make_keypair
new file mode 100755
index 00000000..5506b45f
--- /dev/null
+++ b/utility/dev_make_keypair
@@ -0,0 +1,82 @@
+#!/bin/bash -e
+# Copyright (c) 2010 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.
+#
+
+# Check args first.
+if [ "$#" -lt "1" ]; then
+  cat <<EOF 1>&2
+
+Usage:  ${0##*/} BASENAME [ALG]
+
+This creates BASENAME.vbpubk and BASENAME.vbprivk pairs for use in signing
+developer files. This also creates a BASENAME.keyblock file containing the
+BASENAME.vbpubk, which can be used to sign a developer kernel.
+
+If specified, ALG is one of:
+
+  0    =  RSA1024 with SHA1
+  1    =  RSA1024 with SHA256
+  2    =  RSA1024 with SHA512
+  3    =  RSA2048 with SHA1
+  4    =  RSA2048 with SHA256
+  5    =  RSA2048 with SHA512
+  6    =  RSA4096 with SHA1
+  7    =  RSA4096 with SHA256
+  8    =  RSA4096 with SHA512
+  9    =  RSA8192 with SHA1
+  10   =  RSA8192 with SHA256
+  11   =  RSA8192 with SHA512
+
+If ALG is not specified, a default value will be used.
+
+EOF
+  exit 1
+fi
+
+
+# Compute the key length assuming the sizes shown above.
+function alg_to_keylen {
+  echo $(( 1 << (10 + ($1 / 3)) ))
+}
+
+# Emit .vbpubk and .vbprivk using given basename and algorithm.
+function make_pair {
+  local base=$1
+  local alg=$2
+  local len=$(alg_to_keylen $alg)
+
+  # make the RSA keypair
+  openssl genrsa -F4 -out "${base}_${len}.pem" $len
+  # create a self-signed certificate
+  openssl req -batch -new -x509 -key "${base}_${len}.pem" \
+    -out "${base}_${len}.crt"
+  # generate pre-processed RSA public key
+  dumpRSAPublicKey "${base}_${len}.crt" > "${base}_${len}.keyb"
+
+  # wrap the public key
+  vbutil_key \
+    --pack "${base}.vbpubk" \
+    --key "${base}_${len}.keyb" \
+    --version 1 \
+    --algorithm $alg
+
+  # wrap the private key
+  vbutil_key \
+    --pack "${base}.vbprivk" \
+    --key "${base}_${len}.pem" \
+    --algorithm $alg
+
+  # remove intermediate files
+  rm -f "${base}_${len}.pem" "${base}_${len}.crt" "${base}_${len}.keyb"
+}
+
+# First create the .vbpubk and .vbprivk pair.
+make_pair "$1" "${2:-4}"
+
+# Now create a .vblock to hold our .vbpubk. Since it's for developer use, it
+# won't be signed, just checksummed. Developer kernels can only be run in
+# non-recovery mode with the developer switch enabled, but it won't hurt us to
+# turn on all the flags bits anyway.
+vbutil_keyblock --pack "$1.keyblock" --datapubkey "$1.vbpubk" --flags 15
diff --git a/utility/load_kernel_test.c b/utility/load_kernel_test.c
index 4ac9330b..9d1f14c3 100644
--- a/utility/load_kernel_test.c
+++ b/utility/load_kernel_test.c
@@ -12,6 +12,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <sys/types.h>
+#include <unistd.h>
 
 #include "load_kernel_fw.h"
 #include "boot_device.h"
@@ -75,29 +76,75 @@ int BootDeviceWriteLBA(uint64_t lba_start, uint64_t lba_count,
 int main(int argc, char* argv[]) {
 
   const char* image_name;
-  const char* keyfile_name;
-  int rv;
+  int rv, c, argsleft;
+  int errorcnt = 0;
+  char *e = 0;
 
   Memset(&lkp, 0, sizeof(LoadKernelParams));
   lkp.bytes_per_lba = LBA_BYTES;
+  lkp.boot_flags = BOOT_FLAG_RECOVERY;
 
-  /* Read command line parameters */
-  if (3 > argc) {
-    fprintf(stderr, "usage: %s <drive_image> <sign_key> [boot flag]\n", argv[0]);
+  /* Parse options */
+  opterr = 0;
+  while ((c=getopt(argc, argv, ":b:")) != -1)
+  {
+    switch (c)
+    {
+    case 'b':
+      lkp.boot_flags = strtoull(optarg, &e, 0);
+      if (!*optarg || (e && *e))
+      {
+        fprintf(stderr, "Invalid argument to -%c: \"%s\"\n", c, optarg);
+        errorcnt++;
+      }
+      break;
+    case '?':
+      fprintf(stderr, "Unrecognized switch: -%c\n", optopt);
+      errorcnt++;
+      break;
+    case ':':
+      fprintf(stderr, "Missing argument to -%c\n", optopt);
+      errorcnt++;
+      break;
+    default:
+      errorcnt++;
+      break;
+    }
+  }
+
+  /* Update argc */
+  argsleft = argc - optind;
+
+  if (errorcnt || !argsleft)
+  {
+    fprintf(stderr, "usage: %s [options] <drive_image> [<sign_key>]\n",
+            argv[0]);
+    fprintf(stderr, "\noptions:\n");
+    fprintf(stderr, "  -b NUM     boot flag bits (default %" PRIu64 "):\n",
+            BOOT_FLAG_RECOVERY);
+    fprintf(stderr, "               %" PRIu64 " = developer mode on\n",
+            BOOT_FLAG_DEVELOPER);
+    fprintf(stderr, "               %" PRIu64 " = recovery mode on\n",
+            BOOT_FLAG_RECOVERY);
     return 1;
   }
-  image_name = argv[1];
-  keyfile_name = argv[2];
+
+  image_name = argv[optind];
 
   /* Read header signing key blob */
-  {
+  if (argsleft > 1) {
     uint64_t key_size;
-    lkp.header_sign_key_blob = ReadFile(keyfile_name, &key_size);
+    lkp.header_sign_key_blob = ReadFile(argv[optind+1], &key_size);
     if (!lkp.header_sign_key_blob) {
-      fprintf(stderr, "Unable to read key file %s\n", keyfile_name);
+      fprintf(stderr, "Unable to read key file %s\n", argv[optind+1]);
       return 1;
     }
   }
+  /* Need to skip the address check, since we're putting it somewhere on the
+   * heap instead of its actual target address in the firmware. */
+  lkp.boot_flags |= BOOT_FLAG_SKIP_ADDR_CHECK;
+
+  printf("bootflags = %" PRIu64 "\n", lkp.boot_flags);
 
   /* Get image size */
   printf("Reading from image: %s\n", image_name);
@@ -119,14 +166,6 @@ int main(int argc, char* argv[]) {
   }
   lkp.kernel_buffer_size = KERNEL_BUFFER_SIZE;
 
-  /* Need to skip the address check, since we're putting it somewhere on the
-   * heap instead of its actual target address in the firmware. */
-  if (argc == 4) {
-    lkp.boot_flags = atoi(argv[3]) | BOOT_FLAG_SKIP_ADDR_CHECK;
-  } else {
-    /* Default to recovery. */
-    lkp.boot_flags = BOOT_FLAG_SKIP_ADDR_CHECK | BOOT_FLAG_RECOVERY;
-  }
   /* Call LoadKernel() */
   rv = LoadKernel(&lkp);
   printf("LoadKernel() returned %d\n", rv);
diff --git a/utility/vbutil_kernel.c b/utility/vbutil_kernel.c
index 893f889c..eeae96e1 100644
--- a/utility/vbutil_kernel.c
+++ b/utility/vbutil_kernel.c
@@ -76,7 +76,8 @@ static int PrintHelp(char *progname) {
           "\n"
           "  Required parameters:\n"
           "    --keyblock <file>         Key block in .keyblock format\n"
-          "    --signprivate <file>      Signing private key in .pem format\n"
+          "    --signprivate <file>"
+          "      Private key to sign kernel data, in .vbprivk format\n"
           "    --version <number>        Kernel version\n"
           "    --vmlinuz <file>          Linux kernel bzImage file\n"
           "    --bootloader <file>       Bootloader stub\n"
@@ -93,7 +94,8 @@ static int PrintHelp(char *progname) {
           "  Required parameters (of --keyblock and --config at least "
           "one is required):\n"
           "    --keyblock <file>         Key block in .keyblock format\n"
-          "    --signprivate <file>      Signing private key in .pem format\n"
+          "    --signprivate <file>"
+          "      Private key to sign kernel data, in .vbprivk format\n"
           "    --oldblob <file>          Previously packed kernel blob\n"
           "    --config <file>           New command line file\n"
           "\n"
@@ -105,10 +107,9 @@ static int PrintHelp(char *progname) {
           "\nOR\n\n"
           "Usage:  %s --verify <file> [PARAMETERS]\n"
           "\n"
-          "  Required parameters:\n"
-          "    --signpubkey <file>       Signing public key in .vbpubk format\n"
-          "\n"
           "  Optional:\n"
+          "    --signpubkey <file>"
+          "       Public key to verify kernel keyblock, in .vbpubk format\n"
           "    --verbose                 Print a more detailed report\n"
           "\n",
           progname);
@@ -611,22 +612,24 @@ static int Verify(const char* infile, const char* signpubkey, int verbose) {
   VbKeyBlockHeader* key_block;
   VbKernelPreambleHeader* preamble;
   VbPublicKey* data_key;
-  VbPublicKey* sign_key;
+  VbPublicKey* sign_key = NULL;
   RSAPublicKey* rsa;
   blob_t* bp;
   uint64_t now;
   int rv = 1;
 
-  if (!infile || !signpubkey) {
-    error("Must specify filename and signpubkey\n");
+  if (!infile) {
+    error("Must specify filename\n");
     return 1;
   }
 
   /* Read public signing key */
-  sign_key = PublicKeyRead(signpubkey);
-  if (!sign_key) {
-    error("Error reading signpubkey.\n");
-    return 1;
+  if (signpubkey) {
+    sign_key = PublicKeyRead(signpubkey);
+    if (!sign_key) {
+      error("Error reading signpubkey.\n");
+      return 1;
+    }
   }
 
   /* Read blob */
@@ -646,6 +649,8 @@ static int Verify(const char* infile, const char* signpubkey, int verbose) {
 
   printf("Key block:\n");
   data_key = &key_block->data_key;
+  if (verbose)
+    printf("  Signature:           %s\n", sign_key ? "valid" : "ignored");
   printf("  Size:                0x%" PRIx64 "\n", key_block->key_block_size);
   printf("  Data key algorithm:  %" PRIu64 " %s\n", data_key->algorithm,
          (data_key->algorithm < kNumAlgorithms ?
@@ -662,7 +667,7 @@ static int Verify(const char* infile, const char* signpubkey, int verbose) {
   /* Verify preamble */
   preamble = bp->preamble;
   if (0 != VerifyKernelPreamble(
-          preamble, bp->blob_size - key_block->key_block_size, rsa)) {
+        preamble, bp->blob_size - key_block->key_block_size, rsa)) {
     error("Error verifying preamble.\n");
     goto verify_exit;
   }
@@ -737,6 +742,10 @@ int main(int argc, char* argv[]) {
         parse_error = 1;
         break;
 
+      case 0:
+        /* silently handled option */
+        break;
+
       case OPT_MODE_PACK:
       case OPT_MODE_REPACK:
       case OPT_MODE_VERIFY:
diff --git a/utility/vbutil_keyblock.c b/utility/vbutil_keyblock.c
index 7c3f0279..a540ab59 100644
--- a/utility/vbutil_keyblock.c
+++ b/utility/vbutil_keyblock.c
@@ -46,16 +46,19 @@ static int PrintHelp(char *progname) {
           "\n"
           "For '--pack <file>', required OPTIONS are:\n"
           "  --datapubkey <file>         Data public key in .vbpubk format\n"
-          "  --signprivate <file>"
-          "        Signing private key in .vbprivk format\n"
           "\n"
           "Optional OPTIONS are:\n"
-          "  --flags <number>            Flags\n"
+          "  --signprivate <file>"
+          "        Signing private key in .vbprivk format. Without this arg,\n"
+          "                                the keyblock will not be signed.\n"
+          "  --flags <number>            Specifies allowed use conditions.\n"
           "\n"
-          "For '--unpack <file>', required OPTIONS are:\n"
-          "  --signpubkey <file>         Signing public key in .vbpubk format\n"
-          "Optional OPTIONS are:\n"
-          "  --datapubkey <file>         Data public key output file\n",
+          "For '--unpack <file>', optional OPTIONS are:\n"
+          "  --signpubkey <file>"
+          "         Signing public key in .vbpubk format. This is required to\n"
+          "                                verify a signed keyblock.\n"
+          "  --datapubkey <file>"
+          "         Write the data public key to this file.\n",
           progname);
   return 1;
 }
@@ -65,15 +68,15 @@ static int PrintHelp(char *progname) {
 static int Pack(const char* outfile, const char* datapubkey,
                 const char* signprivate, uint64_t flags) {
   VbPublicKey* data_key;
-  VbPrivateKey* signing_key;
+  VbPrivateKey* signing_key = NULL;
   VbKeyBlockHeader* block;
 
   if (!outfile) {
-    fprintf(stderr, "vbutil_keyblock: Must specify output filename\n");
+    fprintf(stderr, "vbutil_keyblock: Must specify output filename.\n");
     return 1;
   }
-  if (!datapubkey || !signprivate) {
-    fprintf(stderr, "vbutil_keyblock: Must specify all keys\n");
+  if (!datapubkey) {
+    fprintf(stderr, "vbutil_keyblock: Must specify data public key.\n");
     return 1;
   }
 
@@ -82,15 +85,19 @@ static int Pack(const char* outfile, const char* datapubkey,
     fprintf(stderr, "vbutil_keyblock: Error reading data key.\n");
     return 1;
   }
-  signing_key = PrivateKeyRead(signprivate);
-  if (!signing_key) {
-    fprintf(stderr, "vbutil_keyblock: Error reading signing key.\n");
-    return 1;
+
+  if (signprivate) {
+    signing_key = PrivateKeyRead(signprivate);
+    if (!signing_key) {
+      fprintf(stderr, "vbutil_keyblock: Error reading signing key.\n");
+      return 1;
+    }
   }
 
   block = KeyBlockCreate(data_key, signing_key, flags);
   Free(data_key);
-  Free(signing_key);
+  if (signing_key)
+    Free(signing_key);
 
   if (0 != KeyBlockWrite(outfile, block)) {
     fprintf(stderr, "vbutil_keyblock: Error writing key block.\n");
@@ -104,17 +111,11 @@ static int Pack(const char* outfile, const char* datapubkey,
 static int Unpack(const char* infile, const char* datapubkey,
                   const char* signpubkey) {
   VbPublicKey* data_key;
-  VbPublicKey* sign_key;
+  VbPublicKey* sign_key = NULL;
   VbKeyBlockHeader* block;
 
-  if (!infile || !signpubkey) {
-    fprintf(stderr, "vbutil_keyblock: Must specify filename and signpubkey\n");
-    return 1;
-  }
-
-  sign_key = PublicKeyRead(signpubkey);
-  if (!sign_key) {
-    fprintf(stderr, "vbutil_keyblock: Error reading signpubkey.\n");
+  if (!infile) {
+    fprintf(stderr, "vbutil_keyblock: Must specify filename\n");
     return 1;
   }
 
@@ -123,27 +124,48 @@ static int Unpack(const char* infile, const char* datapubkey,
     fprintf(stderr, "vbutil_keyblock: Error reading key block.\n");
     return 1;
   }
-  /* Verify the block with the signing public key, since
-   * KeyBlockRead() only verified the hash. */
-  /* TODO: should just print a warning, since self-signed key blocks
-   * won't have a public key; signpubkey should also be an optional
-   * argument. */
-  if (0 != KeyBlockVerify(block, block->key_block_size, sign_key)) {
-    fprintf(stderr, "vbutil_keyblock: Error verifying key block.\n");
-    return 1;
+
+  /* If the block is signed, then verify it with the signing public key, since
+     KeyBlockRead() only verified the hash. */
+  if (block->key_block_signature.sig_size) {
+    if (!signpubkey) {
+      fprintf(stderr,
+              "vbutil_keyblock: keyblock requires public key to verify\n");
+      return 1;
+    }
+    sign_key = PublicKeyRead(signpubkey);
+    if (!sign_key) {
+      fprintf(stderr, "vbutil_keyblock: Error reading signpubkey.\n");
+      return 1;
+    }
+    if (0 != KeyBlockVerify(block, block->key_block_size, sign_key)) {
+      fprintf(stderr, "vbutil_keyblock: Error verifying key block.\n");
+      return 1;
+    }
+    printf("Signature algorithm:  %" PRIu64 " %s\n", sign_key->algorithm,
+           (sign_key->algorithm < kNumAlgorithms ?
+            algo_strings[sign_key->algorithm] : "(invalid)"));
+    Free(sign_key);
+  } else {
+    printf("Signature Algorithm:  <none>\n");
   }
-  Free(sign_key);
 
-  printf("Key block file:      %s\n", infile);
-  printf("Flags:               %" PRIu64 "\n", block->key_block_flags);
+  printf("Key block file:       %s\n", infile);
+  printf("Flags:                %" PRIu64 "\n", block->key_block_flags);
 
   data_key = &block->data_key;
-  printf("Data key algorithm:  %" PRIu64 " %s\n", data_key->algorithm,
+  printf("Data key algorithm:   %" PRIu64 " %s\n", data_key->algorithm,
          (data_key->algorithm < kNumAlgorithms ?
           algo_strings[data_key->algorithm] : "(invalid)"));
-  printf("Data key version:    %" PRIu64 "\n", data_key->key_version);
+  printf("Data key version:     %" PRIu64 "\n", data_key->key_version);
 
-  /* TODO: write key data, if output file specified */
+  if (datapubkey) {
+    if (0 != PublicKeyWrite(datapubkey, data_key)) {
+      fprintf(stderr,
+              "vbutil_keyblock: unable to write public key\n");
+      return 1;
+    }
+  }
 
   Free(block);
   return 0;