aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRuss Dill <russ.dill@gmail.com>2023-10-02 12:34:50 -0700
committerDenys Vlasenko <vda.linux@googlemail.com>2023-10-04 16:46:35 +0200
commite265c8d4c039729f2a68f3b1fb589c13c38d86f8 (patch)
tree0397500be8956ab8d54df82ab8a91f3d03a91486
parent5fa39d48d53c7a3360d4f4da2c00232eb674678e (diff)
downloadbusybox-w32-e265c8d4c039729f2a68f3b1fb589c13c38d86f8.tar.gz
busybox-w32-e265c8d4c039729f2a68f3b1fb589c13c38d86f8.tar.bz2
busybox-w32-e265c8d4c039729f2a68f3b1fb589c13c38d86f8.zip
udhcp: Avoid leaking uninitialized/stale data
I noticed a commit in connman: "gdhcp: Avoid leaking stack data via unitiialized variable" [1] Since gdhcp is just BusyBox udhcp with the serial numbers filed off, I checked if BusyBox udhcp has a related issue. The issue is that the get_option logic assumes any data within the memory area of the buffer is "valid". This reduces the complexity of the function at the cost of reading past the end of the actually received data in the case of specially crafted packets. This is not a problem for the udhcp_recv_kernel_packet data path as the entire memory area is zeroed. However, d4/d6_recv_raw_packet does not zero the memory. Note that a related commit [2] is not required as we are zeroing any data that can be read by the get_option function. [1] https://git.kernel.org/pub/scm/network/connman/connman.git/commit/?id=a74524b3e3fad81b0fd1084ffdf9f2ea469cd9b1 [2] https://git.kernel.org/pub/scm/network/connman/connman.git/commit/?id=58d397ba74873384aee449690a9070bacd5676fa function old new delta d4_recv_raw_packet 484 497 +13 d6_recv_raw_packet 216 228 +12 .rodata 105390 105381 -9 ------------------------------------------------------------------------------ (add/remove: 0/0 grow/shrink: 2/1 up/down: 25/-9) Total: 16 bytes Signed-off-by: Russ Dill <russ.dill@gmail.com> Cc: Colin Wee <cwee@tesla.com> Cc: Denys Vlasenko <vda.linux@googlemail.com> Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
-rw-r--r--networking/udhcp/d6_dhcpc.c1
-rw-r--r--networking/udhcp/dhcpc.c3
2 files changed, 3 insertions, 1 deletions
diff --git a/networking/udhcp/d6_dhcpc.c b/networking/udhcp/d6_dhcpc.c
index cdd06188e..a72fd31bd 100644
--- a/networking/udhcp/d6_dhcpc.c
+++ b/networking/udhcp/d6_dhcpc.c
@@ -961,6 +961,7 @@ static NOINLINE int d6_recv_raw_packet(struct in6_addr *peer_ipv6, struct d6_pac
961 d6_dump_packet(&packet.data); 961 d6_dump_packet(&packet.data);
962 962
963 bytes -= sizeof(packet.ip6) + sizeof(packet.udp); 963 bytes -= sizeof(packet.ip6) + sizeof(packet.udp);
964 memset(d6_pkt, 0, sizeof(*d6_pkt));
964 memcpy(d6_pkt, &packet.data, bytes); 965 memcpy(d6_pkt, &packet.data, bytes);
965 return bytes; 966 return bytes;
966} 967}
diff --git a/networking/udhcp/dhcpc.c b/networking/udhcp/dhcpc.c
index 200a2fb8a..07e2eadfe 100644
--- a/networking/udhcp/dhcpc.c
+++ b/networking/udhcp/dhcpc.c
@@ -966,7 +966,7 @@ static NOINLINE int d4_recv_raw_packet(struct dhcp_packet *dhcp_pkt, int fd)
966 check = packet.udp.check; 966 check = packet.udp.check;
967 packet.udp.check = 0; 967 packet.udp.check = 0;
968 if (check && check != inet_cksum(&packet, bytes)) { 968 if (check && check != inet_cksum(&packet, bytes)) {
969 log1s("packet with bad UDP checksum received, ignoring"); 969 log1s("packet with bad UDP checksum, ignoring");
970 return -2; 970 return -2;
971 } 971 }
972 skip_udp_sum_check: 972 skip_udp_sum_check:
@@ -981,6 +981,7 @@ static NOINLINE int d4_recv_raw_packet(struct dhcp_packet *dhcp_pkt, int fd)
981 udhcp_dump_packet(&packet.data); 981 udhcp_dump_packet(&packet.data);
982 982
983 bytes -= sizeof(packet.ip) + sizeof(packet.udp); 983 bytes -= sizeof(packet.ip) + sizeof(packet.udp);
984 memset(dhcp_pkt, 0, sizeof(*dhcp_pkt));
984 memcpy(dhcp_pkt, &packet.data, bytes); 985 memcpy(dhcp_pkt, &packet.data, bytes);
985 return bytes; 986 return bytes;
986} 987}