Fix ManagedNtlm on big-endian architectures - #124598

Merged
rzikm merged 4 commits into
dotnet:mainfrom
shreyarao4:ntlm
Feb 23, 2026
Merged

Fix ManagedNtlm on big-endian architectures#124598
rzikm merged 4 commits into
dotnet:mainfrom
shreyarao4:ntlm

Conversation

@shreyarao4

@shreyarao4shreyarao4 commented Feb 19, 2026

Copy link
Copy Markdown
Contributor

Fixes multiple endianness bugs in ManagedNtlm that caused incorrect behavior on big-endian platforms.
Validated via existing Ntlm test cases.

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Feb 19, 2026
@shreyarao4

shreyarao4 commented Feb 19, 2026

Copy link
Copy Markdown
ContributorAuthor

@uweigand@giritrivedi@saitama951

@dotnet-policy-service

dotnet-policy-serviceBot commented Feb 19, 2026

Copy link
Copy Markdown
Contributor

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

@build-analysisbuild-analysisBot mentioned this pull request Feb 19, 2026
Closed
wfurt
wfurt reviewed Feb 19, 2026
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs Outdated
wfurt
wfurt approved these changes Feb 19, 2026
View reviewed changes

@wfurtwfurt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general.

rzikm
rzikm reviewed Feb 20, 2026
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs Outdated
rzikm
rzikm approved these changes Feb 23, 2026
View reviewed changes

@rzikmrzikm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks!

@build-analysisbuild-analysisBot mentioned this pull request Feb 23, 2026
Closed
@rzikm
rzikm merged commit 84057bb into dotnet:mainFeb 23, 2026
86 of 90 checks passed
gfoidl
gfoidl reviewed Feb 23, 2026
View reviewed changes

@gfoidlgfoidl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: spaces for code-style.

Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs
Comment on lines +134 to +135
readonly get =>BitConverter.IsLittleEndian? _payloadOffset: BinaryPrimitives.ReverseEndianness(_payloadOffset);
set =>_payloadOffset = BitConverter.IsLittleEndian? value: BinaryPrimitives.ReverseEndianness(value);

@gfoidlgfoidlFeb 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
readonlyget=>BitConverter.IsLittleEndian?_payloadOffset:BinaryPrimitives.ReverseEndianness(_payloadOffset);
set =>_payloadOffset=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);
readonlyget=>BitConverter.IsLittleEndian?_payloadOffset:BinaryPrimitives.ReverseEndianness(_payloadOffset);
set =>_payloadOffset=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);

Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs
Comment on lines +161 to +162
readonly get =>BitConverter.IsLittleEndian? _productBuild: BinaryPrimitives.ReverseEndianness(_productBuild);
set =>_productBuild = BitConverter.IsLittleEndian? value: BinaryPrimitives.ReverseEndianness(value);

@gfoidlgfoidlFeb 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
readonlyget=>BitConverter.IsLittleEndian?_productBuild:BinaryPrimitives.ReverseEndianness(_productBuild);
set =>_productBuild=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);
readonlyget=>BitConverter.IsLittleEndian?_productBuild:BinaryPrimitives.ReverseEndianness(_productBuild);
set =>_productBuild=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);

Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs
Comment on lines +177 to +178
readonly get =>BitConverter.IsLittleEndian? _flags: (Flags)BinaryPrimitives.ReverseEndianness((uint)_flags);
set =>_flags = BitConverter.IsLittleEndian? value: (Flags)BinaryPrimitives.ReverseEndianness((uint)value);

@gfoidlgfoidlFeb 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
readonlyget=>BitConverter.IsLittleEndian?_flags:(Flags)BinaryPrimitives.ReverseEndianness((uint)_flags);
set =>_flags=BitConverter.IsLittleEndian?value:(Flags)BinaryPrimitives.ReverseEndianness((uint)value);
readonlyget=>BitConverter.IsLittleEndian?_flags:(Flags)BinaryPrimitives.ReverseEndianness((uint)_flags);
set =>_flags=BitConverter.IsLittleEndian?value:(Flags)BinaryPrimitives.ReverseEndianness((uint)value);

@dotnet-maestrodotnet-maestroBot mentioned this pull request Feb 24, 2026
Merged
@shreyarao4

shreyarao4 commented Feb 26, 2026•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@rzikm@wfurt Unfortunately, the getters and setters method doesn't resolve the endianness issues because of MemoryMarshal.AsRef. We would have to go with the previous commit 91fc6d6. What can be done?

@rzikm

rzikm commented Feb 26, 2026

Copy link
Copy Markdown
Member

@rzikm@wfurt Unfortunately, the getters and setters method doesn't resolve the endianness issues because of MemoryMarshal.AsRef. We would have to go with the previous commit 91fc6d6. What can be done?

Can you be more specific, what about the MemoryMarshal.AsRef is making the changes broken?

@shreyarao4

shreyarao4 commented Feb 27, 2026

Copy link
Copy Markdown
ContributorAuthor

MemoryMarshal.AsRef is writing the fields in little-endian, not in the correct byte order overwriting the struct fields with little-endian data.

@rzikm

rzikm commented Feb 27, 2026

Copy link
Copy Markdown
Member

MemoryMarshal.AsRef is not writing anything, if you cast Span<byte> to e.g. ref MessageField, the getters/setters added in the PR should take care of endianness conversion on read/write. Similarly, if you create MessageField variable on it's own and copy it to a ref MessageField variable, the backing data should already have the correct endianness. Same if you cast the source MessageField to Span<byte> and copy the raw bytes to the target buffer.

Can you point to the specific piece of code where you see the problem, or give a code example where the pattern does not work?

@shreyarao4

shreyarao4 commented Feb 27, 2026

Copy link
Copy Markdown
ContributorAuthor

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs#L582
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs#L593

Here, challengeMessage.TargetInfo.Length and challengeMessage.TargetInfo.MaximumLength are endian flipped. Can you help me with this?

@rzikm

rzikm commented Mar 2, 2026

Copy link
Copy Markdown
Member

I believe that is because the Length and MaximumLength fields are not corrected for endianness when reading

runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs

Lines 126 to 137 in 1fd4dda

[StructLayout(LayoutKind.Sequential)]
privateunsafestructMessageField
{
publicushortLength;
publicushortMaximumLength;
privateint_payloadOffset;
publicintPayloadOffset
{
readonlyget=>BitConverter.IsLittleEndian?_payloadOffset:BinaryPrimitives.ReverseEndianness(_payloadOffset);
set=>_payloadOffset=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);
}
}

CopilotAI mentioned this pull request Mar 2, 2026
Merged
iremyux pushed a commit to iremyux/dotnet-runtime that referenced this pull request Mar 2, 2026
@shreyarao4@iremyux
Fix ManagedNtlm on big-endian architectures (dotnet#124598)
d3dc818
Fixes multiple endianness bugs in ManagedNtlm that caused incorrect
behavior on big-endian platforms.
Validated via existing Ntlm test cases.
@saitama951

saitama951 commented Mar 3, 2026•
edited
Loading

Copy link
Copy Markdown
Contributor

@rzikm I think
the assertion happens here (http://148.100.85.217:8080/job/daily-builds/211/consoleFull)

runtime/src/libraries/Common/tests/System/Net/Security/FakeNtlmServer.cs

Lines 306 to 307 in c30eec2

Flagsflags=(Flags)BinaryPrimitives.ReadUInt32LittleEndian(incomingBlob.AsSpan(60));
Assert.Equal(_requiredFlags,(flags&_requiredFlags));

we don't write in little endian order here, since response is of type AuthenticateMessage I don't see a getter / setter for the endianess here.

runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs

Lines 645 to 646 in 7356734

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

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs#L206

 while the original commit did handle this (the case where the test passed)
91fc6d6#diff-07ef1e3bc82826f3e53cb6d9d9f4efef56b93b428ebef12c806ca209063ce74bR632

The file already consists of various getters and setter, which take care of the endinanness, i.e GetField and SetField ,

runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs

Lines 324 to 360 in 7356734

privatestaticunsafeintGetFieldLength(MessageFieldfield)
{
ReadOnlySpan<byte>span=newReadOnlySpan<byte>(&field,sizeof(MessageField));
returnBinaryPrimitives.ReadInt16LittleEndian(span);
}
privatestaticunsafeintGetFieldOffset(MessageFieldfield)
{
ReadOnlySpan<byte>span=newReadOnlySpan<byte>(&field,sizeof(MessageField));
returnBinaryPrimitives.ReadInt16LittleEndian(span.Slice(4));
}
privatestaticReadOnlySpan<byte>GetField(MessageFieldfield,ReadOnlySpan<byte>payload)
{
intoffset=GetFieldOffset(field);
intlength=GetFieldLength(field);
if(length==0||offset+length>payload.Length)
{
returnReadOnlySpan<byte>.Empty;
}
returnpayload.Slice(GetFieldOffset(field),GetFieldLength(field));
}
privatestaticunsafevoidSetField(refMessageFieldfield,intlength,intoffset)
{
if(lengthis<0 or >short.MaxValue)
{
thrownewWin32Exception(NTE_FAIL);
}
Span<byte>span=MemoryMarshal.AsBytes(newSpan<MessageField>(reffield));
BinaryPrimitives.WriteInt16LittleEndian(span,(short)length);
BinaryPrimitives.WriteInt16LittleEndian(span.Slice(2),(short)length);
BinaryPrimitives.WriteInt32LittleEndian(span.Slice(4),offset);
}

can't we reuse those?

shreyarao4 added a commit to shreyarao4/runtime that referenced this pull request Mar 3, 2026
@shreyarao4
Fix ManagedNTLM faiures on Big endian architectures
091b9b4
- Fixed several endian bugs in ManagedNTLM implementation.
- This is a follow-up to PR dotnet#124598
@shreyarao4shreyarao4 mentioned this pull request Mar 3, 2026
Closed
@shreyarao4
shreyarao4 deleted the ntlm branch March 3, 2026 07:27
@rzikm

rzikm commented Mar 3, 2026

Copy link
Copy Markdown
Member

#125039 should be addressing these, let's move conversation there if necessary.

rzikm added a commit that referenced this pull request Mar 12, 2026
@rzikm
Fix missing big-endian conversions for multi-byte NTLM wire fields (#…
b4b0bdc
…125039)
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`:
```csharp
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.
<!-- START COPILOT ORIGINAL PROMPT -->
<details>
<summary>Original prompt</summary>
## 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:
```csharp
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:
```csharp
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:
```csharp
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:
```csharp
private T _backingField;
public T Property
{
readonly get => BitConverter.IsLittleEndian ? _backingField : BinaryPrimitives.ReverseEndianness(_backingField);
set => _backingField = BitConverter.IsLittleEndian ? value : BinaryPrimitives.ReverseEndianness(value);
}
```
</details>
<!-- START COPILOT CODING AGENT SUFFIX -->
*This pull request was created from Copilot chat.*
>
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
CopilotAI added a commit that referenced this pull request Mar 13, 2026
@rzikm
Fix missing big-endian conversions for multi-byte NTLM wire fields (#…
024e901
…125039)
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`:
```csharp
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.
<!-- START COPILOT ORIGINAL PROMPT -->
<details>
<summary>Original prompt</summary>
## 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:
```csharp
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:
```csharp
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:
```csharp
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:
```csharp
private T _backingField;
public T Property
{
readonly get => BitConverter.IsLittleEndian ? _backingField : BinaryPrimitives.ReverseEndianness(_backingField);
set => _backingField = BitConverter.IsLittleEndian ? value : BinaryPrimitives.ReverseEndianness(value);
}
```
</details>
<!-- START COPILOT CODING AGENT SUFFIX -->
*This pull request was created from Copilot chat.*
>
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 3, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@wfurtwfurtwfurt approved these changes
@rzikmrzikmrzikm approved these changes
+1 more reviewer
@gfoidlgfoidlgfoidl left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Net.Securitycommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@shreyarao4@rzikm@saitama951@gfoidl@wfurt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all \u003cpre\u003e\u003ccode\u003e blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks"); } } catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); } })(); (function(){ try { var __m = "github.com"; var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Fix ManagedNtlm on big-endian architectures - #124598

Merged
rzikm merged 4 commits into
dotnet:mainfrom
shreyarao4:ntlm
Feb 23, 2026
Merged

Fix ManagedNtlm on big-endian architectures#124598
rzikm merged 4 commits into
dotnet:mainfrom
shreyarao4:ntlm

Conversation

@shreyarao4

@shreyarao4shreyarao4 commented Feb 19, 2026

Copy link
Copy Markdown
Contributor

Fixes multiple endianness bugs in ManagedNtlm that caused incorrect behavior on big-endian platforms.
Validated via existing Ntlm test cases.

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Feb 19, 2026
@shreyarao4

shreyarao4 commented Feb 19, 2026

Copy link
Copy Markdown
ContributorAuthor

@uweigand@giritrivedi@saitama951

@dotnet-policy-service

dotnet-policy-serviceBot commented Feb 19, 2026

Copy link
Copy Markdown
Contributor

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

@build-analysisbuild-analysisBot mentioned this pull request Feb 19, 2026
Closed
wfurt
wfurt reviewed Feb 19, 2026
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs Outdated
wfurt
wfurt approved these changes Feb 19, 2026
View reviewed changes

@wfurtwfurt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general.

rzikm
rzikm reviewed Feb 20, 2026
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs Outdated
rzikm
rzikm approved these changes Feb 23, 2026
View reviewed changes

@rzikmrzikm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks!

@build-analysisbuild-analysisBot mentioned this pull request Feb 23, 2026
Closed
@rzikm
rzikm merged commit 84057bb into dotnet:mainFeb 23, 2026
86 of 90 checks passed
gfoidl
gfoidl reviewed Feb 23, 2026
View reviewed changes

@gfoidlgfoidl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: spaces for code-style.

Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs
Comment on lines +134 to +135
readonly get =>BitConverter.IsLittleEndian? _payloadOffset: BinaryPrimitives.ReverseEndianness(_payloadOffset);
set =>_payloadOffset = BitConverter.IsLittleEndian? value: BinaryPrimitives.ReverseEndianness(value);

@gfoidlgfoidlFeb 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
readonlyget=>BitConverter.IsLittleEndian?_payloadOffset:BinaryPrimitives.ReverseEndianness(_payloadOffset);
set =>_payloadOffset=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);
readonlyget=>BitConverter.IsLittleEndian?_payloadOffset:BinaryPrimitives.ReverseEndianness(_payloadOffset);
set =>_payloadOffset=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);

Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs
Comment on lines +161 to +162
readonly get =>BitConverter.IsLittleEndian? _productBuild: BinaryPrimitives.ReverseEndianness(_productBuild);
set =>_productBuild = BitConverter.IsLittleEndian? value: BinaryPrimitives.ReverseEndianness(value);

@gfoidlgfoidlFeb 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
readonlyget=>BitConverter.IsLittleEndian?_productBuild:BinaryPrimitives.ReverseEndianness(_productBuild);
set =>_productBuild=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);
readonlyget=>BitConverter.IsLittleEndian?_productBuild:BinaryPrimitives.ReverseEndianness(_productBuild);
set =>_productBuild=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);

Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs
Comment on lines +177 to +178
readonly get =>BitConverter.IsLittleEndian? _flags: (Flags)BinaryPrimitives.ReverseEndianness((uint)_flags);
set =>_flags = BitConverter.IsLittleEndian? value: (Flags)BinaryPrimitives.ReverseEndianness((uint)value);

@gfoidlgfoidlFeb 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
readonlyget=>BitConverter.IsLittleEndian?_flags:(Flags)BinaryPrimitives.ReverseEndianness((uint)_flags);
set =>_flags=BitConverter.IsLittleEndian?value:(Flags)BinaryPrimitives.ReverseEndianness((uint)value);
readonlyget=>BitConverter.IsLittleEndian?_flags:(Flags)BinaryPrimitives.ReverseEndianness((uint)_flags);
set =>_flags=BitConverter.IsLittleEndian?value:(Flags)BinaryPrimitives.ReverseEndianness((uint)value);

@dotnet-maestrodotnet-maestroBot mentioned this pull request Feb 24, 2026
Merged
@shreyarao4

shreyarao4 commented Feb 26, 2026•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@rzikm@wfurt Unfortunately, the getters and setters method doesn't resolve the endianness issues because of MemoryMarshal.AsRef. We would have to go with the previous commit 91fc6d6. What can be done?

@rzikm

rzikm commented Feb 26, 2026

Copy link
Copy Markdown
Member

@rzikm@wfurt Unfortunately, the getters and setters method doesn't resolve the endianness issues because of MemoryMarshal.AsRef. We would have to go with the previous commit 91fc6d6. What can be done?

Can you be more specific, what about the MemoryMarshal.AsRef is making the changes broken?

@shreyarao4

shreyarao4 commented Feb 27, 2026

Copy link
Copy Markdown
ContributorAuthor

MemoryMarshal.AsRef is writing the fields in little-endian, not in the correct byte order overwriting the struct fields with little-endian data.

@rzikm

rzikm commented Feb 27, 2026

Copy link
Copy Markdown
Member

MemoryMarshal.AsRef is not writing anything, if you cast Span<byte> to e.g. ref MessageField, the getters/setters added in the PR should take care of endianness conversion on read/write. Similarly, if you create MessageField variable on it's own and copy it to a ref MessageField variable, the backing data should already have the correct endianness. Same if you cast the source MessageField to Span<byte> and copy the raw bytes to the target buffer.

Can you point to the specific piece of code where you see the problem, or give a code example where the pattern does not work?

@shreyarao4

shreyarao4 commented Feb 27, 2026

Copy link
Copy Markdown
ContributorAuthor

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs#L582
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs#L593

Here, challengeMessage.TargetInfo.Length and challengeMessage.TargetInfo.MaximumLength are endian flipped. Can you help me with this?

@rzikm

rzikm commented Mar 2, 2026

Copy link
Copy Markdown
Member

I believe that is because the Length and MaximumLength fields are not corrected for endianness when reading

runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs

Lines 126 to 137 in 1fd4dda

[StructLayout(LayoutKind.Sequential)]
privateunsafestructMessageField
{
publicushortLength;
publicushortMaximumLength;
privateint_payloadOffset;
publicintPayloadOffset
{
readonlyget=>BitConverter.IsLittleEndian?_payloadOffset:BinaryPrimitives.ReverseEndianness(_payloadOffset);
set=>_payloadOffset=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);
}
}

CopilotAI mentioned this pull request Mar 2, 2026
Merged
iremyux pushed a commit to iremyux/dotnet-runtime that referenced this pull request Mar 2, 2026
@shreyarao4@iremyux
Fix ManagedNtlm on big-endian architectures (dotnet#124598)
d3dc818
Fixes multiple endianness bugs in ManagedNtlm that caused incorrect
behavior on big-endian platforms.
Validated via existing Ntlm test cases.
@saitama951

saitama951 commented Mar 3, 2026•
edited
Loading

Copy link
Copy Markdown
Contributor

@rzikm I think
the assertion happens here (http://148.100.85.217:8080/job/daily-builds/211/consoleFull)

runtime/src/libraries/Common/tests/System/Net/Security/FakeNtlmServer.cs

Lines 306 to 307 in c30eec2

Flagsflags=(Flags)BinaryPrimitives.ReadUInt32LittleEndian(incomingBlob.AsSpan(60));
Assert.Equal(_requiredFlags,(flags&_requiredFlags));

we don't write in little endian order here, since response is of type AuthenticateMessage I don't see a getter / setter for the endianess here.

runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs

Lines 645 to 646 in 7356734

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

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs#L206

 while the original commit did handle this (the case where the test passed)
91fc6d6#diff-07ef1e3bc82826f3e53cb6d9d9f4efef56b93b428ebef12c806ca209063ce74bR632

The file already consists of various getters and setter, which take care of the endinanness, i.e GetField and SetField ,

runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs

Lines 324 to 360 in 7356734

privatestaticunsafeintGetFieldLength(MessageFieldfield)
{
ReadOnlySpan<byte>span=newReadOnlySpan<byte>(&field,sizeof(MessageField));
returnBinaryPrimitives.ReadInt16LittleEndian(span);
}
privatestaticunsafeintGetFieldOffset(MessageFieldfield)
{
ReadOnlySpan<byte>span=newReadOnlySpan<byte>(&field,sizeof(MessageField));
returnBinaryPrimitives.ReadInt16LittleEndian(span.Slice(4));
}
privatestaticReadOnlySpan<byte>GetField(MessageFieldfield,ReadOnlySpan<byte>payload)
{
intoffset=GetFieldOffset(field);
intlength=GetFieldLength(field);
if(length==0||offset+length>payload.Length)
{
returnReadOnlySpan<byte>.Empty;
}
returnpayload.Slice(GetFieldOffset(field),GetFieldLength(field));
}
privatestaticunsafevoidSetField(refMessageFieldfield,intlength,intoffset)
{
if(lengthis<0 or >short.MaxValue)
{
thrownewWin32Exception(NTE_FAIL);
}
Span<byte>span=MemoryMarshal.AsBytes(newSpan<MessageField>(reffield));
BinaryPrimitives.WriteInt16LittleEndian(span,(short)length);
BinaryPrimitives.WriteInt16LittleEndian(span.Slice(2),(short)length);
BinaryPrimitives.WriteInt32LittleEndian(span.Slice(4),offset);
}

can't we reuse those?

shreyarao4 added a commit to shreyarao4/runtime that referenced this pull request Mar 3, 2026
@shreyarao4
Fix ManagedNTLM faiures on Big endian architectures
091b9b4
- Fixed several endian bugs in ManagedNTLM implementation.
- This is a follow-up to PR dotnet#124598
@shreyarao4shreyarao4 mentioned this pull request Mar 3, 2026
Closed
@shreyarao4
shreyarao4 deleted the ntlm branch March 3, 2026 07:27
@rzikm

rzikm commented Mar 3, 2026

Copy link
Copy Markdown
Member

#125039 should be addressing these, let's move conversation there if necessary.

rzikm added a commit that referenced this pull request Mar 12, 2026
@rzikm
Fix missing big-endian conversions for multi-byte NTLM wire fields (#…
b4b0bdc
…125039)
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`:
```csharp
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.
<!-- START COPILOT ORIGINAL PROMPT -->
<details>
<summary>Original prompt</summary>
## 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:
```csharp
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:
```csharp
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:
```csharp
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:
```csharp
private T _backingField;
public T Property
{
readonly get => BitConverter.IsLittleEndian ? _backingField : BinaryPrimitives.ReverseEndianness(_backingField);
set => _backingField = BitConverter.IsLittleEndian ? value : BinaryPrimitives.ReverseEndianness(value);
}
```
</details>
<!-- START COPILOT CODING AGENT SUFFIX -->
*This pull request was created from Copilot chat.*
>
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
CopilotAI added a commit that referenced this pull request Mar 13, 2026
@rzikm
Fix missing big-endian conversions for multi-byte NTLM wire fields (#…
024e901
…125039)
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`:
```csharp
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.
<!-- START COPILOT ORIGINAL PROMPT -->
<details>
<summary>Original prompt</summary>
## 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:
```csharp
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:
```csharp
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:
```csharp
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:
```csharp
private T _backingField;
public T Property
{
readonly get => BitConverter.IsLittleEndian ? _backingField : BinaryPrimitives.ReverseEndianness(_backingField);
set => _backingField = BitConverter.IsLittleEndian ? value : BinaryPrimitives.ReverseEndianness(value);
}
```
</details>
<!-- START COPILOT CODING AGENT SUFFIX -->
*This pull request was created from Copilot chat.*
>
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 3, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@wfurtwfurtwfurt approved these changes
@rzikmrzikmrzikm approved these changes
+1 more reviewer
@gfoidlgfoidlgfoidl left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Net.Securitycommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@shreyarao4@rzikm@saitama951@gfoidl@wfurt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix ManagedNtlm on big-endian architectures - #124598

Merged
rzikm merged 4 commits into
dotnet:mainfrom
shreyarao4:ntlm
Feb 23, 2026
Merged

Fix ManagedNtlm on big-endian architectures#124598
rzikm merged 4 commits into
dotnet:mainfrom
shreyarao4:ntlm

Conversation

@shreyarao4

@shreyarao4shreyarao4 commented Feb 19, 2026

Copy link
Copy Markdown
Contributor

Fixes multiple endianness bugs in ManagedNtlm that caused incorrect behavior on big-endian platforms.
Validated via existing Ntlm test cases.

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Feb 19, 2026
@shreyarao4

shreyarao4 commented Feb 19, 2026

Copy link
Copy Markdown
ContributorAuthor

@uweigand@giritrivedi@saitama951

@dotnet-policy-service

dotnet-policy-serviceBot commented Feb 19, 2026

Copy link
Copy Markdown
Contributor

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

@build-analysisbuild-analysisBot mentioned this pull request Feb 19, 2026
Closed
wfurt
wfurt reviewed Feb 19, 2026
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs Outdated
wfurt
wfurt approved these changes Feb 19, 2026
View reviewed changes

@wfurtwfurt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general.

rzikm
rzikm reviewed Feb 20, 2026
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs Outdated
rzikm
rzikm approved these changes Feb 23, 2026
View reviewed changes

@rzikmrzikm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks!

@build-analysisbuild-analysisBot mentioned this pull request Feb 23, 2026
Closed
@rzikm
rzikm merged commit 84057bb into dotnet:mainFeb 23, 2026
86 of 90 checks passed
gfoidl
gfoidl reviewed Feb 23, 2026
View reviewed changes

@gfoidlgfoidl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: spaces for code-style.

Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs
Comment on lines +134 to +135
readonly get =>BitConverter.IsLittleEndian? _payloadOffset: BinaryPrimitives.ReverseEndianness(_payloadOffset);
set =>_payloadOffset = BitConverter.IsLittleEndian? value: BinaryPrimitives.ReverseEndianness(value);

@gfoidlgfoidlFeb 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
readonlyget=>BitConverter.IsLittleEndian?_payloadOffset:BinaryPrimitives.ReverseEndianness(_payloadOffset);
set =>_payloadOffset=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);
readonlyget=>BitConverter.IsLittleEndian?_payloadOffset:BinaryPrimitives.ReverseEndianness(_payloadOffset);
set =>_payloadOffset=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);

Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs
Comment on lines +161 to +162
readonly get =>BitConverter.IsLittleEndian? _productBuild: BinaryPrimitives.ReverseEndianness(_productBuild);
set =>_productBuild = BitConverter.IsLittleEndian? value: BinaryPrimitives.ReverseEndianness(value);

@gfoidlgfoidlFeb 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
readonlyget=>BitConverter.IsLittleEndian?_productBuild:BinaryPrimitives.ReverseEndianness(_productBuild);
set =>_productBuild=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);
readonlyget=>BitConverter.IsLittleEndian?_productBuild:BinaryPrimitives.ReverseEndianness(_productBuild);
set =>_productBuild=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);

Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs
Comment on lines +177 to +178
readonly get =>BitConverter.IsLittleEndian? _flags: (Flags)BinaryPrimitives.ReverseEndianness((uint)_flags);
set =>_flags = BitConverter.IsLittleEndian? value: (Flags)BinaryPrimitives.ReverseEndianness((uint)value);

@gfoidlgfoidlFeb 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
readonlyget=>BitConverter.IsLittleEndian?_flags:(Flags)BinaryPrimitives.ReverseEndianness((uint)_flags);
set =>_flags=BitConverter.IsLittleEndian?value:(Flags)BinaryPrimitives.ReverseEndianness((uint)value);
readonlyget=>BitConverter.IsLittleEndian?_flags:(Flags)BinaryPrimitives.ReverseEndianness((uint)_flags);
set =>_flags=BitConverter.IsLittleEndian?value:(Flags)BinaryPrimitives.ReverseEndianness((uint)value);

@dotnet-maestrodotnet-maestroBot mentioned this pull request Feb 24, 2026
Merged
@shreyarao4

shreyarao4 commented Feb 26, 2026•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@rzikm@wfurt Unfortunately, the getters and setters method doesn't resolve the endianness issues because of MemoryMarshal.AsRef. We would have to go with the previous commit 91fc6d6. What can be done?

@rzikm

rzikm commented Feb 26, 2026

Copy link
Copy Markdown
Member

@rzikm@wfurt Unfortunately, the getters and setters method doesn't resolve the endianness issues because of MemoryMarshal.AsRef. We would have to go with the previous commit 91fc6d6. What can be done?

Can you be more specific, what about the MemoryMarshal.AsRef is making the changes broken?

@shreyarao4

shreyarao4 commented Feb 27, 2026

Copy link
Copy Markdown
ContributorAuthor

MemoryMarshal.AsRef is writing the fields in little-endian, not in the correct byte order overwriting the struct fields with little-endian data.

@rzikm

rzikm commented Feb 27, 2026

Copy link
Copy Markdown
Member

MemoryMarshal.AsRef is not writing anything, if you cast Span<byte> to e.g. ref MessageField, the getters/setters added in the PR should take care of endianness conversion on read/write. Similarly, if you create MessageField variable on it's own and copy it to a ref MessageField variable, the backing data should already have the correct endianness. Same if you cast the source MessageField to Span<byte> and copy the raw bytes to the target buffer.

Can you point to the specific piece of code where you see the problem, or give a code example where the pattern does not work?

@shreyarao4

shreyarao4 commented Feb 27, 2026

Copy link
Copy Markdown
ContributorAuthor

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs#L582
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs#L593

Here, challengeMessage.TargetInfo.Length and challengeMessage.TargetInfo.MaximumLength are endian flipped. Can you help me with this?

@rzikm

rzikm commented Mar 2, 2026

Copy link
Copy Markdown
Member

I believe that is because the Length and MaximumLength fields are not corrected for endianness when reading

runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs

Lines 126 to 137 in 1fd4dda

[StructLayout(LayoutKind.Sequential)]
privateunsafestructMessageField
{
publicushortLength;
publicushortMaximumLength;
privateint_payloadOffset;
publicintPayloadOffset
{
readonlyget=>BitConverter.IsLittleEndian?_payloadOffset:BinaryPrimitives.ReverseEndianness(_payloadOffset);
set=>_payloadOffset=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);
}
}

CopilotAI mentioned this pull request Mar 2, 2026
Merged
iremyux pushed a commit to iremyux/dotnet-runtime that referenced this pull request Mar 2, 2026
@shreyarao4@iremyux
Fix ManagedNtlm on big-endian architectures (dotnet#124598)
d3dc818
Fixes multiple endianness bugs in ManagedNtlm that caused incorrect
behavior on big-endian platforms.
Validated via existing Ntlm test cases.
@saitama951

saitama951 commented Mar 3, 2026•
edited
Loading

Copy link
Copy Markdown
Contributor

@rzikm I think
the assertion happens here (http://148.100.85.217:8080/job/daily-builds/211/consoleFull)

runtime/src/libraries/Common/tests/System/Net/Security/FakeNtlmServer.cs

Lines 306 to 307 in c30eec2

Flagsflags=(Flags)BinaryPrimitives.ReadUInt32LittleEndian(incomingBlob.AsSpan(60));
Assert.Equal(_requiredFlags,(flags&_requiredFlags));

we don't write in little endian order here, since response is of type AuthenticateMessage I don't see a getter / setter for the endianess here.

runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs

Lines 645 to 646 in 7356734

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

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs#L206

 while the original commit did handle this (the case where the test passed)
91fc6d6#diff-07ef1e3bc82826f3e53cb6d9d9f4efef56b93b428ebef12c806ca209063ce74bR632

The file already consists of various getters and setter, which take care of the endinanness, i.e GetField and SetField ,

runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs

Lines 324 to 360 in 7356734

privatestaticunsafeintGetFieldLength(MessageFieldfield)
{
ReadOnlySpan<byte>span=newReadOnlySpan<byte>(&field,sizeof(MessageField));
returnBinaryPrimitives.ReadInt16LittleEndian(span);
}
privatestaticunsafeintGetFieldOffset(MessageFieldfield)
{
ReadOnlySpan<byte>span=newReadOnlySpan<byte>(&field,sizeof(MessageField));
returnBinaryPrimitives.ReadInt16LittleEndian(span.Slice(4));
}
privatestaticReadOnlySpan<byte>GetField(MessageFieldfield,ReadOnlySpan<byte>payload)
{
intoffset=GetFieldOffset(field);
intlength=GetFieldLength(field);
if(length==0||offset+length>payload.Length)
{
returnReadOnlySpan<byte>.Empty;
}
returnpayload.Slice(GetFieldOffset(field),GetFieldLength(field));
}
privatestaticunsafevoidSetField(refMessageFieldfield,intlength,intoffset)
{
if(lengthis<0 or >short.MaxValue)
{
thrownewWin32Exception(NTE_FAIL);
}
Span<byte>span=MemoryMarshal.AsBytes(newSpan<MessageField>(reffield));
BinaryPrimitives.WriteInt16LittleEndian(span,(short)length);
BinaryPrimitives.WriteInt16LittleEndian(span.Slice(2),(short)length);
BinaryPrimitives.WriteInt32LittleEndian(span.Slice(4),offset);
}

can't we reuse those?

shreyarao4 added a commit to shreyarao4/runtime that referenced this pull request Mar 3, 2026
@shreyarao4
Fix ManagedNTLM faiures on Big endian architectures
091b9b4
- Fixed several endian bugs in ManagedNTLM implementation.
- This is a follow-up to PR dotnet#124598
@shreyarao4shreyarao4 mentioned this pull request Mar 3, 2026
Closed
@shreyarao4
shreyarao4 deleted the ntlm branch March 3, 2026 07:27
@rzikm

rzikm commented Mar 3, 2026

Copy link
Copy Markdown
Member

#125039 should be addressing these, let's move conversation there if necessary.

rzikm added a commit that referenced this pull request Mar 12, 2026
@rzikm
Fix missing big-endian conversions for multi-byte NTLM wire fields (#…
b4b0bdc
…125039)
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`:
```csharp
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.
<!-- START COPILOT ORIGINAL PROMPT -->
<details>
<summary>Original prompt</summary>
## 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:
```csharp
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:
```csharp
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:
```csharp
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:
```csharp
private T _backingField;
public T Property
{
readonly get => BitConverter.IsLittleEndian ? _backingField : BinaryPrimitives.ReverseEndianness(_backingField);
set => _backingField = BitConverter.IsLittleEndian ? value : BinaryPrimitives.ReverseEndianness(value);
}
```
</details>
<!-- START COPILOT CODING AGENT SUFFIX -->
*This pull request was created from Copilot chat.*
>
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
CopilotAI added a commit that referenced this pull request Mar 13, 2026
@rzikm
Fix missing big-endian conversions for multi-byte NTLM wire fields (#…
024e901
…125039)
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`:
```csharp
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.
<!-- START COPILOT ORIGINAL PROMPT -->
<details>
<summary>Original prompt</summary>
## 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:
```csharp
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:
```csharp
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:
```csharp
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:
```csharp
private T _backingField;
public T Property
{
readonly get => BitConverter.IsLittleEndian ? _backingField : BinaryPrimitives.ReverseEndianness(_backingField);
set => _backingField = BitConverter.IsLittleEndian ? value : BinaryPrimitives.ReverseEndianness(value);
}
```
</details>
<!-- START COPILOT CODING AGENT SUFFIX -->
*This pull request was created from Copilot chat.*
>
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 3, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@wfurtwfurtwfurt approved these changes
@rzikmrzikmrzikm approved these changes
+1 more reviewer
@gfoidlgfoidlgfoidl left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Net.Securitycommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@shreyarao4@rzikm@saitama951@gfoidl@wfurt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length \u003e 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix ManagedNtlm on big-endian architectures - #124598

Merged
rzikm merged 4 commits into
dotnet:mainfrom
shreyarao4:ntlm
Feb 23, 2026
Merged

Fix ManagedNtlm on big-endian architectures#124598
rzikm merged 4 commits into
dotnet:mainfrom
shreyarao4:ntlm

Conversation

@shreyarao4

@shreyarao4shreyarao4 commented Feb 19, 2026

Copy link
Copy Markdown
Contributor

Fixes multiple endianness bugs in ManagedNtlm that caused incorrect behavior on big-endian platforms.
Validated via existing Ntlm test cases.

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Feb 19, 2026
@shreyarao4

shreyarao4 commented Feb 19, 2026

Copy link
Copy Markdown
ContributorAuthor

@uweigand@giritrivedi@saitama951

@dotnet-policy-service

dotnet-policy-serviceBot commented Feb 19, 2026

Copy link
Copy Markdown
Contributor

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

@build-analysisbuild-analysisBot mentioned this pull request Feb 19, 2026
Closed
wfurt
wfurt reviewed Feb 19, 2026
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs Outdated
wfurt
wfurt approved these changes Feb 19, 2026
View reviewed changes

@wfurtwfurt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general.

rzikm
rzikm reviewed Feb 20, 2026
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs Outdated
rzikm
rzikm approved these changes Feb 23, 2026
View reviewed changes

@rzikmrzikm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks!

@build-analysisbuild-analysisBot mentioned this pull request Feb 23, 2026
Closed
@rzikm
rzikm merged commit 84057bb into dotnet:mainFeb 23, 2026
86 of 90 checks passed
gfoidl
gfoidl reviewed Feb 23, 2026
View reviewed changes

@gfoidlgfoidl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: spaces for code-style.

Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs
Comment on lines +134 to +135
readonly get =>BitConverter.IsLittleEndian? _payloadOffset: BinaryPrimitives.ReverseEndianness(_payloadOffset);
set =>_payloadOffset = BitConverter.IsLittleEndian? value: BinaryPrimitives.ReverseEndianness(value);

@gfoidlgfoidlFeb 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
readonlyget=>BitConverter.IsLittleEndian?_payloadOffset:BinaryPrimitives.ReverseEndianness(_payloadOffset);
set =>_payloadOffset=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);
readonlyget=>BitConverter.IsLittleEndian?_payloadOffset:BinaryPrimitives.ReverseEndianness(_payloadOffset);
set =>_payloadOffset=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);

Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs
Comment on lines +161 to +162
readonly get =>BitConverter.IsLittleEndian? _productBuild: BinaryPrimitives.ReverseEndianness(_productBuild);
set =>_productBuild = BitConverter.IsLittleEndian? value: BinaryPrimitives.ReverseEndianness(value);

@gfoidlgfoidlFeb 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
readonlyget=>BitConverter.IsLittleEndian?_productBuild:BinaryPrimitives.ReverseEndianness(_productBuild);
set =>_productBuild=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);
readonlyget=>BitConverter.IsLittleEndian?_productBuild:BinaryPrimitives.ReverseEndianness(_productBuild);
set =>_productBuild=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);

Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs
Comment on lines +177 to +178
readonly get =>BitConverter.IsLittleEndian? _flags: (Flags)BinaryPrimitives.ReverseEndianness((uint)_flags);
set =>_flags = BitConverter.IsLittleEndian? value: (Flags)BinaryPrimitives.ReverseEndianness((uint)value);

@gfoidlgfoidlFeb 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
readonlyget=>BitConverter.IsLittleEndian?_flags:(Flags)BinaryPrimitives.ReverseEndianness((uint)_flags);
set =>_flags=BitConverter.IsLittleEndian?value:(Flags)BinaryPrimitives.ReverseEndianness((uint)value);
readonlyget=>BitConverter.IsLittleEndian?_flags:(Flags)BinaryPrimitives.ReverseEndianness((uint)_flags);
set =>_flags=BitConverter.IsLittleEndian?value:(Flags)BinaryPrimitives.ReverseEndianness((uint)value);

@dotnet-maestrodotnet-maestroBot mentioned this pull request Feb 24, 2026
Merged
@shreyarao4

shreyarao4 commented Feb 26, 2026•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@rzikm@wfurt Unfortunately, the getters and setters method doesn't resolve the endianness issues because of MemoryMarshal.AsRef. We would have to go with the previous commit 91fc6d6. What can be done?

@rzikm

rzikm commented Feb 26, 2026

Copy link
Copy Markdown
Member

@rzikm@wfurt Unfortunately, the getters and setters method doesn't resolve the endianness issues because of MemoryMarshal.AsRef. We would have to go with the previous commit 91fc6d6. What can be done?

Can you be more specific, what about the MemoryMarshal.AsRef is making the changes broken?

@shreyarao4

shreyarao4 commented Feb 27, 2026

Copy link
Copy Markdown
ContributorAuthor

MemoryMarshal.AsRef is writing the fields in little-endian, not in the correct byte order overwriting the struct fields with little-endian data.

@rzikm

rzikm commented Feb 27, 2026

Copy link
Copy Markdown
Member

MemoryMarshal.AsRef is not writing anything, if you cast Span<byte> to e.g. ref MessageField, the getters/setters added in the PR should take care of endianness conversion on read/write. Similarly, if you create MessageField variable on it's own and copy it to a ref MessageField variable, the backing data should already have the correct endianness. Same if you cast the source MessageField to Span<byte> and copy the raw bytes to the target buffer.

Can you point to the specific piece of code where you see the problem, or give a code example where the pattern does not work?

@shreyarao4

shreyarao4 commented Feb 27, 2026

Copy link
Copy Markdown
ContributorAuthor

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs#L582
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs#L593

Here, challengeMessage.TargetInfo.Length and challengeMessage.TargetInfo.MaximumLength are endian flipped. Can you help me with this?

@rzikm

rzikm commented Mar 2, 2026

Copy link
Copy Markdown
Member

I believe that is because the Length and MaximumLength fields are not corrected for endianness when reading

runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs

Lines 126 to 137 in 1fd4dda

[StructLayout(LayoutKind.Sequential)]
privateunsafestructMessageField
{
publicushortLength;
publicushortMaximumLength;
privateint_payloadOffset;
publicintPayloadOffset
{
readonlyget=>BitConverter.IsLittleEndian?_payloadOffset:BinaryPrimitives.ReverseEndianness(_payloadOffset);
set=>_payloadOffset=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);
}
}

CopilotAI mentioned this pull request Mar 2, 2026
Merged
iremyux pushed a commit to iremyux/dotnet-runtime that referenced this pull request Mar 2, 2026
@shreyarao4@iremyux
Fix ManagedNtlm on big-endian architectures (dotnet#124598)
d3dc818
Fixes multiple endianness bugs in ManagedNtlm that caused incorrect
behavior on big-endian platforms.
Validated via existing Ntlm test cases.
@saitama951

saitama951 commented Mar 3, 2026•
edited
Loading

Copy link
Copy Markdown
Contributor

@rzikm I think
the assertion happens here (http://148.100.85.217:8080/job/daily-builds/211/consoleFull)

runtime/src/libraries/Common/tests/System/Net/Security/FakeNtlmServer.cs

Lines 306 to 307 in c30eec2

Flagsflags=(Flags)BinaryPrimitives.ReadUInt32LittleEndian(incomingBlob.AsSpan(60));
Assert.Equal(_requiredFlags,(flags&_requiredFlags));

we don't write in little endian order here, since response is of type AuthenticateMessage I don't see a getter / setter for the endianess here.

runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs

Lines 645 to 646 in 7356734

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

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs#L206

 while the original commit did handle this (the case where the test passed)
91fc6d6#diff-07ef1e3bc82826f3e53cb6d9d9f4efef56b93b428ebef12c806ca209063ce74bR632

The file already consists of various getters and setter, which take care of the endinanness, i.e GetField and SetField ,

runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs

Lines 324 to 360 in 7356734

privatestaticunsafeintGetFieldLength(MessageFieldfield)
{
ReadOnlySpan<byte>span=newReadOnlySpan<byte>(&field,sizeof(MessageField));
returnBinaryPrimitives.ReadInt16LittleEndian(span);
}
privatestaticunsafeintGetFieldOffset(MessageFieldfield)
{
ReadOnlySpan<byte>span=newReadOnlySpan<byte>(&field,sizeof(MessageField));
returnBinaryPrimitives.ReadInt16LittleEndian(span.Slice(4));
}
privatestaticReadOnlySpan<byte>GetField(MessageFieldfield,ReadOnlySpan<byte>payload)
{
intoffset=GetFieldOffset(field);
intlength=GetFieldLength(field);
if(length==0||offset+length>payload.Length)
{
returnReadOnlySpan<byte>.Empty;
}
returnpayload.Slice(GetFieldOffset(field),GetFieldLength(field));
}
privatestaticunsafevoidSetField(refMessageFieldfield,intlength,intoffset)
{
if(lengthis<0 or >short.MaxValue)
{
thrownewWin32Exception(NTE_FAIL);
}
Span<byte>span=MemoryMarshal.AsBytes(newSpan<MessageField>(reffield));
BinaryPrimitives.WriteInt16LittleEndian(span,(short)length);
BinaryPrimitives.WriteInt16LittleEndian(span.Slice(2),(short)length);
BinaryPrimitives.WriteInt32LittleEndian(span.Slice(4),offset);
}

can't we reuse those?

shreyarao4 added a commit to shreyarao4/runtime that referenced this pull request Mar 3, 2026
@shreyarao4
Fix ManagedNTLM faiures on Big endian architectures
091b9b4
- Fixed several endian bugs in ManagedNTLM implementation.
- This is a follow-up to PR dotnet#124598
@shreyarao4shreyarao4 mentioned this pull request Mar 3, 2026
Closed
@shreyarao4
shreyarao4 deleted the ntlm branch March 3, 2026 07:27
@rzikm

rzikm commented Mar 3, 2026

Copy link
Copy Markdown
Member

#125039 should be addressing these, let's move conversation there if necessary.

rzikm added a commit that referenced this pull request Mar 12, 2026
@rzikm
Fix missing big-endian conversions for multi-byte NTLM wire fields (#…
b4b0bdc
…125039)
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`:
```csharp
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.
<!-- START COPILOT ORIGINAL PROMPT -->
<details>
<summary>Original prompt</summary>
## 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:
```csharp
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:
```csharp
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:
```csharp
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:
```csharp
private T _backingField;
public T Property
{
readonly get => BitConverter.IsLittleEndian ? _backingField : BinaryPrimitives.ReverseEndianness(_backingField);
set => _backingField = BitConverter.IsLittleEndian ? value : BinaryPrimitives.ReverseEndianness(value);
}
```
</details>
<!-- START COPILOT CODING AGENT SUFFIX -->
*This pull request was created from Copilot chat.*
>
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
CopilotAI added a commit that referenced this pull request Mar 13, 2026
@rzikm
Fix missing big-endian conversions for multi-byte NTLM wire fields (#…
024e901
…125039)
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`:
```csharp
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.
<!-- START COPILOT ORIGINAL PROMPT -->
<details>
<summary>Original prompt</summary>
## 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:
```csharp
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:
```csharp
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:
```csharp
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:
```csharp
private T _backingField;
public T Property
{
readonly get => BitConverter.IsLittleEndian ? _backingField : BinaryPrimitives.ReverseEndianness(_backingField);
set => _backingField = BitConverter.IsLittleEndian ? value : BinaryPrimitives.ReverseEndianness(value);
}
```
</details>
<!-- START COPILOT CODING AGENT SUFFIX -->
*This pull request was created from Copilot chat.*
>
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 3, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@wfurtwfurtwfurt approved these changes
@rzikmrzikmrzikm approved these changes
+1 more reviewer
@gfoidlgfoidlgfoidl left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Net.Securitycommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@shreyarao4@rzikm@saitama951@gfoidl@wfurt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Fix ManagedNtlm on big-endian architectures - #124598

Merged
rzikm merged 4 commits into
dotnet:mainfrom
shreyarao4:ntlm
Feb 23, 2026
Merged

Fix ManagedNtlm on big-endian architectures#124598
rzikm merged 4 commits into
dotnet:mainfrom
shreyarao4:ntlm

Conversation

@shreyarao4

@shreyarao4shreyarao4 commented Feb 19, 2026

Copy link
Copy Markdown
Contributor

Fixes multiple endianness bugs in ManagedNtlm that caused incorrect behavior on big-endian platforms.
Validated via existing Ntlm test cases.

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Feb 19, 2026
@shreyarao4

shreyarao4 commented Feb 19, 2026

Copy link
Copy Markdown
ContributorAuthor

@uweigand@giritrivedi@saitama951

@dotnet-policy-service

dotnet-policy-serviceBot commented Feb 19, 2026

Copy link
Copy Markdown
Contributor

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

@build-analysisbuild-analysisBot mentioned this pull request Feb 19, 2026
Closed
wfurt
wfurt reviewed Feb 19, 2026
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs Outdated
wfurt
wfurt approved these changes Feb 19, 2026
View reviewed changes

@wfurtwfurt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general.

rzikm
rzikm reviewed Feb 20, 2026
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs Outdated
rzikm
rzikm approved these changes Feb 23, 2026
View reviewed changes

@rzikmrzikm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks!

@build-analysisbuild-analysisBot mentioned this pull request Feb 23, 2026
Closed
@rzikm
rzikm merged commit 84057bb into dotnet:mainFeb 23, 2026
86 of 90 checks passed
gfoidl
gfoidl reviewed Feb 23, 2026
View reviewed changes

@gfoidlgfoidl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: spaces for code-style.

Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs
Comment on lines +134 to +135
readonly get =>BitConverter.IsLittleEndian? _payloadOffset: BinaryPrimitives.ReverseEndianness(_payloadOffset);
set =>_payloadOffset = BitConverter.IsLittleEndian? value: BinaryPrimitives.ReverseEndianness(value);

@gfoidlgfoidlFeb 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
readonlyget=>BitConverter.IsLittleEndian?_payloadOffset:BinaryPrimitives.ReverseEndianness(_payloadOffset);
set =>_payloadOffset=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);
readonlyget=>BitConverter.IsLittleEndian?_payloadOffset:BinaryPrimitives.ReverseEndianness(_payloadOffset);
set =>_payloadOffset=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);

Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs
Comment on lines +161 to +162
readonly get =>BitConverter.IsLittleEndian? _productBuild: BinaryPrimitives.ReverseEndianness(_productBuild);
set =>_productBuild = BitConverter.IsLittleEndian? value: BinaryPrimitives.ReverseEndianness(value);

@gfoidlgfoidlFeb 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
readonlyget=>BitConverter.IsLittleEndian?_productBuild:BinaryPrimitives.ReverseEndianness(_productBuild);
set =>_productBuild=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);
readonlyget=>BitConverter.IsLittleEndian?_productBuild:BinaryPrimitives.ReverseEndianness(_productBuild);
set =>_productBuild=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);

Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs
Comment on lines +177 to +178
readonly get =>BitConverter.IsLittleEndian? _flags: (Flags)BinaryPrimitives.ReverseEndianness((uint)_flags);
set =>_flags = BitConverter.IsLittleEndian? value: (Flags)BinaryPrimitives.ReverseEndianness((uint)value);

@gfoidlgfoidlFeb 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
readonlyget=>BitConverter.IsLittleEndian?_flags:(Flags)BinaryPrimitives.ReverseEndianness((uint)_flags);
set =>_flags=BitConverter.IsLittleEndian?value:(Flags)BinaryPrimitives.ReverseEndianness((uint)value);
readonlyget=>BitConverter.IsLittleEndian?_flags:(Flags)BinaryPrimitives.ReverseEndianness((uint)_flags);
set =>_flags=BitConverter.IsLittleEndian?value:(Flags)BinaryPrimitives.ReverseEndianness((uint)value);

@dotnet-maestrodotnet-maestroBot mentioned this pull request Feb 24, 2026
Merged
@shreyarao4

shreyarao4 commented Feb 26, 2026•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@rzikm@wfurt Unfortunately, the getters and setters method doesn't resolve the endianness issues because of MemoryMarshal.AsRef. We would have to go with the previous commit 91fc6d6. What can be done?

@rzikm

rzikm commented Feb 26, 2026

Copy link
Copy Markdown
Member

@rzikm@wfurt Unfortunately, the getters and setters method doesn't resolve the endianness issues because of MemoryMarshal.AsRef. We would have to go with the previous commit 91fc6d6. What can be done?

Can you be more specific, what about the MemoryMarshal.AsRef is making the changes broken?

@shreyarao4

shreyarao4 commented Feb 27, 2026

Copy link
Copy Markdown
ContributorAuthor

MemoryMarshal.AsRef is writing the fields in little-endian, not in the correct byte order overwriting the struct fields with little-endian data.

@rzikm

rzikm commented Feb 27, 2026

Copy link
Copy Markdown
Member

MemoryMarshal.AsRef is not writing anything, if you cast Span<byte> to e.g. ref MessageField, the getters/setters added in the PR should take care of endianness conversion on read/write. Similarly, if you create MessageField variable on it's own and copy it to a ref MessageField variable, the backing data should already have the correct endianness. Same if you cast the source MessageField to Span<byte> and copy the raw bytes to the target buffer.

Can you point to the specific piece of code where you see the problem, or give a code example where the pattern does not work?

@shreyarao4

shreyarao4 commented Feb 27, 2026

Copy link
Copy Markdown
ContributorAuthor

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs#L582
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs#L593

Here, challengeMessage.TargetInfo.Length and challengeMessage.TargetInfo.MaximumLength are endian flipped. Can you help me with this?

@rzikm

rzikm commented Mar 2, 2026

Copy link
Copy Markdown
Member

I believe that is because the Length and MaximumLength fields are not corrected for endianness when reading

runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs

Lines 126 to 137 in 1fd4dda

[StructLayout(LayoutKind.Sequential)]
privateunsafestructMessageField
{
publicushortLength;
publicushortMaximumLength;
privateint_payloadOffset;
publicintPayloadOffset
{
readonlyget=>BitConverter.IsLittleEndian?_payloadOffset:BinaryPrimitives.ReverseEndianness(_payloadOffset);
set=>_payloadOffset=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);
}
}

CopilotAI mentioned this pull request Mar 2, 2026
Merged
iremyux pushed a commit to iremyux/dotnet-runtime that referenced this pull request Mar 2, 2026
@shreyarao4@iremyux
Fix ManagedNtlm on big-endian architectures (dotnet#124598)
d3dc818
Fixes multiple endianness bugs in ManagedNtlm that caused incorrect
behavior on big-endian platforms.
Validated via existing Ntlm test cases.
@saitama951

saitama951 commented Mar 3, 2026•
edited
Loading

Copy link
Copy Markdown
Contributor

@rzikm I think
the assertion happens here (http://148.100.85.217:8080/job/daily-builds/211/consoleFull)

runtime/src/libraries/Common/tests/System/Net/Security/FakeNtlmServer.cs

Lines 306 to 307 in c30eec2

Flagsflags=(Flags)BinaryPrimitives.ReadUInt32LittleEndian(incomingBlob.AsSpan(60));
Assert.Equal(_requiredFlags,(flags&_requiredFlags));

we don't write in little endian order here, since response is of type AuthenticateMessage I don't see a getter / setter for the endianess here.

runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs

Lines 645 to 646 in 7356734

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

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs#L206

 while the original commit did handle this (the case where the test passed)
91fc6d6#diff-07ef1e3bc82826f3e53cb6d9d9f4efef56b93b428ebef12c806ca209063ce74bR632

The file already consists of various getters and setter, which take care of the endinanness, i.e GetField and SetField ,

runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs

Lines 324 to 360 in 7356734

privatestaticunsafeintGetFieldLength(MessageFieldfield)
{
ReadOnlySpan<byte>span=newReadOnlySpan<byte>(&field,sizeof(MessageField));
returnBinaryPrimitives.ReadInt16LittleEndian(span);
}
privatestaticunsafeintGetFieldOffset(MessageFieldfield)
{
ReadOnlySpan<byte>span=newReadOnlySpan<byte>(&field,sizeof(MessageField));
returnBinaryPrimitives.ReadInt16LittleEndian(span.Slice(4));
}
privatestaticReadOnlySpan<byte>GetField(MessageFieldfield,ReadOnlySpan<byte>payload)
{
intoffset=GetFieldOffset(field);
intlength=GetFieldLength(field);
if(length==0||offset+length>payload.Length)
{
returnReadOnlySpan<byte>.Empty;
}
returnpayload.Slice(GetFieldOffset(field),GetFieldLength(field));
}
privatestaticunsafevoidSetField(refMessageFieldfield,intlength,intoffset)
{
if(lengthis<0 or >short.MaxValue)
{
thrownewWin32Exception(NTE_FAIL);
}
Span<byte>span=MemoryMarshal.AsBytes(newSpan<MessageField>(reffield));
BinaryPrimitives.WriteInt16LittleEndian(span,(short)length);
BinaryPrimitives.WriteInt16LittleEndian(span.Slice(2),(short)length);
BinaryPrimitives.WriteInt32LittleEndian(span.Slice(4),offset);
}

can't we reuse those?

shreyarao4 added a commit to shreyarao4/runtime that referenced this pull request Mar 3, 2026
@shreyarao4
Fix ManagedNTLM faiures on Big endian architectures
091b9b4
- Fixed several endian bugs in ManagedNTLM implementation.
- This is a follow-up to PR dotnet#124598
@shreyarao4shreyarao4 mentioned this pull request Mar 3, 2026
Closed
@shreyarao4
shreyarao4 deleted the ntlm branch March 3, 2026 07:27
@rzikm

rzikm commented Mar 3, 2026

Copy link
Copy Markdown
Member

#125039 should be addressing these, let's move conversation there if necessary.

rzikm added a commit that referenced this pull request Mar 12, 2026
@rzikm
Fix missing big-endian conversions for multi-byte NTLM wire fields (#…
b4b0bdc
…125039)
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`:
```csharp
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.
<!-- START COPILOT ORIGINAL PROMPT -->
<details>
<summary>Original prompt</summary>
## 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:
```csharp
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:
```csharp
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:
```csharp
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:
```csharp
private T _backingField;
public T Property
{
readonly get => BitConverter.IsLittleEndian ? _backingField : BinaryPrimitives.ReverseEndianness(_backingField);
set => _backingField = BitConverter.IsLittleEndian ? value : BinaryPrimitives.ReverseEndianness(value);
}
```
</details>
<!-- START COPILOT CODING AGENT SUFFIX -->
*This pull request was created from Copilot chat.*
>
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
CopilotAI added a commit that referenced this pull request Mar 13, 2026
@rzikm
Fix missing big-endian conversions for multi-byte NTLM wire fields (#…
024e901
…125039)
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`:
```csharp
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.
<!-- START COPILOT ORIGINAL PROMPT -->
<details>
<summary>Original prompt</summary>
## 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:
```csharp
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:
```csharp
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:
```csharp
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:
```csharp
private T _backingField;
public T Property
{
readonly get => BitConverter.IsLittleEndian ? _backingField : BinaryPrimitives.ReverseEndianness(_backingField);
set => _backingField = BitConverter.IsLittleEndian ? value : BinaryPrimitives.ReverseEndianness(value);
}
```
</details>
<!-- START COPILOT CODING AGENT SUFFIX -->
*This pull request was created from Copilot chat.*
>
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 3, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@wfurtwfurtwfurt approved these changes
@rzikmrzikmrzikm approved these changes
+1 more reviewer
@gfoidlgfoidlgfoidl left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Net.Securitycommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@shreyarao4@rzikm@saitama951@gfoidl@wfurt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix ManagedNtlm on big-endian architectures - #124598

Merged
rzikm merged 4 commits into
dotnet:mainfrom
shreyarao4:ntlm
Feb 23, 2026
Merged

Fix ManagedNtlm on big-endian architectures#124598
rzikm merged 4 commits into
dotnet:mainfrom
shreyarao4:ntlm

Conversation

@shreyarao4

@shreyarao4shreyarao4 commented Feb 19, 2026

Copy link
Copy Markdown
Contributor

Fixes multiple endianness bugs in ManagedNtlm that caused incorrect behavior on big-endian platforms.
Validated via existing Ntlm test cases.

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Feb 19, 2026
@shreyarao4

shreyarao4 commented Feb 19, 2026

Copy link
Copy Markdown
ContributorAuthor

@uweigand@giritrivedi@saitama951

@dotnet-policy-service

dotnet-policy-serviceBot commented Feb 19, 2026

Copy link
Copy Markdown
Contributor

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

@build-analysisbuild-analysisBot mentioned this pull request Feb 19, 2026
Closed
wfurt
wfurt reviewed Feb 19, 2026
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs Outdated
wfurt
wfurt approved these changes Feb 19, 2026
View reviewed changes

@wfurtwfurt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general.

rzikm
rzikm reviewed Feb 20, 2026
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs Outdated
rzikm
rzikm approved these changes Feb 23, 2026
View reviewed changes

@rzikmrzikm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks!

@build-analysisbuild-analysisBot mentioned this pull request Feb 23, 2026
Closed
@rzikm
rzikm merged commit 84057bb into dotnet:mainFeb 23, 2026
86 of 90 checks passed
gfoidl
gfoidl reviewed Feb 23, 2026
View reviewed changes

@gfoidlgfoidl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: spaces for code-style.

Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs
Comment on lines +134 to +135
readonly get =>BitConverter.IsLittleEndian? _payloadOffset: BinaryPrimitives.ReverseEndianness(_payloadOffset);
set =>_payloadOffset = BitConverter.IsLittleEndian? value: BinaryPrimitives.ReverseEndianness(value);

@gfoidlgfoidlFeb 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
readonlyget=>BitConverter.IsLittleEndian?_payloadOffset:BinaryPrimitives.ReverseEndianness(_payloadOffset);
set =>_payloadOffset=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);
readonlyget=>BitConverter.IsLittleEndian?_payloadOffset:BinaryPrimitives.ReverseEndianness(_payloadOffset);
set =>_payloadOffset=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);

Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs
Comment on lines +161 to +162
readonly get =>BitConverter.IsLittleEndian? _productBuild: BinaryPrimitives.ReverseEndianness(_productBuild);
set =>_productBuild = BitConverter.IsLittleEndian? value: BinaryPrimitives.ReverseEndianness(value);

