[PATCH v3 0/1] tpm: Don't propagate measurement failures to the verifiers layer

[PATCH v3 0/1] tpm: Don't propagate measurement failures to the verifiers layer

[Robbie Harwood]

Address Daniel's and James's feedback on previous version by adding an
environment variable to restore the TPM hard failure behavior.  Interdiff
attached.

Be well,
--Robbie

Robbie Harwood (1):
  tpm: Don't propagate measurement failures to the verifiers layer

 docs/grub.texi           |  9 +++++++++
 grub-core/commands/tpm.c | 29 ++++++++++++++++++++++++++---
 2 files changed, 35 insertions(+), 3 deletions(-)

Interdiff against v2:
diff --git a/docs/grub.texi b/docs/grub.texi
index 2d6cd83580..eb43d8970d 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -3318,6 +3318,7 @@ These variables have special meaning to GRUB.
 * theme::
 * timeout::
 * timeout_style::
+* tpm_fail_fatal::
 @end menu
 
 
@@ -3825,6 +3826,14 @@ displaying the menu.  See the documentation of @samp{GRUB_TIMEOUT_STYLE}
 (@pxref{Simple configuration}) for details.
 
 
+@node tpm_fail_fatal
+@subsection tpm_fail_fatal
+
+If this variable is enabled, TPM measurements that fail will be treated
+as fatal.  Otherwise, they will merely be debug-logged and boot will
+continue.
+
+
 @node Environment block
 @section The GRUB environment block
 
diff --git a/grub-core/commands/tpm.c b/grub-core/commands/tpm.c
index 24874ffacb..ca088055dd 100644
--- a/grub-core/commands/tpm.c
+++ b/grub-core/commands/tpm.c
@@ -18,6 +18,7 @@
  *  Core TPM support code.
  */
 
+#include <grub/env.h>
 #include <grub/err.h>
 #include <grub/i18n.h>
 #include <grub/misc.h>
@@ -26,6 +27,7 @@
 #include <grub/term.h>
 #include <grub/verify.h>
 #include <grub/dl.h>
+#include <stdbool.h>
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
@@ -39,14 +41,27 @@ grub_tpm_verify_init (grub_file_t io,
   return GRUB_ERR_NONE;
 }
 
+static inline bool
+is_tpm_fail_fatal (void)
+{
+  const char *val = grub_env_get ("tpm_fail_fatal");
+
+  if (val == NULL || grub_strlen (val) < 1 || grub_strcmp (val, "0") == 0 ||
+      grub_strcmp (val, "false") == 0)
+    return false;
+  return true;
+}
+
 static grub_err_t
 grub_tpm_verify_write (void *context, void *buf, grub_size_t size)
 {
   grub_err_t status = grub_tpm_measure (buf, size, GRUB_BINARY_PCR, context);
 
-  if (status)
-    grub_dprintf ("tpm", "Measuring buffer failed: %d\n", status);
-  return GRUB_ERR_NONE;
+  if (status == GRUB_ERR_NONE)
+    return GRUB_ERR_NONE;
+
+  grub_dprintf ("tpm", "Measuring buffer failed: %d\n", status);
+  return is_tpm_fail_fatal () ? status : GRUB_ERR_NONE;
 }
 
 static grub_err_t
@@ -77,10 +92,12 @@ grub_tpm_verify_string (char *str, enum grub_verify_string_type type)
   status =
     grub_tpm_measure ((unsigned char *) str, grub_strlen (str),
 		      GRUB_STRING_PCR, description);
-  if (status)
-    grub_dprintf ("tpm", "Measuring string %s failed: %d\n", str, status);
   grub_free (description);
-  return GRUB_ERR_NONE;
+  if (status == GRUB_ERR_NONE)
+    return GRUB_ERR_NONE;
+
+  grub_dprintf ("tpm", "Measuring string %s failed: %d\n", str, status);
+  return is_tpm_fail_fatal () ? status : GRUB_ERR_NONE;
 }
 
 struct grub_file_verifier grub_tpm_verifier = {
-- 
2.35.1

[PATCH v3 1/1] tpm: Don't propagate measurement failures to the verifiers layer

[Robbie Harwood]

Currently if an EFI firmware fails to do a TPM measurement for a file,
the error will be propagated to the verifiers framework which will
prevent it to be opened.  This mean that buggy firmwares will lead to
the system not booting because files won't be allowed to be loaded. But
a failure to do a TPM measurement isn't expected to be a fatal error
that causes the system to be unbootable.

To avoid this, don't return errors from .write and .verify_string
callbacks and just print a debug message in the case of a TPM
measurement failure.  Add an environment variable (tpm_fail_fatal) to
restore the previous behavior.

Also-authored-by: Javier Martinez Canillas <javierm@redhat.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Signed-off-by: Robbie Harwood <rharwood@redhat.com>
---
 docs/grub.texi           |  9 +++++++++
 grub-core/commands/tpm.c | 29 ++++++++++++++++++++++++++---
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/docs/grub.texi b/docs/grub.texi
index 2d6cd83580..eb43d8970d 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -3318,6 +3318,7 @@ These variables have special meaning to GRUB.
 * theme::
 * timeout::
 * timeout_style::
+* tpm_fail_fatal::
 @end menu
 
 
@@ -3825,6 +3826,14 @@ displaying the menu.  See the documentation of @samp{GRUB_TIMEOUT_STYLE}
 (@pxref{Simple configuration}) for details.
 
 
+@node tpm_fail_fatal
+@subsection tpm_fail_fatal
+
+If this variable is enabled, TPM measurements that fail will be treated
+as fatal.  Otherwise, they will merely be debug-logged and boot will
+continue.
+
+
 @node Environment block
 @section The GRUB environment block
 
diff --git a/grub-core/commands/tpm.c b/grub-core/commands/tpm.c
index 2052c36eab..ca088055dd 100644
--- a/grub-core/commands/tpm.c
+++ b/grub-core/commands/tpm.c
@@ -18,6 +18,7 @@
  *  Core TPM support code.
  */
 
+#include <grub/env.h>
 #include <grub/err.h>
 #include <grub/i18n.h>
 #include <grub/misc.h>
@@ -26,6 +27,7 @@
 #include <grub/term.h>
 #include <grub/verify.h>
 #include <grub/dl.h>
+#include <stdbool.h>
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
@@ -39,10 +41,27 @@ grub_tpm_verify_init (grub_file_t io,
   return GRUB_ERR_NONE;
 }
 
+static inline bool
+is_tpm_fail_fatal (void)
+{
+  const char *val = grub_env_get ("tpm_fail_fatal");
+
+  if (val == NULL || grub_strlen (val) < 1 || grub_strcmp (val, "0") == 0 ||
+      grub_strcmp (val, "false") == 0)
+    return false;
+  return true;
+}
+
 static grub_err_t
 grub_tpm_verify_write (void *context, void *buf, grub_size_t size)
 {
-  return grub_tpm_measure (buf, size, GRUB_BINARY_PCR, context);
+  grub_err_t status = grub_tpm_measure (buf, size, GRUB_BINARY_PCR, context);
+
+  if (status == GRUB_ERR_NONE)
+    return GRUB_ERR_NONE;
+
+  grub_dprintf ("tpm", "Measuring buffer failed: %d\n", status);
+  return is_tpm_fail_fatal () ? status : GRUB_ERR_NONE;
 }
 
 static grub_err_t
@@ -66,7 +85,7 @@ grub_tpm_verify_string (char *str, enum grub_verify_string_type type)
     }
   description = grub_malloc (grub_strlen (str) + grub_strlen (prefix) + 1);
   if (!description)
-    return grub_errno;
+    return GRUB_ERR_NONE;
   grub_memcpy (description, prefix, grub_strlen (prefix));
   grub_memcpy (description + grub_strlen (prefix), str,
 	       grub_strlen (str) + 1);
@@ -74,7 +93,11 @@ grub_tpm_verify_string (char *str, enum grub_verify_string_type type)
     grub_tpm_measure ((unsigned char *) str, grub_strlen (str),
 		      GRUB_STRING_PCR, description);
   grub_free (description);
-  return status;
+  if (status == GRUB_ERR_NONE)
+    return GRUB_ERR_NONE;
+
+  grub_dprintf ("tpm", "Measuring string %s failed: %d\n", str, status);
+  return is_tpm_fail_fatal () ? status : GRUB_ERR_NONE;
 }
 
 struct grub_file_verifier grub_tpm_verifier = {
-- 
2.35.1

Re: [PATCH v3 1/1] tpm: Don't propagate measurement failures to the verifiers layer

[Daniel Kiper]

On Mon, Oct 31, 2022 at 05:31:40PM -0400, Robbie Harwood wrote:
> Currently if an EFI firmware fails to do a TPM measurement for a file,
> the error will be propagated to the verifiers framework which will
> prevent it to be opened.  This mean that buggy firmwares will lead to
> the system not booting because files won't be allowed to be loaded. But
> a failure to do a TPM measurement isn't expected to be a fatal error
> that causes the system to be unbootable.
>
> To avoid this, don't return errors from .write and .verify_string
> callbacks and just print a debug message in the case of a TPM
> measurement failure.  Add an environment variable (tpm_fail_fatal) to
> restore the previous behavior.
>
> Also-authored-by: Javier Martinez Canillas <javierm@redhat.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> Signed-off-by: Robbie Harwood <rharwood@redhat.com>
> ---
>  docs/grub.texi           |  9 +++++++++
>  grub-core/commands/tpm.c | 29 ++++++++++++++++++++++++++---
>  2 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/docs/grub.texi b/docs/grub.texi
> index 2d6cd83580..eb43d8970d 100644
> --- a/docs/grub.texi
> +++ b/docs/grub.texi
> @@ -3318,6 +3318,7 @@ These variables have special meaning to GRUB.
>  * theme::
>  * timeout::
>  * timeout_style::
> +* tpm_fail_fatal::
>  @end menu
>
>
> @@ -3825,6 +3826,14 @@ displaying the menu.  See the documentation of @samp{GRUB_TIMEOUT_STYLE}
>  (@pxref{Simple configuration}) for details.
>
>
> +@node tpm_fail_fatal
> +@subsection tpm_fail_fatal
> +
> +If this variable is enabled, TPM measurements that fail will be treated

What do you mean by enabled? Please align description in doc with the
code below.

> +as fatal.  Otherwise, they will merely be debug-logged and boot will
> +continue.
> +
> +
>  @node Environment block
>  @section The GRUB environment block
>
> diff --git a/grub-core/commands/tpm.c b/grub-core/commands/tpm.c
> index 2052c36eab..ca088055dd 100644
> --- a/grub-core/commands/tpm.c
> +++ b/grub-core/commands/tpm.c
> @@ -18,6 +18,7 @@
>   *  Core TPM support code.
>   */
>
> +#include <grub/env.h>
>  #include <grub/err.h>
>  #include <grub/i18n.h>
>  #include <grub/misc.h>
> @@ -26,6 +27,7 @@
>  #include <grub/term.h>
>  #include <grub/verify.h>
>  #include <grub/dl.h>
> +#include <stdbool.h>
>
>  GRUB_MOD_LICENSE ("GPLv3+");
>
> @@ -39,10 +41,27 @@ grub_tpm_verify_init (grub_file_t io,
>    return GRUB_ERR_NONE;
>  }
>
> +static inline bool
> +is_tpm_fail_fatal (void)
> +{
> +  const char *val = grub_env_get ("tpm_fail_fatal");
> +
> +  if (val == NULL || grub_strlen (val) < 1 || grub_strcmp (val, "0") == 0 ||
> +      grub_strcmp (val, "false") == 0)

I would add "disable" and "no" to the list.

> +    return false;
> +  return true;
> +}
> +
>  static grub_err_t
>  grub_tpm_verify_write (void *context, void *buf, grub_size_t size)
>  {
> -  return grub_tpm_measure (buf, size, GRUB_BINARY_PCR, context);
> +  grub_err_t status = grub_tpm_measure (buf, size, GRUB_BINARY_PCR, context);
> +
> +  if (status == GRUB_ERR_NONE)
> +    return GRUB_ERR_NONE;
> +
> +  grub_dprintf ("tpm", "Measuring buffer failed: %d\n", status);
> +  return is_tpm_fail_fatal () ? status : GRUB_ERR_NONE;
>  }
>
>  static grub_err_t
> @@ -66,7 +85,7 @@ grub_tpm_verify_string (char *str, enum grub_verify_string_type type)
>      }
>    description = grub_malloc (grub_strlen (str) + grub_strlen (prefix) + 1);
>    if (!description)
> -    return grub_errno;
> +    return GRUB_ERR_NONE;

This change seems wrong to me. I think it should be dropped.

Daniel