In preparation to enabling -Wimplicit-fallthrough, mark switch
cases where we are expecting to fall through.
This patch fixes the following warnings:
lib/asn1_decoder.c:386:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
lib/asn1_decoder.c:449:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
Warning level 3 was used: -Wimplicit-fallthrough=3
This patch is part of the ongoing efforts to enabling
-Wimplicit-fallthrough.
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
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: 42d5ec27f8 ("X.509: Add an ASN.1 decoder")
Cc: <stable@vger.kernel.org> # v3.7+
Signed-off-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: James Morris <james.l.morris@oracle.com>
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: 42d5ec27f8 ("X.509: Add an ASN.1 decoder")
Reported-by: Alexander Potapenko <glider@google.com>
Cc: <stable@vger.kernel.org> # v3.7+
Signed-off-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: David Howells <dhowells@redhat.com>
syzkaller with KASAN reported an out-of-bounds read in
asn1_ber_decoder(). It can be reproduced by the following command,
assuming CONFIG_X509_CERTIFICATE_PARSER=y and CONFIG_KASAN=y:
keyctl add asymmetric desc $'\x30\x30' @s
The bug is that the length of an ASN.1 data value isn't validated in the
case where it is encoded using the short form, causing the decoder to
read past the end of the input buffer. Fix it by validating the length.
The bug report was:
BUG: KASAN: slab-out-of-bounds in asn1_ber_decoder+0x10cb/0x1730 lib/asn1_decoder.c:233
Read of size 1 at addr ffff88003cccfa02 by task syz-executor0/6818
CPU: 1 PID: 6818 Comm: syz-executor0 Not tainted 4.14.0-rc7-00008-g5f479447d983 #2
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:16 [inline]
dump_stack+0xb3/0x10b lib/dump_stack.c:52
print_address_description+0x79/0x2a0 mm/kasan/report.c:252
kasan_report_error mm/kasan/report.c:351 [inline]
kasan_report+0x236/0x340 mm/kasan/report.c:409
__asan_report_load1_noabort+0x14/0x20 mm/kasan/report.c:427
asn1_ber_decoder+0x10cb/0x1730 lib/asn1_decoder.c:233
x509_cert_parse+0x1db/0x650 crypto/asymmetric_keys/x509_cert_parser.c:89
x509_key_preparse+0x64/0x7a0 crypto/asymmetric_keys/x509_public_key.c:174
asymmetric_key_preparse+0xcb/0x1a0 crypto/asymmetric_keys/asymmetric_type.c:388
key_create_or_update+0x347/0xb20 security/keys/key.c:855
SYSC_add_key security/keys/keyctl.c:122 [inline]
SyS_add_key+0x1cd/0x340 security/keys/keyctl.c:62
entry_SYSCALL_64_fastpath+0x1f/0xbe
RIP: 0033:0x447c89
RSP: 002b:00007fca7a5d3bd8 EFLAGS: 00000246 ORIG_RAX: 00000000000000f8
RAX: ffffffffffffffda RBX: 00007fca7a5d46cc RCX: 0000000000447c89
RDX: 0000000020006f4a RSI: 0000000020006000 RDI: 0000000020001ff5
RBP: 0000000000000046 R08: fffffffffffffffd R09: 0000000000000000
R10: 0000000000000002 R11: 0000000000000246 R12: 0000000000000000
R13: 0000000000000000 R14: 00007fca7a5d49c0 R15: 00007fca7a5d4700
Fixes: 42d5ec27f8 ("X.509: Add an ASN.1 decoder")
Cc: <stable@vger.kernel.org> # v3.7+
Signed-off-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: James Morris <james.l.morris@oracle.com>
Pull crypto update from Herbert Xu:
"API:
- Crypto self tests can now be disabled at boot/run time.
- Add async support to algif_aead.
Algorithms:
- A large number of fixes to MPI from Nicolai Stange.
- Performance improvement for HMAC DRBG.
Drivers:
- Use generic crypto engine in omap-des.
- Merge ppc4xx-rng and crypto4xx drivers.
- Fix lockups in sun4i-ss driver by disabling IRQs.
- Add DMA engine support to ccp.
- Reenable talitos hash algorithms.
- Add support for Hisilicon SoC RNG.
- Add basic crypto driver for the MXC SCC.
Others:
- Do not allocate crypto hash tfm in NORECLAIM context in ecryptfs"
* 'linus' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6: (77 commits)
crypto: qat - change the adf_ctl_stop_devices to void
crypto: caam - fix caam_jr_alloc() ret code
crypto: vmx - comply with ABIs that specify vrsave as reserved.
crypto: testmgr - Add a flag allowing the self-tests to be disabled at runtime.
crypto: ccp - constify ccp_actions structure
crypto: marvell/cesa - Use dma_pool_zalloc
crypto: qat - make adf_vf_isr.c dependant on IOV config
crypto: qat - Fix typo in comments
lib: asn1_decoder - add MODULE_LICENSE("GPL")
crypto: omap-sham - Use dma_request_chan() for requesting DMA channel
crypto: omap-des - Use dma_request_chan() for requesting DMA channel
crypto: omap-aes - Use dma_request_chan() for requesting DMA channel
crypto: omap-des - Integrate with the crypto engine framework
crypto: s5p-sss - fix incorrect usage of scatterlists api
crypto: s5p-sss - Fix missed interrupts when working with 8 kB blocks
crypto: s5p-sss - Use common BIT macro
crypto: mxc-scc - fix unwinding in mxc_scc_crypto_register()
crypto: mxc-scc - signedness bugs in mxc_scc_ablkcipher_req_init()
crypto: talitos - fix ahash algorithms registration
crypto: ccp - Ensure all dependencies are specified
...
This fixes CVE-2016-0758.
In the ASN.1 decoder, when the length field of an ASN.1 value is extracted,
it isn't validated against the remaining amount of data before being added
to the cursor. With a sufficiently large size indicated, the check:
datalen - dp < 2
may then fail due to integer overflow.
Fix this by checking the length indicated against the amount of remaining
data in both places a definite length is determined.
Whilst we're at it, make the following changes:
(1) Check the maximum size of extended length does not exceed the capacity
of the variable it's being stored in (len) rather than the type that
variable is assumed to be (size_t).
(2) Compare the EOC tag to the symbolic constant ASN1_EOC rather than the
integer 0.
(3) To reduce confusion, move the initialisation of len outside of:
for (len = 0; n > 0; n--) {
since it doesn't have anything to do with the loop counter n.
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Acked-by: David Woodhouse <David.Woodhouse@intel.com>
Acked-by: Peter Jones <pjones@redhat.com>
A kernel taint results when loading the rsa_generic module:
root@(none):~# modprobe rsa_generic
asn1_decoder: module license 'unspecified' taints kernel.
Disabling lock debugging due to kernel taint
"Tainting" of the kernel is (usually) a way of indicating that
a proprietary module has been inserted, which is not the case here.
Signed-off-by: Tudor Ambarus <tudor-dan.ambarus@nxp.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
An ANY object in an ASN.1 grammar that is marked OPTIONAL should be skipped
if there is no more data to be had.
This can be tested by editing X.509 certificates or PKCS#7 messages to
remove the NULL from subobjects that look like the following:
SEQUENCE {
OBJECT(2a864886f70d01010b);
NULL();
}
This is an algorithm identifier plus an optional parameter.
The modified DER can be passed to one of:
keyctl padd asymmetric "" @s </tmp/modified.x509
keyctl padd pkcs7_test foo @s </tmp/modified.pkcs7
It should work okay with the patch and produce EBADMSG without.
Signed-off-by: David Howells <dhowells@redhat.com>
Tested-by: Marcel Holtmann <marcel@holtmann.org>
Reviewed-by: David Woodhouse <David.Woodhouse@intel.com>
If the ASN.1 decoder is asked to parse a sequence of objects, non-optional
matches get skipped if there's no more data to be had rather than a
data-overrun error being reported.
This is due to the code segment that decides whether to skip optional
matches (ie. matches that could get ignored because an element is marked
OPTIONAL in the grammar) due to a lack of data also skips non-optional
elements if the data pointer has reached the end of the buffer.
This can be tested with the data decoder for the new RSA akcipher algorithm
that takes three non-optional integers. Currently, it skips the last
integer if there is insufficient data.
Without the fix, #defining DEBUG in asn1_decoder.c will show something
like:
next_op: pc=0/13 dp=0/270 C=0 J=0
- match? 30 30 00
- TAG: 30 266 CONS
next_op: pc=2/13 dp=4/270 C=1 J=0
- match? 02 02 00
- TAG: 02 257
- LEAF: 257
next_op: pc=5/13 dp=265/270 C=1 J=0
- match? 02 02 00
- TAG: 02 3
- LEAF: 3
next_op: pc=8/13 dp=270/270 C=1 J=0
next_op: pc=11/13 dp=270/270 C=1 J=0
- end cons t=4 dp=270 l=270/270
The next_op line for pc=8/13 should be followed by a match line.
This is not exploitable for X.509 certificates by means of shortening the
message and fixing up the ASN.1 CONS tags because:
(1) The relevant records being built up are cleared before use.
(2) If the message is shortened sufficiently to remove the public key, the
ASN.1 parse of the RSA key will fail quickly due to a lack of data.
(3) Extracted signature data is either turned into MPIs (which cope with a
0 length) or is simpler integers specifying algoritms and suchlike
(which can validly be 0); and
(4) The AKID and SKID extensions are optional and their removal is handled
without risking passing a NULL to asymmetric_key_generate_id().
(5) If the certificate is truncated sufficiently to remove the subject,
issuer or serialNumber then the ASN.1 decoder will fail with a 'Cons
stack underflow' return.
This is not exploitable for PKCS#7 messages by means of removal of elements
from such a message from the tail end of a sequence:
(1) Any shortened X.509 certs embedded in the PKCS#7 message are survivable
as detailed above.
(2) The message digest content isn't used if it shows a NULL pointer,
similarly, the authattrs aren't used if that shows a NULL pointer.
(3) A missing signature results in a NULL MPI - which the MPI routines deal
with.
(4) If data is NULL, it is expected that the message has detached content and
that is handled appropriately.
(5) If the serialNumber is excised, the unconditional action associated
with it will pick up the containing SEQUENCE instead, so no NULL
pointer will be seen here.
If both the issuer and the serialNumber are excised, the ASN.1 decode
will fail with an 'Unexpected tag' return.
In either case, there's no way to get to asymmetric_key_generate_id()
with a NULL pointer.
(6) Other fields are decoded to simple integers. Shortening the message
to omit an algorithm ID field will cause checks on this to fail early
in the verification process.
This can also be tested by snipping objects off of the end of the ASN.1 stream
such that mandatory tags are removed - or even from the end of internal
SEQUENCEs. If any mandatory tag is missing, the error EBADMSG *should* be
produced. Without this patch ERANGE or ENOPKG might be produced or the parse
may apparently succeed, perhaps with ENOKEY or EKEYREJECTED being produced
later, depending on what gets snipped.
Just snipping off the final BIT_STRING or OCTET_STRING from either sample
should be a start since both are mandatory and neither will cause an EBADMSG
without the patches
Reported-by: Marcel Holtmann <marcel@holtmann.org>
Signed-off-by: David Howells <dhowells@redhat.com>
Tested-by: Marcel Holtmann <marcel@holtmann.org>
Reviewed-by: David Woodhouse <David.Woodhouse@intel.com>
In an ASN.1 description where there is a CHOICE construct that contains
elements with IMPLICIT tags that refer to constructed types, actions to be
taken on those elements should be conditional on the corresponding element
actually being matched. Currently, however, such actions are performed
unconditionally in the middle of processing the CHOICE.
For example, look at elements 'b' and 'e' here:
A ::= SEQUENCE {
CHOICE {
b [0] IMPLICIT B ({ do_XXXXXXXXXXXX_b }),
c [1] EXPLICIT C ({ do_XXXXXXXXXXXX_c }),
d [2] EXPLICIT B ({ do_XXXXXXXXXXXX_d }),
e [3] IMPLICIT C ({ do_XXXXXXXXXXXX_e }),
f [4] IMPLICIT INTEGER ({ do_XXXXXXXXXXXX_f })
}
} ({ do_XXXXXXXXXXXX_A })
B ::= SET OF OBJECT IDENTIFIER ({ do_XXXXXXXXXXXX_oid })
C ::= SET OF INTEGER ({ do_XXXXXXXXXXXX_int })
They each have an action (do_XXXXXXXXXXXX_b and do_XXXXXXXXXXXX_e) that
should only be processed if that element is matched.
The problem is that there's no easy place to hang the action off in the
subclause (type B for element 'b' and type C for element 'e') because
subclause opcode sequences can be shared.
To fix this, introduce a conditional action opcode(ASN1_OP_MAYBE_ACT) that
the decoder only processes if the preceding match was successful. This can
be seen in an excerpt from the output of the fixed ASN.1 compiler for the
above ASN.1 description:
[ 13] = ASN1_OP_COND_MATCH_JUMP_OR_SKIP, // e
[ 14] = _tagn(CONT, CONS, 3),
[ 15] = _jump_target(45), // --> C
[ 16] = ASN1_OP_MAYBE_ACT,
[ 17] = _action(ACT_do_XXXXXXXXXXXX_e),
In this, if the op at [13] is matched (ie. element 'e' above) then the
action at [16] will be performed. However, if the op at [13] doesn't match
or is skipped because it is conditional and some previous op matched, then
the action at [16] will be ignored.
Note that to make this work in the decoder, the ASN1_OP_RETURN op must set
the flag to indicate that a match happened. This is necessary because the
_jump_target() seen above introduces a subclause (in this case an object of
type 'C') which is likely to alter the flag. Setting the flag here is okay
because to process a subclause, a match must have happened and caused a
jump.
This cannot be tested with the code as it stands, but rather affects future
code.
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: David Woodhouse <David.Woodhouse@intel.com>
to verify the source of the module (ChromeOS) and/or use standard IMA on it
or other security hooks.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
iQIcBAABAgAGBQJQ0VKlAAoJENkgDmzRrbjxjuEQALVHpD1cSmryOzVwkNn7rVGP
PV3KVbUs+qzUCm2c3AafIIlSBm2LOUl+cR3uNC7di8aHarRF3VHkK2OQ4Fx97ECd
KKBqAyY3R0q1mAKujb/MWwiK0YgosEDIOzGGn2yQhNFsxKqnMB02P4j82IO7+g+w
Cc3XuDyWHoH2I+ySgz0Q8NHAqufD/DMZUKud7jw2Lsv6PuICJ1Oqgl/Gd/muxort
4a5tV3tjhRGywHS/8b2fbDUXkybC5NKK0FN+gyoaROmJ/THeHEQDGXZT9bc2vmVx
HvRy/5k8dzQ6LAJ2mLnPvy0pmv0u7NYMvjxTxxUlUkFMkYuVticikQfwSYDbDPt4
mbsLxchpgi8z4x8HltEERffCX5tldo/5hz1uemqhqIsMRIrRFnlHkSIgkGjVHf2u
LXQBLT8uTm6C0VyNQPrI/hUZzIax7WtKbPSoK9lmExNbKqloEFh/mVXvfQxei2kp
wnUZcnmPIqSvw7b4CWu7HibMYu2VvGBgm3YIfJRi4AQme1mzFYLpZoxF5Pj+Ykbt
T//Hb1EsNQTTFCg7MZhnJSAw/EVUvNDUoullORClyqw6+xxjVKqWpPJgYDRfWOlJ
Xa+s7DNrL+Oo1WWR8l5ruoQszbR8szIyeyPKKxRUcQj2zsqghoWuzKAx2saSEw3W
pNkoJU+dGC7kG/yVAS8N
=uoJj
-----END PGP SIGNATURE-----
Merge tag 'modules-next-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux
Pull module update from Rusty Russell:
"Nothing all that exciting; a new module-from-fd syscall for those who
want to verify the source of the module (ChromeOS) and/or use standard
IMA on it or other security hooks."
* tag 'modules-next-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/rusty/linux:
MODSIGN: Fix kbuild output when using default extra_certificates
MODSIGN: Avoid using .incbin in C source
modules: don't hand 0 to vmalloc.
module: Remove a extra null character at the top of module->strtab.
ASN.1: Use the ASN1_LONG_TAG and ASN1_INDEFINITE_LENGTH constants
ASN.1: Define indefinite length marker constant
moduleparam: use __UNIQUE_ID()
__UNIQUE_ID()
MODSIGN: Add modules_sign make target
powerpc: add finit_module syscall.
ima: support new kernel module syscall
add finit_module syscall to asm-generic
ARM: add finit_module syscall to ARM
security: introduce kernel_module_from_file hook
module: add flags arg to sys_finit_module()
module: add syscall to load module from fd
Use the ASN1_LONG_TAG and ASN1_INDEFINITE_LENGTH constants in the ASN.1
general decoder instead of the equivalent numbers.
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Fix an error in asn1_find_indefinite_length() whereby small definite length
elements of size 0x7f are incorrecly classified as non-small. Without this
fix, an error will be given as the length of the length will be perceived as
being very much greater than the maximum supported size.
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
asn1_find_indefinite_length() returns an error indicator of -1, which the
caller asn1_ber_decoder() places in a size_t (which is usually unsigned) and
then checks to see whether it is less than 0 (which it can't be). This can
lead to the following warning:
lib/asn1_decoder.c:320 asn1_ber_decoder()
warn: unsigned 'len' is never less than zero.
Instead, asn1_find_indefinite_length() update the caller's idea of the data
cursor and length separately from returning the error code.
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Add an ASN.1 BER/DER/CER decoder. This uses the bytecode from the ASN.1
compiler in the previous patch to inform it as to what to expect to find in the
encoded byte stream. The output from the compiler also tells it what functions
to call on what tags, thus allowing the caller to retrieve information.
The decoder is called as follows:
int asn1_decoder(const struct asn1_decoder *decoder,
void *context,
const unsigned char *data,
size_t datalen);
The decoder argument points to the bytecode from the ASN.1 compiler. context
is the caller's context and is passed to the action functions. data and
datalen define the byte stream to be decoded.
Note that the decoder is currently limited to datalen being less than 64K.
This reduces the amount of stack space used by the decoder because ASN.1 is a
nested construct. Similarly, the decoder is limited to a maximum of 10 levels
of constructed data outside of a leaf node also in an effort to keep stack
usage down.
These restrictions can be raised if necessary.
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Gustavo A. R. Silva