@gfoidlgfoidlFeb 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
readonlyget=>BitConverter.IsLittleEndian?_productBuild:BinaryPrimitives.ReverseEndianness(_productBuild);
set =>_productBuild=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);
readonlyget=>BitConverter.IsLittleEndian?_productBuild:BinaryPrimitives.ReverseEndianness(_productBuild);
set =>_productBuild=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);

Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs
Comment on lines +177 to +178
readonly get =>BitConverter.IsLittleEndian? _flags: (Flags)BinaryPrimitives.ReverseEndianness((uint)_flags);
set =>_flags = BitConverter.IsLittleEndian? value: (Flags)BinaryPrimitives.ReverseEndianness((uint)value);

@gfoidlgfoidlFeb 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
readonlyget=>BitConverter.IsLittleEndian?_flags:(Flags)BinaryPrimitives.ReverseEndianness((uint)_flags);
set =>_flags=BitConverter.IsLittleEndian?value:(Flags)BinaryPrimitives.ReverseEndianness((uint)value);
readonlyget=>BitConverter.IsLittleEndian?_flags:(Flags)BinaryPrimitives.ReverseEndianness((uint)_flags);
set =>_flags=BitConverter.IsLittleEndian?value:(Flags)BinaryPrimitives.ReverseEndianness((uint)value);

@dotnet-maestrodotnet-maestroBot mentioned this pull request Feb 24, 2026
Merged
@shreyarao4

shreyarao4 commented Feb 26, 2026•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@rzikm@wfurt Unfortunately, the getters and setters method doesn't resolve the endianness issues because of MemoryMarshal.AsRef. We would have to go with the previous commit 91fc6d6. What can be done?

@rzikm

rzikm commented Feb 26, 2026

Copy link
Copy Markdown
Member

@rzikm@wfurt Unfortunately, the getters and setters method doesn't resolve the endianness issues because of MemoryMarshal.AsRef. We would have to go with the previous commit 91fc6d6. What can be done?

Can you be more specific, what about the MemoryMarshal.AsRef is making the changes broken?

@shreyarao4

shreyarao4 commented Feb 27, 2026

Copy link
Copy Markdown
ContributorAuthor

MemoryMarshal.AsRef is writing the fields in little-endian, not in the correct byte order overwriting the struct fields with little-endian data.

@rzikm

rzikm commented Feb 27, 2026

Copy link
Copy Markdown
Member

MemoryMarshal.AsRef is not writing anything, if you cast Span<byte> to e.g. ref MessageField, the getters/setters added in the PR should take care of endianness conversion on read/write. Similarly, if you create MessageField variable on it's own and copy it to a ref MessageField variable, the backing data should already have the correct endianness. Same if you cast the source MessageField to Span<byte> and copy the raw bytes to the target buffer.

Can you point to the specific piece of code where you see the problem, or give a code example where the pattern does not work?

@shreyarao4

shreyarao4 commented Feb 27, 2026

Copy link
Copy Markdown
ContributorAuthor

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs#L582
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs#L593

Here, challengeMessage.TargetInfo.Length and challengeMessage.TargetInfo.MaximumLength are endian flipped. Can you help me with this?

@rzikm

rzikm commented Mar 2, 2026

Copy link
Copy Markdown
Member

I believe that is because the Length and MaximumLength fields are not corrected for endianness when reading

runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs

Lines 126 to 137 in 1fd4dda

[StructLayout(LayoutKind.Sequential)]
privateunsafestructMessageField
{
publicushortLength;
publicushortMaximumLength;
privateint_payloadOffset;
publicintPayloadOffset
{
readonlyget=>BitConverter.IsLittleEndian?_payloadOffset:BinaryPrimitives.ReverseEndianness(_payloadOffset);
set=>_payloadOffset=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);
}
}

CopilotAI mentioned this pull request Mar 2, 2026
Merged
iremyux pushed a commit to iremyux/dotnet-runtime that referenced this pull request Mar 2, 2026
@shreyarao4@iremyux
Fix ManagedNtlm on big-endian architectures (dotnet#124598)
d3dc818
Fixes multiple endianness bugs in ManagedNtlm that caused incorrect
behavior on big-endian platforms.
Validated via existing Ntlm test cases.
@saitama951

saitama951 commented Mar 3, 2026•
edited
Loading

Copy link
Copy Markdown
Contributor

@rzikm I think
the assertion happens here (http://148.100.85.217:8080/job/daily-builds/211/consoleFull)

runtime/src/libraries/Common/tests/System/Net/Security/FakeNtlmServer.cs

Lines 306 to 307 in c30eec2

Flagsflags=(Flags)BinaryPrimitives.ReadUInt32LittleEndian(incomingBlob.AsSpan(60));
Assert.Equal(_requiredFlags,(flags&_requiredFlags));

we don't write in little endian order here, since response is of type AuthenticateMessage I don't see a getter / setter for the endianess here.

runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs

Lines 645 to 646 in 7356734

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

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs#L206

 while the original commit did handle this (the case where the test passed)
91fc6d6#diff-07ef1e3bc82826f3e53cb6d9d9f4efef56b93b428ebef12c806ca209063ce74bR632

The file already consists of various getters and setter, which take care of the endinanness, i.e GetField and SetField ,

runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs

Lines 324 to 360 in 7356734

privatestaticunsafeintGetFieldLength(MessageFieldfield)
{
ReadOnlySpan<byte>span=newReadOnlySpan<byte>(&field,sizeof(MessageField));
returnBinaryPrimitives.ReadInt16LittleEndian(span);
}
privatestaticunsafeintGetFieldOffset(MessageFieldfield)
{
ReadOnlySpan<byte>span=newReadOnlySpan<byte>(&field,sizeof(MessageField));
returnBinaryPrimitives.ReadInt16LittleEndian(span.Slice(4));
}
privatestaticReadOnlySpan<byte>GetField(MessageFieldfield,ReadOnlySpan<byte>payload)
{
intoffset=GetFieldOffset(field);
intlength=GetFieldLength(field);
if(length==0||offset+length>payload.Length)
{
returnReadOnlySpan<byte>.Empty;
}
returnpayload.Slice(GetFieldOffset(field),GetFieldLength(field));
}
privatestaticunsafevoidSetField(refMessageFieldfield,intlength,intoffset)
{
if(lengthis<0 or >short.MaxValue)
{
thrownewWin32Exception(NTE_FAIL);
}
Span<byte>span=MemoryMarshal.AsBytes(newSpan<MessageField>(reffield));
BinaryPrimitives.WriteInt16LittleEndian(span,(short)length);
BinaryPrimitives.WriteInt16LittleEndian(span.Slice(2),(short)length);
BinaryPrimitives.WriteInt32LittleEndian(span.Slice(4),offset);
}

can't we reuse those?

shreyarao4 added a commit to shreyarao4/runtime that referenced this pull request Mar 3, 2026
@shreyarao4
Fix ManagedNTLM faiures on Big endian architectures
091b9b4
- Fixed several endian bugs in ManagedNTLM implementation.
- This is a follow-up to PR dotnet#124598
@shreyarao4shreyarao4 mentioned this pull request Mar 3, 2026
Closed
@shreyarao4
shreyarao4 deleted the ntlm branch March 3, 2026 07:27
@rzikm

rzikm commented Mar 3, 2026

Copy link
Copy Markdown
Member

#125039 should be addressing these, let's move conversation there if necessary.

rzikm added a commit that referenced this pull request Mar 12, 2026
@rzikm
Fix missing big-endian conversions for multi-byte NTLM wire fields (#…
b4b0bdc
…125039)
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`:
```csharp
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.
<!-- START COPILOT ORIGINAL PROMPT -->
<details>
<summary>Original prompt</summary>
## 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:
```csharp
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:
```csharp
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:
```csharp
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:
```csharp
private T _backingField;
public T Property
{
readonly get => BitConverter.IsLittleEndian ? _backingField : BinaryPrimitives.ReverseEndianness(_backingField);
set => _backingField = BitConverter.IsLittleEndian ? value : BinaryPrimitives.ReverseEndianness(value);
}
```
</details>
<!-- START COPILOT CODING AGENT SUFFIX -->
*This pull request was created from Copilot chat.*
>
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
CopilotAI added a commit that referenced this pull request Mar 13, 2026
@rzikm
Fix missing big-endian conversions for multi-byte NTLM wire fields (#…
024e901
…125039)
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`:
```csharp
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.
<!-- START COPILOT ORIGINAL PROMPT -->
<details>
<summary>Original prompt</summary>
## 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:
```csharp
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:
```csharp
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:
```csharp
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:
```csharp
private T _backingField;
public T Property
{
readonly get => BitConverter.IsLittleEndian ? _backingField : BinaryPrimitives.ReverseEndianness(_backingField);
set => _backingField = BitConverter.IsLittleEndian ? value : BinaryPrimitives.ReverseEndianness(value);
}
```
</details>
<!-- START COPILOT CODING AGENT SUFFIX -->
*This pull request was created from Copilot chat.*
>
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 3, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@wfurtwfurtwfurt approved these changes
@rzikmrzikmrzikm approved these changes
+1 more reviewer
@gfoidlgfoidlgfoidl left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Net.Securitycommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@shreyarao4@rzikm@saitama951@gfoidl@wfurt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix ManagedNtlm on big-endian architectures - #124598

Merged
rzikm merged 4 commits into
dotnet:mainfrom
shreyarao4:ntlm
Feb 23, 2026
Merged

Fix ManagedNtlm on big-endian architectures#124598
rzikm merged 4 commits into
dotnet:mainfrom
shreyarao4:ntlm

Conversation

@shreyarao4

@shreyarao4shreyarao4 commented Feb 19, 2026

Copy link
Copy Markdown
Contributor

Fixes multiple endianness bugs in ManagedNtlm that caused incorrect behavior on big-endian platforms.
Validated via existing Ntlm test cases.

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Feb 19, 2026
@shreyarao4

shreyarao4 commented Feb 19, 2026

Copy link
Copy Markdown
ContributorAuthor

@uweigand@giritrivedi@saitama951

@dotnet-policy-service

dotnet-policy-serviceBot commented Feb 19, 2026

Copy link
Copy Markdown
Contributor

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

@build-analysisbuild-analysisBot mentioned this pull request Feb 19, 2026
Closed
wfurt
wfurt reviewed Feb 19, 2026
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs Outdated
wfurt
wfurt approved these changes Feb 19, 2026
View reviewed changes

@wfurtwfurt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general.

rzikm
rzikm reviewed Feb 20, 2026
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs Outdated
rzikm
rzikm approved these changes Feb 23, 2026
View reviewed changes

@rzikmrzikm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks!

@build-analysisbuild-analysisBot mentioned this pull request Feb 23, 2026
Closed
@rzikm
rzikm merged commit 84057bb into dotnet:mainFeb 23, 2026
86 of 90 checks passed
gfoidl
gfoidl reviewed Feb 23, 2026
View reviewed changes

@gfoidlgfoidl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: spaces for code-style.

Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs
Comment on lines +134 to +135
readonly get =>BitConverter.IsLittleEndian? _payloadOffset: BinaryPrimitives.ReverseEndianness(_payloadOffset);
set =>_payloadOffset = BitConverter.IsLittleEndian? value: BinaryPrimitives.ReverseEndianness(value);

@gfoidlgfoidlFeb 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
readonlyget=>BitConverter.IsLittleEndian?_payloadOffset:BinaryPrimitives.ReverseEndianness(_payloadOffset);
set =>_payloadOffset=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);
readonlyget=>BitConverter.IsLittleEndian?_payloadOffset:BinaryPrimitives.ReverseEndianness(_payloadOffset);
set =>_payloadOffset=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);

Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs
Comment on lines +161 to +162
readonly get =>BitConverter.IsLittleEndian? _productBuild: BinaryPrimitives.ReverseEndianness(_productBuild);
set =>_productBuild = BitConverter.IsLittleEndian? value: BinaryPrimitives.ReverseEndianness(value);

@gfoidlgfoidlFeb 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
readonlyget=>BitConverter.IsLittleEndian?_productBuild:BinaryPrimitives.ReverseEndianness(_productBuild);
set =>_productBuild=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);
readonlyget=>BitConverter.IsLittleEndian?_productBuild:BinaryPrimitives.ReverseEndianness(_productBuild);
set =>_productBuild=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);

Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs
Comment on lines +177 to +178
readonly get =>BitConverter.IsLittleEndian? _flags: (Flags)BinaryPrimitives.ReverseEndianness((uint)_flags);
set =>_flags = BitConverter.IsLittleEndian? value: (Flags)BinaryPrimitives.ReverseEndianness((uint)value);

@gfoidlgfoidlFeb 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
readonlyget=>BitConverter.IsLittleEndian?_flags:(Flags)BinaryPrimitives.ReverseEndianness((uint)_flags);
set =>_flags=BitConverter.IsLittleEndian?value:(Flags)BinaryPrimitives.ReverseEndianness((uint)value);
readonlyget=>BitConverter.IsLittleEndian?_flags:(Flags)BinaryPrimitives.ReverseEndianness((uint)_flags);
set =>_flags=BitConverter.IsLittleEndian?value:(Flags)BinaryPrimitives.ReverseEndianness((uint)value);

@dotnet-maestrodotnet-maestroBot mentioned this pull request Feb 24, 2026
Merged
@shreyarao4

shreyarao4 commented Feb 26, 2026•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@rzikm@wfurt Unfortunately, the getters and setters method doesn't resolve the endianness issues because of MemoryMarshal.AsRef. We would have to go with the previous commit 91fc6d6. What can be done?

@rzikm

rzikm commented Feb 26, 2026

Copy link
Copy Markdown
Member

@rzikm@wfurt Unfortunately, the getters and setters method doesn't resolve the endianness issues because of MemoryMarshal.AsRef. We would have to go with the previous commit 91fc6d6. What can be done?

Can you be more specific, what about the MemoryMarshal.AsRef is making the changes broken?

@shreyarao4

shreyarao4 commented Feb 27, 2026

Copy link
Copy Markdown
ContributorAuthor

MemoryMarshal.AsRef is writing the fields in little-endian, not in the correct byte order overwriting the struct fields with little-endian data.

@rzikm

rzikm commented Feb 27, 2026

Copy link
Copy Markdown
Member

MemoryMarshal.AsRef is not writing anything, if you cast Span<byte> to e.g. ref MessageField, the getters/setters added in the PR should take care of endianness conversion on read/write. Similarly, if you create MessageField variable on it's own and copy it to a ref MessageField variable, the backing data should already have the correct endianness. Same if you cast the source MessageField to Span<byte> and copy the raw bytes to the target buffer.

Can you point to the specific piece of code where you see the problem, or give a code example where the pattern does not work?

@shreyarao4

shreyarao4 commented Feb 27, 2026

Copy link
Copy Markdown
ContributorAuthor

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs#L582
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs#L593

Here, challengeMessage.TargetInfo.Length and challengeMessage.TargetInfo.MaximumLength are endian flipped. Can you help me with this?

@rzikm

rzikm commented Mar 2, 2026

Copy link
Copy Markdown
Member

I believe that is because the Length and MaximumLength fields are not corrected for endianness when reading

runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs

Lines 126 to 137 in 1fd4dda

[StructLayout(LayoutKind.Sequential)]
privateunsafestructMessageField
{
publicushortLength;
publicushortMaximumLength;
privateint_payloadOffset;
publicintPayloadOffset
{
readonlyget=>BitConverter.IsLittleEndian?_payloadOffset:BinaryPrimitives.ReverseEndianness(_payloadOffset);
set=>_payloadOffset=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);
}
}

CopilotAI mentioned this pull request Mar 2, 2026
Merged
iremyux pushed a commit to iremyux/dotnet-runtime that referenced this pull request Mar 2, 2026
@shreyarao4@iremyux
Fix ManagedNtlm on big-endian architectures (dotnet#124598)
d3dc818
Fixes multiple endianness bugs in ManagedNtlm that caused incorrect
behavior on big-endian platforms.
Validated via existing Ntlm test cases.
@saitama951

saitama951 commented Mar 3, 2026•
edited
Loading

Copy link
Copy Markdown
Contributor

@rzikm I think
the assertion happens here (http://148.100.85.217:8080/job/daily-builds/211/consoleFull)

runtime/src/libraries/Common/tests/System/Net/Security/FakeNtlmServer.cs

Lines 306 to 307 in c30eec2

Flagsflags=(Flags)BinaryPrimitives.ReadUInt32LittleEndian(incomingBlob.AsSpan(60));
Assert.Equal(_requiredFlags,(flags&_requiredFlags));

we don't write in little endian order here, since response is of type AuthenticateMessage I don't see a getter / setter for the endianess here.

runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs

Lines 645 to 646 in 7356734

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

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs#L206

 while the original commit did handle this (the case where the test passed)
91fc6d6#diff-07ef1e3bc82826f3e53cb6d9d9f4efef56b93b428ebef12c806ca209063ce74bR632

The file already consists of various getters and setter, which take care of the endinanness, i.e GetField and SetField ,

runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs

Lines 324 to 360 in 7356734

privatestaticunsafeintGetFieldLength(MessageFieldfield)
{
ReadOnlySpan<byte>span=newReadOnlySpan<byte>(&field,sizeof(MessageField));
returnBinaryPrimitives.ReadInt16LittleEndian(span);
}
privatestaticunsafeintGetFieldOffset(MessageFieldfield)
{
ReadOnlySpan<byte>span=newReadOnlySpan<byte>(&field,sizeof(MessageField));
returnBinaryPrimitives.ReadInt16LittleEndian(span.Slice(4));
}
privatestaticReadOnlySpan<byte>GetField(MessageFieldfield,ReadOnlySpan<byte>payload)
{
intoffset=GetFieldOffset(field);
intlength=GetFieldLength(field);
if(length==0||offset+length>payload.Length)
{
returnReadOnlySpan<byte>.Empty;
}
returnpayload.Slice(GetFieldOffset(field),GetFieldLength(field));
}
privatestaticunsafevoidSetField(refMessageFieldfield,intlength,intoffset)
{
if(lengthis<0 or >short.MaxValue)
{
thrownewWin32Exception(NTE_FAIL);
}
Span<byte>span=MemoryMarshal.AsBytes(newSpan<MessageField>(reffield));
BinaryPrimitives.WriteInt16LittleEndian(span,(short)length);
BinaryPrimitives.WriteInt16LittleEndian(span.Slice(2),(short)length);
BinaryPrimitives.WriteInt32LittleEndian(span.Slice(4),offset);
}

can't we reuse those?

shreyarao4 added a commit to shreyarao4/runtime that referenced this pull request Mar 3, 2026
@shreyarao4
Fix ManagedNTLM faiures on Big endian architectures
091b9b4
- Fixed several endian bugs in ManagedNTLM implementation.
- This is a follow-up to PR dotnet#124598
@shreyarao4shreyarao4 mentioned this pull request Mar 3, 2026
Closed
@shreyarao4
shreyarao4 deleted the ntlm branch March 3, 2026 07:27
@rzikm

rzikm commented Mar 3, 2026

Copy link
Copy Markdown
Member

#125039 should be addressing these, let's move conversation there if necessary.

rzikm added a commit that referenced this pull request Mar 12, 2026
@rzikm
Fix missing big-endian conversions for multi-byte NTLM wire fields (#…
b4b0bdc
…125039)
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`:
```csharp
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.
<!-- START COPILOT ORIGINAL PROMPT -->
<details>
<summary>Original prompt</summary>
## 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:
```csharp
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:
```csharp
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:
```csharp
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:
```csharp
private T _backingField;
public T Property
{
readonly get => BitConverter.IsLittleEndian ? _backingField : BinaryPrimitives.ReverseEndianness(_backingField);
set => _backingField = BitConverter.IsLittleEndian ? value : BinaryPrimitives.ReverseEndianness(value);
}
```
</details>
<!-- START COPILOT CODING AGENT SUFFIX -->
*This pull request was created from Copilot chat.*
>
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
CopilotAI added a commit that referenced this pull request Mar 13, 2026
@rzikm
Fix missing big-endian conversions for multi-byte NTLM wire fields (#…
024e901
…125039)
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`:
```csharp
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.
<!-- START COPILOT ORIGINAL PROMPT -->
<details>
<summary>Original prompt</summary>
## 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:
```csharp
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:
```csharp
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:
```csharp
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:
```csharp
private T _backingField;
public T Property
{
readonly get => BitConverter.IsLittleEndian ? _backingField : BinaryPrimitives.ReverseEndianness(_backingField);
set => _backingField = BitConverter.IsLittleEndian ? value : BinaryPrimitives.ReverseEndianness(value);
}
```
</details>
<!-- START COPILOT CODING AGENT SUFFIX -->
*This pull request was created from Copilot chat.*
>
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 3, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@wfurtwfurtwfurt approved these changes
@rzikmrzikmrzikm approved these changes
+1 more reviewer
@gfoidlgfoidlgfoidl left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Net.Securitycommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@shreyarao4@rzikm@saitama951@gfoidl@wfurt
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Fix ManagedNtlm on big-endian architectures - #124598

Merged
rzikm merged 4 commits into
dotnet:mainfrom
shreyarao4:ntlm
Feb 23, 2026
Merged

Fix ManagedNtlm on big-endian architectures#124598
rzikm merged 4 commits into
dotnet:mainfrom
shreyarao4:ntlm

Conversation

@shreyarao4

@shreyarao4shreyarao4 commented Feb 19, 2026

Copy link
Copy Markdown
Contributor

Fixes multiple endianness bugs in ManagedNtlm that caused incorrect behavior on big-endian platforms.
Validated via existing Ntlm test cases.

@dotnet-policy-servicedotnet-policy-serviceBot added the community-contribution Indicates that the PR has been added by a community member label Feb 19, 2026
@shreyarao4

shreyarao4 commented Feb 19, 2026

Copy link
Copy Markdown
ContributorAuthor

@uweigand@giritrivedi@saitama951

@dotnet-policy-service

dotnet-policy-serviceBot commented Feb 19, 2026

Copy link
Copy Markdown
Contributor

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

@build-analysisbuild-analysisBot mentioned this pull request Feb 19, 2026
Closed
wfurt
wfurt reviewed Feb 19, 2026
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs Outdated
wfurt
wfurt approved these changes Feb 19, 2026
View reviewed changes

@wfurtwfurt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general.

rzikm
rzikm reviewed Feb 20, 2026
View reviewed changes
Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs Outdated
rzikm
rzikm approved these changes Feb 23, 2026
View reviewed changes

@rzikmrzikm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks!

@build-analysisbuild-analysisBot mentioned this pull request Feb 23, 2026
Closed
@rzikm
rzikm merged commit 84057bb into dotnet:mainFeb 23, 2026
86 of 90 checks passed
gfoidl
gfoidl reviewed Feb 23, 2026
View reviewed changes

@gfoidlgfoidl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: spaces for code-style.

Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs
Comment on lines +134 to +135
readonly get =>BitConverter.IsLittleEndian? _payloadOffset: BinaryPrimitives.ReverseEndianness(_payloadOffset);
set =>_payloadOffset = BitConverter.IsLittleEndian? value: BinaryPrimitives.ReverseEndianness(value);

@gfoidlgfoidlFeb 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

Suggested change
readonlyget=>BitConverter.IsLittleEndian?_payloadOffset:BinaryPrimitives.ReverseEndianness(_payloadOffset);
set =>_payloadOffset=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);
readonlyget=>BitConverter.IsLittleEndian?_payloadOffset:BinaryPrimitives.ReverseEndianness(_payloadOffset);
set =>_payloadOffset=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);

Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs
Comment on lines +161 to +162
readonly get =>BitConverter.IsLittleEndian? _productBuild: BinaryPrimitives.ReverseEndianness(_productBuild);
set =>_productBuild = BitConverter.IsLittleEndian? value: BinaryPrimitives.ReverseEndianness(value);

