dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.98k stars 4.66k forks source link

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

Closed lorenzo93 closed 2 years ago

lorenzo93 commented 2 years ago

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

ghost commented 2 years ago

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 commented 2 years ago

cc: @filipnavara

filipnavara commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@wfurt It fails here:

https://github.com/dotnet/runtime/blob/a3b186b9904844b8001d4bb5d7474f2591ee96d9/src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.RawSocket.cs#L132-L137

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

I suspect Linux may have same problem.

karelz commented 2 years ago

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

filipnavara commented 2 years ago

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 commented 2 years ago

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

wfurt commented 2 years ago

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