Re: [U-Boot] [PATCH 05/16] efi_loader: add signature verification functions

18 Nov 2019

Heinrich,
I will address other comments in a separate e-mail, but
it is worth mentioning that Patrick deserves credit for initial
implementation of efi secure boot, which the main logic of my current
patch set heavily relies on;
On Sat, Nov 16, 2019 at 09:00:12PM +0100, Heinrich Schuchardt wrote:
...
On 11/13/19 1:52 AM, AKASHI Takahiro wrote:
...

diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
new file mode 100644
index 000000000000..04f43e47d1a3
--- /dev/null
+++ b/lib/efi_loader/efi_signature.c
@@ -0,0 +1,600 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*


Copyright (c) 2018 Patrick Wildt patrick@blueri.se



From which upstream did you take the code?
The original code has never been upstreamed, and I took over
his patch directly from him.
Thanks,
-Takahiro Akashi
...
...


Copyright (c) 2019 Linaro Limited, Author: AKASHI Takahiro


*/


debug() checks the value of _DEBUG.
Please, include common.h as first include.
Use the sequence of includes as defined in
https://www.denx.de/wiki/U-Boot/CodingStyle
...
+#include <charset.h>
+#include <efi_loader.h>
+#include <image.h>
+#include <hexdump.h>
+#include <malloc.h>
+#include <pe.h>
+#include <linux/compat.h>
+#include <linux/oid_registry.h>
+#include <u-boot/rsa.h>
Shouldn't we move all cryptography stuff into a directory crypto/?
...
+#include <u-boot/sha256.h>
+/*


avoid duplicated inclusion:



#include "../lib/crypto/x509_parser.h"


*/

+#include "../lib/crypto/pkcs7_parser.h"



+const efi_guid_t efi_guid_image_security_database =

EFI_IMAGE_SECURITY_DATABASE_GUID;



+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;



+#ifdef CONFIG_EFI_SECURE_BOOT
+static const unsigned char WinIndirectSha256[] = {
We don't use camel case.
...

0x30, 0x33, 0x06, 0x0a, 0x2b, 0x06, 0x01, 0x04, 0x01, 0x82, 0x37, 0x02,
0x01, 0x0f, 0x30, 0x25, 0x03, 0x01, 0x00, 0xa0, 0x20, 0xa2, 0x1e, 0x80,
0x1c, 0x00, 0x3c, 0x00, 0x3c, 0x00, 0x3c, 0x00, 0x4f, 0x00, 0x62, 0x00,
0x73, 0x00, 0x6f, 0x00, 0x6c, 0x00, 0x65, 0x00, 0x74, 0x00, 0x65, 0x00,
0x3e, 0x00, 0x3e, 0x00, 0x3e, 0x30, 0x31, 0x30, 0x0d, 0x06, 0x09, 0x60,
0x86, 0x48, 0x01, 0x65, 0x03, 0x04, 0x02, 0x01, 0x05, 0x00, 0x04, 0x20

+};
What secret sauce is this? - Please, add comments where applicable. Add
references where needed for verification of correctness.
Below you describe it as DER wrapper. So why call the variable Sha256?
...



+/**


efi_hash_regions - calculate a hash value



@regs:	List of regions



@hash:	Pointer to a pointer to buffer holding a hash value



@size:	Size of buffer to be returned







Calculate a sha256 value of @regs and return a value in @hash.



Is this a hash of the concatenation of all regions - or is this a list
of hashes one for each regions?
...






Return:	true on success, false on error


*/

+static bool efi_hash_regions(struct efi_image_regions *regs, void **hash,

	     size_t *size)



Why use void** for hash? There should be a structure for SHA256 hashes.
...
+{

*size = 0;
*hash = calloc(1, SHA256_SUM_LEN);
if (!*hash) {
debug("Out of memory\n");


return false;


}
*size = SHA256_SUM_LEN;

hash_calculate("sha256", regs->reg, regs->num, *hash);

+#ifdef DEBUG

debug("hash calculated:\n");

debug checks the value of _DEBUG not of DEBUG. So wouldn't you use
#ifdef _DEBUG to be consistent?
...

print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1,
       *hash, SHA256_SUM_LEN, false);



