Merge pull request #361 from nuclearcat/fix-protocol-buffer-access

Fix several unsafe or unaligned integer accesses found in protocol parsing paths, including option-gated MPPE, DHCP, PPPoE, RADIUS, IPCP and IPv6CP code.

3 files changed, 61 insertions, 29 deletions

diff --git a/accel-pppd/ctrl/ipoe/dhcpv4.c b/accel-pppd/ctrl/ipoe/dhcpv4.c
index d80a80f3..f1427e17 100644
--- a/accel-pppd/ctrl/ipoe/dhcpv4.c
+++ b/accel-pppd/ctrl/ipoe/dhcpv4.c
@@ -349,6 +349,12 @@ static int dhcpv4_parse_packet(struct dhcpv4_packet *pack, int len)
 
 		list_add_tail(&opt->entry, &pack->options);
 
+	}
+
+	if (dhcpv4_check_options(pack))
+		return -1;
+
+	list_for_each_entry(opt, &pack->options, entry) {
 		if (opt->type == 53)
 			pack->msg_type = opt->data[0];
 		else if (opt->type == 82)
@@ -356,17 +362,14 @@ static int dhcpv4_parse_packet(struct dhcpv4_packet *pack, int len)
 		else if (opt->type == 62)
 			pack->client_id = opt;
 		else if (opt->type == 50)
-			memcpy(&pack->request_ip, opt->data, 4);
+			memcpy(&pack->request_ip, opt->data, sizeof(pack->request_ip));
 		else if (opt->type == 54)
-			memcpy(&pack->server_id, opt->data, 4);
+			memcpy(&pack->server_id, opt->data, sizeof(pack->server_id));
 	}
 
 	if (pack->msg_type == 0 || pack->msg_type > 8)
 		return -1;
 
-	if (dhcpv4_check_options(pack))
-		return -1;
-
 	pack->ptr = ptr;
 
 	/*if (conf_verbose) {
@@ -933,6 +936,7 @@ void dhcpv4_send_notify(struct dhcpv4_serv *serv, struct dhcpv4_packet *req, uns
 {
 	struct dhcpv4_packet *pack = dhcpv4_packet_alloc();
 	uint8_t opt[8 + ETH_ALEN];
+	uint32_t value;
 
 	if (!pack) {
 		log_emerg("out of memory\n");
@@ -946,8 +950,10 @@ void dhcpv4_send_notify(struct dhcpv4_serv *serv, struct dhcpv4_packet *req, uns
 	pack->hdr->siaddr = 0;
 	pack->hdr->giaddr = 0;
 
-	*(uint32_t *)opt = htonl(ACCEL_PPP_MAGIC);
-	*(uint32_t *)(opt + 4) = htonl(weight);
+	value = htonl(ACCEL_PPP_MAGIC);
+	memcpy(opt, &value, sizeof(value));
+	value = htonl(weight);
+	memcpy(opt + sizeof(value), &value, sizeof(value));
 	memcpy(opt + 8, serv->hwaddr, ETH_ALEN);
 
 	dhcpv4_packet_add_opt_u8(pack, 53, DHCPDISCOVER);
@@ -1100,8 +1106,8 @@ int dhcpv4_relay_send(struct dhcpv4_relay *relay, struct dhcpv4_packet *request,
 	if (server_id) {
 		opt = dhcpv4_packet_find_opt(request, 54);
 		if (opt) {
-			_server_id = *(uint32_t *)opt->data;
-			*(uint32_t *)opt->data = server_id;
+			memcpy(&_server_id, opt->data, sizeof(_server_id));
+			memcpy(opt->data, &server_id, sizeof(server_id));
 		}
 	}
 
@@ -1123,7 +1129,7 @@ int dhcpv4_relay_send(struct dhcpv4_relay *relay, struct dhcpv4_packet *request,
 	request->hdr->giaddr = giaddr;
 
 	if (opt)
-		*(uint32_t *)opt->data = _server_id;
+		memcpy(opt->data, &_server_id, sizeof(_server_id));
 
 	if (n != len)
 		return -1;
diff --git a/accel-pppd/ctrl/ipoe/dhcpv4_options.c b/accel-pppd/ctrl/ipoe/dhcpv4_options.c
index 042a7406..fe6c2a5f 100644
--- a/accel-pppd/ctrl/ipoe/dhcpv4_options.c
+++ b/accel-pppd/ctrl/ipoe/dhcpv4_options.c
@@ -205,7 +205,10 @@ static void print_message_type(const struct dhcpv4_option *opt, int elem_size, v
 {
 	const char *msg_name[] = {"", "Discover", "Offer", "Request", "Decline", "Ack", "Nak", "Release", "Inform"};
 
-	print("%s", msg_name[opt->data[0]]);
+	if (opt->data[0] < sizeof(msg_name) / sizeof(msg_name[0]))
+		print("%s", msg_name[opt->data[0]]);
+	else
+		print("%u", opt->data[0]);
 }
 
 static void print_request_list(const struct dhcpv4_option *opt, int elem_size, void (*print)(const char *fmt, ...))
@@ -232,14 +235,19 @@ static void print_relay_agent(const struct dhcpv4_option *opt, int elem_size, vo
 	int type, len;
 
 	while (ptr < endptr) {
+		if (endptr - ptr < 2) {
+			print("invalid");
+			return;
+		}
+
 		if (ptr != opt->data)
 			print(" ");
 		type = *ptr++;
 		len = *ptr++;
-		/*if (ptr + len > endptr) {
+		if (endptr - ptr < len) {
 			print(" invalid");
 			return;
-		}*/
+		}
 		if (type == 1)
 			print("{Agent-Circuit-ID ");
 		else if (type == 2)
diff --git a/accel-pppd/ctrl/ipoe/ipoe.c b/accel-pppd/ctrl/ipoe/ipoe.c
index 1e3f7054..e7b1320f 100644
--- a/accel-pppd/ctrl/ipoe/ipoe.c
+++ b/accel-pppd/ctrl/ipoe/ipoe.c
@@ -1835,6 +1835,7 @@ static int check_notify(struct ipoe_serv *serv, struct dhcpv4_packet *pack)
 	struct dhcpv4_option *opt = dhcpv4_packet_find_opt(pack, 43);
 	struct ipoe_session *ses;
 	unsigned int w;
+	uint32_t value;
 
 	if (!opt)
 		return 0;
@@ -1842,10 +1843,12 @@ static int check_notify(struct ipoe_serv *serv, struct dhcpv4_packet *pack)
 	if (opt->len != 8 + ETH_ALEN)
 		return 0;
 
-	if (*(uint32_t *)opt->data != htonl(ACCEL_PPP_MAGIC))
+	memcpy(&value, opt->data, sizeof(value));
+	if (value != htonl(ACCEL_PPP_MAGIC))
 		return 0;
 
-	w = htonl(*(uint32_t *)(opt->data + 4));
+	memcpy(&value, opt->data + sizeof(value), sizeof(value));
+	w = ntohl(value);
 
 	list_for_each_entry(ses, &serv->sessions, entry) {
 		if (ses->xid == pack->hdr->xid && memcmp(pack->hdr->chaddr, ses->hwaddr, ETH_ALEN) == 0) {
@@ -2029,6 +2032,7 @@ static void ipoe_ses_recv_dhcpv4_relay(struct dhcpv4_packet *pack)
 {
 	struct ipoe_session *ses = container_of(triton_context_self(), typeof(*ses), ctx);
 	struct dhcpv4_option *opt;
+	uint32_t value;
 
 	if (ses->dhcpv4_relay_reply)
 		dhcpv4_packet_free(ses->dhcpv4_relay_reply);
@@ -2047,24 +2051,32 @@ static void ipoe_ses_recv_dhcpv4_relay(struct dhcpv4_packet *pack)
 	}
 
 	opt = dhcpv4_packet_find_opt(pack, 51);
-	if (opt)
-		ses->lease_time = ntohl(*(uint32_t *)opt->data);
+	if (opt) {
+		memcpy(&value, opt->data, sizeof(value));
+		ses->lease_time = ntohl(value);
+	}
 
 	opt = dhcpv4_packet_find_opt(pack, 58);
-	if (opt)
-		ses->renew_time = ntohl(*(uint32_t *)opt->data);
+	if (opt) {
+		memcpy(&value, opt->data, sizeof(value));
+		ses->renew_time = ntohl(value);
+	}
 
 	opt = dhcpv4_packet_find_opt(pack, 59);
-	if (opt)
-		ses->rebind_time = ntohl(*(uint32_t *)opt->data);
+	if (opt) {
+		memcpy(&value, opt->data, sizeof(value));
+		ses->rebind_time = ntohl(value);
+	}
 
 	opt = dhcpv4_packet_find_opt(pack, 1);
-	if (opt)
-		ses->mask = parse_dhcpv4_mask(ntohl(*(uint32_t *)opt->data));
+	if (opt) {
+		memcpy(&value, opt->data, sizeof(value));
+		ses->mask = parse_dhcpv4_mask(ntohl(value));
+	}
 
 	opt = dhcpv4_packet_find_opt(pack, 3);
 	if (opt)
-		ses->router = *(uint32_t *)opt->data;
+		memcpy(&ses->router, opt->data, sizeof(ses->router));
 
 	if (pack->msg_type == DHCPOFFER) {
 		if (ses->ses.state == AP_STATE_STARTING) {
@@ -2409,7 +2421,7 @@ static void ev_radius_access_accept(struct ev_radius_t *ev)
 					ses->siaddr = attr->val.ipaddr;
 					break;
 				case DHCP_Router_Address:
-					ses->router = *(in_addr_t *)attr->raw;
+					memcpy(&ses->router, attr->raw, sizeof(ses->router));
 					break;
 				case DHCP_Subnet_Mask:
 					ses->mask = ipaddr_to_prefix(attr->val.ipaddr);
@@ -2977,32 +2989,38 @@ static void ipoe_serv_timeout(struct triton_timer_t *t)
 static void ipoe_ipv6_enable(struct ipoe_serv *serv)
 {
 	struct ifreq ifr;
+	uint32_t addr;
 
 	strcpy(ifr.ifr_name, serv->ifname);
 
 	ifr.ifr_hwaddr.sa_family = AF_UNSPEC;
 	ifr.ifr_hwaddr.sa_data[0] = 0x33;
 	ifr.ifr_hwaddr.sa_data[1] = 0x33;
-	*(uint32_t *)(ifr.ifr_hwaddr.sa_data + 2) = htonl(0x02);
+	addr = htonl(0x02);
+	memcpy(ifr.ifr_hwaddr.sa_data + 2, &addr, sizeof(addr));
 	ioctl(sock_fd, SIOCADDMULTI, &ifr);
 
-	*(uint32_t *)(ifr.ifr_hwaddr.sa_data + 2) = htonl(0x010002);
+	addr = htonl(0x010002);
+	memcpy(ifr.ifr_hwaddr.sa_data + 2, &addr, sizeof(addr));
 	ioctl(sock_fd, SIOCADDMULTI, &ifr);
 }
 
 static void ipoe_ipv6_disable(struct ipoe_serv *serv)
 {
 	struct ifreq ifr;
+	uint32_t addr;
 
 	strcpy(ifr.ifr_name, serv->ifname);
 
 	ifr.ifr_hwaddr.sa_family = AF_UNSPEC;
 	ifr.ifr_hwaddr.sa_data[0] = 0x33;
 	ifr.ifr_hwaddr.sa_data[1] = 0x33;
-	*(uint32_t *)(ifr.ifr_hwaddr.sa_data + 2) = htonl(0x02);
+	addr = htonl(0x02);
+	memcpy(ifr.ifr_hwaddr.sa_data + 2, &addr, sizeof(addr));
 	ioctl(sock_fd, SIOCDELMULTI, &ifr);
 
-	*(uint32_t *)(ifr.ifr_hwaddr.sa_data + 2) = htonl(0x010002);
+	addr = htonl(0x010002);
+	memcpy(ifr.ifr_hwaddr.sa_data + 2, &addr, sizeof(addr));
 	ioctl(sock_fd, SIOCDELMULTI, &ifr);
 }