Skip to content

guard malformed attribute in unpackAttributes - #612

Merged
cpuschma merged 2 commits into
go-ldap:masterfrom
netliomax25-code:search-attributes-bounds
Aug 10, 2026
Merged

guard malformed attribute in unpackAttributes#612
cpuschma merged 2 commits into
go-ldap:masterfrom
netliomax25-code:search-attributes-bounds

Conversation

@netliomax25-code

@netliomax25-code netliomax25-code commented Jun 22, 2026

Copy link
Copy Markdown
Contributor
  1. unpackAttributes indexes child.Children[1] (the vals SET) and asserts the type and each value to string for every attribute of a SearchResultEntry, without checking the server returned a well-formed PartialAttribute.
  2. A non-conforming or malicious server that omits the vals element, or sends a non-string type/value, makes the attribute decode index out of range or fail the assertion, which panics the synchronous Search goroutine and the SearchAsync worker. Neither path has a recover, so the caller process crashes.

Return an error for an attribute that lacks the two expected children or whose type or value is not a string, so the caller can handle it: Search returns it to the caller, SearchAsync delivers it on the result channel as SearchSingleResult.Error. Well-formed entries decode exactly as before. Added a unit test covering the missing-vals and non-string-type cases.

@netliomax25-code

Copy link
Copy Markdown
Contributor Author

any update?

Comment thread v3/search.go Outdated
// non-string type/value; index and assert defensively so a malformed
// attribute is skipped instead of panicking the search goroutine.
if len(child.Children) < 2 {
continue

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.

Discarding the PartialAttribute without throwing an error because of a non-conforming result is undefined behaviour compared to the LDAPv3 RFC. I'd suggest to throw an error instead and allow the user to handle it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Pushed a change so unpackAttributes now returns an error for a malformed attribute instead of skipping it:

  1. Search returns the error to the caller; SearchAsync delivers it on the result channel as SearchSingleResult.Error.
  2. Applied the same treatment to the per-value assertion, so a non-string value also reports an error rather than silently decoding as an empty string.
  3. Updated the test to expect errors for the missing-vals and non-string-type cases; go build, go vet and the unit tests pass locally.

Comment thread v3/search.go Outdated
}
name, ok := child.Children[0].Value.(string)
if !ok {
continue

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.

Same goes here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done here as well, this path returns an error now too.

@cpuschma cpuschma self-assigned this Jul 5, 2026
@cpuschma cpuschma added enhancement go Pull requests that update go code labels Jul 5, 2026
@netliomax25-code

Copy link
Copy Markdown
Contributor Author

gentle ping

@cpuschma
cpuschma merged commit 56dc6fe into go-ldap:master Aug 10, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement go Pull requests that update go code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants