Re: dhclient new commit are good but....

2020-09-21 Thread Sven F.
On Sat, Sep 19, 2020 at 2:07 PM Denis Fondras  wrote:

> On Wed, Sep 16, 2020 at 03:06:32PM -0400, Sven F. wrote:
> > On Wed, Jun 3, 2020 at 2:13 PM sven falempin 
> > wrote:
> >
>
> Can you send a proper diff please for review please ?
>

bpf.c is sort of ok, the rest need choices .
The configuration of bpf should probably be updated if someone changes the
lladdr after launching dhclient.
Index: bpf.c
===
RCS file: /cvs/src/sbin/dhclient/bpf.c,v
retrieving revision 1.75
diff -u -p -r1.75 bpf.c
--- bpf.c	18 Mar 2019 00:00:59 -	1.75
+++ bpf.c	21 Sep 2020 19:18:52 -
@@ -102,6 +102,49 @@ get_udp_sock(int rdomain)
 	return sock;
 }
 
+
+/*
+ * Packet MAC filter program.
+ *
+ * N.B.: Changes to the filter program may require changes to the
+ * constant offsets used in if_register_receive to patch the BPF program!
+ */
+
+struct bpf_insn dhcp_bpf_mac_filter[] = {
+	/* Make sure this is an IP packet. */
+	BPF_STMT(BPF_LD + BPF_H + BPF_ABS, 12),
+	BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, ETHERTYPE_IP, 0, 8),
+
+	/* Make sure it's a UDP packet. */
+	BPF_STMT(BPF_LD + BPF_B + BPF_ABS, 23),
+	BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, IPPROTO_UDP, 0, 6),
+
+	/* Make sure this isn't a fragment. */
+	BPF_STMT(BPF_LD + BPF_H + BPF_ABS, 20),
+	BPF_JUMP(BPF_JMP + BPF_JSET + BPF_K, 0x1fff, 8, 0),
+
+	/* Get the IP header length. */
+	BPF_STMT(BPF_LDX + BPF_B + BPF_MSH, 14),
+
+	/* Make sure it's to the right port. */
+	BPF_STMT(BPF_LD + BPF_H + BPF_IND, 16),
+	BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, 67, 0, 1),		/* patch */
+
+	/* check bootp.hw.addr 2 bytes */
+	BPF_STMT(BPF_LD + BPF_H + BPF_IND, 50),
+	BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, 0x, 0, 3),  /* patch */
+	/* check bootp.hw.addr 4 bytes */
+	BPF_STMT(BPF_LD + BPF_W + BPF_IND, 52),
+	BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, 0x, 0, 1),  /* patch */ 
+
+	/* If we passed all the tests, ask for the whole packet. */
+	BPF_STMT(BPF_RET+BPF_K, (unsigned int)-1),
+
+	/* Otherwise, drop it. */
+	BPF_STMT(BPF_RET+BPF_K, 0),
+};
+int dhcp_bpf_mac_filter_len = sizeof(dhcp_bpf_mac_filter) / sizeof(struct bpf_insn);
+
 /*
  * Packet filter program.
  *
@@ -177,13 +220,19 @@ struct bpf_insn dhcp_bpf_wfilter[] = {
 
 int dhcp_bpf_wfilter_len = sizeof(dhcp_bpf_wfilter) / sizeof(struct bpf_insn);
 
+
+extern int cmd_opts;
+
 int
-configure_bpf_sock(int bpffd)
+configure_bpf_sock(struct interface_info *ifi)
 {
 	struct bpf_version	 v;
 	struct bpf_program	 p;
 	int			 flag = 1, sz;
 	int			 fildrop = BPF_FILDROP_CAPTURE;
+	uint32_t	 bits;
+	uint16_t	 bits16;
+	int bpffd = ifi->bpffd;
 
 	/* Make sure the BPF version is in range. */
 	if (ioctl(bpffd, BIOCVERSION, &v) == -1)
@@ -218,7 +267,18 @@ configure_bpf_sock(int bpffd)
 	 * N.B.: changes to filter program may require changes to the
 	 * insn number(s) used below!
 	 */
-	dhcp_bpf_filter[8].k = LOCAL_PORT;
+
+	if (cmd_opts & OPT_MAC_FITLER) {
+		p.bf_len = dhcp_bpf_mac_filter_len;
+		p.bf_insns = dhcp_bpf_mac_filter;
+		dhcp_bpf_mac_filter[8].k = LOCAL_PORT;
+		memcpy(&bits16, ifi->hw_address.ether_addr_octet, sizeof(bits16));
+		dhcp_bpf_mac_filter[10].k = ntohs(bits16);
+		memcpy(&bits, ifi->hw_address.ether_addr_octet + 2, sizeof(bits));
+		dhcp_bpf_mac_filter[12].k = ntohl(bits);
+	} else {
+		dhcp_bpf_filter[8].k = LOCAL_PORT;
+	}
 
 	if (ioctl(bpffd, BIOCSETF, &p) == -1)
 		fatal("BIOCSETF");
Index: dhclient.c
===
RCS file: /cvs/src/sbin/dhclient/dhclient.c,v
retrieving revision 1.678
diff -u -p -r1.678 dhclient.c
--- dhclient.c	31 Jul 2020 12:12:11 -	1.678
+++ dhclient.c	21 Sep 2020 19:18:53 -
@@ -471,7 +471,7 @@ main(int argc, char *argv[])
 
 	log_setverbose(0);	/* Don't show log_debug() messages. */
 
-	while ((ch = getopt(argc, argv, "c:di:L:nrv")) != -1)
+	while ((ch = getopt(argc, argv, "c:di:L:nrvm")) != -1)
 		switch (ch) {
 		case 'c':
 			if (optarg == NULL)
@@ -494,6 +494,9 @@ main(int argc, char *argv[])
 			cmd_opts |= OPT_DBPATH;
 			path_option_db = optarg;
 			break;
+		case 'm':
+			cmd_opts |= OPT_MAC_FITLER;
+			break;
 		case 'n':
 			cmd_opts |= OPT_NOACTION;
 			break;
@@ -670,7 +673,7 @@ main(int argc, char *argv[])
 	/* Create the udp and bpf sockets, growing rbuf if needed. */
 	ifi->udpfd = get_udp_sock(ifi->rdomain);
 	ifi->bpffd = get_bpf_sock(ifi->name);
-	newsize = configure_bpf_sock(ifi->bpffd);
+	newsize = configure_bpf_sock(ifi);
 	if (newsize > ifi->rbuf_max) {
 		if ((newp = realloc(ifi->rbuf, newsize)) == NULL)
 			fatal("rbuf");
Index: dhcpd.h
===
RCS file: /cvs/src/sbin/dhclient/dhcpd.h,v
retrieving revision 1.287
diff -u -p -r1.287 dhcpd.h
--- dhcpd.h	21 May 2020 01:07:52 -	1.287
+++ dhcpd.h	21 Sep 2020 19:18:53 -
@@ -191,7 +191,7 @@ void		 parse_warn(char *);
 /* bpf.c */
 int		 get_bpf_sock(char *);
 int		 get_udp_sock(int);
-int		 configure_bpf_sock(int);
+int		 configure_bp

dhclient new commit are good but....

2020-06-03 Thread sven falempin
Dear readers,

  since 5.8 i ve been carrying around patches to manage :
* crazy server sending hostname like "crazy ISP name with space" ( in
one case the ignore or supersede failed to workaround the problem ),
it  is a bit hard to test, and it looks like some improvement was made
to crash fatal on all invalid network information,
* and a bridging case, which is more realistic :

Assuming the EGRESS is Bridged to a vether, a pair or something to
serve the same LAN to a VM or something else, the first dhclient will eat
the paquet in the BPF filter because MAC are ignored.

MAC are ignored for some obscure historic case where the dhcp server responded
with broadcast or something.

To see the problem , assuming em0 is egress like :

# ifconfig  em0
em0: 
flags=808b43
mtu 1500
lladdr 00:ff:18:0b:4a:ee
index 1 priority 0 llprio 3
groups: egress
media: Ethernet autoselect (100baseTX full-duplex,rxpause,txpause)
status: active
inet 172.16.1.4 netmask 0xff00 broadcast 172.16.1.255

Bridge it to something

ifconfig  vether0 create
ifconfig  vether0 rdomain 1
ifconfig  bridge0 create
ifconfig  bridge0 add em0
ifconfig  bridge0 add vether0
#safety net
echo <<< EOF >> /etc/dhclient
interface "vether1" {
send host-name "bridged-sub-host-1";
send dhcp-lease-time 3600;
ignore domain-name-servers,routers;
require subnet-mask,domain-name-servers;
}
#testing
ifconfig  bridge0 up
route -T 1 exec dhclient vether0

No lease ! because dhclient on em0 is actually filtering them at bpf level
A year ago the hw_addr was kept inside the daemon so i am not sure how
to apply my bpf filter.

To not break something the path add a -m option to dhclient that
enable mac bpf filter awareness,

In configure_bpf_sock, I added a  ` uint8_t *  ether_addr_octet` param
that is not null
and setup in case -m is passed to fill in a slightly different filter.

/* Set up the bpf filter program structure. */
p.bf_len = dhcp_bpf_mac_filter_len;
p.bf_insns = dhcp_bpf_mac_filter;
dhcp_bpf_mac_filter[8].k = LOCAL_PORT;
memcpy(&bits16, ether_addr_octet, sizeof(bits16));
dhcp_bpf_mac_filter[10].k = ntohs(bits16);
memcpy(&bits, ether_addr_octet + 2, sizeof(bits));
dhcp_bpf_mac_filter[12].k = ntohl(bits);

with the filter :

struct bpf_insn dhcp_bpf_mac_filter[] = {
/* Make sure this is an IP packet. */
BPF_STMT(BPF_LD + BPF_H + BPF_ABS, 12),
BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, ETHERTYPE_IP, 0, 8),

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

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

/* Get the IP header length. */
BPF_STMT(BPF_LDX + BPF_B + BPF_MSH, 14),

/* Make sure it's to the right port. */
BPF_STMT(BPF_LD + BPF_H + BPF_IND, 16),
BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, 67, 0, 1),  /* patch */

/* check bootp.hw.addr 2 bytes */
BPF_STMT(BPF_LD + BPF_H + BPF_IND, 50),
BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, 0x, 0, 3),  /* patch */
/* check bootp.hw.addr 4 bytes */
BPF_STMT(BPF_LD + BPF_W + BPF_IND, 52),
BPF_JUMP(BPF_JMP + BPF_JEQ + BPF_K, 0x, 0, 1),  /* patch */

/* If we passed all the tests, ask for the whole packet. */
BPF_STMT(BPF_RET+BPF_K, (unsigned int)-1),

/* Otherwise, drop it. */
BPF_STMT(BPF_RET+BPF_K, 0),
};

int dhcp_bpf_filter_len = sizeof(dhcp_bpf_filter) / sizeof(struct bpf_insn);
int dhcp_bpf_mac_filter_len = sizeof(dhcp_bpf_mac_filter) /
sizeof(struct bpf_insn);

I would gladly test an officially made diff because this is becoming
hard to maintain,
and there should be use cases out there.

The only workaround I know is to put egress in a vether behind the bridges,
certainly something that could break anyway.

Thanks for reading up to there !

-- 
--
-
Knowing is not enough; we must apply. Willing is not enough; we must do