DHCPv6: Add support for sending Option 17 (VSIO) #383

spoljak-ent · 2024-10-10T12:23:59Z

My apologies, some lines were not included in #ifndef SMALL

Original pull request:
#374

Signed-off-by: Stipe Poljak (EXT) <[email protected]>

src/if-options.c

ColinMcInnes · 2024-10-28T20:55:53Z

I recommend adding a few config examples to src/dhcpcd-definitions.conf

spoljak-ent · 2024-10-29T09:54:13Z

I recommend adding a few config examples to src/dhcpcd-definitions.conf

No problem, but just to double-check, isn't dhcpcd-definitions.conf just for decoding options? This improvement is actually to enable dhcpcd to send those options.

ColinMcInnes · 2024-10-29T13:59:46Z

Good point

# DHCP option definitions for dhcpcd(8)
# These are used to translate DHCP options into shell variables
# for use in dhcpcd-run-hooks(8)
# See dhcpcd.conf(5) for details

Maybe put it in dhcpcd.conf instead.

spoljak-ent · 2024-10-30T09:37:34Z

Good point

# DHCP option definitions for dhcpcd(8)
# These are used to translate DHCP options into shell variables
# for use in dhcpcd-run-hooks(8)
# See dhcpcd.conf(5) for details

Maybe put it in dhcpcd.conf instead.

Done, updated as requested.

src/dhcp6.c

rsmarples

Looking good so for, just a few formatting nits.
I'll see if I can find the time to actually test it soon.

src/dhcp6.c

ColinMcInnes · 2024-10-31T14:58:31Z

Looks good to me!

vsio 4491 1,00:20:00:21:00:22:00:24:00:25:00:26:00:27:00:3D
vsio 4491 2,"Device-class"
vsio 4491 3,123456

rsmarples · 2024-10-31T22:40:15Z

nvm, I was being dumb

The config could potentially be re-used for the same option in DHCP which works in a very similar way, just with different sub option codes in the same EN.

rsmarples · 2024-10-31T23:22:21Z

@spoljak-ent @ColinMcInnes I added a change to simplify things, which looks good from here.
Was able to test it sending many EN definitions without issue. We could re-use this for DHCP equivalent as well now.

Does this look OK to everyone?

This will allow vsio to be used for DHCP in a separate option space.

spoljak-ent · 2024-11-01T13:34:09Z

@spoljak-ent @ColinMcInnes I added a change to simplify things, which looks good from here. Was able to test it sending many EN definitions without issue. We could re-use this for DHCP equivalent as well now.

Does this look OK to everyone?

Looks OK from my side :)

rsmarples · 2024-11-01T14:47:05Z

@spoljak-ent @ColinMcInnes ok, DHCP option 125 now works! It's different from option 43 in that it splits the options via EN.

Please test this before I merge, ideally later today.

spoljak-ent · 2024-11-01T17:06:57Z

@spoljak-ent @ColinMcInnes ok, DHCP option 125 now works! It's different from option 43 in that it splits the options via EN.

Please test this before I merge, ideally later today.

Tested with the following configs.

DHCPv6:
vsio6 155 01,11:22:33:44:55:66
vsio6 155 02,2001:0db8:3c4d:0015:0000:0000:1a2f:1a2b
vsio6 255 01,"hello world"
vsio6 255 02,55:66:77:88:99:00
vsio6 155 03,"another string"
vsio6 355 05,"/3da2k/"

The result:

DHCPv4:
vsio 155 01,192.168.8.2
vsio 155 02,"hello world"
vsio 255 01,185.53.88.15
vsio 255 02,15:16:17:18:19:20
vsio 155 03,"hello world again"
vsio 355 05,175.33.18.19
The result:

Edit (again):
DHCPv6 Option 17 works OK according to RFC 3315 as every option should be split with the enterprise ID.

However for Option 125, every sub-option is immediately split with every enterprise ID while according to RFC 3925, it must contain multiple enterprise IDs with their sub-options within the same option:

RFC 3925:

My implementation was for that case, however separating into multiple options should be done when the size of the whole option exceeds 255 bytes.

From RFC 3925:

Multiple instances of this option may be present
and MUST be concatenated in accordance with RFC 3396 [4].

And going into RFC 3396 [4]:

This specification applies when a DHCP agent is encoding a packet
containing options, where some of those options must be broken into
parts. This need can occur for two reasons. First, it can occur
because the value of an option that needs to be sent is longer than
255 bytes. In this case, the encoding agent MUST follow the
algorithm specified here. It can also occur because there is not
sufficient space in the current output buffer to store the option,
but there is space for part of the option, and there is space in
another output buffer for the rest. In this case, the encoding agent
MUST either use this algorithm or not send the option at all.

Ensure everything fits when loading config to make message creation easier.

rsmarples · 2024-11-06T11:44:41Z

@spoljak-ent OK, I agree. I've added some code to encode as RFC3396 - not tested that it will overflow the 255 boundary yet though. If you have the time, I'd appreciate it if you could test that.

spoljak-ent · 2024-11-06T12:35:37Z

@spoljak-ent OK, I agree. I've added some code to encode as RFC3396 - not tested that it will overflow the 255 boundary yet though. If you have the time, I'd appreciate it if you could test that.

@rsmarples Thanks for the update... I'll test it right now.

spoljak-ent · 2024-11-06T13:43:04Z

So, I tried 2 configurations, first one using large strings:

And it malforms the packet:

The second one using multiple smaller strings:

And the result was the same as for the first case... the length was set at 159 in both cases where it broke:

rsmarples · 2024-11-06T13:47:28Z

Gah, you posted pictures! I'll manually copy into my config and see if I can fix it later.

spoljak-ent · 2024-11-06T13:50:42Z

Sorry, here's the text:

vsio 193 1,"ShortEntry"
vsio 155 2,"A medium-length entry with enough content to add to the total byte count and push the packet closer to its limit"
vsio 255 3,"A much longer entry that includes a significant amount of configuration data to test larger data sizes in DHCP packets. This is useful for scenarios that require substantial information within a single option, going well beyond typical lengths."
vsio 200 4,"Extra data"
vsio 210 5,"Additional configuration entry to exceed 255 bytes and ensure concatenation is required"

vsio 193 1,"ShortEntryExample01"
vsio 155 2,"ShortEntryExample02"
vsio 255 3,"ShortEntryExample03"
vsio 200 4,"ShortEntryExample04"
vsio 210 5,"ShortEntryExample05"
vsio 193 6,"ShortEntryExample06"
vsio 155 7,"ShortEntryExample07"
vsio 255 8,"ShortEntryExample08"
vsio 200 9,"ShortEntryExample09"
vsio 210 10,"ShortEntryExample10"
vsio 193 11,"ShortEntryExample11"
vsio 155 12,"ShortEntryExample12"
vsio 255 13,"ShortEntryExample13"
vsio 200 14,"ShortEntryExample14"
vsio 210 15,"ShortEntryExample15"

Note wireshark doesn't decode option 125 correctly when this happens.

rsmarples · 2024-11-06T16:48:22Z

@spoljak-ent ok, I've pushed a fix for that, but please note this test data:

vsio 193 1,"ShortEntry"
vsio 255 3,"A much longer entry that includes a significant amount of configuration data to test larger data sizes in DHCP packets. This is useful for scenarios that require substantial information within a single option, going well beyond typical lengths."

Wireshark still thinks it's a malformed packet as it overflows by a few bytes, but the encoding looks correct to me as per https://datatracker.ietf.org/doc/html/rfc3396#section-8

I added another change to easily test small boundaries and it still looks bad in wireshark but looks correct to me.
What do you think? If you agree we could create a Wireshark ticket?

rsmarples · 2024-11-06T16:52:22Z

Basically it's just considering each option singularly and not concatenating the occurrences into a whole.

EDIT: Tested with a boundary length of 10

spoljak-ent · 2024-11-07T09:50:17Z

I've been looking at this a bit locally with Wireshark... Going back to RFC3396 section 8 which you linked, from what I understood, the split should be done by making a completely new option, taking into account everything that option needs (new total length, enterprise ID and sub-opt length) and then adding the rest of the concatenation.

Here the total length is 10, while the subopt length is 11.
In the RFC, if the split occurs, the lengths should be modified appropriately I think:

Tbh, not really sure if Wireshark is taking those lengths into account.

rsmarples · 2024-11-07T12:03:00Z

I've been looking at this a bit locally with Wireshark... Going back to RFC3396 section 8 which you linked, from what I understood, the split should be done by making a completely new option, taking into account everything that option needs (new total length, enterprise ID and sub-opt length) and then adding the rest of the concatenation.

From section 6:

The encoding agent MUST NOT attempt to specify any semantic information based on how the option is split.

So the split option is just code and length - anything else is just a blob to be concatenation to the previous blob. We cannot adjust any sub-opt length.

From section 7, decoding:

In the case that a decoding agent finds a split option, it MUST treat
the contents of that option as a single option, and the contents MUST
be reassembled in the order that was described above under encoding
agent behavior.

Once re-assembled the sub option lengths are now valid.
I'm pretty sure wireshark is not doing the re-assembly for this option.

Note that at the end of the initial wireshark decoded 125 option, you can see another 125 option immediately afterwards and a length.
To prove that the lengths are correct, you can change the code on line 1174 from DHO_VIVSO to 43 and wireshark will now claim the packet is now fine as it's making no attempt to deconstruct the 125 option.

spoljak-ent · 2024-11-07T12:17:28Z

I've been looking at this a bit locally with Wireshark... Going back to RFC3396 section 8 which you linked, from what I understood, the split should be done by making a completely new option, taking into account everything that option needs (new total length, enterprise ID and sub-opt length) and then adding the rest of the concatenation.

From section 6:

The encoding agent MUST NOT attempt to specify any semantic information based on how the option is split.

So the split option is just code and length - anything else is just a blob to be concatenation to the previous blob. We cannot adjust any sub-opt length.

From section 7, decoding:

In the case that a decoding agent finds a split option, it MUST treat
the contents of that option as a single option, and the contents MUST
be reassembled in the order that was described above under encoding
agent behavior.

Once re-assembled the sub option lengths are now valid. I'm pretty sure wireshark is not doing the re-assembly for this option.

Note that at the end of the initial wireshark decoded 125 option, you can see another 125 option immediately afterwards and a length. To prove that the lengths are correct, you can change the code on line 1174 from DHO_VIVSO to 43 and wireshark will now claim the packet is now fine as it's making no attempt to deconstruct the 125 option.

I see... You're right.
Definitely an issue with Wireshark not decoding the option properly after the concatenation in that case.

rsmarples · 2024-11-07T13:35:07Z

Referenced upstream here: https://gitlab.com/wireshark/wireshark/-/issues/20201

spoljak-ent · 2024-11-07T13:51:50Z

Thanks! :)
When I find some time, I'll make an update for Vendor Class options - DHCPv4 Option 124 and DHCPv6 Option 16 using the same logic implemented just now (as those options are nearly identical to 125 and 17 respectively).
For now, those aren't working properly when using multiple enterprise IDs (as it's mentioned in #328).

spoljak-ent · 2024-11-13T13:24:20Z

@rsmarples
For the implementation of Vendor Class Options 16 and 124, shall I wrap them also in #ifndef SMALL ?

billie-alsup · 2024-12-04T01:10:40Z

src/dhcp.c

+			*(*ctx->buf)++ = ctx->code;
+			ctx->len = (*ctx->buf)++;
+			*ctx->len = 0;
+			r += 2;


Do we not risk a buffer overrun because we do not adjust ctx->buflen by the two bytes we just inserted? We appear to not account for those two header bytes on each invocation (it seems the same context can be passed in sequential invocations of rfc3396_write), or even if there is a large write, we don't account for those two bytes in each iteration of the loop. We could even have a buffer overrun writing these two bytes because of this miscalculation, although perhaps more likely the overrun happens in the subsequent memcpy.

We do and this should be fixed in 0f303ab

billie-alsup · 2024-12-04T01:11:16Z

src/dhcp.c

+			return -1;
+		}
+
+		memcpy(*ctx->buf, datap, wlen);


The result being that it is possible to have a buffer overrun here.

DHCPv6: Add support for sending Option 17 (VSIO)

065e884

Signed-off-by: Stipe Poljak (EXT) <[email protected]>

rsmarples reviewed Oct 10, 2024

View reviewed changes

src/if-options.c Show resolved Hide resolved

Requested wrap #ifdef

c0a91a4

Merge branch 'NetworkConfiguration:master' into Option_17

f6d77e7

spoljak-ent added 2 commits October 30, 2024 10:29

Merge branch 'NetworkConfiguration:master' into Option_17

163f886

Update dhcpcd.conf.5.in

1b83839

rsmarples reviewed Oct 31, 2024

View reviewed changes

src/dhcp6.c Outdated Show resolved Hide resolved

rsmarples reviewed Oct 31, 2024

View reviewed changes

src/dhcp6.c Outdated Show resolved Hide resolved

src/dhcp6.c Outdated Show resolved Hide resolved

Formatting updates

c984b74

Simplify VISO config parsing and message generation

f7107e3

The config could potentially be re-used for the same option in DHCP which works in a very similar way, just with different sub option codes in the same EN.

vsio -> vsio6

70d70e6

This will allow vsio to be used for DHCP in a separate option space.

rsmarples added 2 commits November 1, 2024 14:40

Use the same code to support DHCP option 125

401f591

Free the DHCP vsio config

7b756b9

Fix SMALL builds

bcaf6b2

Encode DHCP option 125 as a long option

b67eb69

Ensure everything fits when loading config to make message creation easier.

rsmarples added 2 commits November 6, 2024 16:36

Fix RFC3396 option encoding overflowing the 255 byte boundary.

93b27e5

Note wireshark doesn't decode option 125 correctly when this happens.

Add a macro to easily test RFC3396 boundaries

721cfaa

rsmarples merged commit 371c7c6 into NetworkConfiguration:master Nov 7, 2024

spoljak-ent deleted the Option_17 branch November 7, 2024 13:18

spoljak-ent mentioned this pull request Nov 7, 2024

DHCPv4: Add support for sending Option 125 (VIVSO) #377

Closed

billie-alsup reviewed Dec 4, 2024

View reviewed changes

billie-alsup mentioned this pull request Dec 5, 2024

Buffer overrun possible in rfc3396_write? #418

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DHCPv6: Add support for sending Option 17 (VSIO) #383

DHCPv6: Add support for sending Option 17 (VSIO) #383

spoljak-ent commented Oct 10, 2024

ColinMcInnes commented Oct 28, 2024

spoljak-ent commented Oct 29, 2024

ColinMcInnes commented Oct 29, 2024

spoljak-ent commented Oct 30, 2024

rsmarples left a comment

ColinMcInnes commented Oct 31, 2024

rsmarples commented Oct 31, 2024 •

edited

Loading

rsmarples commented Oct 31, 2024

spoljak-ent commented Nov 1, 2024

rsmarples commented Nov 1, 2024

spoljak-ent commented Nov 1, 2024 •

edited

Loading

rsmarples commented Nov 6, 2024

spoljak-ent commented Nov 6, 2024

spoljak-ent commented Nov 6, 2024

rsmarples commented Nov 6, 2024

spoljak-ent commented Nov 6, 2024

rsmarples commented Nov 6, 2024

rsmarples commented Nov 6, 2024 •

edited

Loading

spoljak-ent commented Nov 7, 2024 •

edited

Loading

rsmarples commented Nov 7, 2024

spoljak-ent commented Nov 7, 2024

rsmarples commented Nov 7, 2024

spoljak-ent commented Nov 7, 2024

spoljak-ent commented Nov 13, 2024 •

edited

Loading

billie-alsup Dec 4, 2024 •

edited

Loading

rsmarples Dec 7, 2024

billie-alsup Dec 4, 2024

DHCPv6: Add support for sending Option 17 (VSIO) #383

DHCPv6: Add support for sending Option 17 (VSIO) #383

Conversation

spoljak-ent commented Oct 10, 2024

ColinMcInnes commented Oct 28, 2024

spoljak-ent commented Oct 29, 2024

ColinMcInnes commented Oct 29, 2024

spoljak-ent commented Oct 30, 2024

rsmarples left a comment

Choose a reason for hiding this comment

ColinMcInnes commented Oct 31, 2024

rsmarples commented Oct 31, 2024 • edited Loading

rsmarples commented Oct 31, 2024

spoljak-ent commented Nov 1, 2024

rsmarples commented Nov 1, 2024

spoljak-ent commented Nov 1, 2024 • edited Loading

rsmarples commented Nov 6, 2024

spoljak-ent commented Nov 6, 2024

spoljak-ent commented Nov 6, 2024

rsmarples commented Nov 6, 2024

spoljak-ent commented Nov 6, 2024

rsmarples commented Nov 6, 2024

rsmarples commented Nov 6, 2024 • edited Loading

spoljak-ent commented Nov 7, 2024 • edited Loading

rsmarples commented Nov 7, 2024

spoljak-ent commented Nov 7, 2024

rsmarples commented Nov 7, 2024

spoljak-ent commented Nov 7, 2024

spoljak-ent commented Nov 13, 2024 • edited Loading

billie-alsup Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

rsmarples Dec 7, 2024

Choose a reason for hiding this comment

billie-alsup Dec 4, 2024

Choose a reason for hiding this comment

rsmarples commented Oct 31, 2024 •

edited

Loading

spoljak-ent commented Nov 1, 2024 •

edited

Loading

rsmarples commented Nov 6, 2024 •

edited

Loading

spoljak-ent commented Nov 7, 2024 •

edited

Loading

spoljak-ent commented Nov 13, 2024 •

edited

Loading

billie-alsup Dec 4, 2024 •

edited

Loading