This will fail to compile if CONFIG_HEXDUMP is not defined?
...
+#endif


return true;

+}



+/**


efi_hash_regions_in_der - calculate a hash value



@regs:	List of regions



@hash:	Pointer to a pointer to buffer holding a hash value



@size:	Size of buffer to be returned







Calculate a sha256 value of @regs and return a value in @hash.



This function is a variant of efi_hash_regions(), and the data will



be wrapped with some header in DER format before hash calculation.



Message digest in signature of PE format will be calculated this way.







Return:	true on success, false on error


*/

+static bool efi_hash_regions_in_der(struct efi_image_regions *regs, void **hash,

		    size_t *size)



+{

void *msg;
size_t msg_size;
struct image_region regtmp[2];

if (!efi_hash_regions(regs, &msg, &msg_size)) {
debug("Hash calculation failed\n");


return false;


;


}

*size = 0;
*hash = calloc(1, SHA256_SUM_LEN);
if (!*hash) {
debug("Out of memory\n");


free(msg);


return false;


}
*size = SHA256_SUM_LEN;

/* File image hash is digested with some DER wrapper. */
regtmp[0].data = WinIndirectSha256;
regtmp[0].size = sizeof(WinIndirectSha256);
regtmp[1].data = msg;
regtmp[1].size = msg_size;

hash_calculate("sha256", regtmp, 2, *hash);

+#ifdef DEBUG

debug("hash calculated with extra header:\n");
print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1,
       *hash, SHA256_SUM_LEN, false);



+#endif


free(msg);

return true;

+}



+/**


efi_signature_verify - verify a signature with a certificate



@regs:		List of regions to be authenticated



@signed_info:	Pointer to PKCS7's signed_info



@cert:		x509 certificate







Signature pointed to by @signed_info against image pointed to by @regs



is verified by a certificate pointed to by @cert.



@signed_info holds a signature, including a message digest which is to be



compared with a hash value calculated from @regs.







Return:	true if signature is verified, false if not


*/

+static bool efi_signature_verify(struct efi_image_regions *regs,

		 struct pkcs7_signed_info *ps_info,


		 struct x509_certificate *cert)



+{

struct image_sign_info info;
struct image_region regtmp[2];
void *hash;
size_t size;
char c;
bool verified;

debug("%s: Enter, %p, %p, %p(issuer: %s, subject: %s)\n", __func__,
     regs, ps_info, cert, cert->issuer, cert->subject);



verified = false;

memset(&info, '\0', sizeof(info));
info.padding = image_get_padding_algo("pkcs-1.5");
/*
* Note: image_get_[checksum|crypto]_algo takes an string


* argument like "<checksum>,<crypto>"


* TODO: support other hash algorithms


*/


if (!strcmp(ps_info->sig->hash_algo, "sha1")) {
info.checksum = image_get_checksum_algo("sha1,rsa2048");


info.name = "sha1,rsa2048";


} else if (!strcmp(ps_info->sig->hash_algo, "sha256")) {
info.checksum = image_get_checksum_algo("sha256,rsa2048");


info.name = "sha256,rsa2048";


} else {
debug("unknown msg digest algo: %s\n", ps_info->sig->hash_algo);


goto out;


}
info.crypto = image_get_crypto_algo(info.name);

info.key = cert->pub->key;
info.keylen = cert->pub->keylen;

/* verify signature */
debug("%s: crypto: %s, signature len:%x\n", __func__,
     info.name, ps_info->sig->s_size);


if (ps_info->aa_set & (1UL << sinfo_has_message_digest)) {
debug("%s: RSA verify authentication attribute\n", __func__);


/*


 * NOTE: This path will be executed only for


 * PE image authentication


 */



/* check if hash matches digest first */


debug("checking msg digest first, len:0x%x\n",


      ps_info->msgdigest_len);




+#ifdef DEBUG

debug("hash in database:\n");


print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1,


	       ps_info->msgdigest, ps_info->msgdigest_len,


	       false);



+#endif

if (efi_hash_regions_in_der(regs, &hash, &size)) {


	if (ps_info->msgdigest_len != size ||


	    memcmp(hash, ps_info->msgdigest, size)) {


		debug("Digest doesn't match\n");


		free(hash);


		goto out;


	}



	free(hash);


} else {


	debug("Digesting image failed\n");


	goto out;


}



/* against digest */


c = 0x31;


regtmp[0].data = &c;


regtmp[0].size = 1;


regtmp[1].data = ps_info->authattrs;


regtmp[1].size = ps_info->authattrs_len;



if (!rsa_verify(&info, regtmp, 2,


		ps_info->sig->s, ps_info->sig->s_size))


	verified = true;


} else {
debug("%s: RSA verify content data\n", __func__);


/* against all data */


if (!rsa_verify(&info, regs->reg, regs->num,


		ps_info->sig->s, ps_info->sig->s_size))


	verified = true;


}


