Re: Need to disable the devicetree command in Secure Boot mode

[Steve McIntyre <steve@einval.com>]

On Wed, Apr 24, 2019 at 05:37:24PM +0100, Steve McIntyre wrote:
>On Wed, Apr 24, 2019 at 05:26:00PM +0100, Steve McIntyre wrote:
>>Source: grub2
>>Version: 2.02+dfsg1-16
>>Severity: serious
>>Tags: security
>>
>>In discussion with upstream EFI and arm64 folks, it's become clear
>>that in SB mode we should also be disabling the devicetree command in
>>Secure Boot mode. I'm testing a patch right now, coming shortly.
>
>We should also blacklist any of our old grub-efi-arm64-signed binaries
>signed with our production key - this is a real hole that can totally
>undermine SB. I'll work out how to do that for the next shim upload,
>due in the next couple of days.

It's taken me a few days to get this tested, but this grub patch works
in a SB-enabled qemu/kvm test image.

I'm working out how how to do the dbx entries for old binaries now.

-- 
Steve McIntyre, Cambridge, UK.                                steve@einval.com
"I can't ever sleep on planes ... call it irrational if you like, but I'm
 afraid I'll miss my stop" -- Vivek Das Mohapatra

>From aaaa6901f2f642219ee721cd8b84ae0246d1098a Mon Sep 17 00:00:00 2001
From: Peter Jones <pjones@redhat.com>
Date: Wed, 24 Apr 2019 10:03:04 -0400
Subject: [PATCH] Forbid the "devicetree" command when Secure Boot is enabled.

Signed-off-by: Peter Jones <pjones@redhat.com>
Signed-off-by: Steve McIntyre <93sam@debian.org>
---
 grub-core/loader/arm/linux.c | 17 ++++++++++++++++-
 grub-core/loader/efi/fdt.c   | 11 +++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/grub-core/loader/arm/linux.c b/grub-core/loader/arm/linux.c
index 9300adc8f..887ad5326 100644
--- a/grub-core/loader/arm/linux.c
+++ b/grub-core/loader/arm/linux.c
@@ -29,6 +29,10 @@
 #include <grub/lib/cmdline.h>
 #include <grub/linux.h>
 
+#ifdef GRUB_MACHINE_EFI
+#include <grub/efi/efi.h>
+#endif
+
 GRUB_MOD_LICENSE ("GPLv3+");
 
 static grub_dl_t my_mod;
@@ -433,9 +437,17 @@ grub_cmd_devicetree (grub_command_t cmd __attribute__ ((unused)),
   if (argc != 1)
     return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("filename expected"));
 
+#ifdef GRUB_MACHINE_EFI
+  if (grub_efi_secure_boot ())
+    {
+      return grub_error (GRUB_ERR_ACCESS_DENIED,
+		  "Secure Boot forbids loading devicetree from %s", argv[0]);
+    }
+#endif
+
   dtb = grub_file_open (argv[0]);
   if (!dtb)
-    goto out;
+    return grub_errno;
 
   size = grub_file_size (dtb);
   if (size == 0)
diff --git a/grub-core/loader/efi/fdt.c b/grub-core/loader/efi/fdt.c
index c9aee74ef..735c56e45 100644
--- a/grub-core/loader/efi/fdt.c
+++ b/grub-core/loader/efi/fdt.c
@@ -123,6 +123,14 @@ grub_cmd_devicetree (grub_command_t cmd __attribute__ ((unused)),
       return GRUB_ERR_NONE;
     }
 
+#ifdef GRUB_MACHINE_EFI
+  if (grub_efi_secure_boot ())
+    {
+      return grub_error (GRUB_ERR_ACCESS_DENIED,
+		  "Secure Boot forbids loading devicetree from %s", argv[0]);
+    }
+#endif
+
   dtb = grub_file_open (argv[0]);
   if (!dtb)
     goto out;