Skip to content

Vendor-specific setters are non-transactional and delete the old value on oversized replacements #155

Description

@code-and-covfefe

TRANSPARENCY: This is an AI-assisted submission – Aikido

Summary
Generated vendor-attribute setters accept 248-253-byte inputs, remove existing matching nested attributes, and only then invoke a constructor that rejects the resulting oversized vendor value. The setter returns an error after the packet has already been modified.

Description
NewBytes and NewString accept values up to 253 bytes, but _ADSLForum_AddVendor prepends a two-byte nested TLV header, producing a 250-255-byte nested value for inputs of 248-253 bytes. NewVendorSpecific rejects nested values over 249 bytes. _ADSLForum_SetVendor removes matching attributes in place before invoking _ADSLForum_AddVendor, with no rollback. Therefore, callers that ignore or mishandle the returned error can emit or continue processing a packet from which the original vendor attribute disappeared instead of being replaced. The issue is systemic because the behavior is emitted by the reusable vendor generator template.

Risk
Low. The attacker must be able to influence a value passed to one of the generated vendor setters, and the consuming application must continue after the returned error. With those prerequisites, a single 248-253-byte value deterministically removes an existing matching vendor attribute without adding the replacement.

Evidence
1
The public byte constructor accepts values up to 253 bytes, including the 248-253-byte range that cannot fit after the nested vendor TLV header is added.

func NewBytes(b []byte) (Attribute, error) {
	if len(b) > 253 {
		return nil, errors.New("value too long")
	}

2
The outer vendor-specific constructor rejects nested values over 249 bytes.

func NewVendorSpecific(vendorID uint32, value Attribute) (Attribute, error) {
	if len(value) > 249 {
		return nil, errors.New("value too long")
	}

3
The helper adds a two-byte nested header before calling NewVendorSpecific; a 248-byte input therefore creates a 250-byte nested value and fails before the replacement is added.

vendor := make(radius.Attribute, 2+len(attr))
vendor[0] = typ
vendor[1] = byte(len(vendor))
copy(vendor[2:], attr)
vsa, err = radius.NewVendorSpecific(_ADSLForum_VendorID, vendor)
if err != nil {
	return
}

4
The setter removes matching records in place and then calls the add helper without rollback.

if vsaTyp == typ {
	vsa = append(vsa[:j], vsa[j+int(vsaLen):]...)
}
...
return _ADSLForum_AddVendor(p, typ, attr)

5
The reusable generator emits the same non-transactional implementation for vendor dictionaries, so the defect is not limited to one generated file.

p(w, `func _`, ident, `_SetVendor(p *radius.Packet, typ byte, attr radius.Attribute) (err error) {`)
...
p(w, `	return _`, ident, `_AddVendor(p, typ, attr)`)
p(w, `}`)

Root Cause Analysis
For an input length of 248 bytes, _ADSLForum_AddVendor allocates a nested value of length 250 (2 + len(attr)), while NewVendorSpecific permits at most 249 bytes and returns an error. The public byte/string constructor permits that input, so the failing path is reachable. Before the failing constructor call, _ADSLForum_SetVendor removes matching nested attributes in place and does not restore them on error. Consequently, the returned error does not imply that the packet remains unchanged. The same ordering is generated for vendor dictionaries by dictionarygen/vendor.go, making this a systemic implementation defect. The security impact remains limited because exploitation requires an attacker to control a setter value and a caller to continue after the error.

Remediation

Validate the effective nested vendor payload size before modifying the packet: reject len(attr) > 247 in the generated setter/add helper, or construct and validate the replacement VSA first and remove existing attributes only after construction succeeds. Prefer a transactional implementation that leaves the packet unchanged whenever the setter returns an error. Regenerate affected files after fixing dictionarygen/vendor.go.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions