[PATCH v2] efi_loader: Fix EFI_VARIABLE_APPEND_WRITE hash check

Heinrich Schuchardt xypron.glpk at gmx.de
Fri May 10 11:23:01 CEST 2024


On 5/8/24 13:13, Weizhao Ouyang wrote:
> According to UEFI v2.10 spec section 8.2.6, if a caller invokes the
> SetVariables() service, it will produce a digest from hash(VariableName,
> VendorGuid, Attributes, TimeStamp, DataNew_variable_content), then the
> firmware that implements the SetVariable() service will compare the
> digest with the result of applying the signer’s public key to the
> signature. For EFI variable append write, efitools sign-efi-sig-list has
> an option "-a" to add EFI_VARIABLE_APPEND_WRITE attr, and u-boot will
> drop this attribute in efi_set_variable_int(). So if a caller uses
> "sign-efi-sig-list -a" to create the authenticated variable, this append
> write will fail in the u-boot due to "hash check failed".
>
> This patch resumes writing the EFI_VARIABLE_APPEND_WRITE attr to ensure
> that the hash check is correct. And also update the "test_efi_secboot"
> test case to compliance with the change.

Looking at EDK II I see that VerifyTimeBasedPayloadAndUpdate() calls
VerifyTimeBasedPayload() before invoking
AuthServiceInternalUpdateVariableWithTimeStamp() which handles the
append flag so this seems to match the intended behavior that you describe.

Should we move the IS_ENABLED(CONFIG_EFI_SECURE_BOOT) check to
efi_variable_authenticate() and call this function directly after
setvariable_allowed() if
EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS is set?

This would make efi_set_variable_int() easier to read and would simplify
the handling of the attribute flag.

The UEFI Self-Certification Test (SCT) never highlighted the issue. If a
test is missing there,it would be a good idea to add an issue in
https://bugzilla.tianocore.org/.

Best regards

Heinrich

>
> Signed-off-by: Weizhao Ouyang <o451686892 at gmail.com>
> ---
> v2: skip attr for efi_var_mem_ins()
> ---
>   lib/efi_loader/efi_variable.c                  |  6 +++---
>   test/py/tests/test_efi_secboot/conftest.py     | 10 ++++++++++
>   test/py/tests/test_efi_secboot/test_authvar.py |  4 ++--
>   test/py/tests/test_efi_secboot/test_signed.py  | 10 +++++-----
>   4 files changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> index 1cc02acb3b..09651d4675 100644
> --- a/lib/efi_loader/efi_variable.c
> +++ b/lib/efi_loader/efi_variable.c
> @@ -288,7 +288,6 @@ efi_status_t efi_set_variable_int(const u16 *variable_name,
>   	/* check if a variable exists */
>   	var = efi_var_mem_find(vendor, variable_name, NULL);
>   	append = !!(attributes & EFI_VARIABLE_APPEND_WRITE);
> -	attributes &= ~EFI_VARIABLE_APPEND_WRITE;
>   	delete = !append && (!data_size || !attributes);
>
>   	/* check attributes */
> @@ -304,7 +303,7 @@ efi_status_t efi_set_variable_int(const u16 *variable_name,
>
>   		/* attributes won't be changed */
>   		if (!delete &&
> -		    ((ro_check && var->attr != attributes) ||
> +		    ((ro_check && var->attr != (attributes & ~EFI_VARIABLE_APPEND_WRITE)) ||
>   		     (!ro_check && ((var->attr & ~EFI_VARIABLE_READ_ONLY)
>   				    != (attributes & ~EFI_VARIABLE_READ_ONLY))))) {
>   			return EFI_INVALID_PARAMETER;
> @@ -378,7 +377,8 @@ efi_status_t efi_set_variable_int(const u16 *variable_name,
>   		for (; *old_data; ++old_data)
>   			;
>   		++old_data;
> -		ret = efi_var_mem_ins(variable_name, vendor, attributes,
> +		ret = efi_var_mem_ins(variable_name, vendor,
> +				      attributes & ~EFI_VARIABLE_APPEND_WRITE,
>   				      var->length, old_data, data_size, data,
>   				      time);
>   	} else {
> diff --git a/test/py/tests/test_efi_secboot/conftest.py b/test/py/tests/test_efi_secboot/conftest.py
> index ff7ac7c810..0fa0747fc7 100644
> --- a/test/py/tests/test_efi_secboot/conftest.py
> +++ b/test/py/tests/test_efi_secboot/conftest.py
> @@ -64,6 +64,12 @@ def efi_boot_env(request, u_boot_config):
>           check_call('cd %s; %scert-to-efi-sig-list -g %s db1.crt db1.esl; %ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key db db1.esl db1.auth'
>                      % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH),
>                      shell=True)
> +        # db2 (APPEND_WRITE)
> +        check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_db2/ -keyout db2.key -out db2.crt -nodes -days 365'
> +                   % mnt_point, shell=True)
> +        check_call('cd %s; %scert-to-efi-sig-list -g %s db2.crt db2.esl; %ssign-efi-sig-list -a -c KEK.crt -k KEK.key db db2.esl db2.auth'
> +                   % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH),
> +                   shell=True)
>           # dbx (TEST_dbx certificate)
>           check_call('cd %s; openssl req -x509 -sha256 -newkey rsa:2048 -subj /CN=TEST_dbx/ -keyout dbx.key -out dbx.crt -nodes -days 365'
>                      % mnt_point, shell=True)
> @@ -84,6 +90,10 @@ def efi_boot_env(request, u_boot_config):
>           check_call('cd %s; %scert-to-efi-hash-list -g %s -s 256 db1.crt dbx_hash1.crl; %ssign-efi-sig-list -t "2020-04-06" -c KEK.crt -k KEK.key dbx dbx_hash1.crl dbx_hash1.auth'
>                      % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH),
>                      shell=True)
> +        # dbx_hash2 (digest of TEST_db2 certificate, with APPEND_WRITE)
> +        check_call('cd %s; %scert-to-efi-hash-list -g %s -s 256 db2.crt dbx_hash2.crl; %ssign-efi-sig-list -a -c KEK.crt -k KEK.key dbx dbx_hash2.crl dbx_hash2.auth'
> +                   % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH),
> +                   shell=True)
>           # dbx_db (with TEST_db certificate)
>           check_call('cd %s; %ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key dbx db.esl dbx_db.auth'
>                      % (mnt_point, EFITOOLS_PATH),
> diff --git a/test/py/tests/test_efi_secboot/test_authvar.py b/test/py/tests/test_efi_secboot/test_authvar.py
> index f99b8270a6..d5aeb65048 100644
> --- a/test/py/tests/test_efi_secboot/test_authvar.py
> +++ b/test/py/tests/test_efi_secboot/test_authvar.py
> @@ -183,7 +183,7 @@ class TestEfiAuthVar(object):
>               assert 'db:' in ''.join(output)
>
>               output = u_boot_console.run_command_list([
> -                'fatload host 0:1 4000000 db1.auth',
> +                'fatload host 0:1 4000000 db2.auth',
>                   'setenv -e -nv -bs -rt -a -i 4000000:$filesize db'])
>               assert 'Failed to set EFI variable' in ''.join(output)
>
> @@ -197,7 +197,7 @@ class TestEfiAuthVar(object):
>           with u_boot_console.log.section('Test Case 3c'):
>               # Test Case 3c, update with correct signature
>               output = u_boot_console.run_command_list([
> -                'fatload host 0:1 4000000 db1.auth',
> +                'fatload host 0:1 4000000 db2.auth',
>                   'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db',
>                   'printenv -e -n -guid d719b2cb-3d3a-4596-a3bc-dad00e67656f db'])
>               assert 'Failed to set EFI variable' not in ''.join(output)
> diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py
> index 5000a4ab7b..f604138a35 100644
> --- a/test/py/tests/test_efi_secboot/test_signed.py
> +++ b/test/py/tests/test_efi_secboot/test_signed.py
> @@ -177,7 +177,7 @@ class TestEfiSignedImage(object):
>           with u_boot_console.log.section('Test Case 5b'):
>               # Test Case 5b, authenticated if both signatures are verified
>               output = u_boot_console.run_command_list([
> -                'fatload host 0:1 4000000 db1.auth',
> +                'fatload host 0:1 4000000 db2.auth',
>                   'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db'])
>               assert 'Failed to set EFI variable' not in ''.join(output)
>               output = u_boot_console.run_command_list([
> @@ -201,7 +201,7 @@ class TestEfiSignedImage(object):
>           with u_boot_console.log.section('Test Case 5d'):
>               # Test Case 5d, rejected if both of signatures are revoked
>               output = u_boot_console.run_command_list([
> -                'fatload host 0:1 4000000 dbx_hash1.auth',
> +                'fatload host 0:1 4000000 dbx_hash2.auth',
>                   'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize dbx'])
>               assert 'Failed to set EFI variable' not in ''.join(output)
>               output = u_boot_console.run_command_list([
> @@ -223,7 +223,7 @@ class TestEfiSignedImage(object):
>                   'setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK',
>                   'fatload host 0:1 4000000 PK.auth',
>                   'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK',
> -                'fatload host 0:1 4000000 db1.auth',
> +                'fatload host 0:1 4000000 db2.auth',
>                   'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db',
>                   'fatload host 0:1 4000000 dbx_hash1.auth',
>                   'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx'])
> @@ -300,7 +300,7 @@ class TestEfiSignedImage(object):
>                   'setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK',
>                   'fatload host 0:1 4000000 PK.auth',
>                   'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK',
> -                'fatload host 0:1 4000000 db1.auth',
> +                'fatload host 0:1 4000000 db2.auth',
>                   'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db',
>                   'fatload host 0:1 4000000 dbx_hash384.auth',
>                   'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx'])
> @@ -323,7 +323,7 @@ class TestEfiSignedImage(object):
>                   'setenv -e -nv -bs -rt -at -i 4000000:$filesize KEK',
>                   'fatload host 0:1 4000000 PK.auth',
>                   'setenv -e -nv -bs -rt -at -i 4000000:$filesize PK',
> -                'fatload host 0:1 4000000 db1.auth',
> +                'fatload host 0:1 4000000 db2.auth',
>                   'setenv -e -nv -bs -rt -at -a -i 4000000:$filesize db',
>                   'fatload host 0:1 4000000 dbx_hash512.auth',
>                   'setenv -e -nv -bs -rt -at -i 4000000:$filesize dbx'])



More information about the U-Boot mailing list