Wireshark-dev: Re: [Wireshark-dev] tvb_get_nstringz0

From: John Thacker <johnthacker@xxxxxxxxx>
Date: Sat, 27 Mar 2021 20:32:28 -0400

On Sat, Mar 27, 2021 at 2:57 PM Dario Lombardo <lomato@xxxxxxxxx> wrote:
Hi John,
thanks, your explanation helped a lot. However I still don't get why the code crashes. Please let me use the actual buffer sizes since the ones I told before were examples. The packet is 49, the local buffer is 15.

When you call tvb_get_nstringz0() you pass in bufsize = 15.
tvb_get_nstringz0() calls _tvb_get_nstringz()
check_offset_len() runs to the end of the packet, setting len to 49.
Since len >= bufsize, it sets limit = bufsize.
stringlen = tvb_strnlen(tvb, abs_offset, limit - 1) looks at the first 9 bytes, doesn't find a NUL, returns -1

That's a point I don't get. This piece of code (stringlen = tvb_strnlen(tvb, 0, 14)) actually returns 49. Despite the fact that NULL is present or not, shouldn't this function fulfill the (limit - 1)? Shouldn't that return 14 at most?
 
stringlen is -1, tvb_memcpy copies over limit (10) bytes into buffer from tvb, bytes_copies is set to 10, _tvb_get_nstringz() returns -1.

That's where things start to get hairy: stringlen is 49, then the actual copy starts against buffer, that is only 15 bytes long. Crash.

Huh, that's odd. I just added some lines to packet-http.c near the top of dissect_http_tcp() like so:

        guint bufsize = 19;
        guint8 buffer[19];
        tvb_get_nstringz0(tvb, 0, bufsize, buffer);

And stepped through with gdb after setting a breakpoint and opening a file with a ~480 byte HTTP packet with no nulls in the HTTP layer:

3578 stringlen = tvb_strnlen(tvb, abs_offset, limit - 1);
(gdb) print limit -1
$12 = 18
(gdb) n
3580 if (stringlen == -1) {
(gdb) print stringlen
$14 = -1
(gdb) print tvb->length
$15 = 479

And all proceeds as expected.

tvb_strnlen(tvb, offset, maxlength) is supposed to return -1 if 'maxlength' (here, 14) is reached before a NULL.

Let's see, tvb_strnlen calls tvb_find_guint8(tvb, abs_offset, maxlength=14, 0), and returns -1 if and only if tvb_find_guint8(...) returns -1. That looks right, so let's look at tvb_find_guint8():

        DISSECTOR_ASSERT(tvb && tvb->initialized);

        exception = compute_offset_and_remaining(tvb, offset, &abs_offset, &limit);
        if (exception)
                THROW(exception);

        /* Only search to end of tvbuff, w/o throwing exception. */
        if (maxlength >= 0 && limit > (guint) maxlength) {
                /* Maximum length doesn't go past end of tvbuff; search
                   to that value. */
                limit = (guint) maxlength;
        }

Have you tried stepping through it with a debugger? The code looks right to me and runs correctly over here. Are you perhaps somehow hitting an exception or a failed assertion?

John