NetScreen file parser crash
This issue was migrated from bug 12396 in our old bug tracker.
Original bug information:
Reporter: Gerald Combs
Status: RESOLVED FIXED
Product: Wireshark
Component: Capture file support (libwiretap)
OS: All
Platform: All
Version: Git
Attachments:
S_xtci0ba8xi_Wireshark_netscreen_PoC.pcap: PoC provided with the report
See also: http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2016-5357
Activity
Created attachment 14525
PoC provided with the report
Build Information:
Paste the COMPLETE build information from "Help->About Wireshark", "wireshark -v", or "tshark -v".
iDefense Labs sent the following: Summary A Heap buffer overflow vulnerability exists in Wireshark when processing malformed packets, which could be exploited by remote attackers to compromise a vulnerable system via a malicious packet or capture file. The issue occurs when the length field inside the Netscreen data ends up being greater than 65536 : 6843828.0: trust(o) len=262176:00121ebbd132->00600868d659/080 Technical Details When processing such a capture, Wireshark starts by retrieving the magic value from headers (wiretap/netscreen.h). If the capture has a NetScreen mime type, or if it contains “(i) len=” or “(o) len=” the NetScreen parser is used and “wtap_read()” is called to reach “netscreen_read()” in “wiretap/netscreen.c”. This function reads the packet trace. First, the header is parsed to retrieve the packet length with “parse_netscreen_rec_hdr()”. As we see in the “wiretap/netscreen.h” file, 65536 bytes were allocated for the data: #define NETSCREEN_MAX_PACKET_LEN 65536 /* Make sure we have enough room for the packet */ buffer_assure_space(wth->frame_buffer, NETSCREEN_MAX_PACKET_LEN); buf = buffer_start_ptr(wth->frame_buffer); Then “parse_netscreen_hex_dump()” is called to store a hexdump of the trace file in the previously allocated buffer. The “pkt_len” argument in “parse_netscreen_hex_dump()” can be 9 digits long. Each hexdump data line is copied by “parse_single_hex_dump_line()” to the allocated buffer of 65536 bytes. With an overly large packet size, a buffer overflow condition may occur. For example by supplying a length of 65568 (> 65536), it is possible to cause a buffer overflow After parsing the trace, Wireshark displays it by calling the “packet_list_dissect_and_cache_record()” function. Next, “packet_list_dissect_and_cache_record()” will copy the trace from into the “pd” variable on the stack and having a size of WTAP_MAX_PACKET_SIZE (0xFFFF), leading to a buffer overflowadded crash libwiretap versiondev labels
-
Change 15176 had a related patch set uploaded by Guy Harris: Fix packet length handling.
-
Change 15176 merged by Guy Harris: Fix packet length handling.
-
Change 15177 had a related patch set uploaded by Guy Harris: Fix packet length handling.
-
Change 15177 merged by Guy Harris: Fix packet length handling.
-
Change 15178 had a related patch set uploaded by Guy Harris: Fix packet length handling.
-
Change 15178 merged by Guy Harris: Fix packet length handling.
-
> This function reads the packet trace. First, the header is parsed to > retrieve the packet length with “parse_netscreen_rec_hdr()”. As we see in > the “wiretap/netscreen.h” file, 65536 bytes were allocated for the data: > > #define NETSCREEN_MAX_PACKET_LEN 65536 > /* Make sure we have enough room for the packet */ > buffer_assure_space(wth->frame_buffer, NETSCREEN_MAX_PACKET_LEN); > buf = buffer_start_ptr(wth->frame_buffer); > > Then “parse_netscreen_hex_dump()” is called to store a hexdump of the trace > file in the previously allocated buffer. The “pkt_len” argument in > “parse_netscreen_hex_dump()” can be 9 digits long. Each hexdump data line is > copied by “parse_single_hex_dump_line()” to the allocated buffer of 65536 > bytes. With an overly large packet size, a buffer overflow condition may > occur. For example by supplying a length of 65568 (> 65536), it is possible > to cause a buffer overflow > > After parsing the trace, Wireshark displays it by calling the > “packet_list_dissect_and_cache_record()” function. Next, > “packet_list_dissect_and_cache_record()” will copy the trace from into the > “pd” variable on the stack and having a size of WTAP_MAX_PACKET_SIZE > (0xFFFF), leading to a buffer overflow Not in the current version; they looked at a pre-1.12 version, in which WTAP_MAX_PACKET_SIZE was 65535, not 262144. In addition, the buffers in question aren't on the stack in the current version. Fix checked into the master, 2.0, and 1.12 branches; we now 1) parse the length as *unsigned* so we don't have to worry about negative values, 2) check it against WTAP_MAX_PACKET_SIZE and fail if it's greater, and 3) use the actual packet size rather than a hard-wired value of 65536 as the size we ensure is present in the buffer. -
Change 15220 had a related patch set uploaded by Guy Harris: Don't treat the packet length as unsigned.
-
Change 15220 merged by Guy Harris: Don't treat the packet length as unsigned.
-
Change 15221 had a related patch set uploaded by Guy Harris: Don't treat the packet length as unsigned.
-
Change 15221 merged by Guy Harris: Don't treat the packet length as unsigned.
-
Change 15222 had a related patch set uploaded by Guy Harris: Don't treat the packet length as unsigned.
-
Change 15222 merged by Guy Harris: Don't treat the packet length as unsigned.
-
(In reply to Guy Harris from comment #7) > Fix checked into the master, 2.0, and 1.12 branches; we now 1) parse the > length as *unsigned* so we don't have to worry about negative values, Sadly, the scarf routines mishandle negative numbers the same way strtoul() does - instead of rejecting numbers with a leading -, "the value resulting from the conversion is negated", to quote the C90 standard. So we now parse it as signed, again, and then *do* worry about negative values - they're treated as an error. Thanks, ANSI/ISO C standards committee (and perhaps pre-C89 C implementations). -
Change 16115 had a related patch set uploaded by Balint Reczey: Fix packet length handling.
-
Change 16115 merged by Balint Reczey: Fix packet length handling.
-
Change 16116 had a related patch set uploaded by Balint Reczey: Don't treat the packet length as unsigned.