Skip to content

Fix parsing of ping error replies on raw sockets #61592

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Nov 30, 2021
Merged

Fix parsing of ping error replies on raw sockets #61592

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a6e276
Fix parsing of ping error replies on raw sockets
filipnavara Nov 15, 2021
36dc5f9
Re-enable ping tests
filipnavara Nov 15, 2021
2419b27
Update the SendPingToExternalHostWithLowTtlTest test validation
filipnavara Nov 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 83 additions & 6 deletions src/libraries/System.Net.Ping/src/System/Net/NetworkInformation/Ping.RawSocket.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ public partial class Ping
private const int IcmpHeaderLengthInBytes = 8;
private const int MinIpHeaderLengthInBytes = 20;
private const int MaxIpHeaderLengthInBytes = 60;
private const int IpV6HeaderLengthInBytes = 40;

private unsafe SocketConfig GetSocketConfig(IPAddress address, byte[] buffer, int timeout, PingOptions? options)
{
Expand Down Expand Up @@ -123,22 +124,98 @@ private bool TryGetPingReply(
}

int icmpHeaderOffset = ipHeaderLength;
int dataOffset = ipHeaderLength + IcmpHeaderLengthInBytes;

// Skip IP header.
IcmpHeader receivedHeader = MemoryMarshal.Read<IcmpHeader>(receiveBuffer.AsSpan(icmpHeaderOffset));
ushort identifier = 0;
type = receivedHeader.Type;
code = receivedHeader.Code;

if (socketConfig.Identifier != receivedHeader.Identifier
|| type == (byte)IcmpV4MessageType.EchoRequest
|| type == (byte)IcmpV6MessageType.EchoRequest) // Echo Request, ignore
// Validate the ICMP header and get the identifier
if (socketConfig.IsIpv4)
{
if (type == (byte)IcmpV4MessageType.EchoReply)
{
// Reply packet has the identifier in the ICMP header.
identifier = receivedHeader.Identifier;
}
else if (type == (byte)IcmpV4MessageType.DestinationUnreachable ||
type == (byte)IcmpV4MessageType.TimeExceeded ||
type == (byte)IcmpV4MessageType.ParameterProblemBadIPHeader ||
type == (byte)IcmpV4MessageType.SourceQuench ||
type == (byte)IcmpV4MessageType.RedirectMessage)
{
// Original IP+ICMP request is in the payload. Read the ICMP header from
// the payload to get identifier.

if (dataOffset + MinIpHeaderLengthInBytes + IcmpHeaderLengthInBytes > bytesReceived)
{
return false;
}

byte ihl = (byte)(receiveBuffer[dataOffset] & 0x0f); // Internet Header Length
int payloadIpHeaderLength = 4 * ihl;

if (bytesReceived - dataOffset - payloadIpHeaderLength < IcmpHeaderLengthInBytes)
{
return false; // Not enough bytes to reconstruct actual IP header + ICMP header.
}

IcmpHeader originalRequestHeader = MemoryMarshal.Read<IcmpHeader>(receiveBuffer.AsSpan(dataOffset + payloadIpHeaderLength));
identifier = originalRequestHeader.Identifier;

// Update the date offset to point past the payload IP+ICMP headers. While the specification
// doesn't indicate there should be any additional data the reality is that we often get the
// original packet data back.
dataOffset += payloadIpHeaderLength + IcmpHeaderLengthInBytes;
}
else
{
return false;
}
}
else
{
if (type == (byte)IcmpV6MessageType.EchoReply)
{
// Reply packet has the identifier in the ICMP header.
identifier = receivedHeader.Identifier;
}
else if (type == (byte)IcmpV6MessageType.DestinationUnreachable ||
type == (byte)IcmpV6MessageType.TimeExceeded ||
type == (byte)IcmpV6MessageType.ParameterProblem ||
type == (byte)IcmpV6MessageType.PacketTooBig)
{
// Original IP+ICMP request is in the payload. Read the ICMP header from
// the payload to get identifier.

if (bytesReceived - dataOffset < IpV6HeaderLengthInBytes + IcmpHeaderLengthInBytes)
{
return false; // Not enough bytes to reconstruct actual IP header + ICMP header.
}

IcmpHeader originalRequestHeader = MemoryMarshal.Read<IcmpHeader>(receiveBuffer.AsSpan(dataOffset + IpV6HeaderLengthInBytes));
identifier = originalRequestHeader.Identifier;

// Update the date offset to point past the payload IP+ICMP headers. While the specification
// doesn't indicate there should be any additional data the reality is that we often get the
// original packet data back.
dataOffset += IpV6HeaderLengthInBytes + IcmpHeaderLengthInBytes;
}
else
{
return false;
}
}

if (socketConfig.Identifier != identifier)
{
return false;
}

sw.Stop();
long roundTripTime = sw.ElapsedMilliseconds;
int dataOffset = ipHeaderLength + IcmpHeaderLengthInBytes;
// We want to return a buffer with the actual data we sent out, not including the header data.
byte[] dataBuffer = new byte[bytesReceived - dataOffset];
Buffer.BlockCopy(receiveBuffer, dataOffset, dataBuffer, 0, dataBuffer.Length);
Expand All @@ -162,7 +239,7 @@ private PingReply SendIcmpEchoRequestOverRawSocket(IPAddress address, byte[] buf
{
socket.SendTo(socketConfig.SendBuffer, SocketFlags.None, socketConfig.EndPoint);

byte[] receiveBuffer = new byte[MaxIpHeaderLengthInBytes + IcmpHeaderLengthInBytes + buffer.Length];
byte[] receiveBuffer = new byte[2 * (MaxIpHeaderLengthInBytes + IcmpHeaderLengthInBytes) + buffer.Length];

long elapsed;
Stopwatch sw = Stopwatch.StartNew();
Expand Down Expand Up @@ -213,7 +290,7 @@ await socket.SendToAsync(
timeoutTokenSource.Token)
.ConfigureAwait(false);

byte[] receiveBuffer = new byte[MaxIpHeaderLengthInBytes + IcmpHeaderLengthInBytes + buffer.Length];
byte[] receiveBuffer = new byte[2 * (MaxIpHeaderLengthInBytes + IcmpHeaderLengthInBytes) + buffer.Length];

Stopwatch sw = Stopwatch.StartNew();
// Read from the socket in a loop. We may receive messages that are not echo replies, or that are not in response
Expand Down
27 changes: 2 additions & 25 deletions src/libraries/System.Net.Ping/tests/FunctionalTests/PingTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,6 @@ public async Task SendPingAsync_InvalidArgs()
[Theory]
[InlineData(AddressFamily.InterNetwork)]
[InlineData(AddressFamily.InterNetworkV6)]
[ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
public void SendPingWithIPAddress(AddressFamily addressFamily)
{
IPAddress localIpAddress = TestSettings.GetLocalIPAddress(addressFamily);
Expand All @@ -145,7 +144,6 @@ public void SendPingWithIPAddress(AddressFamily addressFamily)
[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
[InlineData(AddressFamily.InterNetwork)]
[InlineData(AddressFamily.InterNetworkV6)]
[ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
public async Task SendPingAsyncWithIPAddress(AddressFamily addressFamily)
{
IPAddress localIpAddress = await TestSettings.GetLocalIPAddressAsync(addressFamily);
Expand All @@ -166,7 +164,6 @@ await SendBatchPingAsync(
[Theory]
[InlineData(AddressFamily.InterNetwork)]
[InlineData(AddressFamily.InterNetworkV6)]
[ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
public void SendPingWithIPAddress_AddressAsString(AddressFamily addressFamily)
{
IPAddress localIpAddress = TestSettings.GetLocalIPAddress(addressFamily);
Expand All @@ -185,7 +182,6 @@ public void SendPingWithIPAddress_AddressAsString(AddressFamily addressFamily)
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
public async Task SendPingAsyncWithIPAddress_AddressAsString()
{
IPAddress localIpAddress = await TestSettings.GetLocalIPAddressAsync();
Expand All @@ -199,7 +195,6 @@ await SendBatchPingAsync(
}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
public void SendPingWithIPAddressAndTimeout()
{
IPAddress localIpAddress = TestSettings.GetLocalIPAddress();
Expand All @@ -213,7 +208,6 @@ public void SendPingWithIPAddressAndTimeout()
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
public async Task SendPingAsyncWithIPAddressAndTimeout()
{
IPAddress localIpAddress = await TestSettings.GetLocalIPAddressAsync();
Expand Down Expand Up @@ -260,7 +254,6 @@ await SendBatchPingAsync(

[PlatformSpecific(TestPlatforms.AnyUnix)] // On Unix, Non-root pings cannot send arbitrary data in the buffer, and do not receive it back in the PingReply.
[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
public void SendPingWithIPAddressAndTimeoutAndBuffer_Unix()
{
byte[] buffer = TestSettings.PayloadAsBytes;
Expand All @@ -286,7 +279,6 @@ public void SendPingWithIPAddressAndTimeoutAndBuffer_Unix()

[PlatformSpecific(TestPlatforms.AnyUnix)] // On Unix, Non-root pings cannot send arbitrary data in the buffer, and do not receive it back in the PingReply.
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
public async Task SendPingAsyncWithIPAddressAndTimeoutAndBuffer_Unix()
{
byte[] buffer = TestSettings.PayloadAsBytes;
Expand Down Expand Up @@ -350,7 +342,6 @@ await SendBatchPingAsync(
[Theory]
[InlineData(AddressFamily.InterNetwork)]
[InlineData(AddressFamily.InterNetworkV6)]
[ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
public void SendPingWithIPAddressAndTimeoutAndBufferAndPingOptions_Unix(AddressFamily addressFamily)
{
IPAddress localIpAddress = TestSettings.GetLocalIPAddress(addressFamily);
Expand Down Expand Up @@ -383,7 +374,6 @@ public void SendPingWithIPAddressAndTimeoutAndBufferAndPingOptions_Unix(AddressF
[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
[InlineData(AddressFamily.InterNetwork)]
[InlineData(AddressFamily.InterNetworkV6)]
[ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
public async Task SendPingAsyncWithIPAddressAndTimeoutAndBufferAndPingOptions_Unix(AddressFamily addressFamily)
{
IPAddress localIpAddress = await TestSettings.GetLocalIPAddressAsync(addressFamily);
Expand Down Expand Up @@ -413,7 +403,6 @@ await SendBatchPingAsync(
}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
public void SendPingWithHost()
{
IPAddress[] localIpAddresses = TestSettings.GetLocalIPAddresses();
Expand All @@ -427,7 +416,6 @@ public void SendPingWithHost()
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
public async Task SendPingAsyncWithHost()
{
IPAddress[] localIpAddresses = await TestSettings.GetLocalIPAddressesAsync();
Expand All @@ -441,7 +429,6 @@ await SendBatchPingAsync(
}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
public void SendPingWithHostAndTimeout()
{
IPAddress[] localIpAddresses = TestSettings.GetLocalIPAddresses();
Expand All @@ -455,7 +442,6 @@ public void SendPingWithHostAndTimeout()
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
public async Task SendPingAsyncWithHostAndTimeout()
{
IPAddress[] localIpAddresses = await TestSettings.GetLocalIPAddressesAsync();
Expand Down Expand Up @@ -502,7 +488,6 @@ await SendBatchPingAsync(

[PlatformSpecific(TestPlatforms.AnyUnix)] // On Unix, Non-root pings cannot send arbitrary data in the buffer, and do not receive it back in the PingReply.
[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
public void SendPingWithHostAndTimeoutAndBuffer_Unix()
{
IPAddress[] localIpAddresses = TestSettings.GetLocalIPAddresses();
Expand All @@ -528,7 +513,6 @@ public void SendPingWithHostAndTimeoutAndBuffer_Unix()

[PlatformSpecific(TestPlatforms.AnyUnix)] // On Unix, Non-root pings cannot send arbitrary data in the buffer, and do not receive it back in the PingReply.
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
public async Task SendPingAsyncWithHostAndTimeoutAndBuffer_Unix()
{
IPAddress[] localIpAddresses = await TestSettings.GetLocalIPAddressesAsync();
Expand Down Expand Up @@ -588,7 +572,6 @@ await SendBatchPingAsync(

[PlatformSpecific(TestPlatforms.AnyUnix)] // On Unix, Non-root pings cannot send arbitrary data in the buffer, and do not receive it back in the PingReply.
[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
public void SendPingWithHostAndTimeoutAndBufferAndPingOptions_Unix()
{
IPAddress[] localIpAddresses = TestSettings.GetLocalIPAddresses();
Expand All @@ -614,7 +597,6 @@ public void SendPingWithHostAndTimeoutAndBufferAndPingOptions_Unix()

[PlatformSpecific(TestPlatforms.AnyUnix)] // On Unix, Non-root pings cannot send arbitrary data in the buffer, and do not receive it back in the PingReply.
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
public async Task SendPingAsyncWithHostAndTimeoutAndBufferAndPingOptions_Unix()
{
IPAddress[] localIpAddresses = await TestSettings.GetLocalIPAddressesAsync();
Expand Down Expand Up @@ -663,7 +645,6 @@ public async Task SendPingWithIPAddressAndBigSize()
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
public async Task SendPings_ReuseInstance_Hostname()
{
IPAddress[] localIpAddresses = await TestSettings.GetLocalIPAddressesAsync();
Expand All @@ -679,7 +660,6 @@ public async Task SendPings_ReuseInstance_Hostname()
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
public async Task Sends_ReuseInstance_Hostname()
{
IPAddress[] localIpAddresses = await TestSettings.GetLocalIPAddressesAsync();
Expand All @@ -695,7 +675,6 @@ public async Task Sends_ReuseInstance_Hostname()
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
public async Task SendAsyncs_ReuseInstance_Hostname()
{
IPAddress[] localIpAddresses = await TestSettings.GetLocalIPAddressesAsync();
Expand Down Expand Up @@ -746,7 +725,6 @@ public async Task SendAsyncs_ReuseInstance_Hostname()
}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
public static void Ping_DisposeAfterSend_Success()
{
Ping p = new Ping();
Expand All @@ -755,7 +733,6 @@ public static void Ping_DisposeAfterSend_Success()
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
public static async Task PingAsync_DisposeAfterSend_Success()
{
Ping p = new Ping();
Expand Down Expand Up @@ -832,7 +809,6 @@ public void CanBeFinalized()
[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsThreadingSupported))]
[InlineData(true)]
[InlineData(false)]
[ActiveIssue("https://github.com/dotnet/runtime/issues/52617", TestPlatforms.iOS | TestPlatforms.tvOS | TestPlatforms.MacCatalyst)]
public async Task SendPingAsyncWithHostAndTtlAndFragmentPingOptions(bool fragment)
{
IPAddress[] localIpAddresses = await TestSettings.GetLocalIPAddressesAsync();
Expand Down Expand Up @@ -878,7 +854,8 @@ public async Task SendPingToExternalHostWithLowTtlTest()
options.Ttl = 1;
// This should always fail unless host is one IP hop away.
pingReply = await ping.SendPingAsync(host, TestSettings.PingTimeout, TestSettings.PayloadAsBytesShort, options);
Assert.NotEqual(IPStatus.Success, pingReply.Status);
Assert.Equal(IPStatus.TimeExceeded, pingReply.Status);
Assert.NotEqual(IPAddress.Any, pingReply.Address);
}

[Fact]
Expand Down