Skip to content

Commit

Permalink
Option 77 Additions to dhclient
Browse files Browse the repository at this point in the history
These changes were present in the dhclient used in pfSense 2.3, they
seem not to have made it into FreeBSD 11 as used in pfSense 2.4.

I have merely taken the changes and applied them to the FreeBSD 11
dhclient. Users who require this have tested it and confirm it is
working fine.

Reworked-by: Franco Fichtner <franco@opnsense.org>
  • Loading branch information
marjohn56 authored and fichtner committed Mar 3, 2018
1 parent 5f9b491 commit b179b46
Show file tree
Hide file tree
Showing 6 changed files with 61 additions and 14 deletions.
17 changes: 14 additions & 3 deletions sbin/dhclient/bpf.c
Original file line number Diff line number Diff line change
Expand Up @@ -94,19 +94,30 @@ if_register_bpf(struct interface_info *info, int flags)
* 'ip and udp and src port bootps and dst port (bootps or bootpc)'
*/
struct bpf_insn dhcp_bpf_wfilter[] = {
/* Set packet index for IP packet... */
BPF_STMT(BPF_LDX + BPF_W + BPF_IMM, 0),

/* Test whether this is a VLAN packet... */
BPF_STMT(BPF_LD + BPF_B + BPF_IND, 12),
BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, ETHERTYPE_VLAN, 0, 1),

/* Correct the packet index for VLAN... */
BPF_STMT(BPF_LDX + BPF_W + BPF_IMM, 4),

/* Make sure it is an IPv4 packet... */
BPF_STMT(BPF_LD + BPF_B + BPF_IND, 14),
BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, (IPVERSION << 4) + 5, 0, 12),

/* Make sure this is an IP packet... */
BPF_STMT(BPF_LD + BPF_H + BPF_ABS, 12),
BPF_STMT(BPF_LD + BPF_H + BPF_IND, 12),
BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, ETHERTYPE_IP, 0, 10),

/* Make sure it's a UDP packet... */
BPF_STMT(BPF_LD + BPF_B + BPF_ABS, 23),
BPF_STMT(BPF_LD + BPF_B + BPF_IND, 23),
BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, IPPROTO_UDP, 0, 8),

/* Make sure this isn't a fragment... */
BPF_STMT(BPF_LD + BPF_H + BPF_ABS, 20),
BPF_STMT(BPF_LD + BPF_H + BPF_IND, 20),
BPF_JUMP(BPF_JMP + BPF_JSET + BPF_K, 0x1fff, 6, 0), /* patched */

