On Fri, Aug 16, 2024 at 6:58 PM Daniel P. Berrangé <berra...@redhat.com> wrote:
>
> On Fri, Aug 16, 2024 at 06:50:34PM +0600, Dorjoy Chowdhury wrote:
> > Hi Daniel,
> >
> > On Mon, Aug 12, 2024 at 8:07 PM Daniel P. Berrangé <berra...@redhat.com>
> > wrote:
> > >
> > > On Sat, Aug 10, 2024 at 10:45:01PM +0600, 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.
> > > >
> > > > A new optional nitro-enclave machine option 'id' has been added which
> > > > will
> > > > be the enclave identifier reflected in the module-id of the NSM device.
> > > > Otherwise, the device will have a default id set.
> > > >
> > > > Signed-off-by: Dorjoy Chowdhury <dorjoychy...@gmail.com>
> > > > ---
> > > > hw/core/eif.c | 205 +++++++++++++++++++++++++++++++-
> > > > hw/core/eif.h | 5 +-
> > > > hw/core/meson.build | 4 +-
> > > > hw/i386/Kconfig | 1 +
> > > > hw/i386/nitro_enclave.c | 85 ++++++++++++-
> > > > include/hw/i386/nitro_enclave.h | 19 +++
> > > > 6 files changed, 310 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/hw/core/eif.c b/hw/core/eif.c
> > > > index 5558879a96..d2c65668ef 100644
> > > > --- a/hw/core/eif.c
> > > > +++ b/hw/core/eif.c
> > > > @@ -12,6 +12,9 @@
> > > > #include "qemu/bswap.h"
> > > > #include "qapi/error.h"
> > > > #include <zlib.h> /* for crc32 */
> > > > +#include <cbor.h>
> > > > +#include <gnutls/gnutls.h>
> > > > +#include <gnutls/x509.h>
> > > >
> > > > #include "hw/core/eif.h"
> > > >
> > >
> > > > @@ -269,6 +284,125 @@ static bool read_eif_ramdisk(FILE *eif, FILE
> > > > *initrd, uint64_t size,
> > > > return false;
> > > > }
> > > >
> > > > +static bool get_fingerprint_sha384_from_cert(uint8_t *cert, size_t
> > > > size,
> > > > + uint8_t *sha384, Error
> > > > **errp)
> > > > +{
> > > > + gnutls_x509_crt_t crt;
> > > > + size_t hash_size = 48;
> > > > + gnutls_datum_t datum = {.data = cert, .size = size};
> > > > +
> > > > + gnutls_global_init();
> > > > + 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;
> > > > + }
> > > > +
> > > > + if (gnutls_x509_crt_get_fingerprint(crt, GNUTLS_DIG_SHA384, sha384,
> > > > + &hash_size) != 0) {
> > > > + error_setg(errp, "Failed to compute SHA384 fingerprint");
> > > > + goto cleanup;
> > > > + }
> > > > +
> > > > + return true;
> > > > +
> > > > + cleanup:
> > > > + gnutls_x509_crt_deinit(crt);
> > > > + gnutls_global_deinit();
> > > > + return false;
> > > > +}
> > >
> > > I'd suggest this go into qcrypto/x509-utils.c &
> > > include/qcrypto/x509-utils.h,
> > > as:
> > >
> > > int qcrypto_get_x509_cert_fingerprint(uint8_t *cert,
> > > size_t size,
> > > QCryptoHashAlgorith hash,
> > > Error **errp);
> > >
> > > there's no need to be calling gnutls_global_init() / deinit() either.
> > >
> > >
> > > > @@ -299,7 +433,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,9 +444,33 @@ 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 */
> > > > + GChecksum *image_hasher = NULL;
> > > > + /* kernel + boot ramdisk + cmdline sha384 hash */
> > > > + GChecksum *bootstrap_hasher = NULL;
> > > > + /* application ramdisk(s) hash */
> > > > + GChecksum *app_hasher = NULL;
> > > > + size_t digest_len;
> > > > +
> > > > + *signature_found = false;
> > > > *kernel_path = *initrd_path = *cmdline = NULL;
> > > >
> > > > + image_hasher = g_checksum_new(G_CHECKSUM_SHA384);
> > > > + if (image_hasher == NULL) {
> > > > + error_setg(errp, "Failed to initialize sha384 hash for image");
> > > > + goto cleanup;
> > > > + }
> > > > + bootstrap_hasher = g_checksum_new(G_CHECKSUM_SHA384);
> > > > + if (bootstrap_hasher == NULL) {
> > > > + error_setg(errp, "Failed to initialize sha384 hash for
> > > > bootstrap");
> > > > + goto cleanup;
> > > > + }
> > > > + app_hasher = g_checksum_new(G_CHECKSUM_SHA384);
> > > > + if (app_hasher == NULL) {
> > > > + error_setg(errp, "Failed to initialize sha384 hash for app");
> > > > + goto cleanup;
> > > > + }
> > >
> > > Don't use GChecksum APIs please, use the qcrypto hash APIs instead,
> > > as we need all code to be using the designated QEMU crypto backend.
> > >
> >
> > Thanks for the reviews. I was looking into replacing the GChecksum
> > uses with qcrypto apis and was able to do it in the extendPCR function
> > but I need some help with how I can do this in the eif.c file. For
> > example, the "image_hash" needs to be a SHA384 hash of the kernel,
> > cmdline, ramdisks sections' data as they appear in the order that is
> > in the EIF file. Using GChecksum it was easy as I was able to just
> > pass the hashers to the "read_eif_kernel", "read_eif_ramdisk" etc
> > functions and call "update" on them. But the qcrypto apis are
> > stateless i.e., I would need to pass all the buffers in a single api
> > call so it wouldn't work right now out of the box. Do you have any
> > suggestions how I should modify/create qcrypto apis so that I can
> > easily do this (considering that I would need to implement for
> > different qcrypto backends)? Thanks!
>
> Looking at the read_eif_* methods:
>
> @ -213,6 +218,8 @@ static bool read_eif_kernel(FILE *f, uint64_t size, char
> **kernel_path,
> }
>
> *crc = crc32(*crc, kernel, size);
> + g_checksum_update(image_hasher, kernel, size);
> + g_checksum_update(bootstrap_hasher, kernel, size);
> g_free(kernel);
> fclose(tmp_file);
>
> @@ -230,6 +237,8 @@ static bool read_eif_kernel(FILE *f, uint64_t size, char
> **kernel_path,
> }
>
> static bool read_eif_cmdline(FILE *f, uint64_t size, char *cmdline,
> + GChecksum *image_hasher,
> + GChecksum *bootstrap_hasher,
> uint32_t *crc, Error **errp)
> {
> size_t got = fread(cmdline, 1, size, f);
> @@ -239,10 +248,14 @@ static bool read_eif_cmdline(FILE *f, uint64_t size,
> char *cmdline,
> }
>
> *crc = crc32(*crc, (uint8_t *)cmdline, size);
> + g_checksum_update(image_hasher, (uint8_t *)cmdline, size);
> + g_checksum_update(bootstrap_hasher, (uint8_t *)cmdline, size);
> return true;
> }
>
> static bool read_eif_ramdisk(FILE *eif, FILE *initrd, uint64_t size,
> + GChecksum *image_hasher,
> + GChecksum *bootstrap_or_app_hasher,
> uint32_t *crc, Error **errp)
> {
> size_t got;
> @@ -261,6 +274,8 @@ static bool read_eif_ramdisk(FILE *eif, FILE *initrd,
> uint64_t size,
> }
>
> *crc = crc32(*crc, ramdisk, size);
> + g_checksum_update(image_hasher, ramdisk, size);
> + g_checksum_update(bootstrap_or_app_hasher, ramdisk, size);
> g_free(ramdisk);
> return true;
>
>
> For the kernel + ramdisk, these methods are freeing the buffer.
> For the cmdline, the pointer is held by the caller.
>
> I'd suggest that read_eif_kernel+ramddisk are changed to return
> the data pointer instead of free'ing it.
>
> The caller can then stash the kernl+cmdline+ramdisk into an iovec
> and calc the hash in a single stateless operation.
>
>
>
> If you really don't want to do that, then you'll have to wait for
> this series to be finalized & merged next dev cycle:
>
> https://lists.nongnu.org/archive/html/qemu-devel/2024-08/msg01173.html
>
> It is getting close to ready, so might not be too long, but I can't
> guarantee a timeframe.
>
Thanks Daniel! It looks like returning the buffers is the way to move
forward for now.
Regards,
Dorjoy
