On Thu, Dec 08, 2016 at 05:07:41PM +0100, Jeremie Courreges-Anglas wrote:
> ---sniped---
> I think you've summoned Cthulhu with this diff. :)
>
> I started noticing a wrong behavior with when testing a multi-relay
> setup, as prompted by Patrick's diff. See
>
> marc.info/?l=openbsd-tech&m=148096893918967&w=2
>
> Such as setup didn't work before, and is still half-broken in -current.
>
> For the record:
>
> --8<--
> Addresses start with 192.168...
>
> [server] <-> [relay1] <-> [relay2] <-> [client]
> 1.1 1.2 2.1 2.2 3.1
>
> Here dhcrelay on relay2 ignores replies sent by 192.168.1.1 to
> 192.168.3.1, because its UDP socket is bound to 192.168.3.1 but
> connected to 192.168.2.2.
> -->8--
>
> A second problem is that, if [relay1] is on the path between [server] and
> [relay2], the BPF socket will catch [server]'s reply, think it has to
> handle it (I don't think it should), and send it to [client]. The code
> in dhcrelay(8) doesn't make enough checks to cope with such a situation.
> This is not a big problem in practice, but now your diff comes in.
>
> In got_response(), you store client_port, 68, in
> ss2sin(&pc->pc_dst)->sin_port. But in got_one() -> receive_packet() ->
> decode_udp_ip_header(), you store the UDP port of the incoming packet.
> In case of a [server]->[relay2] messages, the port is 67. [relay1] thus
> sends a packet to the client with 67 as the destination port. This
> packet matches the BPF filter of [relay1], which then goes in an
> infinite loop.
>
> IIUC, an obvious improvement would be to ignore BOOTREPLY packets with
> a giaddr that doesn't match our primary address. relay() could use such
> a check:
>
> /* If it's a bootreply, forward it to the client. */
> if (packet->op == BOOTREPLY) {
>
> /* Is this packet actually for us? */
> if (packet->giaddr.s_addr != interfaces->primary_address.s_addr)
> return;
> [...]
> }
>
> Maybe there is a drawback that I can't see, but blindly relaying like we
> do now can't be a good thing.
This is not the purpose of this diff, but we can fix it later in the next
diffs. I want to keep the scope on reducing the functions prototype and
less in fixing previous problems.
>
> The reason why port 67 ends up on the wire instead of port 68 is
> probably this:
>
> bzero(&to, sizeof(to));
> if (!(packet->flags & htons(BOOTP_BROADCAST))) {
> to.sin_addr = packet->yiaddr;
> to.sin_port = client_port;
> } else {
> to.sin_addr.s_addr = htonl(INADDR_BROADCAST);
> to.sin_port = client_port;
> }
> to.sin_family = AF_INET;
> to.sin_len = sizeof to;
> memcpy(&ss2sin(&pc->pc_dst)->sin_addr, &to.sin_addr,
> sizeof(ss2sin(&pc->pc_dst)->sin_addr));
>
> The memcpy copies only the IP address, not the port. This should
> be something like
>
> *ss2sin(&pc->pc_dst) = to;
New diff has this fix, thank you!
>
> Another problem: the relay->server code uses send(2) on a connected
> socket and thus has no destination IP issue. But the relay->client path
> now uses the source address from the server->relay packet. I think
> we should keep using interfaces->primary_address here...
>
> No idea if the proposals above would fix all the problems, but I have to
> finish this mail. :)
>
> Aside from this:
> - please drop the extra netinet/if_ether.h includes
> - not a problem per se, but such a simple function like ss2sin could
> probably be defined static inline in a header
I accept your suggestions and I have applied them.
Here is the new diff with the improvements, ok?
Index: bpf.c
===================================================================
RCS file: /home/obsdcvs/src/usr.sbin/dhcrelay/bpf.c,v
retrieving revision 1.12
diff -u -p -r1.12 bpf.c
--- bpf.c 8 Dec 2016 09:29:50 -0000 1.12
+++ bpf.c 8 Dec 2016 17:03:21 -0000
@@ -258,24 +258,23 @@ if_register_receive(struct interface_inf
ssize_t
send_packet(struct interface_info *interface,
- struct dhcp_packet *raw, size_t len, struct in_addr from,
- struct sockaddr_in *to, struct hardware *hto)
+ struct dhcp_packet *raw, size_t len, struct packet_ctx *pc)
{
unsigned char buf[256];
struct iovec iov[2];
int result, bufp = 0;
if (interface->hw_address.htype == HTYPE_IPSEC_TUNNEL) {
- socklen_t slen = sizeof(*to);
+ socklen_t slen = pc->pc_dst.ss_len;
result = sendto(server_fd, raw, len, 0,
- (struct sockaddr *)to, slen);
+ (struct sockaddr *)&pc->pc_dst, slen);
goto done;
}
/* Assemble the headers... */
- assemble_hw_header(interface, buf, &bufp, hto);
- assemble_udp_ip_header(interface, buf, &bufp, from.s_addr,
- to->sin_addr.s_addr, to->sin_port, (unsigned char *)raw, len);
+ assemble_hw_header(interface, buf, &bufp, pc);
+ assemble_udp_ip_header(interface, buf, &bufp, pc,
+ (unsigned char *)raw, len);
/* Fire it off */
iov[0].iov_base = (char *)buf;
@@ -292,7 +291,7 @@ send_packet(struct interface_info *inter
ssize_t
receive_packet(struct interface_info *interface, unsigned char *buf,
- size_t len, struct sockaddr_in *from, struct hardware *hfrom)
+ size_t len, struct packet_ctx *pc)
{
int length = 0, offset = 0;
struct bpf_hdr hdr;
@@ -358,7 +357,7 @@ receive_packet(struct interface_info *in
/* Decode the physical header... */
offset = decode_hw_header(interface,
- interface->rbuf, interface->rbuf_offset, hfrom);
+ interface->rbuf, interface->rbuf_offset, pc);
/*
* If a physical layer checksum failed (dunno of any
@@ -374,7 +373,7 @@ receive_packet(struct interface_info *in
/* Decode the IP and UDP headers... */
offset = decode_udp_ip_header(interface, interface->rbuf,
- interface->rbuf_offset, from, hdr.bh_caplen);
+ interface->rbuf_offset, pc, hdr.bh_caplen);
/* If the IP or UDP checksum was bad, skip the packet... */
if (offset < 0) {
Index: dhcpd.h
===================================================================
RCS file: /home/obsdcvs/src/usr.sbin/dhcrelay/dhcpd.h,v
retrieving revision 1.16
diff -u -p -r1.16 dhcpd.h
--- dhcpd.h 8 Dec 2016 09:29:50 -0000 1.16
+++ dhcpd.h 8 Dec 2016 17:03:21 -0000
@@ -42,15 +42,28 @@
#define SERVER_PORT 67
#define CLIENT_PORT 68
+/* Maximum size of client hardware address. */
+#define CHADDR_SIZE 16
+
+struct packet_ctx {
+ uint8_t pc_htype;
+ uint8_t pc_hlen;
+ uint8_t pc_smac[CHADDR_SIZE];
+ uint8_t pc_dmac[CHADDR_SIZE];
+
+ struct sockaddr_storage pc_src;
+ struct sockaddr_storage pc_dst;
+};
+
struct iaddr {
int len;
- unsigned char iabuf[16];
+ unsigned char iabuf[CHADDR_SIZE];
};
struct hardware {
u_int8_t htype;
u_int8_t hlen;
- u_int8_t haddr[16];
+ u_int8_t haddr[CHADDR_SIZE];
};
/* Possible states in which the client can be. */
@@ -112,15 +125,13 @@ int if_register_bpf(struct interface_inf
void if_register_send(struct interface_info *);
void if_register_receive(struct interface_info *);
ssize_t send_packet(struct interface_info *,
- struct dhcp_packet *, size_t, struct in_addr,
- struct sockaddr_in *, struct hardware *);
+ struct dhcp_packet *, size_t, struct packet_ctx *);
ssize_t receive_packet(struct interface_info *, unsigned char *, size_t,
- struct sockaddr_in *, struct hardware *);
+ struct packet_ctx *);
/* dispatch.c */
extern void (*bootp_packet_handler)(struct interface_info *,
- struct dhcp_packet *, int, unsigned int, struct iaddr,
- struct hardware *);
+ struct dhcp_packet *, int, struct packet_ctx *);
struct interface_info *get_interface(const char *,
void (*)(struct protocol *));
void dispatch(void);
@@ -130,13 +141,13 @@ void remove_protocol(struct protocol *);
/* packet.c */
void assemble_hw_header(struct interface_info *, unsigned char *,
- int *, struct hardware *);
+ int *, struct packet_ctx *);
void assemble_udp_ip_header(struct interface_info *, unsigned char *,
- int *, u_int32_t, u_int32_t, unsigned int, unsigned char *, int);
+ int *, struct packet_ctx *pc, unsigned char *, int);
ssize_t decode_hw_header(struct interface_info *, unsigned char *,
- int, struct hardware *);
+ int, struct packet_ctx *);
ssize_t decode_udp_ip_header(struct interface_info *, unsigned char *,
- int, struct sockaddr_in *, int);
+ int, struct packet_ctx *, int);
/* dhcrelay.c */
extern u_int16_t server_port;
@@ -147,3 +158,9 @@ extern int server_fd;
extern time_t cur_time;
extern int log_priority;
extern int log_perror;
+
+static inline struct sockaddr_in *
+ss2sin(struct sockaddr_storage *ss)
+{
+ return ((struct sockaddr_in *)ss);
+}
Index: dhcrelay.c
===================================================================
RCS file: /home/obsdcvs/src/usr.sbin/dhcrelay/dhcrelay.c,v
retrieving revision 1.48
diff -u -p -r1.48 dhcrelay.c
--- dhcrelay.c 8 Dec 2016 09:29:50 -0000 1.48
+++ dhcrelay.c 8 Dec 2016 17:03:21 -0000
@@ -65,7 +65,7 @@
void usage(void);
int rdaemon(int);
void relay(struct interface_info *, struct dhcp_packet *, int,
- unsigned int, struct iaddr, struct hardware *);
+ struct packet_ctx *);
char *print_hw_addr(int, int, unsigned char *);
void got_response(struct protocol *);
int get_rdomain(char *);
@@ -264,11 +264,10 @@ main(int argc, char *argv[])
void
relay(struct interface_info *ip, struct dhcp_packet *packet, int length,
- unsigned int from_port, struct iaddr from, struct hardware *hfrom)
+ struct packet_ctx *pc)
{
struct server_list *sp;
struct sockaddr_in to;
- struct hardware hto;
if (packet->hlen > sizeof packet->chaddr) {
note("Discarding packet with invalid hlen.");
@@ -287,6 +286,7 @@ relay(struct interface_info *ip, struct
}
to.sin_family = AF_INET;
to.sin_len = sizeof to;
+ *ss2sin(&pc->pc_dst) = to;
/*
* Set up the hardware destination address. If it's a reply
@@ -294,13 +294,13 @@ relay(struct interface_info *ip, struct
* cast as well.
*/
if (!(packet->flags & htons(BOOTP_BROADCAST))) {
- hto.hlen = packet->hlen;
- if (hto.hlen > sizeof hto.haddr)
- hto.hlen = sizeof hto.haddr;
- memcpy(hto.haddr, packet->chaddr, hto.hlen);
- hto.htype = packet->htype;
+ pc->pc_hlen = packet->hlen;
+ if (pc->pc_hlen > CHADDR_SIZE)
+ pc->pc_hlen = CHADDR_SIZE;
+ memcpy(pc->pc_dmac, packet->chaddr, pc->pc_hlen);
+ pc->pc_htype = packet->htype;
} else {
- bzero(&hto, sizeof(hto));
+ memset(pc->pc_dmac, 0xff, sizeof(pc->pc_dmac));
}
if ((length = relay_agentinfo(interfaces,
@@ -321,8 +321,7 @@ relay(struct interface_info *ip, struct
*/
packet->giaddr.s_addr = 0x0;
- if (send_packet(interfaces, packet, length,
- interfaces->primary_address, &to, &hto) != -1)
+ if (send_packet(interfaces, packet, length, pc) != -1)
debug("forwarded BOOTREPLY for %s to %s",
print_hw_addr(packet->htype, packet->hlen,
packet->chaddr), inet_ntoa(to.sin_addr));
@@ -351,7 +350,7 @@ relay(struct interface_info *ip, struct
packet->giaddr = ip->primary_address;
if ((length = relay_agentinfo(ip, packet, length,
- (struct in_addr *)from.iabuf, NULL)) == -1) {
+ &ss2sin(&pc->pc_src)->sin_addr, NULL)) == -1) {
note("ignoring BOOTREQUEST with invalid "
"relay agent information");
return;
@@ -439,8 +438,8 @@ bad:
void
got_response(struct protocol *l)
{
+ struct packet_ctx pc;
ssize_t result;
- struct iaddr ifrom;
union {
/*
* Packet input buffer. Must be as large as largest
@@ -473,13 +472,19 @@ got_response(struct protocol *l)
return;
}
- if (bootp_packet_handler) {
- ifrom.len = 4;
- memcpy(ifrom.iabuf, &sp->to.sin_addr, ifrom.len);
+ memset(&pc, 0, sizeof(pc));
+ pc.pc_src.ss_family = AF_INET;
+ pc.pc_src.ss_len = sizeof(struct sockaddr_in);
+ memcpy(&ss2sin(&pc.pc_src)->sin_addr, &sp->to.sin_addr,
+ sizeof(ss2sin(&pc.pc_src)->sin_addr));
+ ss2sin(&pc.pc_src)->sin_port = server_port;
+
+ pc.pc_dst.ss_family = AF_INET;
+ pc.pc_dst.ss_len = sizeof(struct sockaddr_in);
+ ss2sin(&pc.pc_dst)->sin_port = client_port;
- (*bootp_packet_handler)(NULL, &u.packet, result,
- sp->to.sin_port, ifrom, NULL);
- }
+ if (bootp_packet_handler)
+ (*bootp_packet_handler)(NULL, &u.packet, result, &pc);
}
ssize_t
Index: dispatch.c
===================================================================
RCS file: /home/obsdcvs/src/usr.sbin/dhcrelay/dispatch.c,v
retrieving revision 1.13
diff -u -p -r1.13 dispatch.c
--- dispatch.c 8 Dec 2016 09:29:50 -0000 1.13
+++ dispatch.c 8 Dec 2016 17:03:21 -0000
@@ -69,8 +69,7 @@ static struct timeout *free_timeouts;
static int interfaces_invalidated;
void (*bootp_packet_handler)(struct interface_info *,
- struct dhcp_packet *, int, unsigned int,
- struct iaddr, struct hardware *);
+ struct dhcp_packet *, int, struct packet_ctx *);
static int interface_status(struct interface_info *ifinfo);
@@ -260,9 +259,7 @@ another:
void
got_one(struct protocol *l)
{
- struct sockaddr_in from;
- struct hardware hfrom;
- struct iaddr ifrom;
+ struct packet_ctx pc;
size_t result;
union {
/*
@@ -274,8 +271,9 @@ got_one(struct protocol *l)
} u;
struct interface_info *ip = l->local;
- if ((result = receive_packet(ip, u.packbuf, sizeof(u), &from,
- &hfrom)) == -1) {
+ memset(&pc, 0, sizeof(pc));
+
+ if ((result = receive_packet(ip, u.packbuf, sizeof(u), &pc)) == -1) {
warning("receive_packet failed on %s: %s", ip->name,
strerror(errno));
ip->errors++;
@@ -295,13 +293,8 @@ got_one(struct protocol *l)
if (result == 0)
return;
- if (bootp_packet_handler) {
- ifrom.len = 4;
- memcpy(ifrom.iabuf, &from.sin_addr, ifrom.len);
-
- (*bootp_packet_handler)(ip, &u.packet, result,
- from.sin_port, ifrom, &hfrom);
- }
+ if (bootp_packet_handler)
+ (*bootp_packet_handler)(ip, &u.packet, result, &pc);
}
int
Index: packet.c
===================================================================
RCS file: /home/obsdcvs/src/usr.sbin/dhcrelay/packet.c,v
retrieving revision 1.11
diff -u -p -r1.11 packet.c
--- packet.c 7 Feb 2016 00:49:28 -0000 1.11
+++ packet.c 8 Dec 2016 17:03:21 -0000
@@ -97,17 +97,13 @@ wrapsum(u_int32_t sum)
void
assemble_hw_header(struct interface_info *interface, unsigned char *buf,
- int *bufix, struct hardware *to)
+ int *bufix, struct packet_ctx *pc)
{
struct ether_header eh;
- if (to != NULL && to->hlen == 6) /* XXX */
- memcpy(eh.ether_dhost, to->haddr, sizeof(eh.ether_dhost));
- else
- memset(eh.ether_dhost, 0xff, sizeof(eh.ether_dhost));
-
- /* source address is filled in by the kernel */
- memset(eh.ether_shost, 0x00, sizeof(eh.ether_shost));
+ /* Use the supplied address or let the kernel fill it. */
+ memcpy(eh.ether_shost, pc->pc_smac, ETHER_ADDR_LEN);
+ memcpy(eh.ether_dhost, pc->pc_dmac, ETHER_ADDR_LEN);
eh.ether_type = htons(ETHERTYPE_IP);
@@ -117,8 +113,7 @@ assemble_hw_header(struct interface_info
void
assemble_udp_ip_header(struct interface_info *interface, unsigned char *buf,
- int *bufix, u_int32_t from, u_int32_t to, unsigned int port,
- unsigned char *data, int len)
+ int *bufix, struct packet_ctx *pc, unsigned char *data, int len)
{
struct ip ip;
struct udphdr udp;
@@ -132,15 +127,15 @@ assemble_udp_ip_header(struct interface_
ip.ip_ttl = 16;
ip.ip_p = IPPROTO_UDP;
ip.ip_sum = 0;
- ip.ip_src.s_addr = from;
- ip.ip_dst.s_addr = to;
+ ip.ip_src.s_addr = ss2sin(&pc->pc_src)->sin_addr.s_addr;
+ ip.ip_dst.s_addr = ss2sin(&pc->pc_dst)->sin_addr.s_addr;
ip.ip_sum = wrapsum(checksum((unsigned char *)&ip, sizeof(ip), 0));
memcpy(&buf[*bufix], &ip, sizeof(ip));
*bufix += sizeof(ip);
- udp.uh_sport = server_port; /* XXX */
- udp.uh_dport = port; /* XXX */
+ udp.uh_sport = ss2sin(&pc->pc_src)->sin_port;
+ udp.uh_dport = ss2sin(&pc->pc_dst)->sin_port;
udp.uh_ulen = htons(sizeof(udp) + len);
memset(&udp.uh_sum, 0, sizeof(udp.uh_sum));
@@ -155,9 +150,8 @@ assemble_udp_ip_header(struct interface_
ssize_t
decode_hw_header(struct interface_info *interface, unsigned char *buf,
- int bufix, struct hardware *from)
+ int bufix, struct packet_ctx *pc)
{
- struct ether_header eh;
size_t offset = 0;
if (interface->hw_address.htype == HTYPE_IPSEC_TUNNEL) {
@@ -172,23 +166,24 @@ decode_hw_header(struct interface_info *
if (ip->ip_p != IPPROTO_IPIP)
return (-1);
- bzero(&eh, sizeof(eh));
+ memset(pc->pc_dmac, 0xff, ETHER_ADDR_LEN);
offset = ENC_HDRLEN + ip_len;
- } else {
- memcpy(&eh, buf + bufix, ETHER_HDR_LEN);
- offset = sizeof(eh);
+ } else {
+ memcpy(pc->pc_dmac, buf + bufix, ETHER_ADDR_LEN);
+ memcpy(pc->pc_smac, buf + bufix + ETHER_ADDR_LEN,
+ ETHER_ADDR_LEN);
+ offset = sizeof(struct ether_header);
}
- memcpy(from->haddr, eh.ether_shost, sizeof(eh.ether_shost));
- from->htype = ARPHRD_ETHER;
- from->hlen = sizeof(eh.ether_shost);
+ pc->pc_htype = ARPHRD_ETHER;
+ pc->pc_hlen = ETHER_ADDR_LEN;
return (offset);
}
ssize_t
decode_udp_ip_header(struct interface_info *interface, unsigned char *buf,
- int bufix, struct sockaddr_in *from, int buflen)
+ int bufix, struct packet_ctx *pc, int buflen)
{
struct ip *ip;
struct udphdr *udp;
@@ -224,7 +219,15 @@ decode_udp_ip_header(struct interface_in
return (-1);
}
- memcpy(&from->sin_addr, &ip->ip_src, sizeof(from->sin_addr));
+ pc->pc_src.ss_len = sizeof(struct sockaddr_in);
+ pc->pc_src.ss_family = AF_INET;
+ memcpy(&ss2sin(&pc->pc_src)->sin_addr, &ip->ip_src,
+ sizeof(ss2sin(&pc->pc_src)->sin_addr));
+
+ pc->pc_dst.ss_len = sizeof(struct sockaddr_in);
+ pc->pc_dst.ss_family = AF_INET;
+ memcpy(&ss2sin(&pc->pc_dst)->sin_addr, &ip->ip_dst,
+ sizeof(ss2sin(&pc->pc_dst)->sin_addr));
#ifdef DEBUG
if (ntohs(ip->ip_len) != buflen)
@@ -291,7 +294,8 @@ decode_udp_ip_header(struct interface_in
return (-1);
}
- memcpy(&from->sin_port, &udp->uh_sport, sizeof(udp->uh_sport));
+ ss2sin(&pc->pc_src)->sin_port = udp->uh_sport;
+ ss2sin(&pc->pc_dst)->sin_port = udp->uh_dport;
return (ip_len + sizeof(*udp));
}
