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

[maddsua]

Device: Juniper MX80
Dictionary: https://fossies.org/linux/freeradius-server/share/dictionary.erx
Error: runtime error: slice bounds out of range [34:0]
Failing generated function: ERXServiceActivate_Set

Code line in question:

radius/dictionarygen/vendor.go

Line 90 in 1006025

p(w, ` for j := 0; len(vsa[j:]) >= 3; {`)

Proposed solution: replace that slicing and length check with just subtraction like this:

-               for j := 0; len(vsa[j:]) >= 3; {
+               for j := 0; len(vsa)-j >= 3; {
                        vsaTyp, vsaLen := vsa[0], vsa[1]
-                       if int(vsaLen) > len(vsa[j:]) || vsaLen < 3 {
+                       if int(vsaLen) > len(vsa)-j || vsaLen < 3 {
                                i++
                                break
                        }

Let me know if I'm missing something, but I think this should work just fine. I don't really understand the reason for slicing a slice first and then checking it's length, when you can subtract index from original length and get the same result but without a risk of painc.

UPD: just noticed the same pattern in *_DelVendor. I think that it should be replaced as well

[maddsua]

forking and making a PR, as having to monkey-patch this in the actual project seems like a huge delayed footgun

[eugennicoara]

I ran into this same issue while trying to set 3GPP-IMEISV.

[maddsua]

Hey @eugennicoara it looks like the maintainer doesn't hang out on github a lot, could you maybe shoot them an email?
They seem to be quite a busy person, maybe one more email would get 30 or so minutes of their attention needed to merge this

[jamiesun]

Opened #140 with a fix for this. It also covers two things the len(vsa)-j change in #125 / #129 doesn't:

• the scan still reads a fixed offset vsa[0], vsa[1] instead of the cursor vsa[j], vsa[j+1] (wrong sub-attribute type/length once j > 0), and
• the emptied-VSA removal uses p.Attributes[i+i:] instead of i+1 (a no-op at i==0, drops/oob at i>=2).

#140 reads at the cursor, only advances j on a non-match, rewrites the VSA at its correct length (no stale trailing bytes), uses i+1, regenerates the affected dictionaries, and adds regression tests. Thanks @maddsua for the original report and the earlier PRs. This was also hit downstream in talkincode/toughradius#324.