layeh / radius

a Go (golang) RADIUS client and server implementation
https://pkg.go.dev/layeh.com/radius
Mozilla Public License 2.0
567 stars 175 forks source link

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

Open maddsua opened 2 months ago

maddsua commented 2 months ago

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: https://github.com/layeh/radius/blob/1006025d24f8fdd5b5ee8b3c15a714e6b24baaab/dictionarygen/vendor.go#L90

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 commented 1 month ago

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