[Dnsmasq-discuss] [PATCH] rfc2131: Fix range address assignment not honoring vendor option filters

[Hans Dedecker]

Problem is visible when using multiple dhcp-ranges; one dhcp-range is a "catch-all"
range without tags while the second dhcp-range has tags based on vendor-class/user-class/...
If a client sends a DORA with no specific IP and no vendor/user class it will get an IP addres
from the "catch-all" range; if the client renews (or restarts) with a vendor/user class
and a request IP option holding the "catch-all" IP it will get DHCP options from the
dhcp-range based on vendor-class/user-class but will get the requested IP address acked.
This patch solves this problem as it detects a tag is assigned based on the vendor-class/user-class
and actually checks if the requested IP belongs to the correct dhcp-range; if not the client will
be nacked to it can restart its DORA sequence.

Signed-off-by: Daniele Lacamera <***@danielinux.net>
Signed-off-by: Hans Dedecker <***@gmail.com>
---
src/rfc2131.c | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 80 insertions(+), 1 deletion(-)

diff --git a/src/rfc2131.c b/src/rfc2131.c
index 8b99d4b..53e6be1 100644
--- a/src/rfc2131.c
+++ b/src/rfc2131.c
@@ -64,6 +64,23 @@ static int prune_vendor_opts(struct dhcp_netid *netid);
static struct dhcp_opt *pxe_opts(int pxe_arch, struct dhcp_netid *netid, struct in_addr local, time_t now);
struct dhcp_boot *find_boot(struct dhcp_netid *netid);
static int pxe_uefi_workaround(int pxe_arch, struct dhcp_netid *netid, struct dhcp_packet *mess, struct in_addr local, time_t now, int pxe);
+
+static struct dhcp_netid *vendor_forced_netid_filter(struct dhcp_context *context, struct dhcp_vendor *vendor, struct dhcp_netid *netid)
+{
+ struct dhcp_context *context_tmp;
+ for (context_tmp = context; context_tmp; context_tmp = context_tmp->current)
+ {
+ struct dhcp_netid *f;
+ for (f = context_tmp->filter; f; f = f->next)
+ {
+ if (strcmp(f->net, netid->net) == 0)
+ {
+ return netid;
+ }
+ }
+ }
+ return NULL;
+}

size_t dhcp_reply(struct dhcp_context *context, char *iface_name, int int_index,
size_t sz, time_t now, int unicast_dest, int *is_inform, int pxe, struct in_addr fallback)
@@ -98,6 +115,8 @@ size_t dhcp_reply(struct dhcp_context *context, char *iface_name, int int_index,
#ifdef HAVE_SCRIPT
unsigned char *class = NULL;
#endif
+ struct dhcp_netid *ctx_forced_netid_tag = NULL;
+ struct dhcp_context *nwd_context = NULL;

subnet_addr.s_addr = override.s_addr = 0;

@@ -452,6 +471,7 @@ size_t dhcp_reply(struct dhcp_context *context, char *iface_name, int int_index,
{
vendor->netid.next = netid;
netid = &vendor->netid;
+ ctx_forced_netid_tag = vendor_forced_netid_filter(context, vendor, netid);
break;
}
}
@@ -1039,13 +1059,59 @@ size_t dhcp_reply(struct dhcp_context *context, char *iface_name, int int_index,
mess->yiaddr = lease->addr;
else if (opt && address_available(context, addr, tagif_netid) && !lease_find_by_addr(addr) &&
!config_find_by_address(daemon->dhcp_conf, addr))
- mess->yiaddr = addr;
+ {
+ /* Only accept option 50 either if no tag was forced, or if the tag matches one of the filters
+ * of the IP range narrowed via option 60.
+ */
+ if ((!ctx_forced_netid_tag) ||
+ (((nwd_context = narrow_context(context, addr, ctx_forced_netid_tag)) != NULL) &&
+ (nwd_context->filter != NULL) && match_netid(nwd_context->filter, ctx_forced_netid_tag, 0)))
+ {
+ mess->yiaddr = addr;
+ }
+ else
+ {
+ struct dhcp_context *context_tmp;
+ my_syslog(MS_DHCP | LOG_WARNING, _("ignoring option 50 address because netid tag does not match the pool."));
+ opt = NULL;
+
+ for (context_tmp = daemon->dhcp; context_tmp; context_tmp = context_tmp->next)
+ {
+ if (address_allocate(context_tmp, &mess->yiaddr, emac, emac_len, ctx_forced_netid_tag, now))
+ break;
+ }
+
+ if (!context_tmp)
+ message = _("wrong option 50 provided, trying to allocate a new address, no address available");
+
+ ctx_forced_netid_tag = NULL;
+ }
+ }
else if (emac_len == 0)
message = _("no unique-id");
else if (!address_allocate(context, &mess->yiaddr, emac, emac_len, tagif_netid, now))
message = _("no address available");
}

+ if ((ctx_forced_netid_tag) &&
+ (((nwd_context = narrow_context(context, mess->yiaddr, ctx_forced_netid_tag)) == NULL) ||
+ (nwd_context->filter == NULL) || (!match_netid(nwd_context->filter, ctx_forced_netid_tag, 0))))
+ {
+ struct dhcp_context *context_tmp;
+ my_syslog(MS_DHCP | LOG_WARNING, _("Wrong IP address found: not matching VID."));
+ if ((lease) && (mess->yiaddr.s_addr == lease->addr.s_addr))
+ {
+ lease_prune(lease, now);
+ lease = NULL;
+ }
+
+ for (context_tmp = daemon->dhcp; context_tmp; context_tmp = context_tmp->next)
+ {
+ if (address_allocate(context_tmp, &mess->yiaddr, emac, emac_len, ctx_forced_netid_tag, now))
+ break;
+ }
+ }
+
log_packet("DHCPDISCOVER", opt ? option_ptr(opt, 0) : NULL, emac, emac_len, iface_name, NULL, message, mess->xid);

if (message || !(context = narrow_context(context, mess->yiaddr, tagif_netid)))
@@ -1176,6 +1242,7 @@ size_t dhcp_reply(struct dhcp_context *context, char *iface_name, int int_index,
{
struct dhcp_config *addr_config;
struct dhcp_context *tmp = NULL;
+ struct dhcp_context *nwd_context = NULL;

if (have_config(config, CONFIG_ADDR))
for (tmp = context; tmp; tmp = tmp->current)
@@ -1190,6 +1257,18 @@ size_t dhcp_reply(struct dhcp_context *context, char *iface_name, int int_index,
unicast_dest = 0;
}

+ /* Only accept option 50 either if no tag was forced, or if the tag matches one of the filters
+ * of the IP range narrowed via option 60.
+ */
+ else if ((ctx_forced_netid_tag) &&
+ (((nwd_context = narrow_context(context, mess->yiaddr, ctx_forced_netid_tag)) == NULL) ||
+ (nwd_context->filter == NULL) || (match_netid(nwd_context->filter, ctx_forced_netid_tag, 0)) == 0))
+ {
+ message = _("wrong VID/Req IP");
+ /* ensure we broadcast NAK */
+ unicast_dest = 0;
+ }
+
/* Check for renewal of a lease which is outside the allowed range. */
else if (!address_available(context, mess->yiaddr, tagif_netid) &&
(!have_config(config, CONFIG_ADDR) || config->addr.s_addr != mess->yiaddr.s_addr))

[Simon Kelley]

Post by Hans Dedecker
Problem is visible when using multiple dhcp-ranges; one dhcp-range is a "catch-all"
range without tags while the second dhcp-range has tags based on vendor-class/user-class/...
If a client sends a DORA with no specific IP and no vendor/user class it will get an IP addres
from the "catch-all" range; if the client renews (or restarts) with a vendor/user class
and a request IP option holding the "catch-all" IP it will get DHCP options from the
dhcp-range based on vendor-class/user-class but will get the requested IP address acked.
This patch solves this problem as it detects a tag is assigned based on the vendor-class/user-class
and actually checks if the requested IP belongs to the correct dhcp-range; if not the client will
be nacked to it can restart its DORA sequence.

I'm happy to work on understanding this patch if it's really necessary,
but an alternative way of working around this client behaviour, without
code changes, may be to abandon the "catch all" range, and declare the
two ranges explicitly,

dhcp-vendorclass=set:special, .......

dhcp-range = tag:special, ......

# range used for hosts that don't have a special class
dhcp-range = tag:!special, ......


If there are multiple special classes, the catch-all becomes

dhcp-range = tag:!special1, tag:!special2, tag:!special3

Doing it like this, when a previously ordinary client acquires "special"
status, the catch-all range ceases to apply, and the re-addressing
should happen automatically.


Cheers,

Simon.

[Hans Dedecker]

Post by Simon Kelley

Post by Hans Dedecker
Problem is visible when using multiple dhcp-ranges; one dhcp-range is a "catch-all"
range without tags while the second dhcp-range has tags based on vendor-class/user-class/...
If a client sends a DORA with no specific IP and no vendor/user class it will get an IP addres
from the "catch-all" range; if the client renews (or restarts) with a vendor/user class
and a request IP option holding the "catch-all" IP it will get DHCP options from the
dhcp-range based on vendor-class/user-class but will get the requested IP address acked.
This patch solves this problem as it detects a tag is assigned based on the vendor-class/user-class
and actually checks if the requested IP belongs to the correct dhcp-range; if not the client will
be nacked to it can restart its DORA sequence.

I'm happy to work on understanding this patch if it's really necessary,
but an alternative way of working around this client behaviour, without
code changes, may be to abandon the "catch all" range, and declare the
two ranges explicitly,
dhcp-vendorclass=set:special, .......
dhcp-range = tag:special, ......
# range used for hosts that don't have a special class
dhcp-range = tag:!special, ......
If there are multiple special classes, the catch-all becomes
dhcp-range = tag:!special1, tag:!special2, tag:!special3
Doing it like this, when a previously ordinary client acquires "special"
status, the catch-all range ceases to apply, and the re-addressing
should happen automatically.

Maybe I should have added a slightly patch description which describes
the problem more clearly.
If a client sends a DHCP discover containing the requested IP option
dnsmasq acks the IP address (depending on certain conditions like ip
address is available in a defined range, no lease, etc ... )
irrespective from the fact a policy class is defined for the client
(eg based on the vendor class) instructing it to take an IP address
from a different pool.
You have no control over neither the presence or the contents of the
requested IP option in a discover message sent by the client but I
would expect the server to respect the policy class put into place
independent from the requested IP option.
I don't think this problem can be fixed by config unless I'm missing
something; the delivered patch addresses this problem.

Bye,
Hans

Post by Simon Kelley
Cheers,
Simon.