Harden LDAP control decoding against malformed BER input

[Fusion]

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]

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.

[berkant-koc]

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.

[Fusion]

@berkant-koc,

I hear you, these are valid points. Sorry for the delay addressing them.

Let's tackle them in order:

1. Yes I kinda like things failing fast.
   This being said, I made a change that should keep both of us happy: I included the top-level recover(), but am also screaming in the logs :)

2. I would greatly appreciate your help, yes. I'll admit to my motivation here being pure fatigue with dealing with BER (totally out of left field comment: who knew patching opensips' cluster protocol would drag me back in that world?)

3. Well that's a nice merge oopsie. Fixed!

4. I created this fork for GLAuth use. I was checking other forks of nmcclain/ldap, hoping to find an active one, and it's a depressing landscape. I'm afraid I don't really have any proper upstream to push these fixes to.