From: Weizhao Ouyang <o451686892@gmail.com>
To: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>,
Tom Rini <trini@konsulko.com>,
Masahisa Kojima <kojima.masahisa@socionext.com>,
Simon Glass <sjg@chromium.org>, Qu Wenruo <wqu@suse.com>,
Neil Armstrong <neil.armstrong@linaro.org>,
Stefan Roese <sr@denx.de>, Michal Simek <michal.simek@amd.com>,
Joao Marcos Costa <jmcosta944@gmail.com>,
Raymond Mao <raymond.mao@linaro.org>,
u-boot@lists.denx.de
Subject: Re: [PATCH v2] efi_loader: Fix EFI_VARIABLE_APPEND_WRITE hash check
Date: Mon, 13 May 2024 13:48:18 +0800 [thread overview]
Message-ID: <CAHk0HouYxX9W+0VK=zwR+aL8U=y50mUZoKctex-f6FN-DpRaEQ@mail.gmail.com> (raw)
In-Reply-To: <7dfd7a94-b256-437e-97e6-1841c7d53532@gmx.de>
On Fri, May 10, 2024 at 5:23 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> 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.
Yeah, I think it's reasonable.
BR,
Weizhao
>
> 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@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'])
>
prev parent reply other threads:[~2024-05-13 5:48 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-08 11:13 [PATCH v2] efi_loader: Fix EFI_VARIABLE_APPEND_WRITE hash check Weizhao Ouyang
2024-05-10 9:23 ` Heinrich Schuchardt
2024-05-13 5:48 ` Weizhao Ouyang [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAHk0HouYxX9W+0VK=zwR+aL8U=y50mUZoKctex-f6FN-DpRaEQ@mail.gmail.com' \
--to=o451686892@gmail.com \
--cc=ilias.apalodimas@linaro.org \
--cc=jmcosta944@gmail.com \
--cc=kojima.masahisa@socionext.com \
--cc=michal.simek@amd.com \
--cc=neil.armstrong@linaro.org \
--cc=raymond.mao@linaro.org \
--cc=sjg@chromium.org \
--cc=sr@denx.de \
--cc=trini@konsulko.com \
--cc=u-boot@lists.denx.de \
--cc=wqu@suse.com \
--cc=xypron.glpk@gmx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).