Harden LDAP control decoding against malformed BER input#6
Conversation
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.
|
Thanks for the fast turnaround, Chris. I went through the diff against the original report. Summary: the seven unsafe sites in
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
|
Thanks for the fast turnaround, Chris. I went through the diff against the original report. Summary: the seven unsafe sites in
The control-decoding hardening itself looks solid — I'd just like to see (1) and (3) addressed and ideally (2) before merge. |
|
I hear you, these are valid points. Sorry for the delay addressing them. Let's tackle them in order:
|
|
Thanks for turning these around, Chris, and for the quick fixes. Glad the top-level On (2), here is the table-driven test I offered. It is Verified green against this branch ( Coverage:
package ldap
import (
"bytes"
"testing"
ber "github.com/go-asn1-ber/asn1-ber"
)
// redecode serializes a packet and parses it again. DecodeControl runs on
// controls parsed from the wire, not on in-memory packets, and the original
// unchecked type assertions / child indexing only panic on wire-decoded
// input. Round-tripping through bytes reproduces that path faithfully.
func redecode(p *ber.Packet) *ber.Packet {
if p == nil {
return nil
}
return ber.DecodePacket(p.Bytes())
}
func controlSeq(children ...*ber.Packet) *ber.Packet {
seq := ber.Encode(ber.ClassUniversal, ber.TypeConstructed, ber.TagSequence, nil, "Control")
for _, c := range children {
seq.AppendChild(c)
}
return seq
}
func octet(s string) *ber.Packet {
return ber.NewString(ber.ClassUniversal, ber.TypePrimitive, ber.TagOctetString, s, "")
}
// pagingControl builds a paging control whose value wraps the supplied inner
// sequence, mirroring ControlPaging.Encode() so the byte layout matches the
// real wire format.
func pagingControl(inner *ber.Packet) *ber.Packet {
ctrl := ber.Encode(ber.ClassUniversal, ber.TypeConstructed, ber.TagSequence, nil, "Control")
ctrl.AppendChild(octet(ControlTypePaging))
value := ber.Encode(ber.ClassUniversal, ber.TypePrimitive, ber.TagOctetString, nil, "Control Value (Paging)")
value.AppendChild(inner)
ctrl.AppendChild(value)
return ctrl
}
func searchValueSeq(children ...*ber.Packet) *ber.Packet {
seq := ber.Encode(ber.ClassUniversal, ber.TypeConstructed, ber.TagSequence, nil, "Search Control Value")
for _, c := range children {
seq.AppendChild(c)
}
return seq
}
func integer(v uint64) *ber.Packet {
return ber.NewInteger(ber.ClassUniversal, ber.TypePrimitive, ber.TagInteger, v, "")
}
func TestDecodeControl(t *testing.T) {
tests := []struct {
name string
packet *ber.Packet
wantErr bool
check func(t *testing.T, c Control)
}{
{
name: "string control, type only",
packet: redecode((&ControlString{ControlType: "1.2.3.4"}).Encode()),
check: func(t *testing.T, c Control) {
cs, ok := c.(*ControlString)
if !ok {
t.Fatalf("got %T, want *ControlString", c)
}
if cs.ControlType != "1.2.3.4" {
t.Errorf("ControlType = %q, want 1.2.3.4", cs.ControlType)
}
if cs.Criticality {
t.Errorf("Criticality = true, want false")
}
if cs.ControlValue != "" {
t.Errorf("ControlValue = %q, want empty", cs.ControlValue)
}
},
},
{
name: "string control, type and value",
packet: redecode((&ControlString{ControlType: "1.2.3.4", ControlValue: "payload"}).Encode()),
check: func(t *testing.T, c Control) {
cs := c.(*ControlString)
if cs.Criticality {
t.Errorf("Criticality = true, want false")
}
if cs.ControlValue != "payload" {
t.Errorf("ControlValue = %q, want payload", cs.ControlValue)
}
},
},
{
name: "string control, type criticality and value",
packet: redecode((&ControlString{ControlType: "1.2.3.4", Criticality: true, ControlValue: "payload"}).Encode()),
check: func(t *testing.T, c Control) {
cs := c.(*ControlString)
if !cs.Criticality {
t.Errorf("Criticality = false, want true")
}
if cs.ControlValue != "payload" {
t.Errorf("ControlValue = %q, want payload", cs.ControlValue)
}
},
},
{
name: "paging control round-trip",
packet: redecode((&ControlPaging{PagingSize: 100, Cookie: []byte("cookie")}).Encode()),
check: func(t *testing.T, c Control) {
cp, ok := c.(*ControlPaging)
if !ok {
t.Fatalf("got %T, want *ControlPaging", c)
}
if cp.PagingSize != 100 {
t.Errorf("PagingSize = %d, want 100", cp.PagingSize)
}
if !bytes.Equal(cp.Cookie, []byte("cookie")) {
t.Errorf("Cookie = %q, want cookie", cp.Cookie)
}
},
},
{
name: "nil packet",
packet: nil,
wantErr: true,
},
{
name: "empty control sequence",
packet: redecode(controlSeq()),
wantErr: true,
},
{
name: "control type not an octet string",
packet: redecode(controlSeq(integer(5))),
wantErr: true,
},
{
name: "criticality not a boolean",
packet: redecode(controlSeq(
octet("1.2.3.4"),
octet("not-a-bool"),
octet("value"),
)),
wantErr: true,
},
{
name: "paging value sequence too short",
packet: redecode(pagingControl(searchValueSeq(integer(10)))),
wantErr: true,
},
{
name: "paging size exceeds uint32",
packet: redecode(pagingControl(searchValueSeq(integer(uint64(1)<<32), octet("ck")))),
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
c, err := DecodeControl(tt.packet)
if tt.wantErr {
if err == nil {
t.Fatalf("DecodeControl() error = nil, want error")
}
if c != nil {
t.Errorf("DecodeControl() control = %v, want nil on error", c)
}
return
}
if err != nil {
t.Fatalf("DecodeControl() unexpected error: %v", err)
}
if c == nil {
t.Fatalf("DecodeControl() control = nil, want non-nil")
}
if tt.check != nil {
tt.check(t, c)
}
})
}
}Happy to open this as a PR against |
2 participants
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.