[RFC PATCH] efi_loader: Fix EFI_VARIABLE_APPEND_WRITE hash check
Weizhao Ouyang
o451686892 at gmail.com
Wed Apr 10 14:27:43 CEST 2024
On Wed, Apr 10, 2024 at 8:09 PM Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 10.04.24 13:53, Weizhao Ouyang wrote:
> > On Thu, Apr 4, 2024 at 1:48 AM Weizhao Ouyang <o451686892 at gmail.com> wrote:
> >>
> >> Hi Heinrich,
> >>
> >> On Wed, Apr 3, 2024 at 10:54 PM Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >>>
> >>> On 27.03.24 15:06, 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".
> >>>
> >>> Currently U-Boot does not allow to use EFI_VARIABLE_APPEND_WRITE for any
> >>> non-existent variable, signed or not. This does not match the EDK II
> >>> behavior.
> >>
> >> I'm not referring to the non-existent variable situation, it's a normal
> >> existent variable update.
> >>
> >> I mean:
> >> 1. process PK
> >> 2. process KEK
> >> 3. process a new APPEND_WRITE KEK (came from "sign-efi-sig-list -a ...",
> >> so the digest and attrtibute of the KEK auth file will contain
> >> APPEND_WRITE)
> >> Then the hash check will fail.
> >>
> >>>
> >>> There is a patch queued for this issue:
> >>>
> >>> [PATCH v2] efi_loader: fix append write behavior to non-existent variable
> >>> https://lore.kernel.org/u-boot/20240402090950.3819705-1-kojima.masahisa@socionext.com/
> >>>
> >>> Could you, please, retest with that patch.
> >>>
> >>>>
> >>>> 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.
> >>>>
> >>>> Signed-off-by: Weizhao Ouyang <o451686892 at gmail.com>
> >>>> ---
> >>>> lib/efi_loader/efi_variable.c | 3 +--
> >>>> 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, 18 insertions(+), 9 deletions(-)
> >>>>
> >>>> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> >>>> index 40f7a0fb10..f41aa8f9ad 100644
> >>>> --- a/lib/efi_loader/efi_variable.c
> >>>> +++ b/lib/efi_loader/efi_variable.c
> >>>> @@ -259,7 +259,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 &= ~(u32)EFI_VARIABLE_APPEND_WRITE;
> >>>
> >>> According to the UEFI specification for GetVariable():
> >>>
> >>> "The EFI_VARIABLE_APPEND_WRITE attribute will never be set in the
> >>> returned Attributes bitmask parameter."
> >>
> >> Yes, that's why I called it a RFC patch.
> >>
> >>>
> >>> We don't want to set EFI_VARIABLE_APPEND_WRITE in a variable when
> >>> calling efi_var_mem_ins().
> >>
> >> The APPEND_WRITE attribute is allowed to be set prior to invoking the
> >> service (UEFI section 8.2.6). So anyway we should consider compatibility
> >> with this case, along with making changes to GetVariable().
> >
> > Part of the process in edk2 is:
> >
> > VerifyTimeBasedPayloadAndUpdate
> > --> VerifyTimeBasedPayload // calculate digest and compare digest
> > --> AuthServiceInternalUpdateVariableWithTimeStamp
> > --> VariableExLibUpdateVariable
> > --> UpdateVariable // unset the APPEND_WRITE
> >
> > We can align with this implementation.
>
> I was referring to the attribute value passed to efi_var_mem_ins(), not
> to the value passed to SetVariable().
>
> efi_var_mem_ins() does not remove the EFI_VARIABLE_APPEND_WRITE bit. The
> code calling efi_var_mem_ins() should not pass such a value.
Yeah, efi_var_mem_ins() should escape from this bit.
Could we temporarily unset this attr here?
BR,
Weizhao
>
> Best regards
>
> Heinrich
>
> >
> > BR,
> > Weizhao
> >
> >>
> >> BR,
> >> Weizhao
> >>
> >>>
> >>>> delete = !append && (!data_size || !attributes);
> >>>>
> >>>> /* check attributes */
> >>>> @@ -275,7 +274,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 & ~(u32)EFI_VARIABLE_APPEND_WRITE)) ||
> >>>> (!ro_check && ((var->attr & ~(u32)EFI_VARIABLE_READ_ONLY)
> >>>
> >>> The (u32) conversions are superfluous.
> >>>
> >>> Best regards
> >>>
> >>> Heinrich
> >>>
> >>>> != (attributes & ~(u32)EFI_VARIABLE_READ_ONLY))))) {
> >>>> return EFI_INVALID_PARAMETER;
> >>>> 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 2f862a259a..34719ad1f2 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