ASN.1 aligned PER dissector fails to check return value of g_malloc()

This issue was migrated from bug 8722 in our old bug tracker.

Original bug information:

Reporter: Oliver-Tobias Ripka
Status: RESOLVED FIXED
Product: Wireshark
Component: Dissection engine (libwireshark)
OS: All
Platform: All
Version: Git

Attachments:

crash1.pcap: Trace to reproduce crash

See also: http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2013-4935

Oliver-Tobias Ripka said:

Build Information:

The "dissect_per_restricted_character_string_sorted()" function ins packet-per.c fails to check the return value of g_malloc() in line 638:

	buf = (guint8 *)g_malloc(length+1);
	old_offset=offset;

If a large value for the length variable is set e.g 0xffffffff, the malloc fails and the application crashes with a segmentation fault (invalid write, tested on Linux).

This may result in a write to the address 0x00000000 later in the same function at multiple locations (assignments to buf):

		if((bits_per_char==8) || (alphabet==NULL)){
here -> 		buf[char_pos]=val;
		} else {
			if (val < alphabet_length){
here ->				buf[char_pos]=alphabet[val];
			} else {
here -> 			buf[char_pos] = '?';	/* XXX - how to mark this? */
			}
		}

This bug was found by fuzzing the ULP protocol against the latest git version of Wireshark.

added libwireshark versiondev labels

Evan Huus said:

I don't doubt you've found a bug, but g_malloc never returns NULL so that's probably not the source. If you could attach the fuzzed capture to this bug that would make it much easier to determine the actual cause.

Thanks, Evan

Oliver-Tobias Ripka said:

Created attachment 10853 Trace to reproduce crash

Oliver-Tobias Ripka said:

To reproduce the crash, open the file in Wireshark, select a packet and decode as ULP.

Guy Harris said:

