Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DivertAsyncResult with send (and receive) #10

Open
ghost opened this issue Sep 12, 2016 · 11 comments
Open

DivertAsyncResult with send (and receive) #10

ghost opened this issue Sep 12, 2016 · 11 comments

Comments

@ghost
Copy link

ghost commented Sep 12, 2016

In the DivertTests the DivertAsyncResult is used, which works.
However, I cannot get DivertAsyncResult to work in the NetDump example, it keeps failing with error code 87 (invalid parameter).

I modified the Send with:

var divertAsyncResult = new DivertAsyncResult();
if (!diversion.SendAsync(buffer, receiveLength, address, ref sendLength, divertAsyncResult))
{
         if (!divertAsyncResult.NoError)
             Console.WriteLine(divertAsyncResult.ErrorCode);

         if (!divertAsyncResult.Get(250))
             Console.WriteLine(divertAsyncResult.ErrorCode);
 }

This is identical to the code from DivertTests but that's using a combination of different filters and divert handles. Am I doing something wrong or how can I adapt this?

What is interesting is that ReceiveAsync seems to work just fine.

@ghost
Copy link
Author

ghost commented Sep 12, 2016

Found the reason, it's a bug in SendAsync. There are some more bugs and problems with the code, I'll see if I can create a PR soon.

@ghost ghost closed this as completed Sep 12, 2016
@TechnikEmpire
Copy link
Owner

@Areithus It's a bug in my version of SendAsync? Or in WinDivert? If it's a bug in my code I'll re-open.

@TechnikEmpire
Copy link
Owner

TechnikEmpire commented Sep 12, 2016

I have to admit the async part posed a problem because they're static helpers. Actually everything is, so I had to do a lot of trickery to make the appearance of a class based system. Curious what other bugs you found, please let me know and thank you for the report.

@TechnikEmpire
Copy link
Owner

Arg, disregard last message. Deleted it because I was too hasty to read that you are indeed calling Get(TIMEOUT).

@ghost
Copy link
Author

ghost commented Sep 12, 2016

Yes, it's a bug in SendAsync, It's providing packetBuffer->Length as the parameter but it should be packetLength. I don't remember all bugs but:

  • Ipv6 has its destination address check wrong (it checks the source instead).
  • GetPacketProcess does not account for the ip address.
  • The GCHandle.Alloc/pin_ptr does not work as-is. If you parse the packet the pointers won't remain valid after the function returns. The example works because it declares the variables outside of the while so the object location in memory remains (mostly) in tact. Move the variables inside the while and it'll crash after a few packets, unlike the example which can take quite a while. The solution is to always pin the buffer before calling the Receive* methods and manually free them after the Send* methods are called. I tried to pin the data inside the Receive method (so one does not have to pin it) but as there is no way to pass the GCHandle you cannot easily free it in the Send. Perhaps a std::map or similar can be used to store the GCHandle but that also means that after the Send method the data of the packet cannot always be accessed anymore (although that's probably not very useful).

There were also some other things but I have to look that up 👍
Regardless, I'm looking forward to hearing some feedback from you, especially what you think about the GCHandle problems.

@TechnikEmpire
Copy link
Owner

@Areithus Alright thanks, will check into this when I have time ( busy with work right now ) and get back to you. Thanks again.

@TechnikEmpire TechnikEmpire reopened this Sep 12, 2016
@ghost
Copy link
Author

ghost commented Sep 14, 2016

BTW, did you ever find a reliable way to cache the results from GetTcpTable2? Ideally the caching should be done by Divert.Net. What I thought of was to introduce a global variable to store the result in. If a match in that table exists with the address and port of the current packet -> return the process from that table. If not, do a new lookup of GetTcpTable2. However, I'm not sure how reliable that would be. On the other hand, caching data for a certain amount of time might not always be perfect either because of processes getting started and killed. Maybe a combination of both?

@TechnikEmpire
Copy link
Owner

I cache by holding results for 2 seconds. You're right, this definitely will have a degree of inaccuracy but so far this has been a good trade off for me, especially when diverting HTTP.

I was doing some research yesterday that had led me to believe that the binary behind a stream is actually available in the Windows Filtering Platform. It's actually possible to write a filter that matches against a binary's path. I don't think basil, the person behind WinDivert knows this because I've seen him/her comment on my process matching function saying this ain't available in WFP. Ideally, leveraging this is the way to go in the driver itself so we don't have to do expensive and inaccurate GetXXXTable methods.

https://github.com/TechnikEmpire/HttpFilteringEngine/blob/master/src/te/httpengine/mitm/diversion/impl/win/WinDiverter.cpp

@TechnikEmpire
Copy link
Owner

In order to 100℅ accurately map this you'd have to track all captured flows 100℅. That's a lot of work and I found for my purposes that a very short cache period, using thread local caches, was accurate enough and cuts CPU to nil, but I didn't want to impose this on users.

@ghost
Copy link
Author

ghost commented Sep 15, 2016

It indeed seems like that information is available (https://msdn.microsoft.com/en-us/library/windows/hardware/ff552397(v=vs.85).aspx). Would be nice if basil00 could somehow add that information. I did some testing with caching like this:
introduce a global variable to store the result in. If a match in that table exists with the address and port of the current packet -> return the process from that table.
This resulted in slightly better results as I noticed that sometimes for incoming packets there is no entry in the tcp table anymore. I'll do some more experiments with this.

@TechnikEmpire
Copy link
Owner

TechnikEmpire commented Jul 18, 2017

@Areithus A more reliable way to cache the results is to keep an array of length std::numeric_limits<uint16_t>::max() and then look for the TCP handshake and only do a lookup when you see that handshake. So say you see SYN on port 8080, you'd do my_map[8080] = get_pid_from_tcp_table_lookup();. This is the most optimized system I can think of, hitting the TCP table only one time per connection. I've seen a performance boost in other applications doing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant