[PATCH v3 0/3] efi_loader: secure boot using preseed cert db
When implementing secure boot the database with the certificates must be stored in tamper-resistant storage. This implies it cannot be read from a file on the EFI system partition.
We already have the possibility to provide UEFI variables built into U-Boot via CONFIG_EFI_VAR_SEED_FILE. If TF-A validates BL33 alias U-Boot, this seems adequate for secure boot.
With the patch series reading or changing the certificate database is disabled. Furthermore the variable AuditMode and DeployedMode cannot be read from file.
The series has been split of [PATCH v2 0/6] efi_loader: fix secure boot mode transitions https://lists.denx.de/pipermail/u-boot/2021-August/459054.html because the implementation of Secure Boot mode transitions need more thought.
Heinrich Schuchardt (3): efi_loader: don't load signature database from file efi_loader: efi_auth_var_type for AuditMode, DeployedMode efi_loader: correct determination of secure boot state
include/efi_variable.h | 6 ++++- lib/efi_loader/efi_var_common.c | 43 +++++++++++++++++++++++++-------- lib/efi_loader/efi_var_file.c | 41 +++++++++++++++++++------------ lib/efi_loader/efi_variable.c | 6 ++--- 4 files changed, 66 insertions(+), 30 deletions(-)
The UEFI specification requires that the signature database may only be stored in tamper-resistant storage. So these variable may not be read from an unsigned file.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- include/efi_variable.h | 5 +++- lib/efi_loader/efi_var_common.c | 2 -- lib/efi_loader/efi_var_file.c | 41 ++++++++++++++++++++------------- lib/efi_loader/efi_variable.c | 2 +- 4 files changed, 30 insertions(+), 20 deletions(-)
diff --git a/include/efi_variable.h b/include/efi_variable.h index 4623a64142..2d97655e1f 100644 --- a/include/efi_variable.h +++ b/include/efi_variable.h @@ -161,10 +161,13 @@ efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t * /** * efi_var_restore() - restore EFI variables from buffer * + * Only if @safe is set secure boot related variables will be restored. + * * @buf: buffer + * @safe: restoring from tamper-resistant storage * Return: status code */ -efi_status_t efi_var_restore(struct efi_var_file *buf); +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe);
/** * efi_var_from_file() - read variables from file diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c index 3d92afe2eb..005c03ea5f 100644 --- a/lib/efi_loader/efi_var_common.c +++ b/lib/efi_loader/efi_var_common.c @@ -32,10 +32,8 @@ static const struct efi_auth_var_name_type name_type[] = { {u"KEK", &efi_global_variable_guid, EFI_AUTH_VAR_KEK}, {u"db", &efi_guid_image_security_database, EFI_AUTH_VAR_DB}, {u"dbx", &efi_guid_image_security_database, EFI_AUTH_VAR_DBX}, - /* not used yet {u"dbt", &efi_guid_image_security_database, EFI_AUTH_VAR_DBT}, {u"dbr", &efi_guid_image_security_database, EFI_AUTH_VAR_DBR}, - */ };
static bool efi_secure_boot; diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c index de076b8cbc..c7c6805ed0 100644 --- a/lib/efi_loader/efi_var_file.c +++ b/lib/efi_loader/efi_var_file.c @@ -148,9 +148,10 @@ error: #endif }
-efi_status_t efi_var_restore(struct efi_var_file *buf) +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe) { struct efi_var_entry *var, *last_var; + u16 *data; efi_status_t ret;
if (buf->reserved || buf->magic != EFI_VAR_FILE_MAGIC || @@ -160,21 +161,29 @@ efi_status_t efi_var_restore(struct efi_var_file *buf) return EFI_INVALID_PARAMETER; }
- var = buf->var; last_var = (struct efi_var_entry *)((u8 *)buf + buf->length); - while (var < last_var) { - u16 *data = var->name + u16_strlen(var->name) + 1; - - if (var->attr & EFI_VARIABLE_NON_VOLATILE && var->length) { - ret = efi_var_mem_ins(var->name, &var->guid, var->attr, - var->length, data, 0, NULL, - var->time); - if (ret != EFI_SUCCESS) - log_err("Failed to set EFI variable %ls\n", - var->name); - } - var = (struct efi_var_entry *) - ALIGN((uintptr_t)data + var->length, 8); + for (var = buf->var; var < last_var; + var = (struct efi_var_entry *) + ALIGN((uintptr_t)data + var->length, 8)) { + + data = var->name + u16_strlen(var->name) + 1; + + /* + * Secure boot related and non-volatile variables shall only be + * restored from U-Boot's preseed. + */ + if (!safe && + (efi_auth_var_get_type(var->name, &var->guid) != + EFI_AUTH_VAR_NONE || + !(var->attr & EFI_VARIABLE_NON_VOLATILE))) + continue; + if (!var->length) + continue; + ret = efi_var_mem_ins(var->name, &var->guid, var->attr, + var->length, data, 0, NULL, + var->time); + if (ret != EFI_SUCCESS) + log_err("Failed to set EFI variable %ls\n", var->name); } return EFI_SUCCESS; } @@ -213,7 +222,7 @@ efi_status_t efi_var_from_file(void) log_err("Failed to load EFI variables\n"); goto error; } - if (buf->length != len || efi_var_restore(buf) != EFI_SUCCESS) + if (buf->length != len || efi_var_restore(buf, false) != EFI_SUCCESS) log_err("Invalid EFI variables file\n"); error: free(buf); diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index ba0874e9e7..a7d305ffbc 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -426,7 +426,7 @@ efi_status_t efi_init_variables(void)
if (IS_ENABLED(CONFIG_EFI_VARIABLES_PRESEED)) { ret = efi_var_restore((struct efi_var_file *) - __efi_var_file_begin); + __efi_var_file_begin, true); if (ret != EFI_SUCCESS) log_err("Invalid EFI variable seed\n"); }
Heinrich,
On Thu, Sep 02, 2021 at 11:35:29AM +0200, Heinrich Schuchardt wrote:
The UEFI specification requires that the signature database may only be stored in tamper-resistant storage. So these variable may not be read from an unsigned file.
Even with TF-A (or other methods) assumed, I think the behavior here is too restrictive.
If you think TF-A provides the base for "chain of trust", all what you need to do is to protect PK and all the other auth variables should be allowed to be restored even from an unsafe file because the changes of values will be verified anyway by UEFI system as long as SetVariable() is used.
Please think about why UEFI specification defines both PK and KEK. The ability of setting/modifying KEK will add more flexibility of system configuration.
-Takahiro Akashi
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
include/efi_variable.h | 5 +++- lib/efi_loader/efi_var_common.c | 2 -- lib/efi_loader/efi_var_file.c | 41 ++++++++++++++++++++------------- lib/efi_loader/efi_variable.c | 2 +- 4 files changed, 30 insertions(+), 20 deletions(-)
diff --git a/include/efi_variable.h b/include/efi_variable.h index 4623a64142..2d97655e1f 100644 --- a/include/efi_variable.h +++ b/include/efi_variable.h @@ -161,10 +161,13 @@ efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t * /**
- efi_var_restore() - restore EFI variables from buffer
- Only if @safe is set secure boot related variables will be restored.
- @buf: buffer
*/
- @safe: restoring from tamper-resistant storage
- Return: status code
-efi_status_t efi_var_restore(struct efi_var_file *buf); +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe);
/**
- efi_var_from_file() - read variables from file
diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c index 3d92afe2eb..005c03ea5f 100644 --- a/lib/efi_loader/efi_var_common.c +++ b/lib/efi_loader/efi_var_common.c @@ -32,10 +32,8 @@ static const struct efi_auth_var_name_type name_type[] = { {u"KEK", &efi_global_variable_guid, EFI_AUTH_VAR_KEK}, {u"db", &efi_guid_image_security_database, EFI_AUTH_VAR_DB}, {u"dbx", &efi_guid_image_security_database, EFI_AUTH_VAR_DBX},
- /* not used yet {u"dbt", &efi_guid_image_security_database, EFI_AUTH_VAR_DBT}, {u"dbr", &efi_guid_image_security_database, EFI_AUTH_VAR_DBR},
- */
};
static bool efi_secure_boot; diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c index de076b8cbc..c7c6805ed0 100644 --- a/lib/efi_loader/efi_var_file.c +++ b/lib/efi_loader/efi_var_file.c @@ -148,9 +148,10 @@ error: #endif }
-efi_status_t efi_var_restore(struct efi_var_file *buf) +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe) { struct efi_var_entry *var, *last_var;
u16 *data; efi_status_t ret;
if (buf->reserved || buf->magic != EFI_VAR_FILE_MAGIC ||
@@ -160,21 +161,29 @@ efi_status_t efi_var_restore(struct efi_var_file *buf) return EFI_INVALID_PARAMETER; }
- var = buf->var; last_var = (struct efi_var_entry *)((u8 *)buf + buf->length);
- while (var < last_var) {
u16 *data = var->name + u16_strlen(var->name) + 1;if (var->attr & EFI_VARIABLE_NON_VOLATILE && var->length) {ret = efi_var_mem_ins(var->name, &var->guid, var->attr,var->length, data, 0, NULL,var->time);if (ret != EFI_SUCCESS)log_err("Failed to set EFI variable %ls\n",var->name);}var = (struct efi_var_entry *)ALIGN((uintptr_t)data + var->length, 8);
- for (var = buf->var; var < last_var;
var = (struct efi_var_entry *)ALIGN((uintptr_t)data + var->length, 8)) {data = var->name + u16_strlen(var->name) + 1;/** Secure boot related and non-volatile variables shall only be* restored from U-Boot's preseed.*/if (!safe &&(efi_auth_var_get_type(var->name, &var->guid) !=EFI_AUTH_VAR_NONE ||!(var->attr & EFI_VARIABLE_NON_VOLATILE)))continue;if (!var->length)continue;ret = efi_var_mem_ins(var->name, &var->guid, var->attr,var->length, data, 0, NULL,var->time);if (ret != EFI_SUCCESS)log_err("Failed to set EFI variable %ls\n", var->name);} @@ -213,7 +222,7 @@ efi_status_t efi_var_from_file(void) log_err("Failed to load EFI variables\n"); goto error; }
- if (buf->length != len || efi_var_restore(buf) != EFI_SUCCESS)
- if (buf->length != len || efi_var_restore(buf, false) != EFI_SUCCESS) log_err("Invalid EFI variables file\n");
error: free(buf); diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index ba0874e9e7..a7d305ffbc 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -426,7 +426,7 @@ efi_status_t efi_init_variables(void)
if (IS_ENABLED(CONFIG_EFI_VARIABLES_PRESEED)) { ret = efi_var_restore((struct efi_var_file *)
__efi_var_file_begin);
__efi_var_file_begin, true);-- 2.32.0
On 9/6/21 2:12 AM, AKASHI Takahiro wrote:
Heinrich,
On Thu, Sep 02, 2021 at 11:35:29AM +0200, Heinrich Schuchardt wrote:
The UEFI specification requires that the signature database may only be stored in tamper-resistant storage. So these variable may not be read from an unsigned file.
Even with TF-A (or other methods) assumed, I think the behavior here is too restrictive.
If you think TF-A provides the base for "chain of trust", all what you need to do is to protect PK and all the other auth variables should be allowed to be restored even from an unsafe file because the changes of values will be verified anyway by UEFI system as long as SetVariable() is used.
Please think about why UEFI specification defines both PK and KEK. The ability of setting/modifying KEK will add more flexibility of system configuration.
If relying on only PK being fixed, one would still have to get the sequence of import from file right: PK -> KEK -> db,dbx,dbt,dbr.
This would require a change in the file loader (efi_var_restore()). Could you contribute such a patch?
Best regards
Heinrich
-Takahiro Akashi
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com
include/efi_variable.h | 5 +++- lib/efi_loader/efi_var_common.c | 2 -- lib/efi_loader/efi_var_file.c | 41 ++++++++++++++++++++------------- lib/efi_loader/efi_variable.c | 2 +- 4 files changed, 30 insertions(+), 20 deletions(-)
diff --git a/include/efi_variable.h b/include/efi_variable.h index 4623a64142..2d97655e1f 100644 --- a/include/efi_variable.h +++ b/include/efi_variable.h @@ -161,10 +161,13 @@ efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t * /**
- efi_var_restore() - restore EFI variables from buffer
- Only if @safe is set secure boot related variables will be restored.
- @buf: buffer
*/
- @safe: restoring from tamper-resistant storage
- Return: status code
-efi_status_t efi_var_restore(struct efi_var_file *buf); +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe);
/**
- efi_var_from_file() - read variables from file
diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c index 3d92afe2eb..005c03ea5f 100644 --- a/lib/efi_loader/efi_var_common.c +++ b/lib/efi_loader/efi_var_common.c @@ -32,10 +32,8 @@ static const struct efi_auth_var_name_type name_type[] = { {u"KEK", &efi_global_variable_guid, EFI_AUTH_VAR_KEK}, {u"db", &efi_guid_image_security_database, EFI_AUTH_VAR_DB}, {u"dbx", &efi_guid_image_security_database, EFI_AUTH_VAR_DBX},
/* not used yet {u"dbt", &efi_guid_image_security_database, EFI_AUTH_VAR_DBT}, {u"dbr", &efi_guid_image_security_database, EFI_AUTH_VAR_DBR},
*/ };
static bool efi_secure_boot;
diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c index de076b8cbc..c7c6805ed0 100644 --- a/lib/efi_loader/efi_var_file.c +++ b/lib/efi_loader/efi_var_file.c @@ -148,9 +148,10 @@ error: #endif }
-efi_status_t efi_var_restore(struct efi_var_file *buf) +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe) { struct efi_var_entry *var, *last_var;
u16 *data; efi_status_t ret;
if (buf->reserved || buf->magic != EFI_VAR_FILE_MAGIC ||
@@ -160,21 +161,29 @@ efi_status_t efi_var_restore(struct efi_var_file *buf) return EFI_INVALID_PARAMETER; }
- var = buf->var; last_var = (struct efi_var_entry *)((u8 *)buf + buf->length);
- while (var < last_var) {
u16 *data = var->name + u16_strlen(var->name) + 1;if (var->attr & EFI_VARIABLE_NON_VOLATILE && var->length) {ret = efi_var_mem_ins(var->name, &var->guid, var->attr,var->length, data, 0, NULL,var->time);if (ret != EFI_SUCCESS)log_err("Failed to set EFI variable %ls\n",var->name);}var = (struct efi_var_entry *)ALIGN((uintptr_t)data + var->length, 8);
- for (var = buf->var; var < last_var;
var = (struct efi_var_entry *)ALIGN((uintptr_t)data + var->length, 8)) {data = var->name + u16_strlen(var->name) + 1;/** Secure boot related and non-volatile variables shall only be* restored from U-Boot's preseed.*/if (!safe &&(efi_auth_var_get_type(var->name, &var->guid) !=EFI_AUTH_VAR_NONE ||!(var->attr & EFI_VARIABLE_NON_VOLATILE)))continue;if (!var->length)continue;ret = efi_var_mem_ins(var->name, &var->guid, var->attr,var->length, data, 0, NULL,var->time);if (ret != EFI_SUCCESS)log_err("Failed to set EFI variable %ls\n", var->name);@@ -213,7 +222,7 @@ efi_status_t efi_var_from_file(void) log_err("Failed to load EFI variables\n"); goto error; }
- if (buf->length != len || efi_var_restore(buf) != EFI_SUCCESS)
- if (buf->length != len || efi_var_restore(buf, false) != EFI_SUCCESS) log_err("Invalid EFI variables file\n"); error: free(buf);
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index ba0874e9e7..a7d305ffbc 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -426,7 +426,7 @@ efi_status_t efi_init_variables(void)
if (IS_ENABLED(CONFIG_EFI_VARIABLES_PRESEED)) { ret = efi_var_restore((struct efi_var_file *)
__efi_var_file_begin);
__efi_var_file_begin, true);-- 2.32.0
Writing variables AuditMode and DeployedMode serves to switch between Secure Boot modes. Provide a separate value for these in efi_auth_var_type.
With this patch the variables will not be read from from file even if they are marked as non-volatile by mistake.
Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com --- include/efi_variable.h | 1 + lib/efi_loader/efi_var_common.c | 2 ++ lib/efi_loader/efi_variable.c | 4 ++-- 3 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/include/efi_variable.h b/include/efi_variable.h index 2d97655e1f..0440d356bc 100644 --- a/include/efi_variable.h +++ b/include/efi_variable.h @@ -12,6 +12,7 @@
enum efi_auth_var_type { EFI_AUTH_VAR_NONE = 0, + EFI_AUTH_MODE, EFI_AUTH_VAR_PK, EFI_AUTH_VAR_KEK, EFI_AUTH_VAR_DB, diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c index 005c03ea5f..c744e2fd91 100644 --- a/lib/efi_loader/efi_var_common.c +++ b/lib/efi_loader/efi_var_common.c @@ -34,6 +34,8 @@ static const struct efi_auth_var_name_type name_type[] = { {u"dbx", &efi_guid_image_security_database, EFI_AUTH_VAR_DBX}, {u"dbt", &efi_guid_image_security_database, EFI_AUTH_VAR_DBT}, {u"dbr", &efi_guid_image_security_database, EFI_AUTH_VAR_DBR}, + {u"AuditMode", &efi_global_variable_guid, EFI_AUTH_MODE}, + {u"DeployedMode", &efi_global_variable_guid, EFI_AUTH_MODE}, };
static bool efi_secure_boot; diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c index a7d305ffbc..fa2b6bc7a8 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -247,7 +247,7 @@ efi_status_t efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor, return EFI_WRITE_PROTECTED;
if (IS_ENABLED(CONFIG_EFI_VARIABLES_PRESEED)) { - if (var_type != EFI_AUTH_VAR_NONE) + if (var_type >= EFI_AUTH_VAR_PK) return EFI_WRITE_PROTECTED; }
@@ -268,7 +268,7 @@ efi_status_t efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor, return EFI_NOT_FOUND; }
- if (var_type != EFI_AUTH_VAR_NONE) { + if (var_type >= EFI_AUTH_VAR_PK) { /* authentication is mandatory */ if (!(attributes & EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS)) {
From: Heinrich Schuchardt xypron.glpk@gmx.de
When U-Boot is started we have to use the existing variables to determine in which secure boot state we are.
* If a platform key PK is present and DeployedMode=1, we are in deployed mode. * If no platform key PK is present and AuditMode=1, we are in audit mode. * Otherwise if a platform key is present, we are in user mode. * Otherwise if no platform key is present, we are in setup mode.
Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de --- lib/efi_loader/efi_var_common.c | 39 ++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 8 deletions(-)
diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c index c744e2fd91..a00bbf1620 100644 --- a/lib/efi_loader/efi_var_common.c +++ b/lib/efi_loader/efi_var_common.c @@ -314,17 +314,40 @@ err:
efi_status_t efi_init_secure_state(void) { - enum efi_secure_mode mode = EFI_MODE_SETUP; + enum efi_secure_mode mode; u8 efi_vendor_keys = 0; - efi_uintn_t size = 0; + efi_uintn_t size; efi_status_t ret; - - ret = efi_get_variable_int(L"PK", &efi_global_variable_guid, - NULL, &size, NULL, NULL); - if (ret == EFI_BUFFER_TOO_SMALL) { - if (IS_ENABLED(CONFIG_EFI_SECURE_BOOT)) - mode = EFI_MODE_USER; + u8 deployed_mode = 0; + u8 audit_mode = 0; + u8 setup_mode = 1; + + if (IS_ENABLED(CONFIG_EFI_SECURE_BOOT)) { + size = sizeof(deployed_mode); + ret = efi_get_variable_int(u"DeployedMode", &efi_global_variable_guid, + NULL, &size, &deployed_mode, NULL); + size = sizeof(audit_mode); + ret = efi_get_variable_int(u"AuditMode", &efi_global_variable_guid, + NULL, &size, &audit_mode, NULL); + size = 0; + ret = efi_get_variable_int(u"PK", &efi_global_variable_guid, + NULL, &size, NULL, NULL); + if (ret == EFI_BUFFER_TOO_SMALL) { + setup_mode = 0; + audit_mode = 0; + } else { + setup_mode = 1; + deployed_mode = 0; + } } + if (deployed_mode) + mode = EFI_MODE_DEPLOYED; + else if (audit_mode) + mode = EFI_MODE_AUDIT; + else if (setup_mode) + mode = EFI_MODE_SETUP; + else + mode = EFI_MODE_USER;
ret = efi_transfer_secure_state(mode); if (ret != EFI_SUCCESS)
participants (3)
-
AKASHI Takahiro -
Heinrich Schuchardt -
Heinrich Schuchardt