/* Get the IP header length... */
Expand Down
6 changes: 6 additions & 0 deletions sbin/dhclient/conflex.c
Original file line number Diff line number Diff line change
Expand Up @@ -471,6 +471,8 @@ intern(char *atom, int dfv)
case 's':
if (!strcasecmp(atom + 1, "earch"))
return (SEARCH);
if (!strcasecmp(atom + 1, "end-interface"))
return (SEND_INTERFACE);
if (!strcasecmp(atom + 1, "tarts"))
return (STARTS);
if (!strcasecmp(atom + 1, "iaddr"))
Expand Down Expand Up @@ -519,6 +521,10 @@ intern(char *atom, int dfv)
case 'v':
if (!strcasecmp(atom + 1, "endor-class"))
return (VENDOR_CLASS);
if (!strcasecmp(atom + 1, "lan-id"))
return (VLAN_ID);
if (!strcasecmp(atom + 1, "lan-pcp"))
return (VLAN_PCP);
break;
case 'y':
if (!strcasecmp(atom + 1, "iaddr"))
Expand Down
3 changes: 3 additions & 0 deletions sbin/dhclient/dhcpd.h
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,9 @@ struct client_config {
u_int8_t required_options[256];
u_int8_t requested_options[256];
int requested_option_count;
int vlan_id;
int vlan_pcp;
char *send_interface;
time_t timeout;
time_t initial_interval;
time_t retry_interval;
Expand Down
3 changes: 3 additions & 0 deletions sbin/dhclient/dhctoken.h
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,9 @@
#define AUTHORITATIVE 333
#define TOKEN_NOT 334
#define ALWAYS_REPLY_RFC1048 335
#define VLAN_ID 336
#define VLAN_PCP 337
#define SEND_INTERFACE 338

#define is_identifier(x) ((x) >= FIRST_TOKEN && \
(x) != STRING && \
Expand Down
16 changes: 15 additions & 1 deletion sbin/dhclient/dispatch.c
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ static int interface_status(struct interface_info *ifinfo);
void
discover_interfaces(struct interface_info *iface)
{
char *sname = iface->client->config->send_interface;
struct ifaddrs *ifap, *ifa;
struct sockaddr_in foo;
struct ifreq *tif;
Expand Down Expand Up @@ -124,7 +125,20 @@ discover_interfaces(struct interface_info *iface)

/* Register the interface... */
if_register_receive(iface);
if_register_send(iface);
if (sname != NULL) {
char rname[IFNAMSIZ];

/* Change interface name for bpf registration */
strlcpy(rname, iface->name, IFNAMSIZ);
strlcpy(iface->ifp->ifr_name, sname, IFNAMSIZ);

if_register_send(iface);

/* Change name back to original */
strlcpy(iface->ifp->ifr_name, rname, IFNAMSIZ);
} else {
if_register_send(iface);
}
add_protocol(iface->name, iface->rfdesc, got_one, iface);
freeifaddrs(ifap);
}
Expand Down
30 changes: 20 additions & 10 deletions sbin/dhclient/packet.c
Original file line number Diff line number Diff line change
Expand Up @@ -92,19 +92,29 @@ void
assemble_hw_header(struct interface_info *interface, unsigned char *buf,
int *bufix)
{
struct ether_header eh;

memset(eh.ether_dhost, 0xff, sizeof(eh.ether_dhost));
if (interface->hw_address.hlen == sizeof(eh.ether_shost))
memcpy(eh.ether_shost, interface->hw_address.haddr,
sizeof(eh.ether_shost));
int vlpcp = interface->client->config->vlan_pcp;
int vlid = interface->client->config->vlan_id;
int plen = ETHER_HEADER_SIZE;
struct ether_vlan_header eh;

memset(eh.evl_dhost, 0xff, sizeof(eh.evl_dhost));
if (interface->hw_address.hlen == sizeof(eh.evl_shost))
memcpy(eh.evl_shost, interface->hw_address.haddr,
sizeof(eh.evl_shost));
else
memset(eh.ether_shost, 0x00, sizeof(eh.ether_shost));
memset(eh.evl_shost, 0x00, sizeof(eh.evl_shost));

eh.ether_type = htons(ETHERTYPE_IP);
eh.evl_encap_proto = htons(ETHERTYPE_IP);

if (vlid != 0) {
eh.evl_tag = htons(EVL_MAKETAG(vlid, vlpcp, 0));
eh.evl_encap_proto = htons(ETHERTYPE_VLAN);
eh.evl_proto = htons(ETHERTYPE_IP);
plen += ETHER_VLAN_ENCAP_LEN;
}

memcpy(&buf[*bufix], &eh, ETHER_HEADER_SIZE);
*bufix += ETHER_HEADER_SIZE;
memcpy(&buf[*bufix], &eh, plen);
*bufix += plen;
}

void
Expand Down

53 comments on commit b179b46

@fichtner
Copy link
Member

Choose a reason for hiding this comment

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

@marjohn56 I squashed what we have so far. The bpf filter is essentially adapted, it was formerly disabled, but there could be a bug in there, not sure yet.

@marjohn56
Copy link
Member

@marjohn56 marjohn56 commented on b179b46 Mar 5, 2018

Choose a reason for hiding this comment

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

OK, after battling all day Sunday trying to get Wireshark to even see the VLAN, some strangeness in Windows 10 I think, we still appear to have the same issue. dhclient is using the assigned VLAN, but the priority will not set. Here's a snippet from the rules.debug, I cheated and just modded filter.lib.inc to use the same rule creation method used or dhcp6c, in rules debug we get this:

pass out log quick on em0_vlan832 proto udp from {any} port {68} to {any} port {67} set prio 6 label "allow dhcp4 client on WAN_VLAN_832"

However, wireshark still shows it at priority 2, which is what I set in the VLAN config. So it's accepting that. Seems the rule is just not applied... any ideas?

@marjohn56
Copy link
Member

@marjohn56 marjohn56 commented on b179b46 Mar 5, 2018

Choose a reason for hiding this comment

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

In bpf.c, if_register_send(), is it not going to bypass the filters with a RAW socket send at around line 170?

@marjohn56
Copy link
Member

Choose a reason for hiding this comment

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

bump..

@fichtner
Copy link
Member

Choose a reason for hiding this comment

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

where does priority 2 come from? I thought for dhclient we need to do a special dhclient setting, not using a firewall... hence the patch that you proposed?

@marjohn56
Copy link
Member

Choose a reason for hiding this comment

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

I thought you had modded it so it would use the firewall for priority. :)

Priority two is the setting I had set in the VLAN for testing, It's not getting changed to priority 6 by the firewall, so the BPF is not doing anything.

@fichtner
Copy link
Member

Choose a reason for hiding this comment

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

I suppose it's possible the vlan priority written here is overwritten by the system default if specified, which would make this even more complicated.

I'm not sure about the order, but BPF could be after pf, so the firewall can't do it.

@marjohn56
Copy link
Member

Choose a reason for hiding this comment

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

OK, I think the solution then is to modify the client, I'll take the current source and add the bits that set the priority.

The current config string that works with the modified client is this:

<adv_dhcp_option_modifiers>send-interface "igb0", vlan-id 832, vlan-pcp 6</adv_dhcp_option_modifiers>

However, we don't appear have an issue with the vlan ID now, that's working, it's just the pri, I'll set to finding that bit in the source.

However, instead of setting the pri in the client config GUI as a text entry, we can do it the same way as we have done for dhcp6c, pick that up as the dhclient config is created. How does that sound?

@fichtner
Copy link
Member

Choose a reason for hiding this comment

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

sounds ok, but stilly worrying about the setting... can you post your ifconfig from the vlan? I think the system overrides our priority set in dhclient...

@marjohn56
Copy link
Member

@marjohn56 marjohn56 commented on b179b46 Mar 11, 2018

Choose a reason for hiding this comment

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

We are not setting a pri in dhclient, the client I am using is the one you squashed, that has no code in it for setting VLAN pri.

I think we have a crossed line. The dhclient as it stands, the one you squashed last week has no VLAN settings or the extra params needed by Orange FR.

Now I thought you were modifying the standard client to use the filters to change the pri of the packets rather than have it done within dhclient. It will still need mods for the extra params.

Here's my ifconfig:

em0_vlan832: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu 1500
ether 00:0c:29:d2:7e:2f
inet6 fe80::20c:29ff:fed2:7e2f%em0_vlan832 prefixlen 64 scopeid 0x7
inet 0.0.0.0 netmask 0x0 broadcast 255.255.255.255
inet 192.168.6.188 netmask 0xffffffff broadcast 192.168.6.188
inet 192.168.6.189 netmask 0xffffffff broadcast 192.168.6.189
inet 192.168.6.190 netmask 0xffffffff broadcast 192.168.6.190
nd6 options=23<PERFORMNUD,ACCEPT_RTADV,AUTO_LINKLOCAL>
media: Ethernet autoselect (1000baseT )
status: active
vlan: 832 vlanpcp: 2 parent interface: em0
groups: vlan

@fichtner
Copy link
Member

Choose a reason for hiding this comment

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

it should still have vlan-pcp vlan-id and send-interface. if it doesn't work it's a bug that I introduced.

@fichtner
Copy link
Member

Choose a reason for hiding this comment

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

vlan: 832 vlanpcp: 2 parent interface: em0 <-- this would rewrite it if it's not from the firewall maybe? maybe you can verify by changing it to 3 or 4 to see if the packet follows?

@marjohn56
Copy link
Member

Choose a reason for hiding this comment

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

Yes, as soon as I change the priority in the VLAN config, it changes on wireshark.

image

@marjohn56
Copy link
Member

Choose a reason for hiding this comment

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

bump... at some point we need to get back onto this.

@nivek1612
Copy link

Choose a reason for hiding this comment

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

YES please I'll be in France from 28th May and can TEST TEST TEST

@fichtner
Copy link
Member

@fichtner fichtner commented on b179b46 May 11, 2018 via email

Choose a reason for hiding this comment

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

@marjohn56
Copy link
Member

Choose a reason for hiding this comment

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

I agree, it would be best to use the pf to handle the priority, but that part of the code goes over my head. In the meantime, we could just make it work using the patches and kick the pf issue down the road until we get to grips with it.

@fichtner
Copy link
Member

@fichtner fichtner commented on b179b46 May 14, 2018

Choose a reason for hiding this comment

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

Okay, so here is the issue as already more or less established now set in stone:

https://github.com/opnsense/src/blob/master/sys/net/bpf.c#L1173

Meaning that no bpf output goes through pf. The patch is required but there are consistency issues with it. Ideally, there should be no need to jump through bpf for the sending side. Let me try something...

@marjohn56
Copy link
Member

Choose a reason for hiding this comment

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

bump...

Did you try that something? :)

@fichtner
Copy link
Member

Choose a reason for hiding this comment

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

Sure, doesn't work as DHCP requires fiddling with the MAC addresses. It's a weird protocol.

I also figured out why https://github.com/pfsense/FreeBSD-src/pull/8/files cannot possibly work: clparse.c changes are missing, see comment by @fredlubrano

@marjohn56
Copy link
Member

Choose a reason for hiding this comment

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

OK, so according to that, Option77 is now merged upstream and should all work, that just leaves the issue with the vlan priority?

@fichtner
Copy link
Member

Choose a reason for hiding this comment

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

yes, don't mind the "option 77", it's all about VLAN priority sending support.

Try this commit: f3f9a42daf388e

FWIW, I think we can ditch the BPF filter as well, it is only for incoming packets IMO.

@fichtner
Copy link
Member

Choose a reason for hiding this comment

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

@marjohn56 no sorry use this acc5d56fa0

BPF is necessary it seems (even though one could ask why bother restricting the types we can send... maybe for security reasons?). Found a small bug in the setup while looking at it again.

@marjohn56
Copy link
Member

Choose a reason for hiding this comment

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

Still at Pri 0. I did the usual minor hack to filter_lib.inc and set the pri there to follow the dhcp6c pri which gives this in rules.debug -

pass out log quick on em0_vlan802 proto udp from {any} port {68} to {any} port {67} set prio 6 label "allow dhcpv4 client out WAN"

Seems its still bypassing the filter.

@fichtner
Copy link
Member

Choose a reason for hiding this comment

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

not for filter, use the dhclient conf file syntax...

@marjohn56
Copy link
Member

Choose a reason for hiding this comment

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

I think we are at cross-purposes. :)

Is this version intended to use the DHCP Option Modifiers?

@fichtner
Copy link
Member

Choose a reason for hiding this comment

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

Yes, filter will never work for at least two reasons:

  • DHCP rewrites the MAC address, OS sockets can't do this.
  • BPF passes packets out via hardware interface, it does not hit the network stack.

@marjohn56
Copy link
Member

Choose a reason for hiding this comment

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

Aha! - OK, I'll reverse what I have done and attack it the other way,

@fichtner
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for all the crisscrossing. At least now we know there is no other way and can clean up the patch to make it work as expected with minimal impact. I run the latest version on the build server without VLAN, so no regression in the BPF filter is a good start...

@marjohn56
Copy link
Member

Choose a reason for hiding this comment

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

Cool. OK, doing a fresh install to start with so there are no leftovers.

@marjohn56
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't seem to be parsing correctly at present, I've asked Kev to send me his conf file to verify the *.conf is correct, however he's sunning himself on the beach in Nice at the moment.

@marjohn56
Copy link
Member

Choose a reason for hiding this comment

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

well, I don't know if I'm compiling something wrong, but...

Firstly, dhclient is sending out packets on the VLAN, as that's what the WAN interface is. If I specify vlan-id 832, vlan-pcp 6 in the options then I get:

May 31 17:03:16 | dhclient[18103]: send_packet: Operation not permitted
May 31 17:03:16 | dhclient[16505]: DHCPDISCOVER on em0_vlan832 to 255.255.255.255 port 67 interval 6
May 31 17:03:16 | dhclient: Starting delete_old_states()
May 31 17:03:16 | dhclient: PREINIT

@fichtner
Copy link
Member

@fichtner fichtner commented on b179b46 May 31, 2018 via email

Choose a reason for hiding this comment

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

@marjohn56
Copy link
Member

@marjohn56 marjohn56 commented on b179b46 May 31, 2018

Choose a reason for hiding this comment

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

Yes, my arguments in the options are these.

send-interface "em0",vlan-id 832, vlan-pcp 6

the conf file is this:

interface "em0_vlan832" {

# DHCP Protocol Timing Values

# DHCP Protocol Options
	send-interface "em0";
	vlan-id 832;
	vlan-pcp 6;

	script "/usr/local/opnsense/scripts/interfaces/dhclient-script";
}


@nivek1612
Copy link

Choose a reason for hiding this comment

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

watching from France. No test unit here at the moment so cant help out

@marjohn56
Copy link
Member

@marjohn56 marjohn56 commented on b179b46 Jun 10, 2018

Choose a reason for hiding this comment

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

OK.... I have spent a while going through your changes and comparing to the originals & the one I modded. By a process of elimination I can get dhclient_77_4 to work, but I have to remove these lines in bpf.c

if (ioctl(info->wfdesc, BIOCSETWF, &p) < 0) error("Can't install write filter program: %m");

I then see this in wireshark.

image

Otherwise I get this in the dhcp logs

dhclient[12481]: send_packet: Operation not permitted

@fichtner
Copy link
Member

Choose a reason for hiding this comment

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

okay, that means the bpf filter logic is not 100% correct. I'll check tomorrow :)

@fichtner
Copy link
Member

Choose a reason for hiding this comment

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

@marjohn56 this might be it.... f3304a1b

@fichtner
Copy link
Member

Choose a reason for hiding this comment

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

@marjohn56 how about this 1cd325e then?

@marjohn56
Copy link
Member

Choose a reason for hiding this comment

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

Hmm...
image

Give that man a banana... and a cigar. 👍

@nivek1612
Copy link

@nivek1612 nivek1612 commented on b179b46 Jun 11, 2018 via email

Choose a reason for hiding this comment

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

@fichtner
Copy link
Member

Choose a reason for hiding this comment

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

@nivek1612 something like this:

interface "vlanthingy0" {
    send-interface "em0";
    vlan-id 5;
    vlan-pcp 7;
}

@marjohn56
Copy link
Member

Choose a reason for hiding this comment

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

Indeed, your existing config will work. I need to look at the options.. I'll send you a binary shortly, can you check that and see what, if anything is still missing.

@fichtner
Copy link
Member

Choose a reason for hiding this comment

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

Will probably change "send-interface" to "vlan-parent" to make it more consistent, but for now test away and let me know if that's it. Non-VLAN usage works and the BPF filter kills all attempts to send other types of packets as it should.

@nivek1612
Copy link

Choose a reason for hiding this comment

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

tried the binary you sent last night but with my standard parameters for France its not issuing the dhcp request so suspect a parse error but for the life of me I cant remember where the error file is located

@marjohn56
Copy link
Member

Choose a reason for hiding this comment

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

dhcp log is under Services->dhcpv4.

Option modifiers are:
send-interface "em0", vlan-id 832, vlan-pcp 6

We'll work on the other bits now Franco has fixed the bpf. I'll pm you the other bits I'm using as they contain your login details.

@nivek1612
Copy link

Choose a reason for hiding this comment

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

Your new binary you just sent me over
works perfectly :-)

@marjohn56
Copy link
Member

@marjohn56 marjohn56 commented on b179b46 Jun 11, 2018

Choose a reason for hiding this comment

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

Excellent, @fichtner I added the Authentication option back in which seemed to have got lost along the way. Do you want me to change the "send-interface" to "vlan-parent" and PR it or will you finish it off and call it a wrap?

@fichtner
Copy link
Member

Choose a reason for hiding this comment

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

@marjohn56 yeah, please use vlan-parent in the PR. I have to adjust the manual page as well for dhclient.conf.5 just to be sure we don't forget what we have done and why. :)

@marjohn56
Copy link
Member

Choose a reason for hiding this comment

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

OK... done, just waiting on Kev testing and confirming it's good to go.

@nivek1612
Copy link

Choose a reason for hiding this comment

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

All good - we are GO

@fichtner
Copy link
Member

Choose a reason for hiding this comment

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

awesome, thanks for pushing this ❤️

@marjohn56
Copy link
Member

@marjohn56 marjohn56 commented on b179b46 Jun 11, 2018 via email

Choose a reason for hiding this comment

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

Please sign in to comment.