qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v4 4/6] machine/nitro-enclave: Add built-in Nitro Secure Modu


From: Alexander Graf
Subject: Re: [PATCH v4 4/6] machine/nitro-enclave: Add built-in Nitro Secure Module device
Date: Mon, 19 Aug 2024 12:13:06 +0200
User-agent: Mozilla Thunderbird

Hey Dorjoy,

On 18.08.24 13:42, Dorjoy Chowdhury wrote:
AWS Nitro Enclaves have built-in Nitro Secure Module (NSM) device which
is used for stripped down TPM functionality like attestation. This commit
adds the built-in NSM device in the nitro-enclave machine type.

In Nitro Enclaves, all the PCRs start in a known zero state and the first
16 PCRs are locked from boot and reserved. The PCR0, PCR1, PCR2 and PCR8
contain the SHA384 hashes related to the EIF file used to boot the
VM for validation.

Some optional nitro-enclave machine options have been added:
     - 'id': Enclave identifier, reflected in the module-id of the NSM
device. If not provided, a default id will be set.
     - 'parent-role': Parent instance IAM role ARN, reflected in PCR3
of the NSM device.
     - 'parent-id': Parent instance identifier, reflected in PCR4 of the
NSM device.

Signed-off-by: Dorjoy Chowdhury <dorjoychy111@gmail.com>
---
  crypto/meson.build              |   2 +-
  crypto/x509-utils.c             |  73 +++++++++++


Can you please put this new API into its own patch file?


  hw/core/eif.c                   | 225 +++++++++++++++++++++++++++++---
  hw/core/eif.h                   |   5 +-


These changes to eif.c should ideally already be part of the patch that introduces eif.c (patch 1), no? In fact, do you think you can make the whole eif logic its own patch file?


  hw/core/meson.build             |   4 +-
  hw/i386/Kconfig                 |   1 +
  hw/i386/nitro_enclave.c         | 141 +++++++++++++++++++-
  include/crypto/x509-utils.h     |  22 ++++
  include/hw/i386/nitro_enclave.h |  26 ++++
  9 files changed, 479 insertions(+), 20 deletions(-)
  create mode 100644 crypto/x509-utils.c
  create mode 100644 include/crypto/x509-utils.h

diff --git a/crypto/meson.build b/crypto/meson.build
index c46f9c22a7..09633194ed 100644
--- a/crypto/meson.build
+++ b/crypto/meson.build
@@ -62,7 +62,7 @@ endif
  if gcrypt.found()
    util_ss.add(gcrypt, files('random-gcrypt.c'))
  elif gnutls.found()
-  util_ss.add(gnutls, files('random-gnutls.c'))
+  util_ss.add(gnutls, files('random-gnutls.c', 'x509-utils.c'))


What if we don't have gnutls. Will everything still compile or do we need to add any dependencies?


  elif get_option('rng_none')
    util_ss.add(files('random-none.c'))
  else
