Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1331522

[PATCH 3.13.y-ckt 06/30] ASN.1: Fix non-match detection failure on data overrun

Show all headers | View raw

3.13.11-ckt35 -stable review patch.  If anyone has any objections, please let me know.

---8<------------------------------------------------------------

From: David Howells <dhowells@redhat.com>

commit 0d62e9dd6da45bbf0f33a8617afc5fe774c8f45f upstream.

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>
[ kamal: backport to 3.19-stable: context ]
Signed-off-by: Kamal Mostafa <kamal@canonical.com>
---
 lib/asn1_decoder.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/asn1_decoder.c b/lib/asn1_decoder.c
index 11b9b01..3787d02 100644
--- a/lib/asn1_decoder.c
+++ b/lib/asn1_decoder.c
@@ -208,9 +208,8 @@ next_op:
 		unsigned char tmp;
 
 		/* Skip conditional matches if possible */
-		if ((op & ASN1_OP_MATCH__COND &&
-		     flags & FLAG_MATCHED) ||
-		    dp == datalen) {
+		if ((op & ASN1_OP_MATCH__COND && flags & FLAG_MATCHED) ||
+		    (op & ASN1_OP_MATCH__SKIP && dp == datalen)) {
 			pc += asn1_op_lengths[op];
 			goto next_op;
 		}
-- 
1.9.1

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[3.13.y-ckt stable] Linux 3.13.11-ckt35 stable review Kamal Mostafa <kamal@canonical.com> - 2016-02-10 22:50 +0100
  [PATCH 3.13.y-ckt 26/30] ALSA: dummy: Disable switching timer backend via sysfs Kamal Mostafa <kamal@canonical.com> - 2016-02-10 22:50 +0100
  [PATCH 3.13.y-ckt 30/30] powerpc/perf: Remove PPMU_HAS_SSLOT flag for Power8 Kamal Mostafa <kamal@canonical.com> - 2016-02-10 22:50 +0100
  [PATCH 3.13.y-ckt 20/30] USB: option: fix Cinterion AHxx enumeration Kamal Mostafa <kamal@canonical.com> - 2016-02-10 22:50 +0100
  [PATCH 3.13.y-ckt 27/30] drm/vmwgfx: respect 'nomodeset' Kamal Mostafa <kamal@canonical.com> - 2016-02-10 22:50 +0100
  [PATCH 3.13.y-ckt 18/30] ALSA: seq: Degrade the error message for too many opens Kamal Mostafa <kamal@canonical.com> - 2016-02-10 22:50 +0100
  [PATCH 3.13.y-ckt 10/30] usb: cdc-acm: handle unlinked urb in acm read callback Kamal Mostafa <kamal@canonical.com> - 2016-02-10 22:50 +0100
  [PATCH 3.13.y-ckt 07/30] qeth: initialize net_device with carrier off Kamal Mostafa <kamal@canonical.com> - 2016-02-10 22:50 +0100
  [PATCH 3.13.y-ckt 05/30] USB: serial: visor: fix crash on detecting device without write_urbs Kamal Mostafa <kamal@canonical.com> - 2016-02-10 22:50 +0100
  [PATCH 3.13.y-ckt 09/30] x86/irq: Call chip->irq_set_affinity in proper context Kamal Mostafa <kamal@canonical.com> - 2016-02-10 22:50 +0100
  [PATCH 3.13.y-ckt 24/30] SCSI: fix crashes in sd and sr runtime PM Kamal Mostafa <kamal@canonical.com> - 2016-02-10 22:50 +0100
  [PATCH 3.13.y-ckt 06/30] ASN.1: Fix non-match detection failure on data overrun Kamal Mostafa <kamal@canonical.com> - 2016-02-10 22:50 +0100
  [PATCH 3.13.y-ckt 22/30] ALSA: usb-audio: Fix TEAC UD-501/UD-503/NT-503 usb delay Kamal Mostafa <kamal@canonical.com> - 2016-02-10 22:50 +0100
  [PATCH 3.13.y-ckt 17/30] ALSA: seq: Fix incorrect sanity check at snd_seq_oss_synth_cleanup() Kamal Mostafa <kamal@canonical.com> - 2016-02-10 22:50 +0100
  [PATCH 3.13.y-ckt 14/30] USB: cp210x: add ID for IAI USB to RS485 adaptor Kamal Mostafa <kamal@canonical.com> - 2016-02-10 22:50 +0100
  [PATCH 3.13.y-ckt 02/30] [media] usbvision: fix leak of usb_dev on failure paths in usbvision_probe() Kamal Mostafa <kamal@canonical.com> - 2016-02-10 23:00 +0100
  [PATCH 3.13.y-ckt 08/30] iio: adis_buffer: Fix out-of-bounds memory access Kamal Mostafa <kamal@canonical.com> - 2016-02-10 23:00 +0100
  [PATCH 3.13.y-ckt 03/30] [media] usbvision: fix crash on detecting device with invalid configuration Kamal Mostafa <kamal@canonical.com> - 2016-02-10 23:00 +0100

csiph-web