Re: dhcrelay bpf bugs

[Reyk Floeter]

> Am 05.06.2018 um 16:27 schrieb asaxena9021 :
> 
> Hi David,
> 
> I am facing this problem with lpf.c
> 
> In function receive_packet , My packet is somehow is getting corrupted.
> 
> DHCP packet content is like below:
> 
> DHCP Packet Content:
>opcode : 15
>hardwaretype : 205
>hlen : 8
>hops : 0
>xid : 1157628060
>secs : 51797
>flags : 16384
>addr ciaddr : @▒▒dd
>addr yiaddr : dd
>siaddr  : d
>giaddr :
>chaddr :
>sname  : @▒_dd
>file  : @▒▒dd
>options : Vishal add_relay_agent_options:1132
> 
> 
> I am not able to understand where is my dhcp discover packet is getting
> corrupted. I also post a question about it on stackoverflow.
> 
> https://stackoverflow.com/questions/50655937/isc-dhcrelay-is-not-forwarding-the-discover-packets-to-dhcp-server?noredirect=1#comment88380485_50655937
> 
>   
> 

OpenBSD’s dhcrelay is not ISC dhcrelay.

Reyk

> Please check.
> 
> Please help me in this.
> 
> Thanks,
> Akash Saxena.
> 
> 
> 
> --
> Sent from: 
> http://openbsd-archive.7691.n7.nabble.com/openbsd-dev-tech-f151936.html
> 

Re: dhcrelay bpf bugs

[asaxena9021]

Hi David,

I am facing this problem with lpf.c

In function receive_packet , My packet is somehow is getting corrupted.

DHCP packet content is like below:

DHCP Packet Content:
opcode : 15
hardwaretype : 205
hlen : 8
hops : 0
xid : 1157628060
secs : 51797
flags : 16384
addr ciaddr : @▒▒dd
addr yiaddr : dd
siaddr  : d
giaddr :
chaddr :
sname  : @▒_dd
file  : @▒▒dd
options : Vishal add_relay_agent_options:1132


I am not able to understand where is my dhcp discover packet is getting
corrupted. I also post a question about it on stackoverflow.

https://stackoverflow.com/questions/50655937/isc-dhcrelay-is-not-forwarding-the-discover-packets-to-dhcp-server?noredirect=1#comment88380485_50655937

  

Please check.

Please help me in this.

Thanks,
Akash Saxena.



--
Sent from: 
http://openbsd-archive.7691.n7.nabble.com/openbsd-dev-tech-f151936.html

dhcrelay bpf bugs

[David Gwynne]

there are two bugs in the bpf code in dhcrelay. they're obviously
rare though.

the worst one is when skipping a packet, dhcrelay uses the = operator
instead of + when summing the bpf header and capture length.

the second bug is also in calculating how much space a packet
consumes to skip over it. bpf aligns packets to a bpf word boundary,
but reports buffer lenghts in bytes. a program has to calculate the
next packet boundary with these values, but dhcrelay isnt. the
result of this would be dhcrelay using packet data and the wrong
fields in a bpf header, which is bad.

the fix for this is to calculate the offset to the next packet once,
and add that to the offset when skipping a packet. the code currently
skips the bpf header to get at the packet, which is now changed so
the packet start is accounted separately.

ive only tested that the code still works, i havent come up with a
test for the cases this diff fixes. ok?

ps, i think the whole code smells a bit, but i like the free diffz.

Index: bpf.c
===
RCS file: /cvs/src/usr.sbin/dhcrelay/bpf.c,v
retrieving revision 1.19
diff -u -p -r1.19 bpf.c
--- bpf.c   19 Apr 2017 05:36:12 -  1.19
+++ bpf.c   24 Nov 2017 02:10:48 -
@@ -375,6 +375,7 @@ receive_packet(struct interface_info *in
int length = 0;
ssize_t offset = 0;
struct bpf_hdr hdr;
+   size_t bpflen, pktoff;
 
/*
 * All this complexity is because BPF doesn't guarantee that
@@ -412,12 +413,14 @@ receive_packet(struct interface_info *in
memcpy(, >rbuf[interface->rbuf_offset],
sizeof(hdr));
 
+   bpflen = BPF_WORDALIGN(hdr.bh_hdrlen + hdr.bh_caplen);
+   pktoff = interface->rbuf_offset + hdr.bh_hdrlen;
+
/*
 * If the bpf header plus data doesn't fit in what's
 * left of the buffer, stick head in sand yet again...
 */
-   if (interface->rbuf_offset + hdr.bh_hdrlen + hdr.bh_caplen >
-   interface->rbuf_len) {
+   if (interface->rbuf_offset + bpflen > interface->rbuf_len) {
interface->rbuf_offset = interface->rbuf_len;
continue;
}
@@ -427,18 +430,14 @@ receive_packet(struct interface_info *in
 * the packet won't fit in the input buffer, all we can
 * do is drop it.
 */
-   if (hdr.bh_caplen != hdr.bh_datalen) {
-   interface->rbuf_offset += hdr.bh_hdrlen =
-   hdr.bh_caplen;
+   if (hdr.bh_caplen < hdr.bh_datalen) {
+   interface->rbuf_offset += bpflen;
continue;
}
 
-   /* Skip over the BPF header... */
-   interface->rbuf_offset += hdr.bh_hdrlen;
-
/* Decode the physical header... */
offset = decode_hw_header(interface->rbuf,
-   interface->rbuf_len, interface->rbuf_offset, pc,
+   interface->rbuf_len, pktoff, pc,
interface->hw_address.htype);
 
/*
@@ -447,7 +446,7 @@ receive_packet(struct interface_info *in
 * skip this packet.
 */
if (offset < 0) {
-   interface->rbuf_offset += hdr.bh_caplen;
+   interface->rbuf_offset += bpflen;
continue;
}
 
@@ -457,27 +456,23 @@ receive_packet(struct interface_info *in
 
/* If the IP or UDP checksum was bad, skip the packet... */
if (offset < 0) {
-   interface->rbuf_offset += hdr.bh_caplen;
+   interface->rbuf_offset += bpflen;
continue;
}
 
-   hdr.bh_caplen -= offset - interface->rbuf_offset;
-   interface->rbuf_offset = offset;
-
/*
 * If there's not enough room to stash the packet data,
 * we have to skip it (this shouldn't happen in real
 * life, though).
 */
if (hdr.bh_caplen > len) {
-   interface->rbuf_offset += hdr.bh_caplen;
+   interface->rbuf_offset += bpflen;
continue;
}
 
/* Copy out the data in the packet... */
-   memcpy(buf, interface->rbuf + interface->rbuf_offset,
-   hdr.bh_caplen);
-   interface->rbuf_offset += hdr.bh_caplen;
+   memcpy(buf, interface->rbuf + pktoff, hdr.bh_caplen);
+   interface->rbuf_offset += bpflen;
return (hdr.bh_caplen);
} while (!length);
return (0);