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

ICMP management Fix #510

Closed
wants to merge 1 commit into from
Closed

ICMP management Fix #510

wants to merge 1 commit into from

Conversation

top-master
Copy link
Contributor

the first ICMP session that gets created did never stop, when there comes unstop other requests for an other port/id after the first all others will fail since the requestor checks the ICMP id that it last used but it did got from NetGuard the first id used for that site.

we did find the bug using Apps whichs only propose are to ping and only the first ping got ever answered

the first ICMP session that gets created did never stop, when there comes unstop other requests for an other port/id after the first all others will fail since the requestor checks the ICMP id that it last used but it did got from NetGuard the first id used for that site.

we did find the bug using Apps whichs only propose are to ping and only the first ping got ever answered
@M66B
Copy link
Owner

M66B commented Mar 26, 2018

NetGuard will close idle connections automatically independent of the protocol, so I don't see what this adds.

@M66B
Copy link
Owner

M66B commented Mar 26, 2018

After forwarding a packet 'stop' will be set:
https://github.com/M66B/NetGuard/blob/master/app/src/main/jni/netguard/icmp.c#L137

After 'stop' or timeout the socket will be closed:
https://github.com/M66B/NetGuard/blob/master/app/src/main/jni/netguard/icmp.c#L38

@M66B M66B closed this Mar 26, 2018
@top-master
Copy link
Contributor Author

top-master commented Mar 27, 2018

we was not clear at first but will try to be more informative.

Reproduce Failure:

  1. install and run any ping app that does repeat ICMP requests to the same web-site/IP but every time on an other port like ping
  2. start NetGuard and block every thing except that mentioned ping app
  3. provide a valid IP to ping app and press its start button
    all done ping will start but you will see the first request gets answered but for any other request after first ping will say "time out" since every time the app does use an other port/icmp-id.

anyway when we rethink this out:
the bug/problem in your code was that the port/id of any ICMP respond which we have an active request session for the same IP is hacked to the first port\id which the session was created with.
my mistake was I thought that hack is required for security reasons.

so to fix this we just need to remove that hack at file "icmp.c" in the method "check_icmp_socket(...)" which is the single line "icmp->icmp_id = s->icmp.id;"

using this way Ping/ICMP works and we do not even need you to pull from the repository which we only forked to fix ICMP and make a pull request.
we can hold track and remove that single line manually.

I think the reason you did not understand what my code does is that I have saved memory two time's and made code unreadable by doing so:
I mean first I have saved one single byte per session by making stop a three-state Boolean which's third state marks that the session did got it's answer an can be reused.
Second's I did save memory by the actual reuse of old and answered sessions.

@M66B
Copy link
Owner

M66B commented Mar 27, 2018

I will remove the workaround because that seems better to me.

Thanks for your contribution!

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