Skip to content

Fix panic and wrong delete index in generated _SetVendor#140

Open
jamiesun wants to merge 1 commit into
layeh:masterfrom
talkincode:fix/setvendor-panic-and-delete-index
Open

Fix panic and wrong delete index in generated _SetVendor#140
jamiesun wants to merge 1 commit into
layeh:masterfrom
talkincode:fix/setvendor-panic-and-delete-index

Conversation

@jamiesun

@jamiesun jamiesun commented Jun 8, 2026

Copy link
Copy Markdown

Summary

The generated _<Vendor>_SetVendor helper (emitted into every vendor dictionary by radius-dict-gen) can panic with slice bounds out of range, and uses a wrong index when removing an emptied Vendor-Specific attribute. This fixes the generator template and regenerates the affected dictionaries.

This is the same crash reported in #121 (Juniper MX80 / ERX, also hit with 3GPP-IMEISV), and it was independently re-reported downstream in talkincode/toughradius#324, which additionally pinned down the i+i delete-index defect.

Defects (in dictionarygen/vendor.go, _<Vendor>_SetVendor)

  1. Panic in the sub-attribute scan. The loop read the type/length at a fixed offset vsa[0], vsa[1] instead of the running cursor vsa[j], vsa[j+1], and unconditionally advanced j += int(vsaLen). After a matching sub-attribute is removed (vsa = append(vsa[:j], vsa[j+int(vsaLen):]...)) the slice shrinks, so advancing j runs past the end and len(vsa[j:]) panics. Reproduces by setting the same vendor attribute twice on one packet.

  2. Wrong delete index. An emptied VSA was removed with p.Attributes[i+i:] instead of p.Attributes[i+1:] — a no-op at i==0, and dropping unrelated attributes (or panicking) at i>=2.

A third, coupled correctness issue: the len(vsa) > 0 write-back used copy(avp.Attribute[4:], vsa), which never shrinks avp.Attribute, leaving stale trailing bytes when a multi-sub-attribute VSA loses one entry.

Fix

  • Read vsaTyp, vsaLen := vsa[j], vsa[j+1] at the cursor.
  • Advance j only when the sub-attribute does not match (matching entries are spliced out in place).
  • Rewrite the VSA at its correct new length: p.Attributes[i].Attribute = append(avp.Attribute[:4:4], vsa...).
  • Delete an emptied VSA with i+1.
  • Regenerated vendors/{aruba,microsoft,mikrotik,wispr} and rfc4679.

Tests

Added vendors/mikrotik/setvendor_test.go:

  • TestSetVendorReplaceSameAttribute — the reported double-set panic.
  • TestSetVendorDeletesEmptyVSAAtNonZeroIndex — the i+i out-of-range delete at index ≥ 2.
  • TestSetVendorPreservesOtherSubAttributes — a VSA packing several sub-attributes is rewritten without stale bytes.

go build ./..., go vet, and the full go test ./... suite pass. Verified the new tests panic on the pre-fix code.

Relation to prior work

Thanks to @maddsua for reporting #121 and proposing #125 / #129. Those PRs change the loop bound to len(vsa)-j, which stops the panic but keeps the fixed-offset vsa[0], vsa[1] read and does not address the i+i delete index. This PR covers both, plus the write-back resize.

Closes #121

The generated _<Vendor>_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#121, layeh#129, layeh#125
Refs: talkincode/toughradius#324

Co-authored-by: Copilot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Length check in *_SetVendor causes a panic in generated vendor dictionaries

1 participant