+out:

debug("%s: Exit, verified: %d\n", __func__, verified);
return verified;

+}



+/**


efi_signature_verify_with_list - verify a signature with signature list



@regs:		List of regions to be authenticated



@signed_info:	Pointer to PKCS7's signed_info



@siglist:		Signature list for certificates



@valid_cert:		x509 certificate that verifies this signature







Signature pointed to by @signed_info against image pointed to by @regs



is verified by signature list pointed to by @siglist.



Signature database is a simple concatenation of one or more



signature list(s).







Return:	true if signature is verified, false if not


*/

+static
+bool efi_signature_verify_with_list(struct efi_image_regions *regs,

		    struct pkcs7_signed_info *signed_info,


		    struct efi_signature_store *siglist,


		    struct x509_certificate **valid_cert)



+{

struct x509_certificate *cert;
struct efi_sig_data *sig_data;
bool verified = false;

debug("%s: Enter, %p, %p, %p, %p\n", __func__,
     regs, signed_info, siglist, valid_cert);



Shouldn't we use EFI_PRINT() to get proper indentation?
...


if (!signed_info) {
void *hash;


size_t size;



debug("%s: unsigned image\n", __func__);



We already have indicated the function above.
...

/*


 * verify based on calculated hash value


 * TODO: support other hash algorithms


 */


if (guidcmp(&siglist->sig_type, &efi_guid_sha256)) {


	debug("Digest algorithm is not supported: %pUl\n",


	      &siglist->sig_type);


	goto out;


}



if (!efi_hash_regions(regs, &hash, &size)) {


	debug("Digesting unsigned image failed\n");


	goto out;


}



/* go through the list */


for (sig_data = siglist->sig_data_list; sig_data;


     sig_data = sig_data->next) {



+#ifdef DEBUG

	debug("Msg digest in database:\n");


	print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1,


		       sig_data->data, sig_data->size, false);



Does this compile if CONFIG_HEXDUMP is not defined?
...
+#endif

	if ((sig_data->size == size) &&


	    !memcmp(sig_data->data, hash, size)) {


		verified = true;


		free(hash);


		goto out;


	}


}


free(hash);


goto out;


}

debug("%s: signed image\n", __func__);
if (guidcmp(&siglist->sig_type, &efi_guid_cert_x509)) {
debug("Signature type is not supported: %pUl\n",


      &siglist->sig_type);


goto out;


}

/* go through the list */
for (sig_data = siglist->sig_data_list; sig_data;
    sig_data = sig_data->next) {


/* TODO: support owner check based on policy */



cert = x509_cert_parse(sig_data->data, sig_data->size);


if (IS_ERR(cert)) {


	debug("Parsing x509 certificate failed\n");


	goto out;


}



verified = efi_signature_verify(regs, signed_info, cert);



if (verified) {


	if (valid_cert)


		*valid_cert = cert;


	else


		x509_free_certificate(cert);


	break;


} else {


	x509_free_certificate(cert);


}


}


+out:

debug("%s: Exit, verified: %d\n", __func__, verified);
return verified;

+}



+/**


efi_signature_verify_with_sigdb - verify a signature with db



@regs:	List of regions to be authenticated



@msg:	Signature



@db:		Signature database for trusted certificates



@cert:	x509 certificate that verifies this signature







Signature pointed to by @msg against image pointed to by @regs



is verified by signature database pointed to by @db.







Return:	true if signature is verified, false if not


*/

+bool efi_signature_verify_with_sigdb(struct efi_image_regions *regs,

		     struct pkcs7_message *msg,


		     struct efi_signature_store *db,


		     struct x509_certificate **cert)



+{

struct pkcs7_signed_info *info;
struct efi_signature_store *siglist;
bool verified = false;

debug("%s: Enter, %p, %p, %p, %p\n", __func__, regs, msg, db, cert);

if (!db)
goto out;



if (!db->sig_data_list)
goto out;



/* for unsigned image */
if (!msg) {
debug("%s: Verify unsigned image with db\n", __func__);


for (siglist = db; siglist; siglist = siglist->next)


	if (efi_signature_verify_with_list(regs, NULL,


					   siglist, cert)) {


		verified = true;


		goto out;


	}



goto out;


}

/* for signed image or variable */
debug("%s: Verify signed image with db\n", __func__);
for (info = msg->signed_infos; info; info = info->next) {
debug("Signed Info: digest algo: %s, pkey algo: %s\n",


      info->sig->hash_algo, info->sig->pkey_algo);



for (siglist = db; siglist; siglist = siglist->next) {


	if (efi_signature_verify_with_list(regs, info,


					   siglist, cert)) {


		verified = true;


		goto out;


	}


}


}


+out:

debug("%s: Exit, verified: %d\n", __func__, verified);
return verified;

+}



+/**


efi_search_siglist - search signature list for a certificate



@cert:	x509 certificate



@siglist:	Signature list



@revoc_time:	Pointer to buffer for revocation time







Search signature list pointed to by @siglist and find a certificate



pointed to by @cert.



If found, revocation time that is specified in signature database is



returned in @revoc_time.







Return:	true if certificate is found, false if not


*/

+static bool efi_search_siglist(struct x509_certificate *cert,

	       struct efi_signature_store *siglist,


	       time64_t *revoc_time)



+{

struct image_region reg[1];
void *hash = NULL, *msg = NULL;
struct efi_sig_data *sig_data;
bool found = false;

/* can be null */
if (!siglist->sig_data_list)
return false;



if (guidcmp(&siglist->sig_type, &efi_guid_cert_x509_sha256)) {
/* TODO: other hash algos */


debug("Certificate's digest type is not supported: %pUl\n",


      &siglist->sig_type);


goto out;


}

/* calculate hash of TBSCertificate */
msg = calloc(1, SHA256_SUM_LEN);
if (!msg) {
debug("Out of memory\n");


goto out;


}

hash = calloc(1, SHA256_SUM_LEN);
if (!hash) {
debug("Out of memory\n");


goto out;


}

reg[0].data = cert->tbs;
reg[0].size = cert->tbs_size;
hash_calculate("sha256", reg, 1, msg);

/* go through signature list */
for (sig_data = siglist->sig_data_list; sig_data;
    sig_data = sig_data->next) {


/*


 * struct efi_cert_x509_sha256 {


 *	u8 tbs_hash[256/8];


 *	time64_t revocation_time;


 * };


 */


if ((sig_data->size == SHA256_SUM_LEN) &&


    !memcmp(sig_data->data, hash, SHA256_SUM_LEN)) {


	memcpy(revoc_time, sig_data->data + SHA256_SUM_LEN,


	       sizeof(*revoc_time));


	found = true;


	goto out;


}


}


+out:

free(hash);
free(msg);

return found;

+}



+/**


efi_signature_verify_cert - verify a certificate with dbx



@cert:	x509 certificate



@dbx:	Signature database







Search signature database pointed to by @dbx and find a certificate



pointed to by @cert.



This function is expected to be used against "dbx".







Return:	true if a certificate is not rejected, false otherwise.


*/

+bool efi_signature_verify_cert(struct x509_certificate *cert,

	       struct efi_signature_store *dbx)



+{

struct efi_signature_store *siglist;
time64_t revoc_time;
bool found = false;

debug("%s: Enter, %p, %p\n", __func__, dbx, cert);

EFI_PRINT() ?
...


if (!cert)
return false;



for (siglist = dbx; siglist; siglist = siglist->next) {
if (efi_search_siglist(cert, siglist, &revoc_time)) {


	/* TODO */


	/* compare signing time with revocation time */



Is this a difficult thing to do? If not, please, fill the gap.
Best regards
Heinrich
...


	found = true;


	break;


}


}

debug("%s: Exit, verified: %d\n", __func__, !found);
return !found;

+}



+/**


efi_signature_verify_signers - verify signers' certificates with dbx



@msg:	Signature



@dbx:	Signature database







Determine if any of signers' certificates in @msg may be verified



by any of certificates in signature database pointed to by @dbx.



This function is expected to be used against "dbx".







Return:	true if none of certificates is rejected, false otherwise.


*/

+bool efi_signature_verify_signers(struct pkcs7_message *msg,

		  struct efi_signature_store *dbx)



+{

struct pkcs7_signed_info *info;
bool found = false;

debug("%s: Enter, %p, %p\n", __func__, msg, dbx);

if (!msg)
goto out;



for (info = msg->signed_infos; info; info = info->next) {
if (info->signer &&


    !efi_signature_verify_cert(info->signer, dbx)) {


	found = true;


	goto out;


}


}

+out:

debug("%s: Exit, verified: %d\n", __func__, !found);
return !found;

+}



+/**


efi_image_region_add - add an entry of region



@regs:	Pointer to array of regions



@start:	Start address of region



@end:	End address of region



@nocheck:	flag against overlapped regions







Take one entry of region [@start, @end] and append it to the list



pointed to by @regs. If @nocheck is false, overlapping among entries



will be checked first.







Return:	0 on success, status code (negative) on error


*/

+efi_status_t efi_image_region_add(struct efi_image_regions *regs,

		  const void *start, const void *end,


		  int nocheck)



+{

struct image_region *reg;
int i, j;

if (regs->num >= EFI_REGS_MAX) {
debug("%s: no more room for regions\n", __func__);


return EFI_OUT_OF_RESOURCES;


}

if (end < start)
return EFI_INVALID_PARAMETER;



for (i = 0; i < regs->num; i++) {
reg = &regs->reg[i];


if (nocheck)


	continue;



if (start > reg->data + reg->size)


	continue;



if ((start >= reg->data && start < reg->data + reg->size) ||


    (end > reg->data && end < reg->data + reg->size)) {


	debug("%s: new region already part of another\n",


	      __func__);


	return EFI_INVALID_PARAMETER;


}



if (start < reg->data && end < reg->data + reg->size) {


	for (j = regs->num - 1; j >= i; j--)


		memcpy(&regs->reg[j], &regs->reg[j + 1],


		       sizeof(*reg));


	break;


}


}

reg = &regs->reg[i];
reg->data = start;
reg->size = end - start;
regs->num++;

return EFI_SUCCESS;

+}
+#endif /* CONFIG_EFI_SECURE_BOOT */

    