* [PATCH 1/2] efi_loader: add sha384/512 on certificate revocation
@ 2022-04-18 18:07 Ilias Apalodimas
2022-04-18 18:07 ` [PATCH 2/2 v3] test/py: Add more test cases for rejecting an EFI image Ilias Apalodimas
2022-04-19 1:46 ` [PATCH 1/2] efi_loader: add sha384/512 on certificate revocation AKASHI Takahiro
0 siblings, 2 replies; 6+ messages in thread
From: Ilias Apalodimas @ 2022-04-18 18:07 UTC (permalink / raw)
To: xypron.glpk
Cc: takahiro.akashi, Stuart.Yoder, paul.liu, Ilias Apalodimas, u-boot
Currently we don't support sha384/512 for the X.509 certificate
in dbx. Moreover if we come across such a hash we skip the check
and approve the image, although the image might needs to be rejected.
Rework the code a bit and fix it by adding an array of structs with the
supported GUIDs, len and literal used in the U-Boot crypto APIs instead
of hardcoding the GUID types.
It's worth noting here that efi_hash_regions() can now be reused from
efi_signature_lookup_digest() and add sha348/512 support there as well
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
changes since v2:
- updated changelog (there was no v1)
changes since RFC:
- add an array of structs with the algo info info of a function
- checking hash_calculate result in efi_hash_regions()
include/efi_api.h | 6 +++
include/efi_loader.h | 7 +++
lib/efi_loader/efi_helper.c | 85 ++++++++++++++++++++++++++++++++++
lib/efi_loader/efi_signature.c | 76 +++++++++++++++++++++---------
4 files changed, 151 insertions(+), 23 deletions(-)
diff --git a/include/efi_api.h b/include/efi_api.h
index 982c2001728d..b9a04958f9ba 100644
--- a/include/efi_api.h
+++ b/include/efi_api.h
@@ -1873,6 +1873,12 @@ struct efi_system_resource_table {
#define EFI_CERT_X509_SHA256_GUID \
EFI_GUID(0x3bd2a492, 0x96c0, 0x4079, 0xb4, 0x20, \
0xfc, 0xf9, 0x8e, 0xf1, 0x03, 0xed)
+#define EFI_CERT_X509_SHA384_GUID \
+ EFI_GUID(0x7076876e, 0x80c2, 0x4ee6, \
+ 0xaa, 0xd2, 0x28, 0xb3, 0x49, 0xa6, 0x86, 0x5b)
+#define EFI_CERT_X509_SHA512_GUID \
+ EFI_GUID(0x446dbf63, 0x2502, 0x4cda, \
+ 0xbc, 0xfa, 0x24, 0x65, 0xd2, 0xb0, 0xfe, 0x9d)
#define EFI_CERT_TYPE_PKCS7_GUID \
EFI_GUID(0x4aafd29d, 0x68df, 0x49ee, 0x8a, 0xa9, \
0x34, 0x7d, 0x37, 0x56, 0x65, 0xa7)
diff --git a/include/efi_loader.h b/include/efi_loader.h
index af36639ec6a7..ce221ee9317b 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -300,6 +300,8 @@ extern const efi_guid_t efi_guid_image_security_database;
extern const efi_guid_t efi_guid_sha256;
extern const efi_guid_t efi_guid_cert_x509;
extern const efi_guid_t efi_guid_cert_x509_sha256;
+extern const efi_guid_t efi_guid_cert_x509_sha384;
+extern const efi_guid_t efi_guid_cert_x509_sha512;
extern const efi_guid_t efi_guid_cert_type_pkcs7;
/* GUID of RNG protocol */
@@ -671,6 +673,11 @@ efi_status_t efi_file_size(struct efi_file_handle *fh, efi_uintn_t *size);
/* get a device path from a Boot#### option */
struct efi_device_path *efi_get_dp_from_boot(const efi_guid_t guid);
+/* get len, string (used in u-boot crypto from a guid */
+const char *guid_to_sha_str(const efi_guid_t *guid);
+int guid_to_sha_len(const efi_guid_t *guid);
+int algo_to_len(const char *algo);
+
/**
* efi_size_in_pages() - convert size in bytes to size in pages
*
diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
index 802d39ed97b6..c186ba4a3c01 100644
--- a/lib/efi_loader/efi_helper.c
+++ b/lib/efi_loader/efi_helper.c
@@ -92,3 +92,88 @@ err:
free(var_value);
return NULL;
}
+
+const struct guid_to_hash_map {
+ efi_guid_t guid;
+ const char algo[32];
+ u32 bits;
+} guid_to_hash[] = {
+ {
+ EFI_CERT_X509_SHA256_GUID,
+ "sha256",
+ 256,
+ },
+ {
+ EFI_CERT_SHA256_GUID,
+ "sha256",
+ 256,
+ },
+ {
+ EFI_CERT_X509_SHA384_GUID,
+ "sha384",
+ 384,
+ },
+ {
+ EFI_CERT_X509_SHA512_GUID,
+ "sha512",
+ 512,
+ },
+};
+
+#define MAX_GUID_TO_HASH_COUNT ARRAY_SIZE(guid_to_hash)
+
+/** guid_to_sha_str - return the sha string e.g "sha256" for a given guid
+ * used on EFI security databases
+ *
+ * @guid: guid to check
+ *
+ * Return: len or 0 if no match is found
+ */
+const char *guid_to_sha_str(const efi_guid_t *guid)
+{
+ size_t i;
+
+ for (i = 0; i < MAX_GUID_TO_HASH_COUNT; i++) {
+ if (!guidcmp(guid, &guid_to_hash[i].guid))
+ return guid_to_hash[i].algo;
+ }
+
+ return NULL;
+}
+
+/** guid_to_sha_len - return the sha size in bytes for a given guid
+ * used on EFI security databases
+ *
+ * @guid: guid to check
+ *
+ * Return: len or 0 if no match is found
+ */
+int guid_to_sha_len(const efi_guid_t *guid)
+{
+ size_t i;
+
+ for (i = 0; i < MAX_GUID_TO_HASH_COUNT; i++) {
+ if (!guidcmp(guid, &guid_to_hash[i].guid))
+ return guid_to_hash[i].bits / 8;
+ }
+
+ return 0;
+}
+
+/** algo_to_len - return the sha size in bytes for a given string
+ *
+ * @guid: string to check
+ *
+ * Return: len or 0 if no match is found
+ */
+int algo_to_len(const char *algo)
+{
+ size_t i;
+
+ for (i = 0; i < MAX_GUID_TO_HASH_COUNT; i++) {
+ if (!strcmp(algo, guid_to_hash[i].algo))
+ return guid_to_hash[i].bits / 8;
+ }
+
+ return 0;
+}
diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
index 79ed077ae7dd..cf01f21b4d04 100644
--- a/lib/efi_loader/efi_signature.c
+++ b/lib/efi_loader/efi_signature.c
@@ -24,6 +24,8 @@ const efi_guid_t efi_guid_sha256 = EFI_CERT_SHA256_GUID;
const efi_guid_t efi_guid_cert_rsa2048 = EFI_CERT_RSA2048_GUID;
const efi_guid_t efi_guid_cert_x509 = EFI_CERT_X509_GUID;
const efi_guid_t efi_guid_cert_x509_sha256 = EFI_CERT_X509_SHA256_GUID;
+const efi_guid_t efi_guid_cert_x509_sha384 = EFI_CERT_X509_SHA384_GUID;
+const efi_guid_t efi_guid_cert_x509_sha512 = EFI_CERT_X509_SHA512_GUID;
const efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
static u8 pkcs7_hdr[] = {
@@ -124,23 +126,32 @@ struct pkcs7_message *efi_parse_pkcs7_header(const void *buf,
* Return: true on success, false on error
*/
static bool efi_hash_regions(struct image_region *regs, int count,
- void **hash, size_t *size)
+ void **hash, const char *hash_algo, int *len)
{
+ int ret;
+
+ if (!hash_algo || !len)
+ return false;
+
+ *len = algo_to_len(hash_algo);
+ if (!*len)
+ return false;
+
if (!*hash) {
- *hash = calloc(1, SHA256_SUM_LEN);
+ *hash = calloc(1, *len);
if (!*hash) {
EFI_PRINT("Out of memory\n");
return false;
}
}
- if (size)
- *size = SHA256_SUM_LEN;
- hash_calculate("sha256", regs, count, *hash);
+ ret = hash_calculate(hash_algo, regs, count, *hash);
+ if (ret)
+ return false;
#ifdef DEBUG
EFI_PRINT("hash calculated:\n");
print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1,
- *hash, SHA256_SUM_LEN, false);
+ *hash, *len, false);
#endif
return true;
@@ -190,7 +201,6 @@ bool efi_signature_lookup_digest(struct efi_image_regions *regs,
struct efi_signature_store *siglist;
struct efi_sig_data *sig_data;
void *hash = NULL;
- size_t size = 0;
bool found = false;
bool hash_done = false;
@@ -200,6 +210,8 @@ bool efi_signature_lookup_digest(struct efi_image_regions *regs,
goto out;
for (siglist = db; siglist; siglist = siglist->next) {
+ int len = 0;
+ const char *hash_algo = NULL;
/*
* if the hash algorithm is unsupported and we get an entry in
* dbx reject the image
@@ -215,8 +227,14 @@ bool efi_signature_lookup_digest(struct efi_image_regions *regs,
if (guidcmp(&siglist->sig_type, &efi_guid_sha256))
continue;
+ hash_algo = guid_to_sha_str(&efi_guid_sha256);
+ /*
+ * We could check size and hash_algo but efi_hash_regions()
+ * will do that for us
+ */
if (!hash_done &&
- !efi_hash_regions(regs->reg, regs->num, &hash, &size)) {
+ !efi_hash_regions(regs->reg, regs->num, &hash, hash_algo,
+ &len)) {
EFI_PRINT("Digesting an image failed\n");
break;
}
@@ -229,8 +247,8 @@ bool efi_signature_lookup_digest(struct efi_image_regions *regs,
print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1,
sig_data->data, sig_data->size, false);
#endif
- if (sig_data->size == size &&
- !memcmp(sig_data->data, hash, size)) {
+ if (sig_data->size == len &&
+ !memcmp(sig_data->data, hash, len)) {
found = true;
free(hash);
goto out;
@@ -263,8 +281,9 @@ static bool efi_lookup_certificate(struct x509_certificate *cert,
struct efi_sig_data *sig_data;
struct image_region reg[1];
void *hash = NULL, *hash_tmp = NULL;
- size_t size = 0;
+ int len = 0;
bool found = false;
+ const char *hash_algo = NULL;
EFI_PRINT("%s: Enter, %p, %p\n", __func__, cert, db);
@@ -278,7 +297,11 @@ static bool efi_lookup_certificate(struct x509_certificate *cert,
/* calculate hash of TBSCertificate */
reg[0].data = cert->tbs;
reg[0].size = cert->tbs_size;
- if (!efi_hash_regions(reg, 1, &hash, &size))
+
+ /* We just need any sha256 algo to start the matching */
+ hash_algo = guid_to_sha_str(&efi_guid_sha256);
+ len = guid_to_sha_len(&efi_guid_sha256);
+ if (!efi_hash_regions(reg, 1, &hash, hash_algo, &len))
goto out;
EFI_PRINT("%s: searching for %s\n", __func__, cert->subject);
@@ -290,6 +313,7 @@ static bool efi_lookup_certificate(struct x509_certificate *cert,
for (sig_data = siglist->sig_data_list; sig_data;
sig_data = sig_data->next) {
struct x509_certificate *cert_tmp;
+ int len_tmp;
cert_tmp = x509_cert_parse(sig_data->data,
sig_data->size);
@@ -300,12 +324,13 @@ static bool efi_lookup_certificate(struct x509_certificate *cert,
cert_tmp->subject);
reg[0].data = cert_tmp->tbs;
reg[0].size = cert_tmp->tbs_size;
- if (!efi_hash_regions(reg, 1, &hash_tmp, NULL))
+ if (!efi_hash_regions(reg, 1, &hash_tmp, hash_algo,
+ &len_tmp))
goto out;
x509_free_certificate(cert_tmp);
- if (!memcmp(hash, hash_tmp, size)) {
+ if (!memcmp(hash, hash_tmp, len)) {
found = true;
goto out;
}
@@ -400,9 +425,10 @@ static bool efi_signature_check_revocation(struct pkcs7_signed_info *sinfo,
struct efi_sig_data *sig_data;
struct image_region reg[1];
void *hash = NULL;
- size_t size = 0;
+ int len = 0;
time64_t revoc_time;
bool revoked = false;
+ const char *hash_algo = NULL;
EFI_PRINT("%s: Enter, %p, %p, %p\n", __func__, sinfo, cert, dbx);
@@ -411,13 +437,14 @@ static bool efi_signature_check_revocation(struct pkcs7_signed_info *sinfo,
EFI_PRINT("Checking revocation against %s\n", cert->subject);
for (siglist = dbx; siglist; siglist = siglist->next) {
- if (guidcmp(&siglist->sig_type, &efi_guid_cert_x509_sha256))
+ hash_algo = guid_to_sha_str(&siglist->sig_type);
+ if (!hash_algo)
continue;
/* calculate hash of TBSCertificate */
reg[0].data = cert->tbs;
reg[0].size = cert->tbs_size;
- if (!efi_hash_regions(reg, 1, &hash, &size))
+ if (!efi_hash_regions(reg, 1, &hash, hash_algo, &len))
goto out;
for (sig_data = siglist->sig_data_list; sig_data;
@@ -429,18 +456,18 @@ static bool efi_signature_check_revocation(struct pkcs7_signed_info *sinfo,
* };
*/
#ifdef DEBUG
- if (sig_data->size >= size) {
+ if (sig_data->size >= len) {
EFI_PRINT("hash in db:\n");
print_hex_dump(" ", DUMP_PREFIX_OFFSET,
16, 1,
- sig_data->data, size, false);
+ sig_data->data, len, false);
}
#endif
- if ((sig_data->size < size + sizeof(time64_t)) ||
- memcmp(sig_data->data, hash, size))
+ if ((sig_data->size < len + sizeof(time64_t)) ||
+ memcmp(sig_data->data, hash, len))
continue;
- memcpy(&revoc_time, sig_data->data + size,
+ memcpy(&revoc_time, sig_data->data + len,
sizeof(revoc_time));
EFI_PRINT("revocation time: 0x%llx\n", revoc_time);
/*
@@ -488,6 +515,7 @@ bool efi_signature_verify(struct efi_image_regions *regs,
goto out;
for (sinfo = msg->signed_infos; sinfo; sinfo = sinfo->next) {
+ int len;
EFI_PRINT("Signed Info: digest algo: %s, pkey algo: %s\n",
sinfo->sig->hash_algo, sinfo->sig->pkey_algo);
@@ -500,7 +528,9 @@ bool efi_signature_verify(struct efi_image_regions *regs,
*/
if (!msg->data &&
!efi_hash_regions(regs->reg, regs->num,
- (void **)&sinfo->sig->digest, NULL)) {
+ (void **)&sinfo->sig->digest,
+ guid_to_sha_str(&efi_guid_sha256),
+ &len)) {
EFI_PRINT("Digesting an image failed\n");
goto out;
}
--
2.32.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2 v3] test/py: Add more test cases for rejecting an EFI image
2022-04-18 18:07 [PATCH 1/2] efi_loader: add sha384/512 on certificate revocation Ilias Apalodimas
@ 2022-04-18 18:07 ` Ilias Apalodimas
2022-04-19 1:54 ` AKASHI Takahiro
2022-04-19 1:46 ` [PATCH 1/2] efi_loader: add sha384/512 on certificate revocation AKASHI Takahiro
1 sibling, 1 reply; 6+ messages in thread
From: Ilias Apalodimas @ 2022-04-18 18:07 UTC (permalink / raw)
To: xypron.glpk
Cc: takahiro.akashi, Stuart.Yoder, paul.liu, Ilias Apalodimas, u-boot
The previous patch adds support for rejecting images when the sha384/512
of an x.509 certificate is present in dbx. Update the sandbox selftests
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
changes since v2:
- None
changes since RFC:
- new patch
test/py/tests/test_efi_secboot/conftest.py | 6 +++
test/py/tests/test_efi_secboot/test_signed.py | 50 +++++++++++++++++++
2 files changed, 56 insertions(+)
diff --git a/test/py/tests/test_efi_secboot/conftest.py b/test/py/tests/test_efi_secboot/conftest.py
index 69a498ca003c..8a53dabe5414 100644
--- a/test/py/tests/test_efi_secboot/conftest.py
+++ b/test/py/tests/test_efi_secboot/conftest.py
@@ -80,6 +80,12 @@ def efi_boot_env(request, u_boot_config):
check_call('cd %s; %scert-to-efi-hash-list -g %s -t 0 -s 256 db.crt dbx_hash.crl; %ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key dbx dbx_hash.crl dbx_hash.auth'
% (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH),
shell=True)
+ check_call('cd %s; %scert-to-efi-hash-list -g %s -t 0 -s 384 db.crt dbx_hash384.crl; %ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key dbx dbx_hash384.crl dbx_hash384.auth'
+ % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH),
+ shell=True)
+ check_call('cd %s; %scert-to-efi-hash-list -g %s -t 0 -s 512 db.crt dbx_hash512.crl; %ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key dbx dbx_hash512.crl dbx_hash512.auth'
+ % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH),
+ shell=True)
# dbx_hash1 (digest of TEST_db1 certificate)
check_call('cd %s; %scert-to-efi-hash-list -g %s -t 0 -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),
diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py
index cc9396a11d48..80d5eff74be3 100644
--- a/test/py/tests/test_efi_secboot/test_signed.py
+++ b/test/py/tests/test_efi_secboot/test_signed.py
@@ -235,6 +235,56 @@ class TestEfiSignedImage(object):
assert '\'HELLO\' failed' in ''.join(output)
assert 'efi_start_image() returned: 26' in ''.join(output)
+ # sha384 of an x509 cert in dbx
+ u_boot_console.restart_uboot()
+ with u_boot_console.log.section('Test Case 5e'):
+ # Test Case 5f, authenticated even if only one of signatures
+ # is verified. Same as before but reject dbx_hash1.auth only
+ output = u_boot_console.run_command_list([
+ 'host bind 0 %s' % disk_img,
+ 'fatload host 0:1 4000000 db.auth',
+ 'setenv -e -nv -bs -rt -at -i 4000000:$filesize db',
+ 'fatload host 0:1 4000000 KEK.auth',
+ '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',
+ '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'])
+ assert 'Failed to set EFI variable' not in ''.join(output)
+ output = u_boot_console.run_command_list([
+ 'efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi.signed_2sigs -s ""',
+ 'efidebug boot next 1',
+ 'efidebug test bootmgr'])
+ assert '\'HELLO\' failed' in ''.join(output)
+ assert 'efi_start_image() returned: 26' in ''.join(output)
+
+ # sha512 of an x509 cert in dbx
+ u_boot_console.restart_uboot()
+ with u_boot_console.log.section('Test Case 5e'):
+ # Test Case 5G, authenticated even if only one of signatures
+ # is verified. Same as before but reject dbx_hash1.auth only
+ output = u_boot_console.run_command_list([
+ 'host bind 0 %s' % disk_img,
+ 'fatload host 0:1 4000000 db.auth',
+ 'setenv -e -nv -bs -rt -at -i 4000000:$filesize db',
+ 'fatload host 0:1 4000000 KEK.auth',
+ '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',
+ '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'])
+ assert 'Failed to set EFI variable' not in ''.join(output)
+ output = u_boot_console.run_command_list([
+ 'efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi.signed_2sigs -s ""',
+ 'efidebug boot next 1',
+ 'efidebug test bootmgr'])
+ assert '\'HELLO\' failed' in ''.join(output)
+ assert 'efi_start_image() returned: 26' in ''.join(output)
+
def test_efi_signed_image_auth6(self, u_boot_console, efi_boot_env):
"""
Test Case 6 - using digest of signed image in database
--
2.32.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] efi_loader: add sha384/512 on certificate revocation
2022-04-18 18:07 [PATCH 1/2] efi_loader: add sha384/512 on certificate revocation Ilias Apalodimas
2022-04-18 18:07 ` [PATCH 2/2 v3] test/py: Add more test cases for rejecting an EFI image Ilias Apalodimas
@ 2022-04-19 1:46 ` AKASHI Takahiro
2022-04-19 5:49 ` Ilias Apalodimas
1 sibling, 1 reply; 6+ messages in thread
From: AKASHI Takahiro @ 2022-04-19 1:46 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: xypron.glpk, Stuart.Yoder, paul.liu, u-boot
Ilias,
On Mon, Apr 18, 2022 at 09:07:22PM +0300, Ilias Apalodimas wrote:
> Currently we don't support sha384/512 for the X.509 certificate
> in dbx. Moreover if we come across such a hash we skip the check
> and approve the image, although the image might needs to be rejected.
>
> Rework the code a bit and fix it by adding an array of structs with the
> supported GUIDs, len and literal used in the U-Boot crypto APIs instead
> of hardcoding the GUID types.
>
> It's worth noting here that efi_hash_regions() can now be reused from
> efi_signature_lookup_digest() and add sha348/512 support there as well
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> changes since v2:
> - updated changelog (there was no v1)
> changes since RFC:
> - add an array of structs with the algo info info of a function
> - checking hash_calculate result in efi_hash_regions()
> include/efi_api.h | 6 +++
> include/efi_loader.h | 7 +++
> lib/efi_loader/efi_helper.c | 85 ++++++++++++++++++++++++++++++++++
> lib/efi_loader/efi_signature.c | 76 +++++++++++++++++++++---------
> 4 files changed, 151 insertions(+), 23 deletions(-)
>
> diff --git a/include/efi_api.h b/include/efi_api.h
> index 982c2001728d..b9a04958f9ba 100644
> --- a/include/efi_api.h
> +++ b/include/efi_api.h
> @@ -1873,6 +1873,12 @@ struct efi_system_resource_table {
> #define EFI_CERT_X509_SHA256_GUID \
> EFI_GUID(0x3bd2a492, 0x96c0, 0x4079, 0xb4, 0x20, \
> 0xfc, 0xf9, 0x8e, 0xf1, 0x03, 0xed)
> +#define EFI_CERT_X509_SHA384_GUID \
> + EFI_GUID(0x7076876e, 0x80c2, 0x4ee6, \
> + 0xaa, 0xd2, 0x28, 0xb3, 0x49, 0xa6, 0x86, 0x5b)
> +#define EFI_CERT_X509_SHA512_GUID \
> + EFI_GUID(0x446dbf63, 0x2502, 0x4cda, \
> + 0xbc, 0xfa, 0x24, 0x65, 0xd2, 0xb0, 0xfe, 0x9d)
> #define EFI_CERT_TYPE_PKCS7_GUID \
> EFI_GUID(0x4aafd29d, 0x68df, 0x49ee, 0x8a, 0xa9, \
> 0x34, 0x7d, 0x37, 0x56, 0x65, 0xa7)
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index af36639ec6a7..ce221ee9317b 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -300,6 +300,8 @@ extern const efi_guid_t efi_guid_image_security_database;
> extern const efi_guid_t efi_guid_sha256;
> extern const efi_guid_t efi_guid_cert_x509;
> extern const efi_guid_t efi_guid_cert_x509_sha256;
> +extern const efi_guid_t efi_guid_cert_x509_sha384;
> +extern const efi_guid_t efi_guid_cert_x509_sha512;
> extern const efi_guid_t efi_guid_cert_type_pkcs7;
>
> /* GUID of RNG protocol */
> @@ -671,6 +673,11 @@ efi_status_t efi_file_size(struct efi_file_handle *fh, efi_uintn_t *size);
> /* get a device path from a Boot#### option */
> struct efi_device_path *efi_get_dp_from_boot(const efi_guid_t guid);
>
> +/* get len, string (used in u-boot crypto from a guid */
> +const char *guid_to_sha_str(const efi_guid_t *guid);
> +int guid_to_sha_len(const efi_guid_t *guid);
> +int algo_to_len(const char *algo);
> +
> /**
> * efi_size_in_pages() - convert size in bytes to size in pages
> *
> diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> index 802d39ed97b6..c186ba4a3c01 100644
> --- a/lib/efi_loader/efi_helper.c
> +++ b/lib/efi_loader/efi_helper.c
> @@ -92,3 +92,88 @@ err:
> free(var_value);
> return NULL;
> }
> +
> +const struct guid_to_hash_map {
> + efi_guid_t guid;
> + const char algo[32];
> + u32 bits;
> +} guid_to_hash[] = {
> + {
> + EFI_CERT_X509_SHA256_GUID,
> + "sha256",
> + 256,
=> SHA256_SUM_LEN * 8
> + },
> + {
> + EFI_CERT_SHA256_GUID,
> + "sha256",
> + 256,
> + },
> + {
> + EFI_CERT_X509_SHA384_GUID,
> + "sha384",
> + 384,
> + },
> + {
> + EFI_CERT_X509_SHA512_GUID,
> + "sha512",
> + 512,
> + },
> +};
> +
> +#define MAX_GUID_TO_HASH_COUNT ARRAY_SIZE(guid_to_hash)
> +
> +/** guid_to_sha_str - return the sha string e.g "sha256" for a given guid
> + * used on EFI security databases
> + *
> + * @guid: guid to check
> + *
> + * Return: len or 0 if no match is found
> + */
> +const char *guid_to_sha_str(const efi_guid_t *guid)
> +{
> + size_t i;
> +
> + for (i = 0; i < MAX_GUID_TO_HASH_COUNT; i++) {
> + if (!guidcmp(guid, &guid_to_hash[i].guid))
> + return guid_to_hash[i].algo;
> + }
> +
> + return NULL;
> +}
> +
> +/** guid_to_sha_len - return the sha size in bytes for a given guid
> + * used on EFI security databases
> + *
> + * @guid: guid to check
> + *
> + * Return: len or 0 if no match is found
> + */
> +int guid_to_sha_len(const efi_guid_t *guid)
> +{
> + size_t i;
> +
> + for (i = 0; i < MAX_GUID_TO_HASH_COUNT; i++) {
> + if (!guidcmp(guid, &guid_to_hash[i].guid))
> + return guid_to_hash[i].bits / 8;
> + }
> +
> + return 0;
> +}
If we have algo_to_len(), we don't need guid_to_sha_len().
> +
> +/** algo_to_len - return the sha size in bytes for a given string
> + *
> + * @guid: string to check
> + *
> + * Return: len or 0 if no match is found
> + */
> +int algo_to_len(const char *algo)
This function seems to be quite generic and useful for others.
Why not implement it along with hash_calculate() in hash-checksum.c
(without using guid_to_hash[]).
> +{
> + size_t i;
> +
> + for (i = 0; i < MAX_GUID_TO_HASH_COUNT; i++) {
> + if (!strcmp(algo, guid_to_hash[i].algo))
> + return guid_to_hash[i].bits / 8;
> + }
> +
> + return 0;
> +}
> diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
> index 79ed077ae7dd..cf01f21b4d04 100644
> --- a/lib/efi_loader/efi_signature.c
> +++ b/lib/efi_loader/efi_signature.c
> @@ -24,6 +24,8 @@ const efi_guid_t efi_guid_sha256 = EFI_CERT_SHA256_GUID;
> const efi_guid_t efi_guid_cert_rsa2048 = EFI_CERT_RSA2048_GUID;
> const efi_guid_t efi_guid_cert_x509 = EFI_CERT_X509_GUID;
> const efi_guid_t efi_guid_cert_x509_sha256 = EFI_CERT_X509_SHA256_GUID;
> +const efi_guid_t efi_guid_cert_x509_sha384 = EFI_CERT_X509_SHA384_GUID;
> +const efi_guid_t efi_guid_cert_x509_sha512 = EFI_CERT_X509_SHA512_GUID;
> const efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
>
> static u8 pkcs7_hdr[] = {
> @@ -124,23 +126,32 @@ struct pkcs7_message *efi_parse_pkcs7_header(const void *buf,
> * Return: true on success, false on error
> */
> static bool efi_hash_regions(struct image_region *regs, int count,
> - void **hash, size_t *size)
> + void **hash, const char *hash_algo, int *len)
> {
> + int ret;
> +
> + if (!hash_algo || !len)
> + return false;
> +
> + *len = algo_to_len(hash_algo);
Please modify it this way;
int hash_len;
hash_len = algo_to_len(hash_algo);
if (len)
*len = hash_len;
Then use hash_len instead of *len hereafterr.
This way, we don't have to define a unused local variable below code.
> + if (!*len)
> + return false;
> +
> if (!*hash) {
> - *hash = calloc(1, SHA256_SUM_LEN);
> + *hash = calloc(1, *len);
> if (!*hash) {
> EFI_PRINT("Out of memory\n");
> return false;
> }
> }
> - if (size)
> - *size = SHA256_SUM_LEN;
>
> - hash_calculate("sha256", regs, count, *hash);
> + ret = hash_calculate(hash_algo, regs, count, *hash);
> + if (ret)
> + return false;
> #ifdef DEBUG
> EFI_PRINT("hash calculated:\n");
> print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1,
> - *hash, SHA256_SUM_LEN, false);
> + *hash, *len, false);
> #endif
>
> return true;
> @@ -190,7 +201,6 @@ bool efi_signature_lookup_digest(struct efi_image_regions *regs,
> struct efi_signature_store *siglist;
> struct efi_sig_data *sig_data;
> void *hash = NULL;
> - size_t size = 0;
> bool found = false;
> bool hash_done = false;
>
> @@ -200,6 +210,8 @@ bool efi_signature_lookup_digest(struct efi_image_regions *regs,
> goto out;
>
> for (siglist = db; siglist; siglist = siglist->next) {
> + int len = 0;
> + const char *hash_algo = NULL;
> /*
> * if the hash algorithm is unsupported and we get an entry in
> * dbx reject the image
> @@ -215,8 +227,14 @@ bool efi_signature_lookup_digest(struct efi_image_regions *regs,
> if (guidcmp(&siglist->sig_type, &efi_guid_sha256))
> continue;
>
> + hash_algo = guid_to_sha_str(&efi_guid_sha256);
> + /*
> + * We could check size and hash_algo but efi_hash_regions()
> + * will do that for us
> + */
> if (!hash_done &&
> - !efi_hash_regions(regs->reg, regs->num, &hash, &size)) {
> + !efi_hash_regions(regs->reg, regs->num, &hash, hash_algo,
> + &len)) {
> EFI_PRINT("Digesting an image failed\n");
> break;
> }
> @@ -229,8 +247,8 @@ bool efi_signature_lookup_digest(struct efi_image_regions *regs,
> print_hex_dump(" ", DUMP_PREFIX_OFFSET, 16, 1,
> sig_data->data, sig_data->size, false);
> #endif
> - if (sig_data->size == size &&
> - !memcmp(sig_data->data, hash, size)) {
> + if (sig_data->size == len &&
> + !memcmp(sig_data->data, hash, len)) {
> found = true;
> free(hash);
> goto out;
> @@ -263,8 +281,9 @@ static bool efi_lookup_certificate(struct x509_certificate *cert,
> struct efi_sig_data *sig_data;
> struct image_region reg[1];
> void *hash = NULL, *hash_tmp = NULL;
> - size_t size = 0;
> + int len = 0;
> bool found = false;
> + const char *hash_algo = NULL;
>
> EFI_PRINT("%s: Enter, %p, %p\n", __func__, cert, db);
>
> @@ -278,7 +297,11 @@ static bool efi_lookup_certificate(struct x509_certificate *cert,
> /* calculate hash of TBSCertificate */
> reg[0].data = cert->tbs;
> reg[0].size = cert->tbs_size;
> - if (!efi_hash_regions(reg, 1, &hash, &size))
> +
> + /* We just need any sha256 algo to start the matching */
> + hash_algo = guid_to_sha_str(&efi_guid_sha256);
Why not check if hash_algo is NULL?
> + len = guid_to_sha_len(&efi_guid_sha256);
=> len = algo_to_len(hash_algo);
Or even we don't need this line as efi_has_regions() will set it for us.
> + if (!efi_hash_regions(reg, 1, &hash, hash_algo, &len))
> goto out;
>
> EFI_PRINT("%s: searching for %s\n", __func__, cert->subject);
> @@ -290,6 +313,7 @@ static bool efi_lookup_certificate(struct x509_certificate *cert,
> for (sig_data = siglist->sig_data_list; sig_data;
> sig_data = sig_data->next) {
> struct x509_certificate *cert_tmp;
> + int len_tmp;
We don't need len_tmp if the change I mentioned above is made.
# we don't want to define a unused variable.
> cert_tmp = x509_cert_parse(sig_data->data,
> sig_data->size);
> @@ -300,12 +324,13 @@ static bool efi_lookup_certificate(struct x509_certificate *cert,
> cert_tmp->subject);
> reg[0].data = cert_tmp->tbs;
> reg[0].size = cert_tmp->tbs_size;
> - if (!efi_hash_regions(reg, 1, &hash_tmp, NULL))
> + if (!efi_hash_regions(reg, 1, &hash_tmp, hash_algo,
> + &len_tmp))
=> NULL
> goto out;
>
> x509_free_certificate(cert_tmp);
>
> - if (!memcmp(hash, hash_tmp, size)) {
> + if (!memcmp(hash, hash_tmp, len)) {
> found = true;
> goto out;
> }
> @@ -400,9 +425,10 @@ static bool efi_signature_check_revocation(struct pkcs7_signed_info *sinfo,
> struct efi_sig_data *sig_data;
> struct image_region reg[1];
> void *hash = NULL;
> - size_t size = 0;
> + int len = 0;
> time64_t revoc_time;
> bool revoked = false;
> + const char *hash_algo = NULL;
>
> EFI_PRINT("%s: Enter, %p, %p, %p\n", __func__, sinfo, cert, dbx);
>
> @@ -411,13 +437,14 @@ static bool efi_signature_check_revocation(struct pkcs7_signed_info *sinfo,
>
> EFI_PRINT("Checking revocation against %s\n", cert->subject);
> for (siglist = dbx; siglist; siglist = siglist->next) {
> - if (guidcmp(&siglist->sig_type, &efi_guid_cert_x509_sha256))
> + hash_algo = guid_to_sha_str(&siglist->sig_type);
> + if (!hash_algo)
> continue;
>
> /* calculate hash of TBSCertificate */
> reg[0].data = cert->tbs;
> reg[0].size = cert->tbs_size;
> - if (!efi_hash_regions(reg, 1, &hash, &size))
> + if (!efi_hash_regions(reg, 1, &hash, hash_algo, &len))
> goto out;
>
> for (sig_data = siglist->sig_data_list; sig_data;
> @@ -429,18 +456,18 @@ static bool efi_signature_check_revocation(struct pkcs7_signed_info *sinfo,
> * };
> */
> #ifdef DEBUG
> - if (sig_data->size >= size) {
> + if (sig_data->size >= len) {
> EFI_PRINT("hash in db:\n");
> print_hex_dump(" ", DUMP_PREFIX_OFFSET,
> 16, 1,
> - sig_data->data, size, false);
> + sig_data->data, len, false);
> }
> #endif
> - if ((sig_data->size < size + sizeof(time64_t)) ||
> - memcmp(sig_data->data, hash, size))
> + if ((sig_data->size < len + sizeof(time64_t)) ||
> + memcmp(sig_data->data, hash, len))
> continue;
>
> - memcpy(&revoc_time, sig_data->data + size,
> + memcpy(&revoc_time, sig_data->data + len,
> sizeof(revoc_time));
> EFI_PRINT("revocation time: 0x%llx\n", revoc_time);
> /*
> @@ -488,6 +515,7 @@ bool efi_signature_verify(struct efi_image_regions *regs,
> goto out;
>
> for (sinfo = msg->signed_infos; sinfo; sinfo = sinfo->next) {
> + int len;
ditto
> EFI_PRINT("Signed Info: digest algo: %s, pkey algo: %s\n",
> sinfo->sig->hash_algo, sinfo->sig->pkey_algo);
>
> @@ -500,7 +528,9 @@ bool efi_signature_verify(struct efi_image_regions *regs,
> */
> if (!msg->data &&
> !efi_hash_regions(regs->reg, regs->num,
> - (void **)&sinfo->sig->digest, NULL)) {
> + (void **)&sinfo->sig->digest,
> + guid_to_sha_str(&efi_guid_sha256),
> + &len)) {
=> NULL
-Takahiro Akashi
> EFI_PRINT("Digesting an image failed\n");
> goto out;
> }
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2 v3] test/py: Add more test cases for rejecting an EFI image
2022-04-18 18:07 ` [PATCH 2/2 v3] test/py: Add more test cases for rejecting an EFI image Ilias Apalodimas
@ 2022-04-19 1:54 ` AKASHI Takahiro
2022-04-19 5:39 ` Ilias Apalodimas
0 siblings, 1 reply; 6+ messages in thread
From: AKASHI Takahiro @ 2022-04-19 1:54 UTC (permalink / raw)
To: Ilias Apalodimas; +Cc: xypron.glpk, Stuart.Yoder, paul.liu, u-boot
On Mon, Apr 18, 2022 at 09:07:23PM +0300, Ilias Apalodimas wrote:
> The previous patch adds support for rejecting images when the sha384/512
> of an x.509 certificate is present in dbx. Update the sandbox selftests
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> changes since v2:
> - None
> changes since RFC:
> - new patch
>
> test/py/tests/test_efi_secboot/conftest.py | 6 +++
> test/py/tests/test_efi_secboot/test_signed.py | 50 +++++++++++++++++++
> 2 files changed, 56 insertions(+)
>
> diff --git a/test/py/tests/test_efi_secboot/conftest.py b/test/py/tests/test_efi_secboot/conftest.py
> index 69a498ca003c..8a53dabe5414 100644
> --- a/test/py/tests/test_efi_secboot/conftest.py
> +++ b/test/py/tests/test_efi_secboot/conftest.py
> @@ -80,6 +80,12 @@ def efi_boot_env(request, u_boot_config):
> check_call('cd %s; %scert-to-efi-hash-list -g %s -t 0 -s 256 db.crt dbx_hash.crl; %ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key dbx dbx_hash.crl dbx_hash.auth'
> % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH),
> shell=True)
> + check_call('cd %s; %scert-to-efi-hash-list -g %s -t 0 -s 384 db.crt dbx_hash384.crl; %ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key dbx dbx_hash384.crl dbx_hash384.auth'
> + % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH),
> + shell=True)
> + check_call('cd %s; %scert-to-efi-hash-list -g %s -t 0 -s 512 db.crt dbx_hash512.crl; %ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key dbx dbx_hash512.crl dbx_hash512.auth'
> + % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH),
> + shell=True)
> # dbx_hash1 (digest of TEST_db1 certificate)
> check_call('cd %s; %scert-to-efi-hash-list -g %s -t 0 -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),
> diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py
> index cc9396a11d48..80d5eff74be3 100644
> --- a/test/py/tests/test_efi_secboot/test_signed.py
> +++ b/test/py/tests/test_efi_secboot/test_signed.py
> @@ -235,6 +235,56 @@ class TestEfiSignedImage(object):
> assert '\'HELLO\' failed' in ''.join(output)
> assert 'efi_start_image() returned: 26' in ''.join(output)
>
> + # sha384 of an x509 cert in dbx
> + u_boot_console.restart_uboot()
> + with u_boot_console.log.section('Test Case 5e'):
> + # Test Case 5f, authenticated even if only one of signatures
> + # is verified. Same as before but reject dbx_hash1.auth only
Please describe the test scenario more specifically regarding sha384.
> + output = u_boot_console.run_command_list([
> + 'host bind 0 %s' % disk_img,
> + 'fatload host 0:1 4000000 db.auth',
> + 'setenv -e -nv -bs -rt -at -i 4000000:$filesize db',
> + 'fatload host 0:1 4000000 KEK.auth',
> + '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',
> + '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'])
> + assert 'Failed to set EFI variable' not in ''.join(output)
> + output = u_boot_console.run_command_list([
> + 'efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi.signed_2sigs -s ""',
> + 'efidebug boot next 1',
> + 'efidebug test bootmgr'])
> + assert '\'HELLO\' failed' in ''.join(output)
> + assert 'efi_start_image() returned: 26' in ''.join(output)
> +
> + # sha512 of an x509 cert in dbx
> + u_boot_console.restart_uboot()
> + with u_boot_console.log.section('Test Case 5e'):
> + # Test Case 5G, authenticated even if only one of signatures
> + # is verified. Same as before but reject dbx_hash1.auth only
> + output = u_boot_console.run_command_list([
> + 'host bind 0 %s' % disk_img,
> + 'fatload host 0:1 4000000 db.auth',
> + 'setenv -e -nv -bs -rt -at -i 4000000:$filesize db',
> + 'fatload host 0:1 4000000 KEK.auth',
> + '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',
> + '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'])
> + assert 'Failed to set EFI variable' not in ''.join(output)
> + output = u_boot_console.run_command_list([
> + 'efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi.signed_2sigs -s ""',
> + 'efidebug boot next 1',
> + 'efidebug test bootmgr'])
> + assert '\'HELLO\' failed' in ''.join(output)
> + assert 'efi_start_image() returned: 26' in ''.join(output)
> +
I prefer to have two separate test functions for sha384 and sha512.
This way, we can test both cases independently.
In the test run, even if sha384 case fails, sha512 can still be verified.
-Takahiro Akashi
> def test_efi_signed_image_auth6(self, u_boot_console, efi_boot_env):
> """
> Test Case 6 - using digest of signed image in database
> --
> 2.32.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2 v3] test/py: Add more test cases for rejecting an EFI image
2022-04-19 1:54 ` AKASHI Takahiro
@ 2022-04-19 5:39 ` Ilias Apalodimas
0 siblings, 0 replies; 6+ messages in thread
From: Ilias Apalodimas @ 2022-04-19 5:39 UTC (permalink / raw)
To: AKASHI Takahiro, xypron.glpk, Stuart.Yoder, paul.liu, u-boot
On Tue, Apr 19, 2022 at 10:54:14AM +0900, AKASHI Takahiro wrote:
> On Mon, Apr 18, 2022 at 09:07:23PM +0300, Ilias Apalodimas wrote:
> > The previous patch adds support for rejecting images when the sha384/512
> > of an x.509 certificate is present in dbx. Update the sandbox selftests
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> > changes since v2:
> > - None
> > changes since RFC:
> > - new patch
> >
> > test/py/tests/test_efi_secboot/conftest.py | 6 +++
> > test/py/tests/test_efi_secboot/test_signed.py | 50 +++++++++++++++++++
> > 2 files changed, 56 insertions(+)
> >
> > diff --git a/test/py/tests/test_efi_secboot/conftest.py b/test/py/tests/test_efi_secboot/conftest.py
> > index 69a498ca003c..8a53dabe5414 100644
> > --- a/test/py/tests/test_efi_secboot/conftest.py
> > +++ b/test/py/tests/test_efi_secboot/conftest.py
> > @@ -80,6 +80,12 @@ def efi_boot_env(request, u_boot_config):
> > check_call('cd %s; %scert-to-efi-hash-list -g %s -t 0 -s 256 db.crt dbx_hash.crl; %ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key dbx dbx_hash.crl dbx_hash.auth'
> > % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH),
> > shell=True)
> > + check_call('cd %s; %scert-to-efi-hash-list -g %s -t 0 -s 384 db.crt dbx_hash384.crl; %ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key dbx dbx_hash384.crl dbx_hash384.auth'
> > + % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH),
> > + shell=True)
> > + check_call('cd %s; %scert-to-efi-hash-list -g %s -t 0 -s 512 db.crt dbx_hash512.crl; %ssign-efi-sig-list -t "2020-04-05" -c KEK.crt -k KEK.key dbx dbx_hash512.crl dbx_hash512.auth'
> > + % (mnt_point, EFITOOLS_PATH, GUID, EFITOOLS_PATH),
> > + shell=True)
> > # dbx_hash1 (digest of TEST_db1 certificate)
> > check_call('cd %s; %scert-to-efi-hash-list -g %s -t 0 -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),
> > diff --git a/test/py/tests/test_efi_secboot/test_signed.py b/test/py/tests/test_efi_secboot/test_signed.py
> > index cc9396a11d48..80d5eff74be3 100644
> > --- a/test/py/tests/test_efi_secboot/test_signed.py
> > +++ b/test/py/tests/test_efi_secboot/test_signed.py
> > @@ -235,6 +235,56 @@ class TestEfiSignedImage(object):
> > assert '\'HELLO\' failed' in ''.join(output)
> > assert 'efi_start_image() returned: 26' in ''.join(output)
> >
> > + # sha384 of an x509 cert in dbx
> > + u_boot_console.restart_uboot()
> > + with u_boot_console.log.section('Test Case 5e'):
> > + # Test Case 5f, authenticated even if only one of signatures
> > + # is verified. Same as before but reject dbx_hash1.auth only
>
> Please describe the test scenario more specifically regarding sha384.
>
> > + output = u_boot_console.run_command_list([
> > + 'host bind 0 %s' % disk_img,
> > + 'fatload host 0:1 4000000 db.auth',
> > + 'setenv -e -nv -bs -rt -at -i 4000000:$filesize db',
> > + 'fatload host 0:1 4000000 KEK.auth',
> > + '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',
> > + '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'])
> > + assert 'Failed to set EFI variable' not in ''.join(output)
> > + output = u_boot_console.run_command_list([
> > + 'efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi.signed_2sigs -s ""',
> > + 'efidebug boot next 1',
> > + 'efidebug test bootmgr'])
> > + assert '\'HELLO\' failed' in ''.join(output)
> > + assert 'efi_start_image() returned: 26' in ''.join(output)
> > +
> > + # sha512 of an x509 cert in dbx
> > + u_boot_console.restart_uboot()
> > + with u_boot_console.log.section('Test Case 5e'):
> > + # Test Case 5G, authenticated even if only one of signatures
> > + # is verified. Same as before but reject dbx_hash1.auth only
> > + output = u_boot_console.run_command_list([
> > + 'host bind 0 %s' % disk_img,
> > + 'fatload host 0:1 4000000 db.auth',
> > + 'setenv -e -nv -bs -rt -at -i 4000000:$filesize db',
> > + 'fatload host 0:1 4000000 KEK.auth',
> > + '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',
> > + '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'])
> > + assert 'Failed to set EFI variable' not in ''.join(output)
> > + output = u_boot_console.run_command_list([
> > + 'efidebug boot add -b 1 HELLO host 0:1 /helloworld.efi.signed_2sigs -s ""',
> > + 'efidebug boot next 1',
> > + 'efidebug test bootmgr'])
> > + assert '\'HELLO\' failed' in ''.join(output)
> > + assert 'efi_start_image() returned: 26' in ''.join(output)
> > +
>
> I prefer to have two separate test functions for sha384 and sha512.
> This way, we can test both cases independently.
> In the test run, even if sha384 case fails, sha512 can still be verified.
>
Sure, I'll split them on v4
Thanks
/Ilias
> -Takahiro Akashi
>
>
> > def test_efi_signed_image_auth6(self, u_boot_console, efi_boot_env):
> > """
> > Test Case 6 - using digest of signed image in database
> > --
> > 2.32.0
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] efi_loader: add sha384/512 on certificate revocation
2022-04-19 1:46 ` [PATCH 1/2] efi_loader: add sha384/512 on certificate revocation AKASHI Takahiro
@ 2022-04-19 5:49 ` Ilias Apalodimas
0 siblings, 0 replies; 6+ messages in thread
From: Ilias Apalodimas @ 2022-04-19 5:49 UTC (permalink / raw)
To: AKASHI Takahiro, xypron.glpk, Stuart.Yoder, paul.liu, u-boot
Akashi-san,
> > + {
> > + EFI_CERT_X509_SHA256_GUID,
> > + "sha256",
> > + 256,
>
> => SHA256_SUM_LEN * 8
>
Sure
> > + },
> > + {
> > + EFI_CERT_SHA256_GUID,
> > + "sha256",
> > + 256,
> > + },
> > + {
> > + EFI_CERT_X509_SHA384_GUID,
> > + "sha384",
> > + 384,
> > + },
> > + {
> > + EFI_CERT_X509_SHA512_GUID,
> > + "sha512",
> > + 512,
> > + },
> > +};
> > +
> > +#define MAX_GUID_TO_HASH_COUNT ARRAY_SIZE(guid_to_hash)
> > +
> > +/** guid_to_sha_str - return the sha string e.g "sha256" for a given guid
> > + * used on EFI security databases
> > + *
> > + * @guid: guid to check
> > + *
> > + * Return: len or 0 if no match is found
> > + */
> > +const char *guid_to_sha_str(const efi_guid_t *guid)
> > +{
> > + size_t i;
> > +
> > + for (i = 0; i < MAX_GUID_TO_HASH_COUNT; i++) {
> > + if (!guidcmp(guid, &guid_to_hash[i].guid))
> > + return guid_to_hash[i].algo;
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +/** guid_to_sha_len - return the sha size in bytes for a given guid
> > + * used on EFI security databases
> > + *
> > + * @guid: guid to check
> > + *
> > + * Return: len or 0 if no match is found
> > + */
> > +int guid_to_sha_len(const efi_guid_t *guid)
> > +{
> > + size_t i;
> > +
> > + for (i = 0; i < MAX_GUID_TO_HASH_COUNT; i++) {
> > + if (!guidcmp(guid, &guid_to_hash[i].guid))
> > + return guid_to_hash[i].bits / 8;
> > + }
> > +
> > + return 0;
> > +}
>
> If we have algo_to_len(), we don't need guid_to_sha_len().
True, I'll get rid of this
>
> > +
> > +/** algo_to_len - return the sha size in bytes for a given string
> > + *
> > + * @guid: string to check
> > + *
> > + * Return: len or 0 if no match is found
> > + */
> > +int algo_to_len(const char *algo)
>
> This function seems to be quite generic and useful for others.
> Why not implement it along with hash_calculate() in hash-checksum.c
> (without using guid_to_hash[]).
>
Tbh I have similar stuff in efi_tcg2.c, but they are all bound to EFI atm.
We could refactor all of the structures a bit and have a more generic
version. Can we leave it for now and I can send a refactoring series
after this gets merged?
> > +{
> > + size_t i;
> > +
> > + for (i = 0; i < MAX_GUID_TO_HASH_COUNT; i++) {
> > + if (!strcmp(algo, guid_to_hash[i].algo))
> > + return guid_to_hash[i].bits / 8;
> > + }
> > +
> > + return 0;
> > +}
> > diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
> > index 79ed077ae7dd..cf01f21b4d04 100644
> > --- a/lib/efi_loader/efi_signature.c
> > +++ b/lib/efi_loader/efi_signature.c
> > @@ -24,6 +24,8 @@ const efi_guid_t efi_guid_sha256 = EFI_CERT_SHA256_GUID;
> > const efi_guid_t efi_guid_cert_rsa2048 = EFI_CERT_RSA2048_GUID;
> > const efi_guid_t efi_guid_cert_x509 = EFI_CERT_X509_GUID;
> > const efi_guid_t efi_guid_cert_x509_sha256 = EFI_CERT_X509_SHA256_GUID;
> > +const efi_guid_t efi_guid_cert_x509_sha384 = EFI_CERT_X509_SHA384_GUID;
> > +const efi_guid_t efi_guid_cert_x509_sha512 = EFI_CERT_X509_SHA512_GUID;
> > const efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
> >
> > static u8 pkcs7_hdr[] = {
> > @@ -124,23 +126,32 @@ struct pkcs7_message *efi_parse_pkcs7_header(const void *buf,
> > * Return: true on success, false on error
> > */
> > static bool efi_hash_regions(struct image_region *regs, int count,
> > - void **hash, size_t *size)
> > + void **hash, const char *hash_algo, int *len)
> > {
> > + int ret;
> > +
> > + if (!hash_algo || !len)
> > + return false;
> > +
> > + *len = algo_to_len(hash_algo);
>
> Please modify it this way;
> int hash_len;
>
> hash_len = algo_to_len(hash_algo);
> if (len)
> *len = hash_len;
>
> Then use hash_len instead of *len hereafterr.
>
> This way, we don't have to define a unused local variable below code.
Sure
[...]
> > +
> > + /* We just need any sha256 algo to start the matching */
> > + hash_algo = guid_to_sha_str(&efi_guid_sha256);
>
> Why not check if hash_algo is NULL?
>
> > + len = guid_to_sha_len(&efi_guid_sha256);
>
> => len = algo_to_len(hash_algo);
>
> Or even we don't need this line as efi_has_regions() will set it for us.
>
Yea good catch. I was playing with a version where len wasn't a ptr in
efi_hash_regions() and that's a leftover (as well as guid_to_sha_len()).
[...]
Thanks for having a look
/Ilias
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-04-19 5:49 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-18 18:07 [PATCH 1/2] efi_loader: add sha384/512 on certificate revocation Ilias Apalodimas
2022-04-18 18:07 ` [PATCH 2/2 v3] test/py: Add more test cases for rejecting an EFI image Ilias Apalodimas
2022-04-19 1:54 ` AKASHI Takahiro
2022-04-19 5:39 ` Ilias Apalodimas
2022-04-19 1:46 ` [PATCH 1/2] efi_loader: add sha384/512 on certificate revocation AKASHI Takahiro
2022-04-19 5:49 ` Ilias Apalodimas
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.