Dissector TCP reassembly error in Wireshark 1.99
hadrielk opened this issue · 12 comments
Hi,
the plugin works in Wireshark 1.12.x, but in 1.99.x (the current development release of Wireshark, which will become 2.0 eventually) there's an issue. With the capture file w3l_onevsone-game.pcap I get a dissector bug in packet #6, 12, 17, 23, 28, 33, and 38.
The bug is that the script's p_bnetp.dissector() function is returning a "state.used" value of 0 when it also sets pkt.desegment_len and pkt.desegment_offset to non-0 values. Technically, if you set the desegment_len/offset values to something, then it implies the packet is for your dissector, so you should be returning the number of bytes that were yours - in this case, it should be returning "buf:len()".
In Wireshark versions prior to 1.99, there was no verification that the dissector did this correctly, but in 1.99 an assertion was put in place to m make sure that the desegment_len/offset values are not changed if the return value was 0 (because a return value of 0 means "no bytes in this packet were for my dissector").
Wireshark might be relax this check before 2.0 is released, so you might not need to fix your Lua script, but it would be safer/better if you did.
I think you only need to change this:
function p_bnetp.dissector(buf,pkt,root)
if root then
-- bunch of code
-- line 256:
return state.used
...to this:
function p_bnetp.dissector(buf,pkt,root)
if root then
-- bunch of code
-- line 256:
return buf:len()
I'm not near my development machine but if I remember correctly I return
state.used to indicate that everything before was processed and I'm
interested in getting in the next call what is after that offset.
But probably I misunderstood the meaning of the return value and now
Wireshark is checking it's correct (I don't believe they will revert that
BTW).
Let me reread the code and documentation when I'm back home. I want to be
sure it works on pervious versions too.
Thanks for the report! (I didn't know this dissector actually had users
haha)
Well to be honest I'm not a "user" of the script - I'm one of the developers for Wireshark, and I focus most of my development on the Lua API for it, so I found the script while searching for scripts. :)
I tested my proposed change on 1.12.6 and 1.99.8, and it seems to work in both, but I haven't tested it on 1.10 or earlier. I did look at the relevant C-code files going back to Wireshark 1.4, and it looks like it will be ok with the change, but I didn't actually try it.
With regards to the return value of the dissector - yes it's a little confusing because the documentation isn't well written, but basically the return value needs to be something other than 0 if the packet belongs to your dissector, even if you need more bytes to correctly parse it (in which case you set the desegment_* values as you already do).
Oh, I see. Well, I guess you know better than me then :-)
Thanks a lot for investigating and testing! I think supporting only the latest stable release should be ok.
I got confused by the following wording in README.dissector:
If it is a new-style dissector, it should return the number of bytes successfully processed.
particularly the word successfully. I'll do some test with different values just for fun.
Also, browsing the source I found the can_desegment flag. I think I should check it before trying to desegment packets. Do you know if it's supported in Lua API?
Yeah the docs are confusing - I'll fix them.
can_desegment is supported - it's a member of the Pinfo object passed into the Proto.dissector() function (which in your script's case is called "pkt"), so you'd verify it like so:
if pkt.can_desegment > 0 then
-- do your stuff
end
Hmm...I have one more question. If I have a packet that spans two segments (like in the graph below) but only a fraction of the second segment belongs to the packet, what should the dissector function return?
+----------------------+
|Start of Packet | First Segment
| |
|======================|
| End of Packet | ... | Second Segment
| ............ garbage |
+----------------------+
I guess the first time it would return the length of the tvb and request one more segment. The second time, should it return the length of the tvb again? the offset of the first "garbage" byte? 0 to reject the part that is not ours? Would any option allow the "garbage" to be processed by another dissector?
Anyway, I don't think this is something frequently found on real communications; a connection usually carries only one protocol.
The dissector should still return the full Tvb length[1]. The pkt.desegment_offset should be set to the beginning of that second partial packet - that's what Wireshark will actually use internally to figure out what to put in the Tvb buffer the next time it invokes your dissector function.
Getting partial messages after other ones inside the same TCP segment actually happens all the time, for some protocols. Any protocol that can pipeline requests, or doesn't follow a strict synchronous request/response model, can have this happen. (I don't know BNETP so don't know if this can happen for this case)
Another thing that can happen is that your dissector gets invoked with a TCP segment in the middle of a BNETP message - this can happen if the user starts Wireshark in the middle of the game, and it just happens to start capturing while a BNETP message is being sent but misses the beginning of it. To handle that case, most Wireshark dissectors do a small bit of sanity check if possible, such as by looking for some valid leading byte(s) they expect to see, and if the sanity check fails then truly return 0 for that packet. In your case you should probably return 0 if it was rejected (didn't have a handler), which you don't do right now.
-hadriel
[1] technically you can also just return nothing - i.e., just do "return" - because the Lua API assumes that if you return nothing then you really meant to return the full Tvb length. (it does this for historical reasons and backwards compatibility with old Lua scripts)
BTW, out of curiosity, why is part of the dissection done with a Lua coroutine?
I used coroutines to be able to use yield to jump back to the calling point in case the buffer wasn't long enough. Something like throwing an exception.
There is a State "class" which holds a tvb and a cursor, initially pointing to the beginning, which advances as data is read. There are methods to read from the tvb which check if the amount requested is within the limits of the tvb; if it isn't, it yields allowing the caller to request reassembly (in the first coroutine located in the main dissector function) or adding a message to the tree indicating the packet is too short for the description being processed (the other instance of coroutine).
It may be done in other way, I guess. But I wanted to avoid checking and propagating an error through the call stack back to the initial caller.
Does it make any sense? I think I can't explain myself as clearly as I would like :)
Sorry, to bother you again, hehe.
Was init.lua removed? I can't find it in my installation of 1.99.8rc0 (Version 1.99.8-241-gc445570) and the script is complaining that base is not defined :(
Was init.lua removed?
Umm... no. Are you building Wireshark yourself, or using the binary built from wireshark.org? If you're building it yourself, you may need to do a export WIRESHARK_RUN_FROM_BUILD_DIRECTORY=1. (for example if you did "make" but not "make install")
I downloaded it from wireshark.org. It's the last win64 automated build.
Works fine for me (I just downloaded that exact build from the wireshark.org and tried it with your script). What operating system do you run?
I'm running Windows 8.1. OK, I'll try installing again. Maybe something went wrong. I had the stable release and it asked whether I wanted to remove it; I said yes. I'm not sure if that's related.