various additions/fixes #6
Conversation
|
Hi, thanks for your good work on this. I will review the proposed changes properly when I'm at a computer rather than on my phone... That may be a few days so apologies, please bear with me! |
|
My pleasure! Thanks for writing stripe. I did some search for a decapsulating tool, and yours was the best one (or I'm bad at searching :) ) |
|
BTW, on a related issue, I'd suggest that the "-f" setting should be made the default (and maybe add a "-F" CLI setting to do the defragmentation). For me, the main use case of stripe is the removal of outer headers. Therefore, any further processing should be explicit. Does it make sense? |
|
That's how it used to work and it would certainly make it more streamable for the new stdin/stdout modes, however the whole reason I built the reassembly into stripe rather than as a standalone tool (which I originally planned to do) was that any fragmentation at any layer of encap breaks the decap process. I didn't notice but it was reported to me as a bug and it was one of those things where you pull a bit of string and it just keeps coming... I suppose it could default to off and display a "fragments found, may not work, trying anyway, suggest -F" message to stderr if fragmentation is found. What's the driver to change it? If it's just for stdin handling maybe the default could be off for stdin and on for files. |
|
Although thinking about it since the reassembly code was introduced I think it always uses a temporary file as an interim (on my phone again so can't easily check) in which case it won't stream anyway :( |
By this, I assume you mean that there is some encap doing fragmentation, and therefore
On the topic, one of my main concerns about the code is the lack of tests (I'd be very reluctant to accept any new code because of this). Have you put some thought about it? The quickest thing I can think of is dropping some traces before/after the decap process, and checking that they are the same. In other words, something like: #!/usr/bin/env bash
TRACES=(t1 t2 t3)
for trace in "${TRACES[@]}"; do
diff $t.decap.pcap <(./stripe -f -r $t.pcap -w -)
if ! [[ "$?" -eq "0" ]]; then
echo "error on trace $t.pcap"
endif
donePlease feel free to start with the three.pcap trace I added yesterday.
I don't have a use case for the reassembly, so I need to remember to add "-f" every time I run it. |
|
Forgot to ask: Is there any reason you didn't use libpcap to help with the packet processing? |
|
Regarding the multiple fragmentation, it's always IPv4 but it can occur at multiple layers. For example, a 1500 byte packet on a LAN goes over PPPoE and gets fragmented to fit into 1492 bytes, then that gets transported over L2TP. To get the payload out you have to re-assemble at the second IPv4 header. Regarding not using libpcap, as a design principle I don't like dependencies, when possible everything I write compiles out of the box on a default installation of Linux / OSX / Windows. Aside from the fact that people often can't be bothered to sort out dependency issues, it makes portability more difficult. In all honesty I couldn't be bothered to work out the licensing implications, either. I tried to compile this fork on my Mac but it won't work because it includes a Linux header - does the code actually use anything in linux/if_ether.h aside from a couple of constants? Could the constants just be defined in stripe.h? I don't think changing the default behaviour (re: re-assembly & padding) is the answer, I believe re-assembly is desirable in most common use cases - if it defaults to "off" then stripe will just fail on fragmented frames, whereas there's very little penalty for an unwanted re-assembly (plus it only tries to re-assemble when it finds fragments, in which case it probably should try to re-assemble). What would be better in my opinion would be to catch sliced frames and switch off padding / re-assembly in that case. Also maybe it's a good idea to add a CLI flag to disable padding (though I'm not sure what problems unwanted padding would cause)? If I'm honest I'm confused by what the intention would be to decapsulate sliced frames in the first place? You clearly have a use case for it, care to share? Your regression testing mechanism is a good idea and basically what I'd been doing by hand before pushing updates. Somewhere around I have a good stash of pcap files for this purpose, I just need to lay my hands on them again. I may add them to the repo as well so that they don't get lost again 😄 - however some of them are "real" so I'd need to recreate them without any sensitive data in there. |
The review mechanism doesn't make it clear what you refer to |
Next decap was using vlen - pos, where vlen is the GTP length field (GTP payload length, not including the GTP header itself), and pos is the GTP header length (8 for short GTP headers). Therefore it was discounting the GTP header from the payload twice. Right value is length - pos.
This was padding small TCP headers (i.e. 20-byte IP plus 40-byte TCP).
Requires moving all output to stderr. Tested: $ cat one.pcap | ./stripe -f -r - -w - > /tmp/la2 Reassembly disabled... Decapsulating... 1 frames processed. $ file /tmp/la2 file /tmp/la2 /tmp/la2: tcpdump capture file (little-endian) - version 2.4 (Linux "cooked", capture length 200)
Makes sense.
Done.
If the traces are small, pcaptxt may help you.
OK, I hadn't realized there was a strong use case for reassembling. I agree keeping the original behavior is then better. |
|
Hi Foeh, Did you get some time to check the patches? |
No description provided.