Implement DnsResolver for Linux - #2
anurag6569201 wants to merge 1 commit into
Conversation
Source PR: dotnet#129846 Source head: 60c7a85
⛔ Shipwright · BlockedRecommendation: do not merge PR #2 · Tier
Findings (12)
Fireworks usage: 51,047 input · 1,649 output · 52,696 total tokens · $0.0123 · 24s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
![shipwright-agent[bot]](https://avatars.githubusercontent.com/u/140162738?s=60&v=4){kind=small}
| private static readonly SearchValues<byte> s_ldhBytes = | ||
| SearchValues.Create("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-_"u8); | ||
|
|
||
| // Validates that a label has valid length (1-63), contains only LDH (Letters, |
There was a problem hiding this comment.
Shipwright · CRITICAL
DnsLabelEnumerator.MoveNext reads _buffer[_pos] without any bounds check.
Impact: DnsLabelEnumerator.MoveNext reads _buffer[_pos] without any bounds check. The enumerator is public and can be constructed directly via DnsEncodedName.EnumerateLabels() on a default/empty DnsEncodedName, or after TryParse fails, yielding _pos=0 on an empty buffer. This throws IndexOutOfRangeException instead of returning false, violating the Try* contract and crashing on malformed input.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| int labelStart = 0; | ||
| bool isAce = aceName != null; | ||
| while (true) | ||
| { |
There was a problem hiding this comment.
Shipwright · CRITICAL
DnsEncodedName.TryEncode computes wireLen = name.Length + 2 before validating that each label is <= 63 bytes.
Impact: DnsEncodedName.TryEncode computes wireLen = name.Length + 2 before validating that each label is <= 63 bytes. A name with a single label longer than 63 characters (e.g., 200 chars) passes the wireLen <= 255 check, then IsValidLabel rejects it. However, a name with multiple labels where one label is 64-253 bytes can have wireLen <= 255 but the label length byte written to destination[labelStart] truncates to…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| // encoding), label content is also validated for LDH compliance. | ||
| private static bool ValidateName(ReadOnlySpan<byte> buffer, int offset, | ||
| out int wireLength, out int formattedLength, out bool isAce, | ||
| out bool hasPointers, bool validateContent = false) |
There was a problem hiding this comment.
Shipwright · CRITICAL
DnsEncodedName.ValidateName allows compression pointers to jump backward to any offset, including into the middle of a label or into the header.
Impact: DnsEncodedName.ValidateName allows compression pointers to jump backward to any offset, including into the middle of a label or into the header. The pointer target is not validated to be a label boundary. An attacker can craft a response where a pointer targets a length byte that is actually part of RDATA or the header, causing the parser to interpret arbitrary bytes as label lengths and potentially read out of boun…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| internal readonly ref struct DnsEncodedName | ||
| { | ||
| private static readonly IdnMapping s_idnMapping = new IdnMapping { AllowUnassigned = false, UseStd3AsciiRules = true }; | ||
|
|
There was a problem hiding this comment.
Shipwright · CRITICAL
DnsEncodedName.TryEncode uses a static shared IdnMapping instance (s_idnMapping) without synchronization.
Impact: DnsEncodedName.TryEncode uses a static shared IdnMapping instance (s_idnMapping) without synchronization. IdnMapping.GetAscii/TryGetUnicode are not documented thread-safe, and this type is used from the DNS resolver which can be called concurrently from multiple threads. Concurrent use can corrupt internal state or produce incorrect ACE encodings, leading to wrong DNS queries or IDN spoofing.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| public override unsafe string ToString() | ||
| { | ||
| Span<char> chars = stackalloc char[MaxEncodedLength + 1]; | ||
| bool success = TryDecode(chars, out int charsWritten); |
There was a problem hiding this comment.
Shipwright · CRITICAL
DnsEncodedName.TryDecodeAscii widens each byte directly to char ((char)label[i]) for response labels, explicitly allowing bytes outside ASCII range.
Impact: DnsEncodedName.TryDecodeAscii widens each byte directly to char ((char)label[i]) for response labels, explicitly allowing bytes outside ASCII range. This means attacker-controlled DNS responses can inject arbitrary Unicode characters into the decoded name, which is then used for hostname comparison, certificate validation, or displayed to users. This is a DNS response injection / IDN homograph attack vector.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| return OperationStatus.InvalidData; | ||
| } | ||
|
|
||
| if (!isAce) |
There was a problem hiding this comment.
Shipwright · HIGH
DnsEncodedName.TryEncode has a subtle bug: it writes the label length byte to destination[labelStart] BEFORE validating the label with IsValidLabel.
Impact: DnsEncodedName.TryEncode has a subtle bug: it writes the label length byte to destination[labelStart] BEFORE validating the label with IsValidLabel. If validation fails, the destination buffer is left in a partially modified state, and the caller may not realize the buffer was corrupted. The write should happen after validation.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| // Represents a domain name in DNS wire format (RFC 1035 §4.1.4). | ||
| // Works for both the read path (responses with compression pointers) and the | ||
| // write path (flat encoded names). | ||
| internal readonly ref struct DnsEncodedName |
There was a problem hiding this comment.
Shipwright · HIGH
The DnsEncodedName struct is a ref struct with a private ReadOnlySpan<byte> _buffer field.
Impact: The DnsEncodedName struct is a ref struct with a private ReadOnlySpan<byte> _buffer field. This makes it impossible to store in heap objects or async state machines. The DNS resolver is inherently async (Dns.GetHostAddressesAsync), so this design forces all parsing to happen synchronously on the stack, which may not be compatible with the async resolver architecture.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| </ItemGroup> | ||
| <!-- Production code under test: the managed DNS wire-format parsing/encoding types. --> | ||
| <ItemGroup> | ||
| <Compile Include="..\..\src\System\Net\DnsEncodedName.cs" |
There was a problem hiding this comment.
Shipwright · HIGH
The test project compiles production source files directly (Compile Include with Link) rather than referencing the production assembly.
Impact: The test project compiles production source files directly (Compile Include with Link) rather than referencing the production assembly. This means tests exercise a copy of the code, not the actual shipped assembly. If the production csproj and test csproj diverge in compilation conditions or defines, tests can pass while production fails.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| // DnsHeaderFlags enum values are the wire bit positions shifted right by 4, | ||
| // so the enum fits in a byte. Encoding shifts left by 4 to restore wire positions, | ||
| // decoding shifts right by 4. The Z bit (wire bit 6) gap is preserved by the shift. | ||
| // Wire flag bits: AA(10) TC(9) RD(8) RA(7) AD(5) CD(4) |
There was a problem hiding this comment.
Shipwright · HIGH
DnsMessageHeader.EncodeFlagsWord uses FlagsShift=4 and WireFlagsMask=0x07F0, but the comment says the Z bit (wire bit 6) gap is preserved.
Impact: DnsMessageHeader.EncodeFlagsWord uses FlagsShift=4 and WireFlagsMask=0x07F0, but the comment says the Z bit (wire bit 6) gap is preserved. The mask 0x07F0 covers bits 4-10, which includes bit 6 (Z). If the Flags enum ever has a value in the Z bit position, it will be silently encoded. The mask should be 0x0770 to exclude bit 6.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| { | ||
| ascii[..asciiWritten].CopyTo(destination); | ||
| charsWritten = asciiWritten; | ||
| return true; |
There was a problem hiding this comment.
Shipwright · HIGH
DnsEncodedName.GetFormattedLength and ToString use stackalloc char[MaxEncodedLength + 1] (256 chars) and Debug.Assert(success) on TryDecode.
Impact: DnsEncodedName.GetFormattedLength and ToString use stackalloc char[MaxEncodedLength + 1] (256 chars) and Debug.Assert(success) on TryDecode. For ACE names, the Unicode form can be longer than the ASCII wire form in some edge cases (e.g., certain IDN mappings), causing TryDecode to fail and the assert to fire in debug builds, or silent truncation in release builds.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| <ItemGroup Condition="'$(TargetPlatformIdentifier)' == 'unix'"> | ||
| <Compile Include="System\Net\NameResolutionPal.Unix.cs" /> | ||
| <Compile Include="System\Net\DnsResolverPal.Unsupported.cs" /> | ||
| <Compile Include="System\Net\DnsResolverPal.Managed.cs" /> |
There was a problem hiding this comment.
Shipwright · HIGH
The new managed DNS resolver (DnsResolverPal.Managed.cs, DnsSocket.cs, ResolvConf.cs) replaces the unsupported stub on Unix.
Impact: The new managed DNS resolver (DnsResolverPal.Managed.cs, DnsSocket.cs, ResolvConf.cs) replaces the unsupported stub on Unix. This is a significant new attack surface: it parses untrusted DNS responses from the network. The diff shows extensive parsing code but no evidence of response validation against the query (transaction ID, question section matching, source address validation). DNS spoofing/cache poisoning is a…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| } | ||
| } | ||
|
|
||
| // Strip trailing dot from the comparison name. |
There was a problem hiding this comment.
Shipwright · HIGH
DnsEncodedName.Equals converts the comparison name to ACE using s_idnMapping.GetAscii but does not validate that the result is a valid DNS name.
Impact: DnsEncodedName.Equals converts the comparison name to ACE using s_idnMapping.GetAscii but does not validate that the result is a valid DNS name. An attacker-controlled comparison string with invalid IDN characters could throw or produce unexpected results, potentially bypassing name comparison checks.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
1 participant
Follow up on dotnet#129845
Source merge-base:
7600052d92b249ebda3210bd34ff345811faa408Source head:
60c7a85bf7edaf52aa1d3a66dd834bd5f32d3c42