@gfoidlgfoidlFeb 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
readonlyget=>BitConverter.IsLittleEndian?_productBuild:BinaryPrimitives.ReverseEndianness(_productBuild);
set =>_productBuild=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);
readonlyget=>BitConverter.IsLittleEndian?_productBuild:BinaryPrimitives.ReverseEndianness(_productBuild);
set =>_productBuild=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);

Comment threadsrc/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs
Comment on lines +177 to +178
readonly get =>BitConverter.IsLittleEndian? _flags: (Flags)BinaryPrimitives.ReverseEndianness((uint)_flags);
set =>_flags = BitConverter.IsLittleEndian? value: (Flags)BinaryPrimitives.ReverseEndianness((uint)value);

@gfoidlgfoidlFeb 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
readonlyget=>BitConverter.IsLittleEndian?_flags:(Flags)BinaryPrimitives.ReverseEndianness((uint)_flags);
set =>_flags=BitConverter.IsLittleEndian?value:(Flags)BinaryPrimitives.ReverseEndianness((uint)value);
readonlyget=>BitConverter.IsLittleEndian?_flags:(Flags)BinaryPrimitives.ReverseEndianness((uint)_flags);
set =>_flags=BitConverter.IsLittleEndian?value:(Flags)BinaryPrimitives.ReverseEndianness((uint)value);

@dotnet-maestrodotnet-maestroBot mentioned this pull request Feb 24, 2026
Merged
@shreyarao4

shreyarao4 commented Feb 26, 2026•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@rzikm@wfurt Unfortunately, the getters and setters method doesn't resolve the endianness issues because of MemoryMarshal.AsRef. We would have to go with the previous commit 91fc6d6. What can be done?

@rzikm

rzikm commented Feb 26, 2026

Copy link
Copy Markdown
Member

@rzikm@wfurt Unfortunately, the getters and setters method doesn't resolve the endianness issues because of MemoryMarshal.AsRef. We would have to go with the previous commit 91fc6d6. What can be done?

Can you be more specific, what about the MemoryMarshal.AsRef is making the changes broken?

@shreyarao4

shreyarao4 commented Feb 27, 2026

Copy link
Copy Markdown
ContributorAuthor

MemoryMarshal.AsRef is writing the fields in little-endian, not in the correct byte order overwriting the struct fields with little-endian data.

@rzikm

rzikm commented Feb 27, 2026

Copy link
Copy Markdown
Member

MemoryMarshal.AsRef is not writing anything, if you cast Span<byte> to e.g. ref MessageField, the getters/setters added in the PR should take care of endianness conversion on read/write. Similarly, if you create MessageField variable on it's own and copy it to a ref MessageField variable, the backing data should already have the correct endianness. Same if you cast the source MessageField to Span<byte> and copy the raw bytes to the target buffer.

Can you point to the specific piece of code where you see the problem, or give a code example where the pattern does not work?

@shreyarao4

shreyarao4 commented Feb 27, 2026

Copy link
Copy Markdown
ContributorAuthor

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs#L582
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs#L593

Here, challengeMessage.TargetInfo.Length and challengeMessage.TargetInfo.MaximumLength are endian flipped. Can you help me with this?

@rzikm

rzikm commented Mar 2, 2026

Copy link
Copy Markdown
Member

I believe that is because the Length and MaximumLength fields are not corrected for endianness when reading

runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs

Lines 126 to 137 in 1fd4dda

[StructLayout(LayoutKind.Sequential)]
privateunsafestructMessageField
{
publicushortLength;
publicushortMaximumLength;
privateint_payloadOffset;
publicintPayloadOffset
{
readonlyget=>BitConverter.IsLittleEndian?_payloadOffset:BinaryPrimitives.ReverseEndianness(_payloadOffset);
set=>_payloadOffset=BitConverter.IsLittleEndian?value:BinaryPrimitives.ReverseEndianness(value);
}
}

CopilotAI mentioned this pull request Mar 2, 2026
Merged
iremyux pushed a commit to iremyux/dotnet-runtime that referenced this pull request Mar 2, 2026
@shreyarao4@iremyux
Fix ManagedNtlm on big-endian architectures (dotnet#124598)
d3dc818
Fixes multiple endianness bugs in ManagedNtlm that caused incorrect
behavior on big-endian platforms.
Validated via existing Ntlm test cases.
@saitama951

saitama951 commented Mar 3, 2026•
edited
Loading

Copy link
Copy Markdown
Contributor

@rzikm I think
the assertion happens here (http://148.100.85.217:8080/job/daily-builds/211/consoleFull)

runtime/src/libraries/Common/tests/System/Net/Security/FakeNtlmServer.cs

Lines 306 to 307 in c30eec2

Flagsflags=(Flags)BinaryPrimitives.ReadUInt32LittleEndian(incomingBlob.AsSpan(60));
Assert.Equal(_requiredFlags,(flags&_requiredFlags));

we don't write in little endian order here, since response is of type AuthenticateMessage I don't see a getter / setter for the endianess here.

runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs

Lines 645 to 646 in 7356734

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

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs#L206

 while the original commit did handle this (the case where the test passed)
91fc6d6#diff-07ef1e3bc82826f3e53cb6d9d9f4efef56b93b428ebef12c806ca209063ce74bR632

The file already consists of various getters and setter, which take care of the endinanness, i.e GetField and SetField ,

runtime/src/libraries/System.Net.Security/src/System/Net/NegotiateAuthenticationPal.ManagedNtlm.cs

Lines 324 to 360 in 7356734

privatestaticunsafeintGetFieldLength(MessageFieldfield)
{
ReadOnlySpan<byte>span=newReadOnlySpan<byte>(&field,sizeof(MessageField));
returnBinaryPrimitives.ReadInt16LittleEndian(span);
}
privatestaticunsafeintGetFieldOffset(MessageFieldfield)
{
ReadOnlySpan<byte>span=newReadOnlySpan<byte>(&field,sizeof(MessageField));
returnBinaryPrimitives.ReadInt16LittleEndian(span.Slice(4));
}
privatestaticReadOnlySpan<byte>GetField(MessageFieldfield,ReadOnlySpan<byte>payload)
{
intoffset=GetFieldOffset(field);
intlength=GetFieldLength(field);
if(length==0||offset+length>payload.Length)
{
returnReadOnlySpan<byte>.Empty;
}
returnpayload.Slice(GetFieldOffset(field),GetFieldLength(field));
}
privatestaticunsafevoidSetField(refMessageFieldfield,intlength,intoffset)
{
if(lengthis<0 or >short.MaxValue)
{
thrownewWin32Exception(NTE_FAIL);
}
Span<byte>span=MemoryMarshal.AsBytes(newSpan<MessageField>(reffield));
BinaryPrimitives.WriteInt16LittleEndian(span,(short)length);
BinaryPrimitives.WriteInt16LittleEndian(span.Slice(2),(short)length);
BinaryPrimitives.WriteInt32LittleEndian(span.Slice(4),offset);
}

can't we reuse those?

shreyarao4 added a commit to shreyarao4/runtime that referenced this pull request Mar 3, 2026
@shreyarao4
Fix ManagedNTLM faiures on Big endian architectures
091b9b4
- Fixed several endian bugs in ManagedNTLM implementation.
- This is a follow-up to PR dotnet#124598
@shreyarao4shreyarao4 mentioned this pull request Mar 3, 2026
Closed
@shreyarao4
shreyarao4 deleted the ntlm branch March 3, 2026 07:27
@rzikm

rzikm commented Mar 3, 2026

Copy link
Copy Markdown
Member

#125039 should be addressing these, let's move conversation there if necessary.

rzikm added a commit that referenced this pull request Mar 12, 2026
@rzikm
Fix missing big-endian conversions for multi-byte NTLM wire fields (#…
b4b0bdc
…125039)
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`:
```csharp
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.
<!-- START COPILOT ORIGINAL PROMPT -->
<details>
<summary>Original prompt</summary>
## 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:
```csharp
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:
```csharp
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:
```csharp
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:
```csharp
private T _backingField;
public T Property
{
readonly get => BitConverter.IsLittleEndian ? _backingField : BinaryPrimitives.ReverseEndianness(_backingField);
set => _backingField = BitConverter.IsLittleEndian ? value : BinaryPrimitives.ReverseEndianness(value);
}
```
</details>
<!-- START COPILOT CODING AGENT SUFFIX -->
*This pull request was created from Copilot chat.*
>
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
CopilotAI added a commit that referenced this pull request Mar 13, 2026
@rzikm
Fix missing big-endian conversions for multi-byte NTLM wire fields (#…
024e901
…125039)
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`:
```csharp
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.
<!-- START COPILOT ORIGINAL PROMPT -->
<details>
<summary>Original prompt</summary>
## 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:
```csharp
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:
```csharp
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:
```csharp
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:
```csharp
private T _backingField;
public T Property
{
readonly get => BitConverter.IsLittleEndian ? _backingField : BinaryPrimitives.ReverseEndianness(_backingField);
set => _backingField = BitConverter.IsLittleEndian ? value : BinaryPrimitives.ReverseEndianness(value);
}
```
</details>
<!-- START COPILOT CODING AGENT SUFFIX -->
*This pull request was created from Copilot chat.*
>
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/dotnet/runtime/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rzikm <32671551+rzikm@users.noreply.github.com>
@github-actionsgithub-actionsBot locked and limited conversation to collaborators Apr 3, 2026
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@wfurtwfurtwfurt approved these changes
@rzikmrzikmrzikm approved these changes
+1 more reviewer
@gfoidlgfoidlgfoidl left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Net.Securitycommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@shreyarao4@rzikm@saitama951@gfoidl@wfurt