Skip to content

Fix/dhcpv6 fixups #5759

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ethan-thompson wants to merge 9 commits into from
+63 −24
Closed

Fix/dhcpv6 fixups #5759

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
2c333da
As per RFC 9915 the server MUST discoard any Solicit, Confirm, or Reb…
ethan-thompson Feb 25, 2026
f6b0359
RFC 9915 Section 16.12 says the server MUST discard any received Info…
ethan-thompson Feb 25, 2026
4a9cff7
If vp is not appended to tmp, it is not freed with tmp (as expected).…
ethan-thompson Feb 25, 2026
d0192ca
We added support for dhcpv6 lease query and lease query reply. We nee…
ethan-thompson Mar 4, 2026
551b275
Validate DHCPv6 reconfigure message option has a valid message type
ethan-thompson Mar 4, 2026
75d1520
Decoding from network to host should use ntohl().
ethan-thompson Mar 4, 2026
f63fd69
Update comment to use the correct number of hex digits for a 32-bit i…
ethan-thompson Mar 4, 2026
1f58ae5
make dhcpv6 global init/free thread-safe
ethan-thompson Mar 4, 2026
307ab60
fix corruption of existing VSA group on decode failure
ethan-thompson Mar 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 38 additions & 14 deletions src/protocols/dhcpv6/base.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@

static uint32_t instance_count = 0;
static bool instantiated = false;
static pthread_mutex_t init_mutex = PTHREAD_MUTEX_INITIALIZER;
Copy link
Member

@arr2036 arr2036 Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the justification for from claude for needing to make this thread safe? I don't think this is needed.

Copy link
Member

@arr2036 arr2036 Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, it didn't reference any call sites which actually needed this. Remove this commit and add a @note saying that init and deinit are only called by the main thred and do not need to be thread-safe. Ideally you'd add that to all init functions of the same type.


fr_dict_t const *dict_dhcpv6;

Expand Down Expand Up @@ -362,10 +363,18 @@ static bool verify_to_client(uint8_t const *packet, size_t packet_len, fr_dhcpv6
}

/*
* @todo - check reconfigure message type, and
* reject if it doesn't match.
* Check reconfigure message type, and reject
* if it is not valid.
*/

switch (option[4]) {
case FR_DHCPV6_RENEW:
case FR_DHCPV6_REBIND:
case FR_DHCPV6_INFORMATION_REQUEST:
break;
default:
fr_strerror_const("Invalid Reconf-Msg option value");
return false;
}
/*
* @todo - check for authentication option and
* verify it.
Expand Down Expand Up @@ -445,9 +454,8 @@ static bool verify_from_client(uint8_t const *packet, size_t packet_len, fr_dhcp
return false;
}

if (!fr_dhcpv6_option_find(options, end, FR_SERVER_ID)) {
fail_sid:
fr_strerror_const("Packet does not contain a Server-Id option");
if (fr_dhcpv6_option_find(options, end, FR_SERVER_ID)) {
fr_strerror_const("Packet contains a Server-Id option");
return false;
}
break;
Expand All @@ -459,7 +467,10 @@ static bool verify_from_client(uint8_t const *packet, size_t packet_len, fr_dhcp
if (!fr_dhcpv6_option_find(options, end, FR_CLIENT_ID)) goto fail_cid;

option = fr_dhcpv6_option_find(options, end, FR_SERVER_ID);
if (!option) goto fail_sid;
if (!option) {
fr_strerror_const("Packet does not contain a Server-Id option");
return false;
}

if (!duid_match(option, packet_ctx)) {
fail_match:
Expand All @@ -470,9 +481,7 @@ static bool verify_from_client(uint8_t const *packet, size_t packet_len, fr_dhcp

case FR_PACKET_TYPE_VALUE_INFORMATION_REQUEST:
option = fr_dhcpv6_option_find(options, end, FR_SERVER_ID);
if (!option) goto fail_sid;

if (!duid_match(option, packet_ctx)) goto fail_match;
if (option && !duid_match(option, packet_ctx)) goto fail_match;

/*
* IA options are forbidden.
Expand Down Expand Up @@ -551,9 +560,9 @@ bool fr_dhcpv6_verify(uint8_t const *packet, size_t packet_len, fr_dhcpv6_decode
if (packet_len < DHCPV6_HDR_LEN) return false;

/*
* We support up to relaying.
* We support up to lease query reply.
*/
if ((packet[0] == 0) || (packet[0] > FR_PACKET_TYPE_VALUE_RELAY_REPLY)) return false;
if ((packet[0] == 0) || (packet[0] > FR_DHCPV6_LEASE_QUERY_REPLY)) return false;

if (!packet_ctx->duid) return false;

Expand Down Expand Up @@ -614,6 +623,7 @@ ssize_t fr_dhcpv6_decode(TALLOC_CTX *ctx, fr_pair_list_t *out, uint8_t const *pa
if (!vp) goto fail;
if (fr_value_box_from_network(vp, &vp->data, vp->vp_type, NULL,
&FR_DBUFF_TMP(packet + 1, 1), 1, true) < 0) {
talloc_free(vp);
goto fail;
}
fr_pair_append(&tmp, vp);
Expand All @@ -622,6 +632,7 @@ ssize_t fr_dhcpv6_decode(TALLOC_CTX *ctx, fr_pair_list_t *out, uint8_t const *pa
if (!vp) goto fail;
if (fr_value_box_from_network(vp, &vp->data, vp->vp_type, NULL,
&FR_DBUFF_TMP(packet + 2, 16), 16, true) < 0) {
talloc_free(vp);
goto fail;
}
fr_pair_append(&tmp, vp);
Expand All @@ -630,6 +641,7 @@ ssize_t fr_dhcpv6_decode(TALLOC_CTX *ctx, fr_pair_list_t *out, uint8_t const *pa
if (!vp) goto fail;
if (fr_value_box_from_network(vp, &vp->data, vp->vp_type, NULL,
&FR_DBUFF_TMP(packet + 2 + 16, 16), 16, true) < 0) {
talloc_free(vp);
goto fail;
}

Expand Down Expand Up @@ -899,8 +911,10 @@ void fr_dhcpv6_print_hex(FILE *fp, uint8_t const *packet, size_t packet_len)

int fr_dhcpv6_global_init(void)
{
pthread_mutex_lock(&init_mutex);
if (instance_count > 0) {
instance_count++;
pthread_mutex_unlock(&init_mutex);
return 0;
}

Expand All @@ -909,6 +923,7 @@ int fr_dhcpv6_global_init(void)
if (fr_dict_autoload(libfreeradius_dhcpv6_dict) < 0) {
fail:
instance_count--;
pthread_mutex_unlock(&init_mutex);
return -1;
}

Expand All @@ -918,19 +933,28 @@ int fr_dhcpv6_global_init(void)
}

instantiated = true;
pthread_mutex_unlock(&init_mutex);
return 0;
}

void fr_dhcpv6_global_free(void)
{
if (!instantiated) return;
pthread_mutex_lock(&init_mutex);
if (!instantiated) {
pthread_mutex_unlock(&init_mutex);
return;
}

fr_assert(instance_count > 0);

if (--instance_count > 0) return;
if (--instance_count > 0) {
pthread_mutex_unlock(&init_mutex);
return;
}

fr_dict_autofree(libfreeradius_dhcpv6_dict);
instantiated = false;
pthread_mutex_unlock(&init_mutex);
}

static bool attr_valid(fr_dict_attr_t *da)
Expand Down
33 changes: 24 additions & 9 deletions src/protocols/dhcpv6/decode.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ static ssize_t decode_vsa(TALLOC_CTX *ctx, fr_pair_list_t *out,
if (data_len < 8) return fr_pair_raw_from_network(ctx, out, parent, data, data_len);

memcpy(&pen, data, sizeof(pen));
pen = htonl(pen);
pen = ntohl(pen);

/*
* Verify that the parent (which should be a VSA)
Expand Down Expand Up @@ -366,24 +366,39 @@ static ssize_t decode_option(TALLOC_CTX *ctx, fr_pair_list_t *out,
slen = fr_pair_array_from_network(ctx, out, da, data + 4, len, decode_ctx, decode_value);

} else if (da->type == FR_TYPE_VSA) {
bool append = false;
fr_pair_t *vp;

vp = fr_pair_find_by_da(out, NULL, da);
if (!vp) {
fr_pair_list_t staging_list;

vp = fr_pair_afrom_da(ctx, da);
if (!vp) return PAIR_DECODE_FATAL_ERROR;
PAIR_ALLOCED(vp);

append = true;
}

slen = decode_vsa(vp, &vp->vp_group, da, data + 4, len, decode_ctx);
Copy link
Member

@arr2036 arr2036 Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the correct thing here, is to do the same as we do in the raw label, but output the raw attribute within the VSA vp we already allocated, NOT free the vp. @alandekok @ndptech do you agree?

Copy link
Member

@alandekok alandekok Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's a lot of similar issues elsewhere. I'll take a look.

if (append) {
fr_pair_list_init(&staging_list);
slen = decode_vsa(vp, &staging_list, da, data + 4, len, decode_ctx);
if (slen < 0) {
fr_pair_list_free(&staging_list);
TALLOC_FREE(vp);
} else {
fr_pair_append(out, vp);
goto raw;
}
fr_pair_list_append(&vp->vp_group, &staging_list);
fr_pair_append(out, vp);
} else {
fr_pair_t *tail = fr_pair_list_tail(&vp->vp_group);

slen = decode_vsa(vp, &vp->vp_group, da, data + 4, len, decode_ctx);
if (slen < 0) {
/* Remove any new options added to the group */
fr_pair_t *p = tail ? fr_pair_list_next(&vp->vp_group, tail) : fr_pair_list_head(&vp->vp_group);
while (p) {
fr_pair_t *next = fr_pair_list_next(&vp->vp_group, p);
fr_pair_remove(&vp->vp_group, p);
TALLOC_FREE(p);
p = next;
}
goto raw;
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/protocols/dhcpv6/encode.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ static ssize_t encode_value(fr_dbuff_t *dbuff,
*
* In the decoder we add 30 years to any values, so here
* we need to subtract that time, or if the value is less
* than that time, just encode a 0x0000000000
* than that time, just encode a 0x00000000
* value.
*/
case FR_TYPE_DATE:
Expand Down
Loading