Skip to content

Harden LDAP control decoding against malformed BER input#6

Open
Fusion wants to merge 1 commit into
masterfrom
fix/cve2016
Open

Harden LDAP control decoding against malformed BER input#6
Fusion wants to merge 1 commit into
masterfrom
fix/cve2016

Conversation

@Fusion
Copy link
Copy Markdown
Collaborator

@Fusion Fusion commented May 16, 2026

Change DecodeControl to return (Control, error) and validate control structure and value types instead of relying on unchecked access.
Handle decode failures on both server and client paths, returning protocol errors for bad request controls and surfacing response decode errors.

Change DecodeControl to return (Control, error) and validate control structure and value types instead of relying on unchecked access.
Handle decode failures on both server and client paths, returning protocol errors for bad request controls and surfacing response decode errors.
@berkant-koc
Copy link
Copy Markdown

Thanks for the fast turnaround, Chris. I went through the diff against the original report. Summary: the seven unsafe sites in DecodeControl are all covered, and the defer recover() inside DecodeControl is a nice belt-and-braces. A few things I'd like to flag before merge:

  1. handleConnection still has no top-level recover. The original report called out that server.go:224 runs in a goroutine without a defer-recover at entry, and the existing recovers in server_bind.go/server_search.go only fire after the controls block. This PR fixes the immediate crash source, but any future panic on a pre-bind path (a new control decoder, a ber.ReadPacket bug, etc.) would still take the process down. A one-liner at the top of handleConnection would harden this structurally.

  2. No regression tests. There's no control_test.go in the repo, and the PR adds none. My original PoC is a 35-byte BER blob — happy to send a table-driven test covering control-type / criticality / paging-size / paging-children / control-value-string if it helps. Without coverage this fix is one careless refactor away from regressing.

  3. server.TLSConfig != nil guard was dropped at server.go:338. Looks unrelated to the control-decoding fix. With the guard gone, tls.Server(conn, nil) is reachable on StartTLS when the server has no TLS config — which then panics in the handshake. I think this should be reverted or replaced with an explicit error response.

  4. Sibling repos. nmcclain/ldap and cloudogu/go-ldap still carry the same five unguarded assertions in their DecodeControl. Not blocking this PR, but worth a heads-up since glauth is a fork of nmcclain.

The control-decoding hardening itself looks solid — I'd just like to see (1) and (3) addressed and ideally (2) before merge.

1 similar comment
@berkant-koc
Copy link
Copy Markdown

Thanks for the fast turnaround, Chris. I went through the diff against the original report. Summary: the seven unsafe sites in DecodeControl are all covered, and the defer recover() inside DecodeControl is a nice belt-and-braces. A few things I'd like to flag before merge:

  1. handleConnection still has no top-level recover. The original report called out that server.go:224 runs in a goroutine without a defer-recover at entry, and the existing recovers in server_bind.go/server_search.go only fire after the controls block. This PR fixes the immediate crash source, but any future panic on a pre-bind path (a new control decoder, a ber.ReadPacket bug, etc.) would still take the process down. A one-liner at the top of handleConnection would harden this structurally.

  2. No regression tests. There's no control_test.go in the repo, and the PR adds none. My original PoC is a 35-byte BER blob — happy to send a table-driven test covering control-type / criticality / paging-size / paging-children / control-value-string if it helps. Without coverage this fix is one careless refactor away from regressing.

  3. server.TLSConfig != nil guard was dropped at server.go:338. Looks unrelated to the control-decoding fix. With the guard gone, tls.Server(conn, nil) is reachable on StartTLS when the server has no TLS config — which then panics in the handshake. I think this should be reverted or replaced with an explicit error response.

  4. Sibling repos. nmcclain/ldap and cloudogu/go-ldap still carry the same five unguarded assertions in their DecodeControl. Not blocking this PR, but worth a heads-up since glauth is a fork of nmcclain.

The control-decoding hardening itself looks solid — I'd just like to see (1) and (3) addressed and ideally (2) before merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants