1. 08 Dec, 2017 2 commits
    • Eric Biggers's avatar
      ASN.1: check for error from ASN1_OP_END__ACT actions · 81a7be2c
      Eric Biggers authored
      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: 42d5ec27 ("X.509: Add an ASN.1 decoder")
      Cc: <stable@vger.kernel.org> # v3.7+
      Signed-off-by: 's avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: 's avatarDavid Howells <dhowells@redhat.com>
      Reviewed-by: 's avatarJames Morris <james.l.morris@oracle.com>
      81a7be2c
    • Eric Biggers's avatar
      ASN.1: fix out-of-bounds read when parsing indefinite length item · e0058f3a
      Eric Biggers authored
      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-g1d3b78bb #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: 42d5ec27 ("X.509: Add an ASN.1 decoder")
      Reported-by: 's avatarAlexander Potapenko <glider@google.com>
      Cc: <stable@vger.kernel.org> # v3.7+
      Signed-off-by: 's avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: 's avatarDavid Howells <dhowells@redhat.com>
      e0058f3a
  2. 08 Nov, 2017 1 commit
    • Eric Biggers's avatar
      KEYS: fix NULL pointer dereference during ASN.1 parsing [ver #2] · 624f5ab8
      Eric Biggers authored
      syzkaller reported a NULL pointer dereference in asn1_ber_decoder().  It
      can be reproduced by the following command, assuming
      CONFIG_PKCS7_TEST_KEY=y:
      
              keyctl add pkcs7_test desc '' @s
      
      The bug is that if the data buffer is empty, an integer underflow occurs
      in the following check:
      
              if (unlikely(dp >= datalen - 1))
                      goto data_overrun_error;
      
      This results in the NULL data pointer being dereferenced.
      
      Fix it by checking for 'datalen - dp < 2' instead.
      
      Also fix the similar check for 'dp >= datalen - n' later in the same
      function.  That one possibly could result in a buffer overread.
      
      The NULL pointer dereference was reproducible using the "pkcs7_test" key
      type but not the "asymmetric" key type because the "asymmetric" key type
      checks for a 0-length payload before calling into the ASN.1 decoder but
      the "pkcs7_test" key type does not.
      
      The bug report was:
      
          BUG: unable to handle kernel NULL pointer dereference at           (null)
          IP: asn1_ber_decoder+0x17f/0xe60 lib/asn1_decoder.c:233
          PGD 7b708067 P4D 7b708067 PUD 7b6ee067 PMD 0
          Oops: 0000 [#1] SMP
          Modules linked in:
          CPU: 0 PID: 522 Comm: syz-executor1 Not tainted 4.14.0-rc8 #7
          Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.3-20171021_125229-anatol 04/01/2014
          task: ffff9b6b3798c040 task.stack: ffff9b6b37970000
          RIP: 0010:asn1_ber_decoder+0x17f/0xe60 lib/asn1_decoder.c:233
          RSP: 0018:ffff9b6b37973c78 EFLAGS: 00010216
          RAX: 0000000000000000 RBX: 0000000000000000 RCX: 000000000000021c
          RDX: ffffffff814a04ed RSI: ffffb1524066e000 RDI: ffffffff910759e0
          RBP: ffff9b6b37973d60 R08: 0000000000000001 R09: ffff9b6b3caa4180
          R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000002
          R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
          FS:  00007f10ed1f2700(0000) GS:ffff9b6b3ea00000(0000) knlGS:0000000000000000
          CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
          CR2: 0000000000000000 CR3: 000000007b6f3000 CR4: 00000000000006f0
          Call Trace:
           pkcs7_parse_message+0xee/0x240 crypto/asymmetric_keys/pkcs7_parser.c:139
           verify_pkcs7_signature+0x33/0x180 certs/system_keyring.c:216
           pkcs7_preparse+0x41/0x70 crypto/asymmetric_keys/pkcs7_key_type.c:63
           key_create_or_update+0x180/0x530 security/keys/key.c:855
           SYSC_add_key security/keys/keyctl.c:122 [inline]
           SyS_add_key+0xbf/0x250 security/keys/keyctl.c:62
           entry_SYSCALL_64_fastpath+0x1f/0xbe
          RIP: 0033:0x4585c9
          RSP: 002b:00007f10ed1f1bd8 EFLAGS: 00000216 ORIG_RAX: 00000000000000f8
          RAX: ffffffffffffffda RBX: 00007f10ed1f2700 RCX: 00000000004585c9
          RDX: 0000000020000000 RSI: 0000000020008ffb RDI: 0000000020008000
          RBP: 0000000000000000 R08: ffffffffffffffff R09: 0000000000000000
          R10: 0000000000000000 R11: 0000000000000216 R12: 00007fff1b2260ae
          R13: 00007fff1b2260af R14: 00007f10ed1f2700 R15: 0000000000000000
          Code: dd ca ff 48 8b 45 88 48 83 e8 01 4c 39 f0 0f 86 a8 07 00 00 e8 53 dd ca ff 49 8d 46 01 48 89 85 58 ff ff ff 48 8b 85 60 ff ff ff <42> 0f b6 0c 30 89 c8 88 8d 75 ff ff ff 83 e0 1f 89 8d 28 ff ff
          RIP: asn1_ber_decoder+0x17f/0xe60 lib/asn1_decoder.c:233 RSP: ffff9b6b37973c78
          CR2: 0000000000000000
      
      Fixes: 42d5ec27 ("X.509: Add an ASN.1 decoder")
      Reported-by: 's avatarsyzbot <syzkaller@googlegroups.com>
      Cc: <stable@vger.kernel.org> # v3.7+
      Signed-off-by: 's avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: 's avatarDavid Howells <dhowells@redhat.com>
      Signed-off-by: 's avatarJames Morris <james.l.morris@oracle.com>
      624f5ab8
  3. 02 Nov, 2017 1 commit
    • Eric Biggers's avatar
      KEYS: fix out-of-bounds read during ASN.1 parsing · 2eb9eabf
      Eric Biggers authored
      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-g5f479447 #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: 42d5ec27 ("X.509: Add an ASN.1 decoder")
      Cc: <stable@vger.kernel.org> # v3.7+
      Signed-off-by: 's avatarEric Biggers <ebiggers@google.com>
      Signed-off-by: 's avatarDavid Howells <dhowells@redhat.com>
      Signed-off-by: 's avatarJames Morris <james.l.morris@oracle.com>
      2eb9eabf
  4. 12 May, 2016 1 commit
    • David Howells's avatar
      KEYS: Fix ASN.1 indefinite length object parsing · 23c8a812
      David Howells authored
      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: 's avatarDavid Howells <dhowells@redhat.com>
      Reviewed-by: 's avatarMimi Zohar <zohar@linux.vnet.ibm.com>
      Acked-by: 's avatarDavid Woodhouse <David.Woodhouse@intel.com>
      Acked-by: 's avatarPeter Jones <pjones@redhat.com>
      23c8a812
  5. 11 May, 2016 1 commit
  6. 03 May, 2016 1 commit
  7. 05 Aug, 2015 3 commits
    • David Howells's avatar
      ASN.1: Handle 'ANY OPTIONAL' in grammar · 233ce79d
      David Howells authored
      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: 's avatarDavid Howells <dhowells@redhat.com>
      Tested-by: 's avatarMarcel Holtmann <marcel@holtmann.org>
      Reviewed-by: 's avatarDavid Woodhouse <David.Woodhouse@intel.com>
      233ce79d
    • David Howells's avatar
      ASN.1: Fix non-match detection failure on data overrun · 0d62e9dd
      David Howells authored
      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: 's avatarMarcel Holtmann <marcel@holtmann.org>
      Signed-off-by: 's avatarDavid Howells <dhowells@redhat.com>
      Tested-by: 's avatarMarcel Holtmann <marcel@holtmann.org>
      Reviewed-by: 's avatarDavid Woodhouse <David.Woodhouse@intel.com>
      0d62e9dd
    • David Howells's avatar
      ASN.1: Fix actions on CHOICE elements with IMPLICIT tags · 3f3af97d
      David Howells authored
      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: 's avatarDavid Howells <dhowells@redhat.com>
      Reviewed-by: 's avatarDavid Woodhouse <David.Woodhouse@intel.com>
      3f3af97d
  8. 04 Jun, 2014 1 commit
  9. 14 Dec, 2012 1 commit
  10. 05 Dec, 2012 1 commit
  11. 10 Oct, 2012 1 commit
  12. 08 Oct, 2012 1 commit
    • David Howells's avatar
      X.509: Add an ASN.1 decoder · 42d5ec27
      David Howells authored
      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: 's avatarDavid Howells <dhowells@redhat.com>
      Signed-off-by: 's avatarRusty Russell <rusty@rustcorp.com.au>
      42d5ec27