Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Breaking change]: LDAP DirectoryControl parsing performed in managed code #43885

Closed
1 of 3 tasks
edwardneal opened this issue Dec 6, 2024 · 0 comments · Fixed by #44625
Closed
1 of 3 tasks

[Breaking change]: LDAP DirectoryControl parsing performed in managed code #43885

edwardneal opened this issue Dec 6, 2024 · 0 comments · Fixed by #44625
Assignees
Labels
breaking-change Indicates a .NET Core breaking change 🏁 Release: .NET 10 Work items for the .NET 10 release in-pr This issue will be closed (fixed) by an active pull request. 📌 seQUESTered Identifies that an issue has been imported into Quest.

Comments

@edwardneal
Copy link

edwardneal commented Dec 6, 2024

Description

This change is a result of dotnet/runtime#101512.

Previously, .NET used BerConverter to parse the DirectoryControls it received over the network and to generate the DirectoryControl byte arrays it sent; BerConverter would use the OS-specific BER parsing functionality. This parsing functionality is now implemented in managed code.

Version

.NET 10 Preview 1

Previous behavior

As a result of using BerConverter, the parsing of DirectoryControls was very relaxed.

  • The ASN.1 tags of each value weren't checked.
  • Trailing data after the end of the parsed DirectoryControl was ignored, as was trailing data within an ASN.1 SEQUENCE.
  • On Linux, OCTET STRING lengths which extended beyond the end of their parent sequence would return data outside the parent sequence.
  • On earlier versions of Windows, a zero-length OCTET STRING would return null rather than an empty string.
  • When reading the contents of a DirectoryControl as a UTF8-encoded string, an invalid UTF8 sequence would not throw an exception.
  • When passing an invalid UTF8 string to the constructor of VlvRequestControl, no exception was thrown.

While not a breaking change, Windows would always encode ASN.1 tags with a four-byte length while Linux would only use as many bytes for the tag length as it needed. Both representations were valid, but this behavioural difference between platforms is now gone; the Linux behaviour now also appears on Windows.

New behavior

The DirectoryControl parsing is much more stringent, and is now consistent across platforms and versions.

  • ASN.1 tags are now checked.
  • Trailing data is no longer permitted.
  • The length of OCTET STRINGs and SEQUENCEs is now checked.
  • Zero-length OCTET STRINGs will now always return an empty string.
  • If the server sends an invalid UTF8 byte sequence, the DirectoryControl parsing logic will now throw an exception rather than silently substitute the invalid characters with a known value.

We also validate errors more thoroughly when calling the VlvRequestControl constructor. Passing a string which cannot be encoded as a UTF8 value will now throw an EncoderFallbackException.

Type of breaking change

  • Binary incompatible: Existing binaries might encounter a breaking change in behavior, such as failure to load or execute, and if so, require recompilation.
  • Source incompatible: When recompiled using the new SDK or component or to target the new runtime, existing source code might require source changes to compile successfully.
  • Behavioral change: Existing binaries might behave differently at run time.

Reason for change

RFC/spec. compliance. In the various RFCs and sections of MS-ADTS, the controlValue is specified as the BER encoding of an ASN.1 structure with wording similar to the below (from RFC2891, section 1.2):

The controlType is set to "1.2.840.113556.1.4.474". The criticality is FALSE (MAY be absent). The controlValue is an OCTET STRING, whose value is the BER encoding of a value of the following SEQUENCE:

This precludes trailing data. It also rules out BER encodings of ASN.1 structures with differing ASN.1 tags, and of invalid BER encodings (such as OCTET STRINGs which are longer than their containing SEQUENCE.)

For the VlvRequestControl constructor, throwing the exception early means that users can trust that only the values they explicitly specify are sent to the server - there are no circumstances where they can accidentally send EF BF BD to the server because they've passed a string which can't be encoded to valid UTF8 bytes.

Recommended action

Servers should comply with the RFCs and specifications. Users should be aware of the need to handle an EncoderFallbackException when calling the VlvRequestControl constructor.

Feature area

Core .NET libraries, Security

Affected APIs

For the more stringent DirectoryControl parsing:

  • LdapConnection.SendRequest (all overloads)
  • LdapConnection.EndSendRequest

For the VlvRequestControl constructor:

  • VlvRequestControl..ctor(int, int, string)

Associated WorkItem - 360645

@edwardneal edwardneal added breaking-change Indicates a .NET Core breaking change doc-idea labels Dec 6, 2024
@dotnetrepoman dotnetrepoman bot added ⌚ Not Triaged Not triaged labels Dec 6, 2024
@edwardneal edwardneal changed the title [Breaking change]: [Breaking change]: LDAP Directory control parsing performed in managed code Dec 6, 2024
@edwardneal edwardneal changed the title [Breaking change]: LDAP Directory control parsing performed in managed code [Breaking change]: LDAP DirectoryControl parsing performed in managed code Dec 6, 2024
@CamSoper CamSoper added 🗺️ reQUEST Triggers an issue to be imported into Quest. and removed ⌚ Not Triaged Not triaged labels Jan 7, 2025
@dotnetrepoman dotnetrepoman bot added the 🗺️ mapQUEST Only used as a way to mark an issue as updated for quest. RepoMan should instantly remove it. label Jan 7, 2025
@dotnet-policy-service dotnet-policy-service bot removed the 🗺️ mapQUEST Only used as a way to mark an issue as updated for quest. RepoMan should instantly remove it. label Jan 7, 2025
@sequestor sequestor bot added 📌 seQUESTered Identifies that an issue has been imported into Quest. and removed 🗺️ reQUEST Triggers an issue to be imported into Quest. labels Jan 8, 2025
@gewarren gewarren added the 🏁 Release: .NET 10 Work items for the .NET 10 release label Jan 14, 2025
@CamSoper CamSoper moved this from 🔖 Ready to 🏗 In progress in dotnet/docs January 2025 sprint project Jan 30, 2025
@CamSoper CamSoper moved this from 🏗 In progress to 👀 In review in dotnet/docs January 2025 sprint project Jan 30, 2025
@dotnet-policy-service dotnet-policy-service bot added the in-pr This issue will be closed (fixed) by an active pull request. label Jan 31, 2025
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in dotnet/docs January 2025 sprint project Jan 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Indicates a .NET Core breaking change 🏁 Release: .NET 10 Work items for the .NET 10 release in-pr This issue will be closed (fixed) by an active pull request. 📌 seQUESTered Identifies that an issue has been imported into Quest.
Projects
Development

Successfully merging a pull request may close this issue.

4 participants