Reject unescaped special characters in DN values per RFC 4514#588
Merged
Conversation
ParseDN previously accepted ", ;, <, > and NULL (U+0000) in raw form, silently producing values that violate the RFC 4514 grammar and could be misinterpreted by downstream LDAP servers or string-comparison code. decodeString now returns an error when it encounters these characters unescaped; their hex- and backslash-escaped forms continue to decode as before. ParseDN tests are expanded to cover every RFC 4514 escape target in both escape forms, plus the corresponding unescaped-error cases. Leading/trailing U+0020 (SPACE) is intentionally left lenient — the existing whitespace-stripping behavior is depended on by many real inputs, so enforcing it strictly is deferred.
Contributor
Author
|
If, for any reason, you would prefer not to change the behavior of |
Member
Having multiple ParseDN functions could cause confusion. I suggest fixing the current implementation and adding a note to the next release instead. After all, the current behaviour is unintended and does not conform to the RFC. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
I ran into a case where
ldap.ParseDNaccepted a DN like the following without returning an error:This prompted me to re-read the DN string specification, and I noticed that several of the special characters listed in RFC 4514 were not being checked for the unescaped form.
Summary
ParseDNpreviously accepted",;,<,>, and NULL (U+0000) in their raw, unescaped form, silently producingAttributeTypeAndValuevalues that violate the RFC 4514 grammar. Such values can be misinterpreted by downstreamLDAP servers or by string-comparison code that assumes a well-formed DN, which is both a correctness issue and a potential security concern (e.g. DN-based authorization checks operating on a value the server later parses differently).
Changes
decodeStringnow returns an error when it encounters any of",;,<,>, or NULL in unescaped form. The hex-escape (\XX) and backslash-escape (\X) forms continue to decode unchanged, so previously valid inputs are unaffected.TestSuccessfulDNParsingis expanded to cover every RFC 4514 §2.4 escape target in both escape forms (e.g.\,and\2C). NULL is covered only in the hex form, as required by the RFC.TestErrorDNParsingandTestDecodeStringgain matching negative cases for each unescaped character, so the new rejection path has full coverage.Notes / Caveats
Compatibility
This is a behavior change: callers that were (incorrectly) passing raw
",;,<,>, or NULL intoParseDNwill now receive an error instead of a silently malformed DN. Callers using properly escaped input are unaffected.Disclosure on AI assistance
In the interest of transparency:
v3/dn.go(rejecting unescaped special characters indecodeString) was designed and written by me.v3/dn_test.goand the wording of this PR description were drafted with the help of an AI agent. I reviewed every test case against RFC 4514 §2.4 and confirmed the expected behavior locally before submitting.Happy to adjust or rewrite any part of the AI-assisted content if the maintainers prefer.
References