From 541733116cce69302971fbef1a025ed3ee176230 Mon Sep 17 00:00:00 2001 From: jettwang Date: Mon, 8 Jun 2026 12:26:03 +0800 Subject: [PATCH] Fix panic and wrong delete index in generated _SetVendor The generated __SetVendor helper, emitted for every vendor dictionary, had two defects: 1. The sub-attribute scan read a fixed offset (vsa[0], vsa[1]) instead of the running cursor (vsa[j], vsa[j+1]) and always advanced j, even after the matched sub-attribute was removed and vsa was shortened. Setting the same vendor attribute twice -- or setting one already present on an inbound packet -- panicked with "slice bounds out of range". 2. An emptied VSA was removed with the wrong index (i+i instead of i+1): a no-op at i==0 and dropping unrelated attributes / panicking at i>=2. The scan now reads type/length at the cursor, only advances the cursor when the sub-attribute does not match, rewrites the VSA at its correct new length (no stale trailing bytes), and deletes an emptied VSA with i+1. The affected vendor/rfc dictionaries are regenerated and regression tests are added. Refs: layeh/radius#121, layeh/radius#129, layeh/radius#125 Refs: talkincode/toughradius#324 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- dictionarygen/vendor.go | 10 +-- rfc4679/generated.go | 10 +-- vendors/aruba/generated.go | 10 +-- vendors/microsoft/generated.go | 10 +-- vendors/mikrotik/generated.go | 10 +-- vendors/mikrotik/setvendor_test.go | 137 +++++++++++++++++++++++++++++ vendors/wispr/generated.go | 10 +-- 7 files changed, 167 insertions(+), 30 deletions(-) create mode 100644 vendors/mikrotik/setvendor_test.go diff --git a/dictionarygen/vendor.go b/dictionarygen/vendor.go index 8a1befe4..b71ececb 100644 --- a/dictionarygen/vendor.go +++ b/dictionarygen/vendor.go @@ -88,21 +88,21 @@ func (g *Generator) genVendor(w io.Writer, vendor *dictionary.Vendor) { p(w, ` continue`) p(w, ` }`) p(w, ` for j := 0; len(vsa[j:]) >= 3; {`) - p(w, ` vsaTyp, vsaLen := vsa[0], vsa[1]`) + p(w, ` vsaTyp, vsaLen := vsa[j], vsa[j+1]`) p(w, ` if int(vsaLen) > len(vsa[j:]) || vsaLen < 3 {`) // malformed - p(w, ` i++`) p(w, ` break`) p(w, ` }`) p(w, ` if vsaTyp == typ {`) p(w, ` vsa = append(vsa[:j], vsa[j+int(vsaLen):]...)`) + p(w, ` } else {`) + p(w, ` j += int(vsaLen)`) p(w, ` }`) - p(w, ` j += int(vsaLen)`) p(w, ` }`) p(w, ` if len(vsa) > 0 {`) - p(w, ` copy(avp.Attribute[4:], vsa)`) + p(w, ` p.Attributes[i].Attribute = append(avp.Attribute[:4:4], vsa...)`) p(w, ` i++`) p(w, ` } else {`) - p(w, ` p.Attributes = append(p.Attributes[:i], p.Attributes[i+i:]...)`) + p(w, ` p.Attributes = append(p.Attributes[:i], p.Attributes[i+1:]...)`) p(w, ` }`) p(w, ` }`) p(w, ` return _`, ident, `_AddVendor(p, typ, attr)`) diff --git a/rfc4679/generated.go b/rfc4679/generated.go index b27d8b9e..86da0e28 100644 --- a/rfc4679/generated.go +++ b/rfc4679/generated.go @@ -88,21 +88,21 @@ func _ADSLForum_SetVendor(p *radius.Packet, typ byte, attr radius.Attribute) (er continue } for j := 0; len(vsa[j:]) >= 3; { - vsaTyp, vsaLen := vsa[0], vsa[1] + vsaTyp, vsaLen := vsa[j], vsa[j+1] if int(vsaLen) > len(vsa[j:]) || vsaLen < 3 { - i++ break } if vsaTyp == typ { vsa = append(vsa[:j], vsa[j+int(vsaLen):]...) + } else { + j += int(vsaLen) } - j += int(vsaLen) } if len(vsa) > 0 { - copy(avp.Attribute[4:], vsa) + p.Attributes[i].Attribute = append(avp.Attribute[:4:4], vsa...) i++ } else { - p.Attributes = append(p.Attributes[:i], p.Attributes[i+i:]...) + p.Attributes = append(p.Attributes[:i], p.Attributes[i+1:]...) } } return _ADSLForum_AddVendor(p, typ, attr) diff --git a/vendors/aruba/generated.go b/vendors/aruba/generated.go index ba88f90d..bd6d9898 100644 --- a/vendors/aruba/generated.go +++ b/vendors/aruba/generated.go @@ -90,21 +90,21 @@ func _Aruba_SetVendor(p *radius.Packet, typ byte, attr radius.Attribute) (err er continue } for j := 0; len(vsa[j:]) >= 3; { - vsaTyp, vsaLen := vsa[0], vsa[1] + vsaTyp, vsaLen := vsa[j], vsa[j+1] if int(vsaLen) > len(vsa[j:]) || vsaLen < 3 { - i++ break } if vsaTyp == typ { vsa = append(vsa[:j], vsa[j+int(vsaLen):]...) + } else { + j += int(vsaLen) } - j += int(vsaLen) } if len(vsa) > 0 { - copy(avp.Attribute[4:], vsa) + p.Attributes[i].Attribute = append(avp.Attribute[:4:4], vsa...) i++ } else { - p.Attributes = append(p.Attributes[:i], p.Attributes[i+i:]...) + p.Attributes = append(p.Attributes[:i], p.Attributes[i+1:]...) } } return _Aruba_AddVendor(p, typ, attr) diff --git a/vendors/microsoft/generated.go b/vendors/microsoft/generated.go index e485924e..f8b32c19 100644 --- a/vendors/microsoft/generated.go +++ b/vendors/microsoft/generated.go @@ -91,21 +91,21 @@ func _Microsoft_SetVendor(p *radius.Packet, typ byte, attr radius.Attribute) (er continue } for j := 0; len(vsa[j:]) >= 3; { - vsaTyp, vsaLen := vsa[0], vsa[1] + vsaTyp, vsaLen := vsa[j], vsa[j+1] if int(vsaLen) > len(vsa[j:]) || vsaLen < 3 { - i++ break } if vsaTyp == typ { vsa = append(vsa[:j], vsa[j+int(vsaLen):]...) + } else { + j += int(vsaLen) } - j += int(vsaLen) } if len(vsa) > 0 { - copy(avp.Attribute[4:], vsa) + p.Attributes[i].Attribute = append(avp.Attribute[:4:4], vsa...) i++ } else { - p.Attributes = append(p.Attributes[:i], p.Attributes[i+i:]...) + p.Attributes = append(p.Attributes[:i], p.Attributes[i+1:]...) } } return _Microsoft_AddVendor(p, typ, attr) diff --git a/vendors/mikrotik/generated.go b/vendors/mikrotik/generated.go index 334ebd91..b354264d 100644 --- a/vendors/mikrotik/generated.go +++ b/vendors/mikrotik/generated.go @@ -89,21 +89,21 @@ func _Mikrotik_SetVendor(p *radius.Packet, typ byte, attr radius.Attribute) (err continue } for j := 0; len(vsa[j:]) >= 3; { - vsaTyp, vsaLen := vsa[0], vsa[1] + vsaTyp, vsaLen := vsa[j], vsa[j+1] if int(vsaLen) > len(vsa[j:]) || vsaLen < 3 { - i++ break } if vsaTyp == typ { vsa = append(vsa[:j], vsa[j+int(vsaLen):]...) + } else { + j += int(vsaLen) } - j += int(vsaLen) } if len(vsa) > 0 { - copy(avp.Attribute[4:], vsa) + p.Attributes[i].Attribute = append(avp.Attribute[:4:4], vsa...) i++ } else { - p.Attributes = append(p.Attributes[:i], p.Attributes[i+i:]...) + p.Attributes = append(p.Attributes[:i], p.Attributes[i+1:]...) } } return _Mikrotik_AddVendor(p, typ, attr) diff --git a/vendors/mikrotik/setvendor_test.go b/vendors/mikrotik/setvendor_test.go new file mode 100644 index 00000000..e47da19f --- /dev/null +++ b/vendors/mikrotik/setvendor_test.go @@ -0,0 +1,137 @@ +package mikrotik + +import ( + "testing" + + "layeh.com/radius" + "layeh.com/radius/rfc2865" +) + +// countSubAttr returns the number of Mikrotik vendor sub-attributes of the +// given type present across every Vendor-Specific attribute of p. +func countSubAttr(t *testing.T, p *radius.Packet, typ byte) int { + t.Helper() + count := 0 + for _, avp := range p.Attributes { + if avp.Type != rfc2865.VendorSpecific_Type { + continue + } + vendorID, vsa, err := radius.VendorSpecific(avp.Attribute) + if err != nil || vendorID != _Mikrotik_VendorID { + continue + } + for len(vsa) >= 3 { + vsaTyp, vsaLen := vsa[0], vsa[1] + if int(vsaLen) < 3 || int(vsaLen) > len(vsa) { + t.Fatalf("malformed VSA payload: sub-attr length %d in %d remaining bytes", vsaLen, len(vsa)) + } + if vsaTyp == typ { + count++ + } + vsa = vsa[int(vsaLen):] + } + if len(vsa) != 0 { + t.Fatalf("VSA payload has %d trailing/stale byte(s)", len(vsa)) + } + } + return count +} + +func vsaCount(p *radius.Packet) int { + n := 0 + for _, avp := range p.Attributes { + if avp.Type == rfc2865.VendorSpecific_Type { + n++ + } + } + return n +} + +// TestSetVendorReplaceSameAttribute is the regression test for +// talkincode/toughradius#324: setting the same vendor attribute twice used to +// panic ("slice bounds out of range") because the sub-attribute scan read a +// fixed offset (vsa[0]/vsa[1]) instead of the running cursor and advanced the +// cursor past the shrunken slice. +func TestSetVendorReplaceSameAttribute(t *testing.T) { + p := &radius.Packet{} + + if err := MikrotikRecvLimit_Set(p, 1000); err != nil { + t.Fatalf("first set: %v", err) + } + if err := MikrotikRecvLimit_Set(p, 2000); err != nil { + t.Fatalf("second set: %v", err) + } + + if got, err := MikrotikRecvLimit_Lookup(p); err != nil || got != 2000 { + t.Fatalf("MikrotikRecvLimit_Lookup = %d, %v; want 2000, nil", got, err) + } + if n := countSubAttr(t, p, 1); n != 1 { + t.Fatalf("RecvLimit sub-attribute count = %d; want 1", n) + } + if n := vsaCount(p); n != 1 { + t.Fatalf("vendor-specific attribute count = %d; want 1", n) + } +} + +// TestSetVendorDeletesEmptyVSAAtNonZeroIndex exercises the empty-VSA removal +// branch when the matching attribute is not the first packet attribute. The +// old delete index "i+i" went out of range (panic) for i >= 2 and dropped the +// wrong elements; it must be "i+1". +func TestSetVendorDeletesEmptyVSAAtNonZeroIndex(t *testing.T) { + p := &radius.Packet{} + rfc2865.UserName_SetString(p, "alice") + rfc2865.NASIdentifier_SetString(p, "nas-1") + + MikrotikRecvLimit_Set(p, 1000) // VSA now at index 2 + MikrotikRecvLimit_Set(p, 2000) // removes the empty VSA at index 2, re-adds + + if got, err := MikrotikRecvLimit_Lookup(p); err != nil || got != 2000 { + t.Fatalf("MikrotikRecvLimit_Lookup = %d, %v; want 2000, nil", got, err) + } + if got := rfc2865.UserName_GetString(p); got != "alice" { + t.Fatalf("UserName_GetString = %q; want %q", got, "alice") + } + if got := rfc2865.NASIdentifier_GetString(p); got != "nas-1" { + t.Fatalf("NASIdentifier_GetString = %q; want %q", got, "nas-1") + } + if n := vsaCount(p); n != 1 { + t.Fatalf("vendor-specific attribute count = %d; want 1", n) + } +} + +// TestSetVendorPreservesOtherSubAttributes verifies that re-setting one +// sub-attribute inside a VSA that packs several sub-attributes only removes the +// matching one, rewrites the VSA to its new (shorter) length without leaving +// stale trailing bytes, and preserves the other sub-attributes. +func TestSetVendorPreservesOtherSubAttributes(t *testing.T) { + p := &radius.Packet{} + + // A single VSA holding two Mikrotik sub-attributes: + // type 1 (RecvLimit) = 1000, type 2 (XmitLimit) = 500. + sub := []byte{ + 1, 6, 0, 0, 0x03, 0xE8, // RecvLimit = 1000 + 2, 6, 0, 0, 0x01, 0xF4, // XmitLimit = 500 + } + vsa, err := radius.NewVendorSpecific(_Mikrotik_VendorID, sub) + if err != nil { + t.Fatalf("NewVendorSpecific: %v", err) + } + p.Add(rfc2865.VendorSpecific_Type, vsa) + + if err := MikrotikRecvLimit_Set(p, 2000); err != nil { + t.Fatalf("set: %v", err) + } + + if got, err := MikrotikRecvLimit_Lookup(p); err != nil || got != 2000 { + t.Fatalf("MikrotikRecvLimit_Lookup = %d, %v; want 2000, nil", got, err) + } + if got, err := MikrotikXmitLimit_Lookup(p); err != nil || got != 500 { + t.Fatalf("MikrotikXmitLimit_Lookup = %d, %v; want 500, nil", got, err) + } + if n := countSubAttr(t, p, 1); n != 1 { + t.Fatalf("RecvLimit sub-attribute count = %d; want 1", n) + } + if n := countSubAttr(t, p, 2); n != 1 { + t.Fatalf("XmitLimit sub-attribute count = %d; want 1 (stale bytes left behind?)", n) + } +} diff --git a/vendors/wispr/generated.go b/vendors/wispr/generated.go index d00f8dbe..5c354f44 100644 --- a/vendors/wispr/generated.go +++ b/vendors/wispr/generated.go @@ -88,21 +88,21 @@ func _WISPr_SetVendor(p *radius.Packet, typ byte, attr radius.Attribute) (err er continue } for j := 0; len(vsa[j:]) >= 3; { - vsaTyp, vsaLen := vsa[0], vsa[1] + vsaTyp, vsaLen := vsa[j], vsa[j+1] if int(vsaLen) > len(vsa[j:]) || vsaLen < 3 { - i++ break } if vsaTyp == typ { vsa = append(vsa[:j], vsa[j+int(vsaLen):]...) + } else { + j += int(vsaLen) } - j += int(vsaLen) } if len(vsa) > 0 { - copy(avp.Attribute[4:], vsa) + p.Attributes[i].Attribute = append(avp.Attribute[:4:4], vsa...) i++ } else { - p.Attributes = append(p.Attributes[:i], p.Attributes[i+i:]...) + p.Attributes = append(p.Attributes[:i], p.Attributes[i+1:]...) } } return _WISPr_AddVendor(p, typ, attr)