(In reply to comment #1)  
> I don't doubt you've found a bug, but g_malloc never returns NULL  

Actually, sometimes it does, as its javadoc comment says:

/**
 * g_malloc:
 * @n_bytes: the number of bytes to allocate
 *
 * Allocates @n_bytes bytes of memory.
 * If @n_bytes is 0 it returns %NULL.
 *
 * Returns: a pointer to the allocated memory
 */

In 32-bit arithmetic, 0xffffffff+1 = 0, so if the length in question is 0xffffffff, it gets asked by the code in question to allocate 0 bytes, and hence returns NULL.

The right thing to do here is probably to figure out what the right tvb_ensure_bytes_exists() call, to perform *before* allocating the buffer, is, so that a very large length throws a bounds error exception before the g_malloc() call.  (That would also prevent g_malloc() from calling abort() for very large lengths that *aren't* 0xffffffff.)

Evan Huus said:

I am not seeing the crash you describe when I decode this file as ULP, however I do get a number of valgrind errors:

==28152== Conditional jump or move depends on uninitialised value(s)
==28152==    at 0x640A139: fast_ensure_contiguous (tvbuff.c:999)
==28152==    by 0x640A55D: tvb_get_guint8 (tvbuff.c:1213)
==28152==    by 0x68CCDAA: dissect_per_boolean (packet-per.c:968)
==28152==    by 0x68D00CC: dissect_per_choice (packet-per.c:1589)
==28152==    by 0x6C7F5C9: dissect_ulp_UlpMessage (ulp.cnf:39)
==28152==    by 0x68D041E: dissect_per_sequence (packet-per.c:1793)
==28152==    by 0x6C81B16: dissect_ULP_PDU_PDU (ulp.cnf:34)
==28152==    by 0x6A45328: tcp_dissect_pdus (packet-tcp.c:2353)
==28152==    by 0x6C7DE16: dissect_ulp_tcp (packet-ulp-template.c:86)
==28152==    by 0x63D9037: call_dissector_through_handle (packet.c:458)
==28152==    by 0x63D986C: call_dissector_work (packet.c:552)
==28152==    by 0x63DA0BF: dissector_try_uint_new (packet.c:969)
==28152==
==28152== Use of uninitialised value of size 8
==28152==    at 0x640A55E: tvb_get_guint8 (tvbuff.c:1214)
==28152==    by 0x68CCDAA: dissect_per_boolean (packet-per.c:968)
==28152==    by 0x68D00CC: dissect_per_choice (packet-per.c:1589)
==28152==    by 0x6C7F5C9: dissect_ulp_UlpMessage (ulp.cnf:39)
==28152==    by 0x68D041E: dissect_per_sequence (packet-per.c:1793)
==28152==    by 0x6C81B16: dissect_ULP_PDU_PDU (ulp.cnf:34)
==28152==    by 0x6A45328: tcp_dissect_pdus (packet-tcp.c:2353)
==28152==    by 0x6C7DE16: dissect_ulp_tcp (packet-ulp-template.c:86)
==28152==    by 0x63D9037: call_dissector_through_handle (packet.c:458)
==28152==    by 0x63D986C: call_dissector_work (packet.c:552)
==28152==    by 0x63DA0BF: dissector_try_uint_new (packet.c:969)
==28152==    by 0x63DA116: dissector_try_uint (packet.c:995)
==28152==
==28152== Conditional jump or move depends on uninitialised value(s)
==28152==    at 0x640A139: fast_ensure_contiguous (tvbuff.c:999)
==28152==    by 0x640A55D: tvb_get_guint8 (tvbuff.c:1213)
==28152==    by 0x640A89C: _tvb_get_bits64 (tvbuff.c:1835)
==28152==    by 0x68CE031: dissect_per_constrained_integer (packet-per.c:1218)
==28152==    by 0x68CFEFD: dissect_per_choice (packet-per.c:1612)
==28152==    by 0x6C7F5C9: dissect_ulp_UlpMessage (ulp.cnf:39)
==28152==    by 0x68D041E: dissect_per_sequence (packet-per.c:1793)
==28152==    by 0x6C81B16: dissect_ULP_PDU_PDU (ulp.cnf:34)
==28152==    by 0x6A45328: tcp_dissect_pdus (packet-tcp.c:2353)
==28152==    by 0x6C7DE16: dissect_ulp_tcp (packet-ulp-template.c:86)
==28152==    by 0x63D9037: call_dissector_through_handle (packet.c:458)
==28152==    by 0x63D986C: call_dissector_work (packet.c:552)
==28152==
==28152== Use of uninitialised value of size 8
==28152==    at 0x640A55E: tvb_get_guint8 (tvbuff.c:1214)
==28152==    by 0x640A89C: _tvb_get_bits64 (tvbuff.c:1835)
==28152==    by 0x68CE031: dissect_per_constrained_integer (packet-per.c:1218)
==28152==    by 0x68CFEFD: dissect_per_choice (packet-per.c:1612)
==28152==    by 0x6C7F5C9: dissect_ulp_UlpMessage (ulp.cnf:39)
==28152==    by 0x68D041E: dissect_per_sequence (packet-per.c:1793)
==28152==    by 0x6C81B16: dissect_ULP_PDU_PDU (ulp.cnf:34)
==28152==    by 0x6A45328: tcp_dissect_pdus (packet-tcp.c:2353)
==28152==    by 0x6C7DE16: dissect_ulp_tcp (packet-ulp-template.c:86)
==28152==    by 0x63D9037: call_dissector_through_handle (packet.c:458)
==28152==    by 0x63D986C: call_dissector_work (packet.c:552)
==28152==    by 0x63DA0BF: dissector_try_uint_new (packet.c:969)
==28152==
==28152== Conditional jump or move depends on uninitialised value(s)
==28152==    at 0x640A139: fast_ensure_contiguous (tvbuff.c:999)
==28152==    by 0x640A55D: tvb_get_guint8 (tvbuff.c:1213)
==28152==    by 0x68CCDAA: dissect_per_boolean (packet-per.c:968)
==28152==    by 0x68D08C9: dissect_per_sequence (packet-per.c:1748)
==28152==    by 0x6C7EAFA: dissect_ulp_SUPLEND (ulp.cnf:754)
==28152==    by 0x68D00A1: dissect_per_choice (packet-per.c:1642)
==28152==    by 0x6C7F5C9: dissect_ulp_UlpMessage (ulp.cnf:39)
==28152==    by 0x68D041E: dissect_per_sequence (packet-per.c:1793)
==28152==    by 0x6C81B16: dissect_ULP_PDU_PDU (ulp.cnf:34)
==28152==    by 0x6A45328: tcp_dissect_pdus (packet-tcp.c:2353)
==28152==    by 0x6C7DE16: dissect_ulp_tcp (packet-ulp-template.c:86)
==28152==    by 0x63D9037: call_dissector_through_handle (packet.c:458)
==28152==
==28152== Use of uninitialised value of size 8
==28152==    at 0x640A55E: tvb_get_guint8 (tvbuff.c:1214)
==28152==    by 0x68CCDAA: dissect_per_boolean (packet-per.c:968)
==28152==    by 0x68D08C9: dissect_per_sequence (packet-per.c:1748)
==28152==    by 0x6C7EAFA: dissect_ulp_SUPLEND (ulp.cnf:754)
==28152==    by 0x68D00A1: dissect_per_choice (packet-per.c:1642)
==28152==    by 0x6C7F5C9: dissect_ulp_UlpMessage (ulp.cnf:39)
==28152==    by 0x68D041E: dissect_per_sequence (packet-per.c:1793)
==28152==    by 0x6C81B16: dissect_ULP_PDU_PDU (ulp.cnf:34)
==28152==    by 0x6A45328: tcp_dissect_pdus (packet-tcp.c:2353)
==28152==    by 0x6C7DE16: dissect_ulp_tcp (packet-ulp-template.c:86)
==28152==    by 0x63D9037: call_dissector_through_handle (packet.c:458)
==28152==    by 0x63D986C: call_dissector_work (packet.c:552)
==28152==
==28152== Conditional jump or move depends on uninitialised value(s)
==28152==    at 0x640A139: fast_ensure_contiguous (tvbuff.c:999)
==28152==    by 0x640A55D: tvb_get_guint8 (tvbuff.c:1213)
==28152==    by 0x68CCDAA: dissect_per_boolean (packet-per.c:968)
==28152==    by 0x68D0325: dissect_per_sequence (packet-per.c:1764)
==28152==    by 0x6C7EAFA: dissect_ulp_SUPLEND (ulp.cnf:754)
==28152==    by 0x68D00A1: dissect_per_choice (packet-per.c:1642)
==28152==    by 0x6C7F5C9: dissect_ulp_UlpMessage (ulp.cnf:39)
==28152==    by 0x68D041E: dissect_per_sequence (packet-per.c:1793)
==28152==    by 0x6C81B16: dissect_ULP_PDU_PDU (ulp.cnf:34)
==28152==    by 0x6A45328: tcp_dissect_pdus (packet-tcp.c:2353)
==28152==    by 0x6C7DE16: dissect_ulp_tcp (packet-ulp-template.c:86)
==28152==    by 0x63D9037: call_dissector_through_handle (packet.c:458)
==28152==
==28152== Use of uninitialised value of size 8
==28152==    at 0x640A55E: tvb_get_guint8 (tvbuff.c:1214)
==28152==    by 0x68CCDAA: dissect_per_boolean (packet-per.c:968)
==28152==    by 0x68D0325: dissect_per_sequence (packet-per.c:1764)
==28152==    by 0x6C7EAFA: dissect_ulp_SUPLEND (ulp.cnf:754)
==28152==    by 0x68D00A1: dissect_per_choice (packet-per.c:1642)
==28152==    by 0x6C7F5C9: dissect_ulp_UlpMessage (ulp.cnf:39)
==28152==    by 0x68D041E: dissect_per_sequence (packet-per.c:1793)
==28152==    by 0x6C81B16: dissect_ULP_PDU_PDU (ulp.cnf:34)
==28152==    by 0x6A45328: tcp_dissect_pdus (packet-tcp.c:2353)
==28152==    by 0x6C7DE16: dissect_ulp_tcp (packet-ulp-template.c:86)
==28152==    by 0x63D9037: call_dissector_through_handle (packet.c:458)
==28152==    by 0x63D986C: call_dissector_work (packet.c:552)
==28152==
==28152== Conditional jump or move depends on uninitialised value(s)
==28152==    at 0x63ED4E0: proto_item_set_len (proto.c:4176)
==28152==    by 0x68D0457: dissect_per_sequence (packet-per.c:1903)
==28152==    by 0x6C7F19A: dissect_ulp_SlpSessionID (packet-ulp-fn.c:196)
==28152==    by 0x68D041E: dissect_per_sequence (packet-per.c:1793)
==28152==    by 0x6C7F17A: dissect_ulp_SessionID (packet-ulp-fn.c:211)
==28152==    by 0x68D041E: dissect_per_sequence (packet-per.c:1793)
==28152==    by 0x6C81B16: dissect_ULP_PDU_PDU (ulp.cnf:34)
==28152==    by 0x6A45328: tcp_dissect_pdus (packet-tcp.c:2353)
==28152==    by 0x6C7DE16: dissect_ulp_tcp (packet-ulp-template.c:86)
==28152==    by 0x63D9037: call_dissector_through_handle (packet.c:458)
==28152==    by 0x63D986C: call_dissector_work (packet.c:552)
==28152==    by 0x63DA0BF: dissector_try_uint_new (packet.c:969)
==28152==
==28152== Conditional jump or move depends on uninitialised value(s)
==28152==    at 0x63ED4E0: proto_item_set_len (proto.c:4176)
==28152==    by 0x68D0457: dissect_per_sequence (packet-per.c:1903)
==28152==    by 0x6C7F17A: dissect_ulp_SessionID (packet-ulp-fn.c:211)
==28152==    by 0x68D041E: dissect_per_sequence (packet-per.c:1793)
==28152==    by 0x6C81B16: dissect_ULP_PDU_PDU (ulp.cnf:34)
==28152==    by 0x6A45328: tcp_dissect_pdus (packet-tcp.c:2353)
==28152==    by 0x6C7DE16: dissect_ulp_tcp (packet-ulp-template.c:86)
==28152==    by 0x63D9037: call_dissector_through_handle (packet.c:458)
==28152==    by 0x63D986C: call_dissector_work (packet.c:552)
==28152==    by 0x63DA0BF: dissector_try_uint_new (packet.c:969)
==28152==    by 0x63DA116: dissector_try_uint (packet.c:995)
==28152==    by 0x6A455C6: decode_tcp_ports (packet-tcp.c:3984)
==28152==
==28152== Conditional jump or move depends on uninitialised value(s)
==28152==    at 0x63ED4E0: proto_item_set_len (proto.c:4176)
==28152==    by 0x68D0457: dissect_per_sequence (packet-per.c:1903)
==28152==    by 0x6C7EAFA: dissect_ulp_SUPLEND (ulp.cnf:754)
==28152==    by 0x68D00A1: dissect_per_choice (packet-per.c:1642)
==28152==    by 0x6C7F5C9: dissect_ulp_UlpMessage (ulp.cnf:39)
==28152==    by 0x68D041E: dissect_per_sequence (packet-per.c:1793)
==28152==    by 0x6C81B16: dissect_ULP_PDU_PDU (ulp.cnf:34)
==28152==    by 0x6A45328: tcp_dissect_pdus (packet-tcp.c:2353)
==28152==    by 0x6C7DE16: dissect_ulp_tcp (packet-ulp-template.c:86)
==28152==    by 0x63D9037: call_dissector_through_handle (packet.c:458)
==28152==    by 0x63D986C: call_dissector_work (packet.c:552)
==28152==    by 0x63DA0BF: dissector_try_uint_new (packet.c:969)
==28152==
==28152== Conditional jump or move depends on uninitialised value(s)
==28152==    at 0x63ED4E0: proto_item_set_len (proto.c:4176)
==28152==    by 0x68D0066: dissect_per_choice (packet-per.c:1647)
==28152==    by 0x6C7F5C9: dissect_ulp_UlpMessage (ulp.cnf:39)
==28152==    by 0x68D041E: dissect_per_sequence (packet-per.c:1793)
==28152==    by 0x6C81B16: dissect_ULP_PDU_PDU (ulp.cnf:34)
==28152==    by 0x6A45328: tcp_dissect_pdus (packet-tcp.c:2353)
==28152==    by 0x6C7DE16: dissect_ulp_tcp (packet-ulp-template.c:86)
==28152==    by 0x63D9037: call_dissector_through_handle (packet.c:458)
==28152==    by 0x63D986C: call_dissector_work (packet.c:552)
==28152==    by 0x63DA0BF: dissector_try_uint_new (packet.c:969)
==28152==    by 0x63DA116: dissector_try_uint (packet.c:995)
==28152==    by 0x6A455C6: decode_tcp_ports (packet-tcp.c:3984)
==28152==
==28152== Conditional jump or move depends on uninitialised value(s)
==28152==    at 0x63ED4E0: proto_item_set_len (proto.c:4176)
==28152==    by 0x68D0457: dissect_per_sequence (packet-per.c:1903)
==28152==    by 0x6C81B16: dissect_ULP_PDU_PDU (ulp.cnf:34)
==28152==    by 0x6A45328: tcp_dissect_pdus (packet-tcp.c:2353)
==28152==    by 0x6C7DE16: dissect_ulp_tcp (packet-ulp-template.c:86)
==28152==    by 0x63D9037: call_dissector_through_handle (packet.c:458)
==28152==    by 0x63D986C: call_dissector_work (packet.c:552)
==28152==    by 0x63DA0BF: dissector_try_uint_new (packet.c:969)
==28152==    by 0x63DA116: dissector_try_uint (packet.c:995)
==28152==    by 0x6A455C6: decode_tcp_ports (packet-tcp.c:3984)
==28152==    by 0x6A459C1: process_tcp_payload (packet-tcp.c:4043)
==28152==    by 0x6A461C0: dissect_tcp_payload (packet-tcp.c:1931)

This is with trunk r49601

Evan Huus said:

Valgrind errors fixed in r49985. There are some PER bits that we just don't dissect yet, but I think this bug is closed.

closed