From e0058f3a874ebb48b25be7ff79bc3b4e59929f90 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Fri, 8 Dec 2017 15:13:27 +0000 Subject: ASN.1: fix out-of-bounds read when parsing indefinite length item In asn1_ber_decoder(), indefinitely-sized ASN.1 items were being passed to the action functions before their lengths had been computed, using the bogus length of 0x80 (ASN1_INDEFINITE_LENGTH). This resulted in reading data past the end of the input buffer, when given a specially crafted message. Fix it by rearranging the code so that the indefinite length is resolved before the action is called. This bug was originally found by fuzzing the X.509 parser in userspace using libFuzzer from the LLVM project. KASAN report (cleaned up slightly): BUG: KASAN: slab-out-of-bounds in memcpy ./include/linux/string.h:341 [inline] BUG: KASAN: slab-out-of-bounds in x509_fabricate_name.constprop.1+0x1a4/0x940 crypto/asymmetric_keys/x509_cert_parser.c:366 Read of size 128 at addr ffff880035dd9eaf by task keyctl/195 CPU: 1 PID: 195 Comm: keyctl Not tainted 4.14.0-09238-g1d3b78bbc6e9 #26 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.11.0-20171110_100015-anatol 04/01/2014 Call Trace: __dump_stack lib/dump_stack.c:17 [inline] dump_stack+0xd1/0x175 lib/dump_stack.c:53 print_address_description+0x78/0x260 mm/kasan/report.c:252 kasan_report_error mm/kasan/report.c:351 [inline] kasan_report+0x23f/0x350 mm/kasan/report.c:409 memcpy+0x1f/0x50 mm/kasan/kasan.c:302 memcpy ./include/linux/string.h:341 [inline] x509_fabricate_name.constprop.1+0x1a4/0x940 crypto/asymmetric_keys/x509_cert_parser.c:366 asn1_ber_decoder+0xb4a/0x1fd0 lib/asn1_decoder.c:447 x509_cert_parse+0x1c7/0x620 crypto/asymmetric_keys/x509_cert_parser.c:89 x509_key_preparse+0x61/0x750 crypto/asymmetric_keys/x509_public_key.c:174 asymmetric_key_preparse+0xa4/0x150 crypto/asymmetric_keys/asymmetric_type.c:388 key_create_or_update+0x4d4/0x10a0 security/keys/key.c:850 SYSC_add_key security/keys/keyctl.c:122 [inline] SyS_add_key+0xe8/0x290 security/keys/keyctl.c:62 entry_SYSCALL_64_fastpath+0x1f/0x96 Allocated by task 195: __do_kmalloc_node mm/slab.c:3675 [inline] __kmalloc_node+0x47/0x60 mm/slab.c:3682 kvmalloc ./include/linux/mm.h:540 [inline] SYSC_add_key security/keys/keyctl.c:104 [inline] SyS_add_key+0x19e/0x290 security/keys/keyctl.c:62 entry_SYSCALL_64_fastpath+0x1f/0x96 Fixes: 42d5ec27f873 ("X.509: Add an ASN.1 decoder") Reported-by: Alexander Potapenko Cc: # v3.7+ Signed-off-by: Eric Biggers Signed-off-by: David Howells --- lib/asn1_decoder.c | 47 ++++++++++++++++++++++++++--------------------- 1 file changed, 26 insertions(+), 21 deletions(-) (limited to 'lib') diff --git a/lib/asn1_decoder.c b/lib/asn1_decoder.c index 1ef0cec38d78..d77cdfc4b554 100644 --- a/lib/asn1_decoder.c +++ b/lib/asn1_decoder.c @@ -313,42 +313,47 @@ next_op: /* Decide how to handle the operation */ switch (op) { - case ASN1_OP_MATCH_ANY_ACT: - case ASN1_OP_MATCH_ANY_ACT_OR_SKIP: - case ASN1_OP_COND_MATCH_ANY_ACT: - case ASN1_OP_COND_MATCH_ANY_ACT_OR_SKIP: - ret = actions[machine[pc + 1]](context, hdr, tag, data + dp, len); - if (ret < 0) - return ret; - goto skip_data; - - case ASN1_OP_MATCH_ACT: - case ASN1_OP_MATCH_ACT_OR_SKIP: - case ASN1_OP_COND_MATCH_ACT_OR_SKIP: - ret = actions[machine[pc + 2]](context, hdr, tag, data + dp, len); - if (ret < 0) - return ret; - goto skip_data; - case ASN1_OP_MATCH: case ASN1_OP_MATCH_OR_SKIP: + case ASN1_OP_MATCH_ACT: + case ASN1_OP_MATCH_ACT_OR_SKIP: case ASN1_OP_MATCH_ANY: case ASN1_OP_MATCH_ANY_OR_SKIP: + case ASN1_OP_MATCH_ANY_ACT: + case ASN1_OP_MATCH_ANY_ACT_OR_SKIP: case ASN1_OP_COND_MATCH_OR_SKIP: + case ASN1_OP_COND_MATCH_ACT_OR_SKIP: case ASN1_OP_COND_MATCH_ANY: case ASN1_OP_COND_MATCH_ANY_OR_SKIP: - skip_data: + case ASN1_OP_COND_MATCH_ANY_ACT: + case ASN1_OP_COND_MATCH_ANY_ACT_OR_SKIP: + if (!(flags & FLAG_CONS)) { if (flags & FLAG_INDEFINITE_LENGTH) { + size_t tmp = dp; + ret = asn1_find_indefinite_length( - data, datalen, &dp, &len, &errmsg); + data, datalen, &tmp, &len, &errmsg); if (ret < 0) goto error; - } else { - dp += len; } pr_debug("- LEAF: %zu\n", len); } + + if (op & ASN1_OP_MATCH__ACT) { + unsigned char act; + + if (op & ASN1_OP_MATCH__ANY) + act = machine[pc + 1]; + else + act = machine[pc + 2]; + ret = actions[act](context, hdr, tag, data + dp, len); + if (ret < 0) + return ret; + } + + if (!(flags & FLAG_CONS)) + dp += len; pc += asn1_op_lengths[op]; goto next_op; -- cgit v1.2.3 From 81a7be2cd69b412ab6aeacfe5ebf1bb6e5bce955 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Fri, 8 Dec 2017 15:13:27 +0000 Subject: ASN.1: check for error from ASN1_OP_END__ACT actions asn1_ber_decoder() was ignoring errors from actions associated with the opcodes ASN1_OP_END_SEQ_ACT, ASN1_OP_END_SET_ACT, ASN1_OP_END_SEQ_OF_ACT, and ASN1_OP_END_SET_OF_ACT. In practice, this meant the pkcs7_note_signed_info() action (since that was the only user of those opcodes). Fix it by checking for the error, just like the decoder does for actions associated with the other opcodes. This bug allowed users to leak slab memory by repeatedly trying to add a specially crafted "pkcs7_test" key (requires CONFIG_PKCS7_TEST_KEY). In theory, this bug could also be used to bypass module signature verification, by providing a PKCS#7 message that is misparsed such that a signature's ->authattrs do not contain its ->msgdigest. But it doesn't seem practical in normal cases, due to restrictions on the format of the ->authattrs. Fixes: 42d5ec27f873 ("X.509: Add an ASN.1 decoder") Cc: # v3.7+ Signed-off-by: Eric Biggers Signed-off-by: David Howells Reviewed-by: James Morris --- lib/asn1_decoder.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'lib') diff --git a/lib/asn1_decoder.c b/lib/asn1_decoder.c index d77cdfc4b554..dc14beae2c9a 100644 --- a/lib/asn1_decoder.c +++ b/lib/asn1_decoder.c @@ -439,6 +439,8 @@ next_op: else act = machine[pc + 1]; ret = actions[act](context, hdr, 0, data + tdp, len); + if (ret < 0) + return ret; } pc += asn1_op_lengths[op]; goto next_op; -- cgit v1.2.3 From 47e0a208fb9d91e3f3c86309e752b13a36470ae8 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Fri, 8 Dec 2017 15:13:28 +0000 Subject: X.509: fix buffer overflow detection in sprint_oid() In sprint_oid(), if the input buffer were to be more than 1 byte too small for the first snprintf(), 'bufsize' would underflow, causing a buffer overflow when printing the remainder of the OID. Fortunately this cannot actually happen currently, because no users pass in a buffer that can be too small for the first snprintf(). Regardless, fix it by checking the snprintf() return value correctly. For consistency also tweak the second snprintf() check to look the same. Fixes: 4f73175d0375 ("X.509: Add utility functions to render OIDs as strings") Cc: Takashi Iwai Signed-off-by: Eric Biggers Signed-off-by: David Howells Reviewed-by: James Morris --- lib/oid_registry.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'lib') diff --git a/lib/oid_registry.c b/lib/oid_registry.c index 41b9e50711a7..5a75d127995d 100644 --- a/lib/oid_registry.c +++ b/lib/oid_registry.c @@ -120,10 +120,10 @@ int sprint_oid(const void *data, size_t datasize, char *buffer, size_t bufsize) n = *v++; ret = count = snprintf(buffer, bufsize, "%u.%u", n / 40, n % 40); + if (count >= bufsize) + return -ENOBUFS; buffer += count; bufsize -= count; - if (bufsize == 0) - return -ENOBUFS; while (v < end) { num = 0; @@ -141,9 +141,9 @@ int sprint_oid(const void *data, size_t datasize, char *buffer, size_t bufsize) } while (n & 0x80); } ret += count = snprintf(buffer, bufsize, ".%lu", num); - buffer += count; - if (bufsize <= count) + if (count >= bufsize) return -ENOBUFS; + buffer += count; bufsize -= count; } -- cgit v1.2.3 From 8dfd2f22d3bf3ab7714f7495ad5d897b8845e8c1 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Fri, 8 Dec 2017 15:13:28 +0000 Subject: 509: fix printing uninitialized stack memory when OID is empty Callers of sprint_oid() do not check its return value before printing the result. In the case where the OID is zero-length, -EBADMSG was being returned without anything being written to the buffer, resulting in uninitialized stack memory being printed. Fix this by writing "(bad)" to the buffer in the cases where -EBADMSG is returned. Fixes: 4f73175d0375 ("X.509: Add utility functions to render OIDs as strings") Signed-off-by: Eric Biggers Signed-off-by: David Howells --- lib/oid_registry.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) (limited to 'lib') diff --git a/lib/oid_registry.c b/lib/oid_registry.c index 5a75d127995d..0bcac6ccb1b2 100644 --- a/lib/oid_registry.c +++ b/lib/oid_registry.c @@ -116,7 +116,7 @@ int sprint_oid(const void *data, size_t datasize, char *buffer, size_t bufsize) int count; if (v >= end) - return -EBADMSG; + goto bad; n = *v++; ret = count = snprintf(buffer, bufsize, "%u.%u", n / 40, n % 40); @@ -134,7 +134,7 @@ int sprint_oid(const void *data, size_t datasize, char *buffer, size_t bufsize) num = n & 0x7f; do { if (v >= end) - return -EBADMSG; + goto bad; n = *v++; num <<= 7; num |= n & 0x7f; @@ -148,6 +148,10 @@ int sprint_oid(const void *data, size_t datasize, char *buffer, size_t bufsize) } return ret; + +bad: + snprintf(buffer, bufsize, "(bad)"); + return -EBADMSG; } EXPORT_SYMBOL_GPL(sprint_oid); -- cgit v1.2.3