Fix missing big-endian conversions for multi-byte NTLM wire fields

[Copilot]

PR #124598 added big-endian support for ManagedNtlm but missed several multi-byte fields. On big-endian architectures, MessageField.Length/MaximumLength, ChallengeMessage.Flags, AuthenticateMessage.Flags, and NtChallengeResponse.Time were still read/written without byte-swap, corrupting the NTLM wire format.

Description

All changes follow the endianness-aware property pattern already established by _payloadOffset, _productBuild, and NegotiateMessage.Flags:

private T _field;
public T Field
{
    readonly get => BitConverter.IsLittleEndian ? _field : BinaryPrimitives.ReverseEndianness(_field);
    set => _field = BitConverter.IsLittleEndian ? value : BinaryPrimitives.ReverseEndianness(value);
}

MessageField

• Length and MaximumLength (ushort) converted from public fields to private backing fields with endianness-aware properties
• unsafe removed from struct (no fixed arrays, no longer needed)
• GetFieldLength and GetFieldOffset helpers removed; GetField now accesses field.Length and field.PayloadOffset directly
• SetField → direct property assignments (was MemoryMarshal.AsBytes + WriteInt16/32LittleEndian)

ChallengeMessage.Flags (uint)

• Converted to private _flags + property; removed the inline BitConverter.IsLittleEndian conversion at the call site

AuthenticateMessage.Flags (uint)

• Same treatment as ChallengeMessage.Flags

NtChallengeResponse.Time (long)

• Converted to private _time + endianness-aware property

All [StructLayout(LayoutKind.Sequential)] struct layouts are unchanged — backing fields remain in identical declaration positions.

Original prompt

Background

PR #124598 added big-endian support for ManagedNtlm by introducing endianness-aware getters/setters on some struct fields. However, several multi-byte fields were missed and still need conversion. The NTLM wire protocol is little-endian, so all multi-byte fields in the overlay structs must be stored in little-endian format. On big-endian architectures, the fields that are accessed directly (without BinaryPrimitives conversion) will have the wrong byte order.

The file to modify is:
src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs

What PR #124598 already fixed

The following fields already have proper endianness-aware getters/setters:

• MessageField._payloadOffset (int) — has PayloadOffset property with ReverseEndianness
• Version._productBuild (ushort) — has ProductBuild property with ReverseEndianness
• NegotiateMessage._flags (Flags/uint) — has Flags property with ReverseEndianness

Additionally, ChallengeMessage.Flags is read with an inline endianness conversion at the call site (line 592).

What still needs to be fixed

The following multi-byte fields are still directly accessed without endianness conversion and need the same getter/setter treatment:

1. MessageField.Length and MessageField.MaximumLength (both ushort)

These are currently public fields (public ushort Length; public ushort MaximumLength;) accessed directly. They should be made private backing fields with endianness-aware property getters/setters, like _payloadOffset already is.

Note: After adding properties, the helper functions GetFieldLength, GetFieldOffset, and SetField that currently use BinaryPrimitives.ReadInt16LittleEndian / WriteInt16LittleEndian / WriteInt32LittleEndian to read/write the raw bytes of MessageField should be refactored to simply use the new properties directly. This eliminates the redundant byte-level endianness handling since the properties now handle it. Similarly, GetFieldOffset already reads the offset via ReadInt16LittleEndian on raw bytes, but the PayloadOffset property getter already handles this — so GetFieldOffset should just use field.PayloadOffset. After this refactoring, the unsafe modifier can likely be removed from GetFieldLength and GetFieldOffset.

2. ChallengeMessage.Flags (Flags/uint)

Currently a public field. The conversion is done inline at the call site (line 592) with:

Flags flags = BitConverter.IsLittleEndian ? challengeMessage.Flags : (Flags)BinaryPrimitives.ReverseEndianness((uint)challengeMessage.Flags);

This should be converted to a private backing field with an endianness-aware property (like NegotiateMessage.Flags already is), and the call site should simply read challengeMessage.Flags without the inline conversion.

3. AuthenticateMessage.Flags (Flags/uint)

Currently a public field that is written to directly on line 646:

response.Flags = s_requiredFlags | (flags & Flags.NegotiateSeal);

This should be converted to a private backing field with an endianness-aware property getter/setter, like NegotiateMessage.Flags already is.

4. NtChallengeResponse.Time (long)

Currently a public field written on line 424:

temp.Time = time.ToFileTimeUtc();

This needs to be stored as little-endian on the wire. Should be converted to a private backing field with an endianness-aware property.

5. NtChallengeResponse._reserved3 (int) and NtChallengeResponse._reserved4 (int)

These are private int fields. Although they are reserved (always zero-initialized via Clear()), they should still have endianness conversion for correctness and consistency. Since they're always zero, the conversion is a no-op in practice, but it's good form. However, since they are private and only ever zero, these can be left as-is if the team prefers — the key point is Time above.

Summary of changes needed

1. MessageField: Make Length and MaximumLength private with endianness-aware properties. Simplify GetFieldLength, GetFieldOffset, and SetField to use the new properties.
2. ChallengeMessage: Make Flags a private backing field with endianness-aware property (same pattern as NegotiateMessage.Flags). Remove inline conversion at call site.
3. AuthenticateMessage: Make Flags a private backing field with endianness-aware property.
4. NtChallengeResponse: Make Time a private backing field with endianness-aware property.

The pattern for all should be the same as the existing conversions in the file:

private T _backingField;
public T Property
{
    readonly get => BitConverter.IsLittleEndian ? _backingField : BinaryPrimitives.ReverseEndianness(_backingField);
    set => _backingField = BitConverter.IsLittleEndian ? value : BinaryPrimitives.ReverseEndianness(value);
}

This pull request was created from Copilot chat.

---

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

[rzikm]

@ShreyaLaxminarayan Can you test with this change? I don't have a Big-Endian machine available to me at the moment.

[dotnet-policy-service]

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

[Copilot]

Pull request overview

Fixes remaining big-endian issues in Managed NTLM wire-format overlay structs by making several multi-byte fields endianness-aware, aligning them with the existing pattern used for other fields.

Changes:

• Update MessageField.Length/MaximumLength to use private backing fields + endianness-aware properties; simplify GetField/SetField accordingly.
• Update ChallengeMessage.Flags and AuthenticateMessage.Flags to use private backing fields + endianness-aware properties (removing call-site byte swapping).
• Update NtChallengeResponse.Time to use a private backing field + endianness-aware property.

[saitama951]

@Dotnet-s390x build

[Dotnet-s390x]

Build Queued..

To cancel the current build, please comment:

@Dotnet-s390x cancel

[Dotnet-s390x]

Build Failed
Please check the build logs: http://148.100.85.217:8080/job/dotnet-builds/74/console.

Build Error Summary

Build FAILED.

/var/lib/jenkins/workspace/dotnet-builds/repo/eng/testing/tests.targets(208,5): error : One or more tests failed while running tests from 'Microsoft.Extensions.Hosting.Unit.Tests'. Please check /var/lib/jenkins/workspace/dotnet-builds/repo/artifacts/bin/Microsoft.Extensions.Hosting.Unit.Tests/Debug/net11.0/testResults.xml for details! [/var/lib/jenkins/workspace/dotnet-builds/repo/src/libraries/Microsoft.Extensions.Hosting/tests/UnitTests/Microsoft.Extensions.Hosting.Unit.Tests.csproj::TargetFramework=net11.0]
/var/lib/jenkins/workspace/dotnet-builds/repo/eng/testing/tests.targets(208,5): error : One or more tests failed while running tests from 'System.Net.Sockets.Tests'. Please check /var/lib/jenkins/workspace/dotnet-builds/repo/artifacts/bin/System.Net.Sockets.Tests/Debug/net11.0-unix/testResults.xml for details! [/var/lib/jenkins/workspace/dotnet-builds/repo/src/libraries/System.Net.Sockets/tests/FunctionalTests/System.Net.Sockets.Tests.csproj::TargetFramework=net11.0-unix]
    0 Warning(s)
    2 Error(s)

Time Elapsed 01:03:39.10
Build failed with exit code 1. Check errors above.

[saitama951]

Failures are un-related to the changes in the PR

[stephentoub]

@copilot invoke the code-review skill and post the analysis/results as a comment on this PR

[Copilot]

@copilot invoke the code-review skill and post the analysis/results as a comment on this PR

🤖 Copilot Code Review — PR #125039

Holistic Assessment

Motivation: Justified. PR #124598 added big-endian support to ManagedNtlm but inconsistently applied the pattern — NegotiateMessage.Flags, PayloadOffset, and ProductBuild got endianness-aware properties, while MessageField.Length/MaximumLength, ChallengeMessage.Flags, AuthenticateMessage.Flags, and NtChallengeResponse.Time were left as bare fields. This PR closes those gaps.

Approach: Correct. The fix consistently applies the established BitConverter.IsLittleEndian ? _field : BinaryPrimitives.ReverseEndianness(_field) property pattern to all remaining multi-byte overlay-struct fields. On little-endian platforms the branch is a JIT-time constant, so these properties compile to direct field access with zero overhead.

Summary: ✅ LGTM. The change is correct, complete, and consistent with the existing codebase pattern. All multi-byte fields in NTLM wire-format overlay structs are now endianness-aware. Struct layouts are preserved (private backing fields occupy the same positions as the former public fields). The code simplifications (GetFieldLength/GetFieldOffset removal, SetField simplification, call-site cleanup) are natural consequences of the endianness properties and reduce unsafe code surface.

---

Detailed Findings

✅ Struct layout preservation verified

All [StructLayout(LayoutKind.Sequential)] structs have their backing fields in identical declaration positions. Properties don't affect sequential layout. Verified for MessageField, ChallengeMessage, AuthenticateMessage, and NtChallengeResponse.

✅ Endianness coverage is complete

Audited all struct fields in the file:

• Byte-sized fields (MessageType, VersionMajor, VersionMinor, CurrentRevision, Responserversion, HiResponserversion): No conversion needed. ✅
• Fixed byte arrays (Header, ServerChallenge, ClientChallenge, Hmac, Mic, ServerInfo): Byte order irrelevant. ✅
• Multi-byte fields: All now have endianness-aware properties (Length, MaximumLength, PayloadOffset, ProductBuild, Flags on all three message types, Time). ✅
• Reserved/unused fields (_unused, _reserved1-4): Never accessed, no conversion needed. ✅

✅ unsafe removal from MessageField is correct

The struct had no fixed arrays and no pointer members. The unsafe keyword was only needed because the old GetFieldLength/GetFieldOffset took &field. With those removed, unsafe is correctly dropped.

✅ GetFieldLength/GetFieldOffset removal and SetField simplification

The old GetFieldLength read 2 bytes via ReadInt16LittleEndian from offset 0 — correct for Length (ushort). The old GetFieldOffset read 2 bytes via ReadInt16LittleEndian from offset 4 — this was actually subtly wrong: PayloadOffset is a 4-byte int, but GetFieldOffset only read the low 2 bytes. This worked for typical NTLM messages (small offsets) but was technically incorrect. The new code reads the full 4-byte PayloadOffset via the property, which is more correct.

SetField simplification from MemoryMarshal.AsBytes + WriteInt16/32LittleEndian to direct property assignments is clean and equivalent.

✅ ProcessChallenge call-site cleanup

The inline conversion Flags flags = BitConverter.IsLittleEndian ? challengeMessage.Flags : (Flags)BinaryPrimitives.ReverseEndianness((uint)challengeMessage.Flags) is correctly replaced by just Flags flags = challengeMessage.Flags since the property now handles endianness.

✅ GetField bounds check and return simplification

The old GetField called GetFieldOffset/GetFieldLength twice — once for the bounds check and again for the Slice call. The new code stores offset/length in locals and uses them consistently. The bounds check offset + length > payload.Length could theoretically overflow for adversarial inputs, but in practice NTLM messages are small (payload is bounded by available memory and Length is a ushort max 65535).

✅ Remaining BinaryPrimitives.Read/WriteLittleEndian calls are correct

The remaining explicit BinaryPrimitives calls in the file (lines 469, 516, 547, 747, 748, 763) all operate on raw byte buffers for wire-format data (AV pair parsing, channel binding hashing, NTLM signatures). These must always use little-endian regardless of platform and are correctly not converted to struct properties.

💡 Pre-existing: ProcessTargetInfo return value (out of scope)

Line 575: return targetInfoBuffer.AsSpan(targetInfoOffset).ToArray() returns the unfilled tail of the buffer, not the filled portion [0..targetInfoOffset]. This path is reached when the server's challenge includes TargetName or ChannelBindings AV pairs (which get skipped and re-inserted). In practice, FakeNtlmServer doesn't send these entries, so the buffer is always exactly full and this path is never taken. This is a pre-existing issue unrelated to this PR.

[stephentoub]

@rzikm #125201

[rzikm]

@rzikm #125201

I noticed .... just after I posted the comment and switched to the next github notification :D