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

Class Ping not reading response when ICMP Time-to-live exceeded #61465

Closed
lorenzo93 opened this issue Nov 11, 2021 · 15 comments · Fixed by #61592
Closed

Class Ping not reading response when ICMP Time-to-live exceeded #61465

lorenzo93 opened this issue Nov 11, 2021 · 15 comments · Fixed by #61592
Labels
area-System.Net bug os-linux Linux OS (any supported distro) os-mac-os-x macOS aka OSX
Milestone

Comments

@lorenzo93
Copy link

Description

Hi,
I've wrote a small program to run traceroutes, based on the Ping (System.Net.NetworkInformation) class.
The software works like a charm on Windows but if I execute the same code on a MacOS it does not give the IP addresses of the returned ICMP Time-to-live exceeded messages.

I've already tried to run my software from my user account and from the root account (using sudo) without any luck.

Reproduction Steps

I've used a simple code that is based on the System.Net.NetworkInformation.Ping class and uses the SendPingAsync function.
I specify a PingOptions class to manually set the TTL.

Expected behavior

The PingReply class returned should have the Address property set with the correct IP Address.

Actual behavior

The PingReply class returned has the IP Address set to "0.0.0.0".

Regression?

I've tried the code on .NET 5.0 and on .NET 6.0.
With both versions on Windows is perfectly working while on MacOS it gives the bug.

Known Workarounds

No response

Configuration

No response

Other information

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Net untriaged New issue has not been triaged by the area owner labels Nov 11, 2021
@ghost
Copy link

ghost commented Nov 11, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

Hi,
I've wrote a small program to run traceroutes, based on the Ping (System.Net.NetworkInformation) class.
The software works like a charm on Windows but if I execute the same code on a MacOS it does not give the IP addresses of the returned ICMP Time-to-live exceeded messages.

I've already tried to run my software from my user account and from the root account (using sudo) without any luck.

Reproduction Steps

I've used a simple code that is based on the System.Net.NetworkInformation.Ping class and uses the SendPingAsync function.
I specify a PingOptions class to manually set the TTL.

Expected behavior

The PingReply class returned should have the Address property set with the correct IP Address.

Actual behavior

The PingReply class returned has the IP Address set to "0.0.0.0".

Regression?

I've tried the code on .NET 5.0 and on .NET 6.0.
With both versions on Windows is perfectly working while on MacOS it gives the bug.

Known Workarounds

No response

Configuration

No response

Other information

No response

Author: lorenzo93
Assignees: -
Labels:

area-System.Net, untriaged

Milestone: -

@wfurt
Copy link
Member

wfurt commented Nov 14, 2021

cc: @filipnavara

@filipnavara
Copy link
Member

filipnavara commented Nov 14, 2021

My local testing on macOS / .NET 6 shows that I get the Address back if the ping succeeds. However, if I specify ttl it always times out on my machine so there's possibly something broken specifically with that use cases.

using System.Net.NetworkInformation;

var ping = new Ping();
var reply = ping.Send("8.8.8.8", 5000, new byte[32], new PingOptions(1, false)); // Remove PingOptions to make is succeed
Console.WriteLine($"Status: {reply.Status} Address: {reply.Address}");

(this code path may be interesting)

@lorenzo93
Copy link
Author

Hi @filipnavara ,

if you need more information about this, please ask.
But I think you already find the problem.

I also did a little of debugging and the network part is perfectly working. I used Wireshark, the ICMP packets are correctly send with the TTL lowered, and the ICMP Time-to-live exceeded messages are arriving to my machine.

I write a small workaround using raw sockets, it works but requires privileged access to the machine (sudo).

@filipnavara
Copy link
Member

I can also see the packets correctly sent and received in Wireshark. They are just not passed or processed by the application. I'd need to dig in deeper with a debugger but the problem is reproducible. However, in the original description you didn't mention whether you receive the Success status with the empty address. For me the requests fail with a timeout because the response is not read.

@filipnavara
Copy link
Member

filipnavara commented Nov 14, 2021

@wfurt It fails here:

if (socketConfig.Identifier != receivedHeader.Identifier
|| type == (byte)IcmpV4MessageType.EchoRequest
|| type == (byte)IcmpV6MessageType.EchoRequest) // Echo Request, ignore
{
return false;
}

The received message has type IcmpV4MessageType.TimeExceeded and receivedHeader.Identifier == 0 and hence it's ignored. Aside from this condition it would have worked. Wireshark shows matching identifiers though.

@lorenzo93
Copy link
Author

