crypto: asymmetric_keys - fix OOB read in pefile_parse_binary

pefile_parse_binary() reads the size field of the certificate table's data-directory entry, which sits at fixed index 4 of the PE optional header's data directory:

	ctx->certs_size = ddir->certs.size;

but nothing ensures index 4 is present. n_data_dirents (the untrusted NumberOfRvaAndSizes) is only upper-bounded against header_size and may be 0, and header_size need only satisfy cursor < header_size < datalen. A crafted PE with n_data_dirents = 0 and a tiny header_size therefore causes the ddir->certs.size read to land past the end of the image (CWE-125). The chkaddr() that bounds the certificate blob runs only after this read.

verify_pefile_signature() is reached from kexec_file_load() (the lockdown/secure-boot enforced PE-image signature path), and the image is parsed before its signature is checked. The trigger needs CAP_SYS_BOOT and the access is out-of-bounds read only (no write).

Require the certificate table's data-directory entry (index 4) to be present; the existing upper-bound check then keeps ddir->certs within [cursor, header_size).

---
Reproduced under KASAN (CONFIG_KASAN_GENERIC, x86-64) with a KUnit case that builds a crafted PE (valid MZ/PE/PE32 magics, data_dirs = 0, header_size just past the optional header) and calls verify_pefile_signature() with a NULL keyring -- the parse runs before any signature check. On an unpatched kernel:

    BUG: KASAN: slab-out-of-bounds in verify_pefile_signature+0x1d6/0x950
    Read of size 4 ... in pefile_parse_binary() (inlined)

With this patch the crafted image is rejected (-ELIBBAD) before the read and the case passes with no KASAN report. The test is not included here; happy to submit it separately.

 crypto/asymmetric_keys/verify_pefile.c | 4 ++++
 1 file changed, 4 insertions(+)

index cec99db..e7f8bdb 100644
@@ -87,6 +87,10 @@ static int pefile_parse_binary(const void *pebuf, unsigned int pelen,
 	if (ctx->n_data_dirents > (ctx->header_size - cursor) / sizeof(*dde))
 		return -ELIBBAD;
 
+	/* The certificate table entry is at fixed index 4 of the data directory. */
+	if (ctx->n_data_dirents <= 4)
+		return -ELIBBAD;
+
 	ddir = pebuf + cursor;
 	cursor += sizeof(*dde) * ctx->n_data_dirents;
 

base-commit: 3d6d817622b0a9721e3cc404df3469171582be13

2.53.0

Any reason not to commit this as well?

Ignat

pefile_parse_binary() reads the size field of the certificate table's data-directory entry, which sits at fixed index 4 of the PE optional header's data directory:

	ctx->certs_size = ddir->certs.size;

but nothing ensures index 4 is present. n_data_dirents (the untrusted NumberOfRvaAndSizes) is only upper-bounded against header_size and may be 0, and header_size need only satisfy cursor < header_size < datalen. A crafted PE with n_data_dirents = 0 and a tiny header_size therefore causes the ddir->certs.size read to land past the end of the image (CWE-125). The chkaddr() that bounds the certificate blob runs only after this read.

verify_pefile_signature() is reached from kexec_file_load() (the lockdown/secure-boot enforced PE-image signature path), and the image is parsed before its signature is checked. The trigger needs CAP_SYS_BOOT and the access is out-of-bounds read only (no write).

Require the certificate table's data-directory entry (index 4) to be present; the existing upper-bound check then keeps ddir->certs within [cursor, header_size).

---
v2: express the "index 4" bound as
offsetof(struct data_directory, certs) / sizeof(*dde) rather than a literal 4, per Ignat Korchagin's review [1] -- self-documenting and it tracks the struct layout. It is the same value (offsetof is 32, sizeof(*dde) is 8, so the bound is 4). A KUnit test is added as 2/2.

[1] https://lore.kernel.org/all/CAOs+rJVztmvHSkNxP_voc7E=girsstCKmqxG37pvO2kTaEk1TQ@mail.gmail.com/

Reproduced under KASAN (CONFIG_KASAN_GENERIC, x86-64) with the KUnit case in 2/2: a crafted PE with data_dirs = 0 takes a slab-out-of-bounds read in pefile_parse_binary() on an unpatched kernel, and is rejected with -ELIBBAD (no KASAN report) with this patch.

 crypto/asymmetric_keys/verify_pefile.c | 5 +++++
 1 file changed, 5 insertions(+)

index cec99db14..1efd5c0d0 100644
@@ -87,6 +87,11 @@ static int pefile_parse_binary(const void *pebuf, unsigned int pelen,
 	if (ctx->n_data_dirents > (ctx->header_size - cursor) / sizeof(*dde))
 		return -ELIBBAD;
 
+	/* the certificate table entry must be present in the data directory */
+	if (ctx->n_data_dirents <=
+	    offsetof(struct data_directory, certs) / sizeof(*dde))
+		return -ELIBBAD;
+
 	ddir = pebuf + cursor;
 	cursor += sizeof(*dde) * ctx->n_data_dirents;
 

base-commit: d58772d8520c7ef247c4b95c9bd76d3a25da9ff5

2.53.0

Add self-contained KUnit tests for pefile_parse_binary(), exercised through verify_pefile_signature() -- which parses the image before any signature check, so no keyring or real signature is needed. The cases build small malformed PE images in memory:

- a PE declaring 0 data-directory entries: the certificate table entry at fixed index 4 is absent and must be rejected (-ELIBBAD), not read out of bounds;
- a PE declaring 5 entries with a zero (unsigned) certificate entry gets past the directory bound and is rejected as unsigned (-ENODATA).

verify_pefile_signature() is not exported, so the test is built-in only (bool, not tristate).

---
 crypto/asymmetric_keys/Kconfig              |  12 +++
 crypto/asymmetric_keys/Makefile             |   2 +
 crypto/asymmetric_keys/verify_pefile_test.c | 100 ++++++++++++++++++++
 3 files changed, 114 insertions(+)
 create mode 100644 crypto/asymmetric_keys/verify_pefile_test.c

index 6a2f66404..c73979b2e 100644
@@ -114,4 +114,16 @@ config FIPS_SIGNATURE_SELFTEST_ECDSA
 	depends on CRYPTO_SHA256=y || CRYPTO_SHA256=FIPS_SIGNATURE_SELFTEST
 	depends on CRYPTO_ECDSA=y || CRYPTO_ECDSA=FIPS_SIGNATURE_SELFTEST
 
+config VERIFY_PEFILE_KUNIT_TEST
+	bool "KUnit tests for signed PE file parsing" if !KUNIT_ALL_TESTS
+	depends on KUNIT=y && SIGNED_PE_FILE_VERIFICATION
+	default KUNIT_ALL_TESTS
+	help
+	  Enable KUnit tests for the PE binary parser used by signed PE file
+	  signature verification.  The tests build small malformed PE images in
+	  memory and check that pefile_parse_binary() rejects them instead of
+	  reading out of bounds.
+
+	  If unsure, say N.
+
 endif # ASYMMETRIC_KEY_TYPE

index bc65d3b98..35172685c 100644
@@ -79,3 +79,5 @@ verify_signed_pefile-y := \
 
 $(obj)/mscode_parser.o: $(obj)/mscode.asn1.h $(obj)/mscode.asn1.h
 $(obj)/mscode.asn1.o: $(obj)/mscode.asn1.c $(obj)/mscode.asn1.h
+
+obj-$(CONFIG_VERIFY_PEFILE_KUNIT_TEST) += verify_pefile_test.o

2.53.0

All applied. Thanks.

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt