Cater for bad client implementation of vendor class option length

[davebarrau]

Hi team, long time no see.

I needed the attached pull request to cater for a bad client implementation of the vendor class option length using little-endian rather than big-endian that I am seeing in the wild on certain D-Link routers.

Cheers!

[pmazzini]

Is this only happening with the vendor class option?

I am not a great fan of introducing quirks for specific vendors but will wait for other opinions.

Right now if the parsing of 1 option fails in DHCPv6, we fail to parse the whole packet.

This is not the case with DHCPv4 where we do lazy parsing. We should uniform this #22.

[davebarrau]

Is this only happening with the vendor class option?

Yes it is.

I am not a great fan of introducing quirks for specific vendors but will wait for other opinions.

Neither am I!

Right now if the parsing of 1 option fails in DHCPv6, we fail to parse the whole packet.

This is not the case with DHCPv4 where we do lazy parsing. We should uniform this #22.

This would be my preferred option too. I'm not actually doing anything with the vendor class at present, but as you say it causes the whole packet to fail to parse. I also have another parsing failure where the vendor class has an EnterpriseNumber, but no vendor-class-data.

The only other solution I could think of would be to allow the ability to send in a custom replacement for ParseOption, so you could inject your own logic in the option parser if needed, but it would probably be more rework than it's worth.

I'm happy to close this and focus some effort on #22 if you'd like?

[hugelgupf]

Is... just the vendor option in little endian...? All other length fields are the correct endianness, for the other options?

This works if all other lengths happen to be correct, i.e. the correct length buffer for this option was passed.

[davebarrau]

Yes the rest of the packet is fine, it was just the vendor class option with the little-endian length.

The vendor class option I was seeing was:

0 16 0 18 0 0 13 233 12 0 100 115 108 102 111 114 117 109 46 111 114 103

where:

0 16(OptionVendorClass)
0 18(option length)
0 0 13 233(enterprise-number)
12 0(vendor-class-len)
100 115 108 102 111 114 117 109 46 111 114 103 (opaque-data)

Not sure if subsequent items would have followed the same byte order for the vendor-class-len as only one item was present.

[hugelgupf]

When I started all this, I was hoping to turn dhcpv6 into lazy parsing too. (Also, sorry, I see you already answered these questions.)

Maybe it's time to do that. I might give it a try this weekend to avoid perf.

[davebarrau]

When I started all this, I was hoping to turn dhcpv6 into lazy parsing too. (Also, sorry, I see you already answered these questions.)

Maybe it's time to do that. I might give it a try this weekend to avoid perf.

That'd be great!

I have also "fixed" this in FromBytesWithParser by changing:

		opt, err := parser(code, optData)
		if err != nil {
			return err
		}

to

		opt, _ := parser(code, optData)

and in ParseOption(), one could change:

	if err != nil {
		return nil, err
	}
	return opt, err

to:

	return opt, err

to keep the Option (even though part of it might be in a "garbled" state depending on how parsing went). In my case above it results in OptVendorClass{enterprisenum=3561, data=[, , , , , , ]} without the changes in this PR.

You do lose access any to underlying errors, but dhcpv4 suffers from the same issue.

Interestingly enough, the changes here don't result in any failed tests.

[davebarrau]

You do lose access any to underlying errors, but dhcpv4 suffers from the same issue.

Alternatively, we could create a special OptionParseFailure option (or some other name, like OptionGenericParseFailure) which contains the original option and the error and a .String() could call the Option's .String() and also display the error.

[pmazzini]

Shall we close this PR?