diff --git a/crypto/x509-utils.c b/crypto/x509-utils.c
new file mode 100644
index 0000000000..2422eb995c
--- /dev/null
+++ b/crypto/x509-utils.c
@@ -0,0 +1,73 @@
+/*
+ * X.509 certificate related helpers
+ *
+ * Copyright (c) 2024 Dorjoy Chowdhury <dorjoychy111@gmail.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * (at your option) any later version.  See the COPYING file in the
+ * top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "crypto/x509-utils.h"
+#include <gnutls/gnutls.h>
+#include <gnutls/crypto.h>
+#include <gnutls/x509.h>
+
+static int qcrypto_to_gnutls_hash_alg_map[QCRYPTO_HASH_ALG__MAX] = {
+    [QCRYPTO_HASH_ALG_MD5] = GNUTLS_DIG_MD5,
+    [QCRYPTO_HASH_ALG_SHA1] = GNUTLS_DIG_SHA1,
+    [QCRYPTO_HASH_ALG_SHA224] = GNUTLS_DIG_SHA224,
+    [QCRYPTO_HASH_ALG_SHA256] = GNUTLS_DIG_SHA256,
+    [QCRYPTO_HASH_ALG_SHA384] = GNUTLS_DIG_SHA384,
+    [QCRYPTO_HASH_ALG_SHA512] = GNUTLS_DIG_SHA512,
+    [QCRYPTO_HASH_ALG_RIPEMD160] = GNUTLS_DIG_RMD160,
+};
+
+int qcrypto_get_x509_cert_fingerprint(uint8_t *cert, size_t size,
+                                      QCryptoHashAlgorithm alg,
+                                      uint8_t **result,
+                                      size_t *resultlen,
+                                      Error **errp)
+{
+    int ret;
+    gnutls_x509_crt_t crt;
+    gnutls_datum_t datum = {.data = cert, .size = size};
+
+    if (alg >= G_N_ELEMENTS(qcrypto_to_gnutls_hash_alg_map)) {
+        error_setg(errp, "Unknown hash algorithm");
+        return -1;
+    }
+
+    gnutls_x509_crt_init(&crt);
+
+    if (gnutls_x509_crt_import(crt, &datum, GNUTLS_X509_FMT_PEM) != 0) {
+        error_setg(errp, "Failed to import certificate");
+        goto cleanup;
+    }
+
+    ret = gnutls_hash_get_len(qcrypto_to_gnutls_hash_alg_map[alg]);
+    if (*resultlen == 0) {
+        *resultlen = ret;
+        *result = g_new0(uint8_t, *resultlen);
+    } else if (*resultlen < ret) {
+        error_setg(errp,
+                   "Result buffer size %zu is smaller than hash %d",
+                   *resultlen, ret);
+        goto cleanup;
+    }
+
+    if (gnutls_x509_crt_get_fingerprint(crt,
+                                        qcrypto_to_gnutls_hash_alg_map[alg],
+                                        *result, resultlen) != 0) {
+        error_setg(errp, "Failed to get fingerprint from certificate");
+        goto cleanup;
+    }
+
+    return 0;
+
+ cleanup:
+    gnutls_x509_crt_deinit(crt);
+    return -1;
+}
diff --git a/hw/core/eif.c b/hw/core/eif.c
index 5558879a96..8e15142d36 100644
--- a/hw/core/eif.c
+++ b/hw/core/eif.c
@@ -11,7 +11,10 @@
  #include "qemu/osdep.h"
  #include "qemu/bswap.h"
  #include "qapi/error.h"
+#include "crypto/hash.h"
+#include "crypto/x509-utils.h"
  #include <zlib.h> /* for crc32 */
+#include <cbor.h>

  #include "hw/core/eif.h"

@@ -180,11 +183,10 @@ static void safe_unlink(char *f)
   * Upon success, the caller is reponsible for unlinking and freeing 
*kernel_path
   */
  static bool read_eif_kernel(FILE *f, uint64_t size, char **kernel_path,
-                            uint32_t *crc, Error **errp)
+                            uint8_t *kernel, uint32_t *crc, Error **errp)
  {
      size_t got;
      FILE *tmp_file = NULL;
-    uint8_t *kernel = NULL;

      *kernel_path = NULL;
      if (!get_tmp_file("eif-kernel-XXXXXX", kernel_path, errp)) {
@@ -198,7 +200,6 @@ static bool read_eif_kernel(FILE *f, uint64_t size, char 
**kernel_path,
          goto cleanup;
      }

-    kernel = g_malloc(size);
      got = fread(kernel, 1, size, f);
      if ((uint64_t) got != size) {
          error_setg(errp, "Failed to read EIF kernel section data");
@@ -213,7 +214,6 @@ static bool read_eif_kernel(FILE *f, uint64_t size, char 
**kernel_path,
      }

      *crc = crc32(*crc, kernel, size);
-    g_free(kernel);
      fclose(tmp_file);

      return true;
@@ -225,7 +225,6 @@ static bool read_eif_kernel(FILE *f, uint64_t size, char 
**kernel_path,
      g_free(*kernel_path);
      *kernel_path = NULL;

-    g_free(kernel);
      return false;
  }

@@ -243,29 +242,115 @@ static bool read_eif_cmdline(FILE *f, uint64_t size, 
char *cmdline,
  }

  static bool read_eif_ramdisk(FILE *eif, FILE *initrd, uint64_t size,
-                             uint32_t *crc, Error **errp)
+                             uint8_t *ramdisk, uint32_t *crc, Error **errp)
  {
      size_t got;
-    uint8_t *ramdisk = g_malloc(size);

      got = fread(ramdisk, 1, size, eif);
      if ((uint64_t) got != size) {
          error_setg(errp, "Failed to read EIF ramdisk section data");
-        goto cleanup;
+        return false;
      }

      got = fwrite(ramdisk, 1, size, initrd);
      if ((uint64_t) got != size) {
          error_setg(errp, "Failed to write EIF ramdisk data to temporary 
file");
-        goto cleanup;
+        return false;
      }

      *crc = crc32(*crc, ramdisk, size);
-    g_free(ramdisk);
+    return true;
+}
+
+static bool get_signature_fingerprint_sha384(FILE *eif, uint64_t size,
+                                             uint8_t *sha384,
+                                             uint32_t *crc,
+                                             Error **errp)
+{
+    size_t got;
+    uint8_t *sig = NULL;
+    uint8_t *cert = NULL;
+    cbor_item_t *item = NULL;
+    cbor_item_t *pcr0 = NULL;
+    size_t len;
+    size_t hash_len = 48;
+    struct cbor_pair *pair;
+    struct cbor_load_result result;
+
+    sig = g_malloc(size);
+    got = fread(sig, 1, size, eif);
+    if ((uint64_t) got != size) {
+        error_setg(errp, "Failed to read EIF signature section data");
+        goto cleanup;
+    }
+
+    *crc = crc32(*crc, sig, size);
+
+    item = cbor_load(sig, size, &result);
+    if (!item || result.error.code != CBOR_ERR_NONE) {
+        error_setg(errp, "Failed to load signature section data as CBOR");
+        goto cleanup;
+    }
+    if (!cbor_isa_array(item) || cbor_array_size(item) < 1) {
+        error_setg(errp, "Invalid signature CBOR");
+        goto cleanup;
+    }
+    pcr0 = cbor_array_get(item, 0);
+    if (!pcr0) {
+        error_setg(errp, "Failed to get PCR0 signature");
+        goto cleanup;
+    }
+    if (!cbor_isa_map(pcr0) || cbor_map_size(pcr0) != 2) {
+        error_setg(errp, "Invalid signature CBOR");
+        goto cleanup;
+    }
+    pair = cbor_map_handle(pcr0);
+    if (!cbor_isa_string(pair->key) || cbor_string_length(pair->key) != 19 ||
+        memcmp(cbor_string_handle(pair->key), "signing_certificate", 19) != 0) 
{
+        error_setg(errp, "Invalid signautre CBOR");
+        goto cleanup;
+    }
+    if (!cbor_isa_array(pair->value)) {
+        error_setg(errp, "Invalid signature CBOR");
+        goto cleanup;
+    }
+    len = cbor_array_size(pair->value);
+    if (len == 0) {
+        error_setg(errp, "Invalid signature CBOR");
+        goto cleanup;
+    }
+    cert = g_malloc(len);
+    for (int i = 0; i < len; ++i) {
+        cbor_item_t *tmp = cbor_array_get(pair->value, i);
+        if (!tmp) {
+            error_setg(errp, "Invalid signature CBOR");
+            goto cleanup;
+        }
+        if (!cbor_isa_uint(tmp) || cbor_int_get_width(tmp) != CBOR_INT_8) {
+            cbor_decref(&tmp);
+            error_setg(errp, "Invalid signature CBOR");
+            goto cleanup;
+        }
+        cert[i] = cbor_get_uint8(tmp);
+        cbor_decref(&tmp);
+    }
+
+    if (qcrypto_get_x509_cert_fingerprint(cert, len, QCRYPTO_HASH_ALG_SHA384,
+                                          &sha384, &hash_len, errp)) {
+        goto cleanup;
+    }
+
      return true;

   cleanup:
-    g_free(ramdisk);
+    g_free(sig);
+    g_free(cert);
+    if (pcr0) {
+        cbor_decref(&pcr0);
+    }
+    if (item) {
+        cbor_decref(&item);
+    }
      return false;
  }

@@ -299,7 +384,9 @@ static long get_file_size(FILE *f, Error **errp)
   */
  bool read_eif_file(const char *eif_path, const char *machine_initrd,
                     char **kernel_path, char **initrd_path, char **cmdline,
-                   Error **errp)
+                   uint8_t *image_sha384, uint8_t *bootstrap_sha384,
+                   uint8_t *app_sha384, uint8_t *fingerprint_sha384,
+                   bool *signature_found, Error **errp)
  {
      FILE *f = NULL;
      FILE *machine_initrd_f = NULL;
@@ -308,7 +395,19 @@ bool read_eif_file(const char *eif_path, const char 
*machine_initrd,
      uint32_t crc = 0;
      EifHeader eif_header;
      bool seen_sections[EIF_SECTION_MAX] = {false};
-
+    /* kernel + ramdisks + cmdline sha384 hash */
+    struct iovec image_hash_iovecs[MAX_SECTIONS + 1];
+    int image_hash_iovec_cnt = 0;
+    /* kernel + boot ramdisk + cmdline sha384 hash */
+    struct iovec bootstrap_hash_iovecs[3];
+    int bootstrap_hash_iovec_cnt = 0;
+    /* application ramdisk(s) hash */
+    struct iovec app_hash_iovecs[MAX_SECTIONS + 1];
+    int app_hash_iovec_cnt = 0;
+    uint8_t *ptr = NULL;
+    size_t digest_len;
+
+    *signature_found = false;
      *kernel_path = *initrd_path = *cmdline = NULL;

      f = fopen(eif_path, "rb");
@@ -373,8 +472,18 @@ bool read_eif_file(const char *eif_path, const char 
*machine_initrd,
                             "section");
                  goto cleanup;
              }
+
+            ptr = g_malloc(section_header.section_size);
+
+            image_hash_iovecs[image_hash_iovec_cnt].iov_base = ptr;
+            image_hash_iovecs[image_hash_iovec_cnt++].iov_len =
+                section_header.section_size;
+            bootstrap_hash_iovecs[bootstrap_hash_iovec_cnt].iov_base = ptr;
+            bootstrap_hash_iovecs[bootstrap_hash_iovec_cnt++].iov_len =
+                section_header.section_size;


These super long variable names make the code really difficult to read. How about "iov_PCR0"? Also, if you make this a GList, you don't need to keep track of the count and allocate a large chunk of memory on the stack.


+
              if (!read_eif_kernel(f, section_header.section_size, kernel_path,
-                                 &crc, errp)) {
+                                 ptr, &crc, errp)) {
                  goto cleanup;
              }

@@ -382,6 +491,7 @@ bool read_eif_file(const char *eif_path, const char 
*machine_initrd,
          case EIF_SECTION_CMDLINE:
          {
              uint64_t size;
+            uint8_t *cmdline_copy;
              if (seen_sections[EIF_SECTION_CMDLINE]) {
                  error_setg(errp, "Invalid EIF image. More than 1 cmdline "
                             "section");
@@ -394,6 +504,19 @@ bool read_eif_file(const char *eif_path, const char 
*machine_initrd,
              }
              (*cmdline)[size] = '\0';

+            /*
+             * We make a copy of '*cmdline' for putting it in iovecs so that
+             * we can easily free all the iovec entries later as we cannot
+             * free '*cmdline' which is used by the caller.
+             */
+            cmdline_copy = g_malloc(size);
+            memcpy(cmdline_copy, *cmdline, size);


Is this g_memdup2()?


+            image_hash_iovecs[image_hash_iovec_cnt].iov_base = cmdline_copy;
+            image_hash_iovecs[image_hash_iovec_cnt++].iov_len = size;
+            bootstrap_hash_iovecs[bootstrap_hash_iovec_cnt].iov_base =
+                cmdline_copy;
+            bootstrap_hash_iovecs[bootstrap_hash_iovec_cnt++].iov_len = size;
+
              break;
          }
          case EIF_SECTION_RAMDISK:
@@ -414,13 +537,41 @@ bool read_eif_file(const char *eif_path, const char 
*machine_initrd,
                  }
              }

+            ptr = g_malloc(section_header.section_size);
+
+            image_hash_iovecs[image_hash_iovec_cnt].iov_base = ptr;
+            image_hash_iovecs[image_hash_iovec_cnt++].iov_len =
+                section_header.section_size;
+            /*
+             * If it's the first ramdisk, we need to hash it into bootstrap,
+             * otherwise we need to hash it into app.
+             */
+            if (!seen_sections[EIF_SECTION_RAMDISK]) {
+                bootstrap_hash_iovecs[bootstrap_hash_iovec_cnt].iov_base = ptr;
+                bootstrap_hash_iovecs[bootstrap_hash_iovec_cnt++].iov_len =
+                    section_header.section_size;


Seeing all these long lines: What if you just called "section_header" "section" or "hdr" and "section_size" "len"?


+            } else {
+                app_hash_iovecs[app_hash_iovec_cnt].iov_base = ptr;
+                app_hash_iovecs[app_hash_iovec_cnt++].iov_len =
+                    section_header.section_size;
+            }
+
              if (!read_eif_ramdisk(f, initrd_path_f, 
section_header.section_size,
-                                  &crc, errp)) {
+                                  ptr, &crc, errp)) {
                  goto cleanup;
              }

              break;
          }
+        case EIF_SECTION_SIGNATURE:
+            *signature_found = true;
+            if (!get_signature_fingerprint_sha384(f,
+                                                  section_header.section_size,
+                                                  fingerprint_sha384, &crc,
+                                                  errp)) {
+                goto cleanup;
+            }
+            break;
          default:
              /* other sections including invalid or unknown sections */
          {
@@ -483,18 +634,60 @@ bool read_eif_file(const char *eif_path, const char 
*machine_initrd,
              goto cleanup;
          }

+        ptr = g_malloc(machine_initrd_size);
+
+        image_hash_iovecs[image_hash_iovec_cnt].iov_base = ptr;
+        image_hash_iovecs[image_hash_iovec_cnt++].iov_len = 
machine_initrd_size;
+        app_hash_iovecs[app_hash_iovec_cnt].iov_base = ptr;
+        app_hash_iovecs[app_hash_iovec_cnt++].iov_len = machine_initrd_size;
+
          if (!read_eif_ramdisk(machine_initrd_f, initrd_path_f,
-                              machine_initrd_size, &crc, errp)) {
+                              machine_initrd_size, ptr, &crc, errp)) {
              goto cleanup;
          }
      }

+    digest_len = 48;
+    if (qcrypto_hash_bytesv(QCRYPTO_HASH_ALG_SHA384, image_hash_iovecs,
+                            image_hash_iovec_cnt, &image_sha384, &digest_len,
+                            errp) < 0) {
+        goto cleanup;
+
+    }
+
+    digest_len = 48;
+    if (qcrypto_hash_bytesv(QCRYPTO_HASH_ALG_SHA384, bootstrap_hash_iovecs,
+                            bootstrap_hash_iovec_cnt, &bootstrap_sha384,
+                            &digest_len, errp) < 0) {
+        goto cleanup;
+
+    }
+
+    digest_len = 48;
+    if (qcrypto_hash_bytesv(QCRYPTO_HASH_ALG_SHA384, app_hash_iovecs,
+                            app_hash_iovec_cnt, &app_sha384, &digest_len,
+                            errp) < 0) {
+        goto cleanup;
+
+    }
+
+    /*
+     * We only need to free image_hash_iovec entries because bootstrap and
+     * app iovec entries are subsets of image_hash_iovec entries.
+     */
+    for (int i = 0; i < image_hash_iovec_cnt; ++i) {
+        g_free(image_hash_iovecs[i].iov_base);
+    }
      fclose(f);
      fclose(initrd_path_f);
      safe_fclose(machine_initrd_f);
      return true;

   cleanup:
+    for (int i = 0; i < image_hash_iovec_cnt; ++i) {
+        g_free(image_hash_iovecs[i].iov_base);
+    }
+
      safe_fclose(f);
      safe_fclose(initrd_path_f);
      safe_fclose(machine_initrd_f);
diff --git a/hw/core/eif.h b/hw/core/eif.h
index 7063974d93..fed3cb5514 100644
--- a/hw/core/eif.h
+++ b/hw/core/eif.h
@@ -13,7 +13,10 @@

  bool read_eif_file(const char *eif_path, const char *machine_initrd,
                     char **kernel_path, char **initrd_path,
-                   char **kernel_cmdline, Error **errp);
+                   char **kernel_cmdline, uint8_t *image_sha384,
+                   uint8_t *bootstrap_sha384, uint8_t *app_sha384,
+                   uint8_t *fingerprint_sha384, bool *signature_found,
+                   Error **errp);

  #endif

diff --git a/hw/core/meson.build b/hw/core/meson.build
index f32d1ad943..8dc4552e35 100644
--- a/hw/core/meson.build
+++ b/hw/core/meson.build
@@ -12,6 +12,8 @@ hwcore_ss.add(files(
    'qdev-clock.c',
  ))

+libcbor = dependency('libcbor', version: '>=0.7.0')
+
  common_ss.add(files('cpu-common.c'))
  common_ss.add(files('machine-smp.c'))
  system_ss.add(when: 'CONFIG_FITLOADER', if_true: files('loader-fit.c'))
@@ -24,7 +26,7 @@ system_ss.add(when: 'CONFIG_REGISTER', if_true: 
files('register.c'))
  system_ss.add(when: 'CONFIG_SPLIT_IRQ', if_true: files('split-irq.c'))
  system_ss.add(when: 'CONFIG_XILINX_AXI', if_true: files('stream.c'))
  system_ss.add(when: 'CONFIG_PLATFORM_BUS', if_true: files('sysbus-fdt.c'))
-system_ss.add(when: 'CONFIG_NITRO_ENCLAVE', if_true: [files('eif.c'), zlib])
+system_ss.add(when: 'CONFIG_NITRO_ENCLAVE', if_true: [files('eif.c'), zlib, 
libcbor, gnutls])


Ah, you add the gnutls dependency here. Great! However, this means we now make gnutls (and libcbor) a mandatory dependency for the default configuration. Does configure know about that? I believe before gnutls was optional, right?


Alex





Amazon Web Services Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 257764 B
Sitz: Berlin
Ust-ID: DE 365 538 597

reply via email to

[Prev in Thread] Current Thread [Next in Thread]