diff options
author | guy <guy@f5534014-38df-0310-8fa8-9805f1628bb7> | 2009-09-17 02:42:31 +0000 |
---|---|---|
committer | guy <guy@f5534014-38df-0310-8fa8-9805f1628bb7> | 2009-09-17 02:42:31 +0000 |
commit | 3e495514f9b1a71e12001a1a48b85e71d01cd89f (patch) | |
tree | 72c095ac957dd577fce2d423820f5b3225121148 /wiretap | |
parent | 86e1a7b46935fa6fe305db1de9edbd19ed43550a (diff) |
Do *NOT* skip the rest of the header by reading into a fixed-size buffer
on the stack! There is no guarantee that the header length won't cause a
buffer overflow - there could be a bug in some version of Surveyor
generating a bad file, there could be a future version of Surveyor that
has a really big pseudo-header, the file could've been written by
something other than Surveyor that has a bug in it, there could be a
file that's corrupted in transit, or there could be a deliberately
malformed packet trying to cause *Shark to execute arbitrary code.
Also, explicitly check for a too-short header length and fail with
WTAP_ERR_BAD_RECORD in that case.
Add some comments asking some questions about the header.
(The previous change was for bug 3856:
https://bugs.wireshark.org/bugzilla/show_bug.cgi?id=3856
not bug 3865.)
git-svn-id: http://anonsvn.wireshark.org/wireshark/trunk@29958 f5534014-38df-0310-8fa8-9805f1628bb7
Diffstat (limited to 'wiretap')
-rw-r--r-- | wiretap/snoop.c | 33 |
1 files changed, 21 insertions, 12 deletions
diff --git a/wiretap/snoop.c b/wiretap/snoop.c index 73c2f4ecd4..c2324daca8 100644 --- a/wiretap/snoop.c +++ b/wiretap/snoop.c @@ -750,8 +750,7 @@ snoop_read_shomiti_wireless_pseudoheader(FILE_T fh, { shomiti_wireless_header whdr; int bytes_read; - char buffer[250]; - int rsize; + int rsize; errno = WTAP_ERR_CANT_READ; bytes_read = file_read(&whdr, 1, sizeof (shomiti_wireless_header), fh); @@ -763,19 +762,29 @@ snoop_read_shomiti_wireless_pseudoheader(FILE_T fh, } /* the 4th byte of the pad is actually a header length, - * we've already read 8 bytes of it, and it is never - * less than 8 + * we've already read 8 bytes of it, and it must never + * be less than 8. + * + * XXX - presumably that means that the header length + * doesn't include the length field, as we've read + * 12 bytes total. + * + * XXX - what's in the other 3 bytes of the padding? Is it a + * 4-byte length field? + * XXX - is there anything in the rest of the header of interest? + * XXX - are there any files where the header is shorter than + * 4 bytes of length plus 8 bytes of information? */ - rsize = ((int) whdr.pad[3]) - 8; - if(rsize > 0) { - bytes_read = file_read(buffer, 1, rsize, fh); - if (bytes_read != rsize) { - *err = file_error(fh); - if (*err == 0) - *err = WTAP_ERR_SHORT_READ; + if (whdr.pad[3] < 8) { + *err = WTAP_ERR_BAD_RECORD; + *err_info = g_strdup_printf("snoop: Header length in Surveyor record is %u, less than minimum of 8", + whdr.pad[3]); return FALSE; - } } + /* Skip the header. */ + rsize = ((int) whdr.pad[3]) - 8; + if (file_seek(wth->fh, rsize, SEEK_CUR, err) == -1) + return FALSE; pseudo_header->ieee_802_11.fcs_len = 4; pseudo_header->ieee_802_11.channel = whdr.channel; |