Actually, I've never checked (till now).

I ran the exact same traceroute code as the original issue.
Also for me I receive the Status property with TimedOut.

I see you find where the packet is ignored, but if it is so, why on Windows it works?
From your snipped I don't see any platform related switch...

@filipnavara
Copy link
Member

filipnavara commented Nov 14, 2021

It looks like the pings over the raw socket don't correctly handle the TimeExceeded status. The identifier is present in the packet but at a different place from where the code reads it. On Windows the code uses native Ping API so the code paths are different. I'll see if I can fix it.

The response in Wireshark looks like this:

Frame 1511: 102 bytes on wire (816 bits), 102 bytes captured (816 bits) on interface en0, id 0
Ethernet II, Src: BeijingX_1b:ef:22 (9c:9d:7e:1b:ef:22), Dst: Apple_c5:b2:25 (18:3e:ef:c5:b2:25)
Internet Protocol Version 4, Src: 192.168.31.1, Dst: 192.168.31.70
Internet Control Message Protocol
    Type: 11 (Time-to-live exceeded)
    Code: 0 (Time to live exceeded in transit)
    Checksum: 0xf4ff [correct]
    [Checksum Status: Good]
    Unused: 00000000
    Internet Protocol Version 4, Src: 192.168.31.70, Dst: 8.8.8.8
    Internet Control Message Protocol
        Type: 8 (Echo (ping) request)
        Code: 0
        Checksum: 0x64e5 [unverified] [in ICMP error packet]
        [Checksum Status: Unverified]
        Identifier (BE): 37658 (0x931a)
        Identifier (LE): 6803 (0x1a93)
        Sequence Number (BE): 0 (0x0000)
        Sequence Number (LE): 0 (0x0000)
        Data (32 bytes)
            Data: 0000000000000000000000000000000000000000000000000000000000000000
            [Length: 32]

The code examines the first ICMP header which has no identifier and type IcmpV4MessageType.TimeExceeded. However, for this type of reply the data are copy of the original IP+ICMP packet data, so the identifier can be extracted from there and compared to the correct value.

@lorenzo93
Copy link
Author

lorenzo93 commented Nov 14, 2021

On Windows the code uses native Ping API so the code paths are different.

I was struggling over this. Thanks for the clarification.

Moreover, beware of the TimeExceeded internal packet. As for the RFC792, the packet includes

the internet header plus the first 64 bits of the original datagram's data.

so it is not the whole packet in all cases. That could be a truncated packet if you send a ICMP Echo Request packet too big (e.g specifying a "big" message in the Ping class)

@filipnavara
Copy link
Member

filipnavara commented Nov 14, 2021

Most of the (interesting) messages in RFC 792 follow the pattern of returning the original IP header + 64 bits of the original datagram (which conveniently fits the ICMP header). In my case it seems to return the original data in addition to that. ICMP reply is actually an exception where the data follow right after the first ICMP header. It should not be difficult to update the code but it's a bit tricky to fix up all the buffer sizes to ensure everything will fit in case of an error.

Thanks for filing the report.

@wfurt
Copy link
Member

wfurt commented Nov 15, 2021

I suspect Linux may have same problem.

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Nov 15, 2021
@karelz
Copy link
Member

karelz commented Nov 16, 2021

Triage: We are getting different response format than we expect.
Not a regression against 5.0.

@karelz karelz added this to the 7.0.0 milestone Nov 16, 2021
@karelz karelz added bug os-linux Linux OS (any supported distro) os-mac-os-x macOS aka OSX and removed untriaged New issue has not been triaged by the area owner labels Nov 16, 2021
@filipnavara
Copy link
Member

I am currently without access to IPv6 infrastructure so if anyone can give the PR a try it would expedite the fix. Something like this would be reasonable code to test:

using System.Net.NetworkInformation;

var ping = new Ping();
var reply = ping.Send("2001:4860:4860::8888", 5000, new byte[32], new PingOptions(1, false)); // Remove PingOptions to make is succeed
Console.WriteLine($"Status: {reply.Status} Address: {reply.Address}");

@wfurt
Copy link
Member

wfurt commented Nov 16, 2021

I will try it @filipnavara. I also skimmed through the change and it generally looks OK to me.

@wfurt
Copy link
Member

wfurt commented Nov 17, 2021

LGTM. I posted reply with suggestion for test on #61592

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Nov 30, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Dec 30, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net bug os-linux Linux OS (any supported distro) os-mac-os-x macOS aka OSX
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants