From: James Jones <jejones3141@gmail.com>
Date: Wed, 24 Aug 2022 18:30:13 +0000 (-0500)
Subject: Make use of the fr_nbo_to*() functions where possible (#4691)
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=3cf1aa72a7aeb47f8c3b83377890d60189f64315;p=thirdparty%2Ffreeradius-server.git

Make use of the fr_nbo_to*() functions where possible (#4691)

It appears to appease coverity complaints about tainted data.
---

diff --git a/src/lib/eap/chbind.c b/src/lib/eap/chbind.c
index 5d4798017f2..01b893fe47c 100644
--- a/src/lib/eap/chbind.c
+++ b/src/lib/eap/chbind.c
@@ -149,7 +149,7 @@ static size_t chbind_get_data(chbind_packet_t const *packet,
 		 */
 		if ((end - ptr) < 4) return 0;
 
-		length = (ptr[0] << 8) | ptr[1];
+		length = fr_nbo_to_uint16(ptr);
 		if (length == 0) return 0;
 
 		if ((ptr + length + 3) > end) return 0;
diff --git a/src/lib/eap_aka_sim/decode.c b/src/lib/eap_aka_sim/decode.c
index 44d1df5b17e..ee29bad4794 100644
--- a/src/lib/eap_aka_sim/decode.c
+++ b/src/lib/eap_aka_sim/decode.c
@@ -317,7 +317,7 @@ static ssize_t sim_decode_array(TALLOC_CTX *ctx, fr_pair_list_t *out,
 	 *	length.
 	 */
 	if (!parent->flags.length || (parent->flags.length % 4)) {
-		actual_len = (p[0] << 8) | p[1];
+		actual_len = fr_nbo_to_uint16(p);
 		if (actual_len > (attr_len - 2)) {
 			fr_strerror_printf("%s: Actual length field value (%hu) > attribute value length (%zu)",
 					   __FUNCTION__, actual_len, attr_len - 2);
@@ -585,7 +585,7 @@ static ssize_t sim_decode_pair_value(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_di
 
 		if (attr_len < 2) goto raw;	/* Need at least two bytes for the length field */
 
-		res_len = (p[0] << 8) | p[1];
+		res_len = fr_nbo_to_uint16(p);
 		if (res_len % 8) {
 			fr_strerror_printf("%s: RES Length (%hu) is not a multiple of 8",
 					   __FUNCTION__, res_len);
@@ -737,7 +737,7 @@ static ssize_t sim_decode_pair_value(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_di
 	 */
 	case FR_TYPE_STRING:
 	{
-		uint16_t actual_len = (p[0] << 8) | p[1];
+		uint16_t actual_len = fr_nbo_to_uint16(p);
 
 		if (actual_len > (attr_len - 2)) {
 			fr_strerror_printf("%s: Actual length field value (%hu) > attribute value length (%zu)",
@@ -754,7 +754,7 @@ static ssize_t sim_decode_pair_value(TALLOC_CTX *ctx, fr_pair_list_t *out, fr_di
 		 *	Variable length octets buffer
 		 */
 		if (!parent->flags.length) {
-			uint16_t actual_len = (p[0] << 8) | p[1];
+			uint16_t actual_len = fr_nbo_to_uint16(p);
 
 			if (actual_len > (attr_len - prefix)) {
 				fr_strerror_printf("%s: Actual length field value (%hu) > attribute value length (%zu)",
diff --git a/src/lib/tls/session.c b/src/lib/tls/session.c
index 0921be31893..05207ad289d 100644
--- a/src/lib/tls/session.c
+++ b/src/lib/tls/session.c
@@ -818,7 +818,7 @@ void fr_tls_session_msg_cb(int write_p, int msg_version, int content_type,
 		if ((len >= 3) && (p[0] == 1)) {
 			size_t payload_len;
 
-			payload_len = (p[1] << 8) | p[2];
+			payload_len = fr_nbo_to_uint16(p + 1);
 			if ((payload_len + 3) > len) {
 				tls_session->invalid = true;
 				ROPTIONAL(REDEBUG, ERROR, "OpenSSL Heartbeat attack detected.  Closing connection");
diff --git a/src/lib/util/inet.c b/src/lib/util/inet.c
index 7f5c235768f..51494062ec6 100644
--- a/src/lib/util/inet.c
+++ b/src/lib/util/inet.c
@@ -1085,8 +1085,8 @@ char *fr_inet_ntop_prefix(char out[static FR_IPADDR_PREFIX_STRLEN], size_t outle
 char *fr_inet_ifid_ntop(char *out, size_t outlen, uint8_t const *ifid)
 {
 	snprintf(out, outlen, "%x:%x:%x:%x",
-		 (ifid[0] << 8) + ifid[1], (ifid[2] << 8) + ifid[3],
-		 (ifid[4] << 8) + ifid[5], (ifid[6] << 8) + ifid[7]);
+		 fr_nbo_to_uint16(ifid),     fr_nbo_to_uint16(ifid + 2),
+		 fr_nbo_to_uint16(ifid + 4), fr_nbo_to_uint16(ifid + 6));
 	return out;
 }
 
diff --git a/src/lib/util/missing.c b/src/lib/util/missing.c
index 9c5c4167370..10c2aa7b44a 100644
--- a/src/lib/util/missing.c
+++ b/src/lib/util/missing.c
@@ -463,14 +463,14 @@ char const *inet_ntop(int af, void const *src, char *dst, size_t cnt)
 		if (cnt <= INET6_ADDRSTRLEN) return NULL;
 
 		snprintf(dst, cnt, "%x:%x:%x:%x:%x:%x:%x:%x",
-			 (ipaddr->s6_addr[0] << 8) | ipaddr->s6_addr[1],
-			 (ipaddr->s6_addr[2] << 8) | ipaddr->s6_addr[3],
-			 (ipaddr->s6_addr[4] << 8) | ipaddr->s6_addr[5],
-			 (ipaddr->s6_addr[6] << 8) | ipaddr->s6_addr[7],
-			 (ipaddr->s6_addr[8] << 8) | ipaddr->s6_addr[9],
-			 (ipaddr->s6_addr[10] << 8) | ipaddr->s6_addr[11],
-			 (ipaddr->s6_addr[12] << 8) | ipaddr->s6_addr[13],
-			 (ipaddr->s6_addr[14] << 8) | ipaddr->s6_addr[15]);
+			 fr_nbo_to_uint16(ipaddr->a6_addr),
+			 fr_nbo_to_uint16(ipaddr->a6_addr + 2),
+			 fr_nbo_to_uint16(ipaddr->a6_addr + 4),
+			 fr_nbo_to_uint16(ipaddr->a6_addr + 6),
+			 fr_nbo_to_uint16(ipaddr->a6_addr + 8),
+			 fr_nbo_to_uint16(ipaddr->a6_addr + 10),
+			 fr_nbo_to_uint16(ipaddr->a6_addr + 12),
+			 fr_nbo_to_uint16(ipaddr->a6_addr + 14));
 		return dst;
 	}
 
diff --git a/src/listen/arp/proto_arp.c b/src/listen/arp/proto_arp.c
index 24d246fe659..a2a991a239f 100644
--- a/src/listen/arp/proto_arp.c
+++ b/src/listen/arp/proto_arp.c
@@ -79,7 +79,7 @@ static int mod_decode(UNUSED void const *instance, request_t *request, uint8_t *
 	}
 
 	arp = (fr_arp_packet_t const *) data;
-	request->packet->code = (arp->op[0] << 8) | arp->op[1];
+	request->packet->code = fr_nbo_to_uint16(arp->op);
 	fr_assert(request->packet->code < FR_ARP_CODE_MAX);
 
 	request->packet->data = talloc_memdup(request->packet, data, data_len);
diff --git a/src/listen/dhcpv4/proto_dhcpv4.c b/src/listen/dhcpv4/proto_dhcpv4.c
index 8146e64caa8..6b6e569ca61 100644
--- a/src/listen/dhcpv4/proto_dhcpv4.c
+++ b/src/listen/dhcpv4/proto_dhcpv4.c
@@ -212,7 +212,7 @@ static int mod_decode(void const *instance, request_t *request, uint8_t *const d
 	/*
 	 *	Hacks for now until we have a lower-level decode routine.
 	 */
-	request->packet->id = (data[4] << 24) | (data[5] << 16) | (data[6] << 8) | data[7];
+	request->packet->id = fr_nbo_to_uint32(data + 4);
 	request->reply->id = request->packet->id;
 	memcpy(request->packet->vector, data + 4, sizeof(request->packet->vector));
 
diff --git a/src/listen/dhcpv6/proto_dhcpv6_udp.c b/src/listen/dhcpv6/proto_dhcpv6_udp.c
index 50808ef32e8..d93420a8c3c 100644
--- a/src/listen/dhcpv6/proto_dhcpv6_udp.c
+++ b/src/listen/dhcpv6/proto_dhcpv6_udp.c
@@ -195,7 +195,7 @@ static ssize_t mod_read(fr_listen_t *li, void **packet_ctx, fr_time_t *recv_time
 	 *	proto_dhcpv6 sets the priority
 	 */
 
-	xid = (packet->transaction_id[0] << 16) | (packet->transaction_id[1] << 8) | packet->transaction_id[2];
+	xid = fr_nbo_to_uint24(packet->transaction_id);
 
 	/*
 	 *	Print out what we received.
@@ -398,7 +398,7 @@ static void *mod_track_create(UNUSED void const *instance, UNUSED void *thread_i
 		option = fr_dhcpv6_option_find(packet + 2 + 32, packet + packet_len, attr_relay_message->attr);
 		if (!option) return NULL;
 
-		option_len = (option[2] << 8) | option[3];
+		option_len = fr_nbo_to_uint16(option + 2);
 
 		packet = option + 4; /* skip option header */
 		packet_len = option_len;
@@ -412,7 +412,7 @@ static void *mod_track_create(UNUSED void const *instance, UNUSED void *thread_i
 	option = fr_dhcpv6_option_find(packet + 4, packet + packet_len, attr_client_id->attr);
 	if (!option) return NULL;
 
-	option_len = (option[2] << 8) | option[3];
+	option_len = fr_nbo_to_uint16(option + 2);
 
 	t = (proto_dhcpv6_track_t *) talloc_zero_array(track, uint8_t, t_size + option_len);
 	if (!t) return NULL;
diff --git a/src/listen/dns/proto_dns.c b/src/listen/dns/proto_dns.c
index 5d3621c780c..edce4b2c89b 100644
--- a/src/listen/dns/proto_dns.c
+++ b/src/listen/dns/proto_dns.c
@@ -195,7 +195,7 @@ static int mod_decode(void const *instance, request_t *request, uint8_t *const d
 	 *	@todo -
 	 */
 	request->packet->code = packet->opcode;
-	request->packet->id = (data[0] << 8) | data[1];
+	request->packet->id = fr_nbo_to_uint16(data);
 	request->reply->id = request->packet->id;
 
 	request->packet->data = talloc_memdup(request->packet, data, data_len);
diff --git a/src/listen/radius/proto_radius_tcp.c b/src/listen/radius/proto_radius_tcp.c
index a9f6c6400a3..56be90e75f9 100644
--- a/src/listen/radius/proto_radius_tcp.c
+++ b/src/listen/radius/proto_radius_tcp.c
@@ -151,7 +151,7 @@ static ssize_t mod_read(fr_listen_t *li, UNUSED void **packet_ctx, fr_time_t *re
 	/*
 	 *	Figure out how large the RADIUS packet is.
 	 */
-	packet_len = (buffer[2] << 8) | buffer[3];
+	packet_len = fr_nbo_to_uint16(buffer + 2);
 
 	/*
 	 *	We don't have a complete RADIUS packet.  Tell the
diff --git a/src/modules/rlm_eap/types/rlm_eap_mschapv2/rlm_eap_mschapv2.c b/src/modules/rlm_eap/types/rlm_eap_mschapv2/rlm_eap_mschapv2.c
index 621794653bd..83b61abaea0 100644
--- a/src/modules/rlm_eap/types/rlm_eap_mschapv2/rlm_eap_mschapv2.c
+++ b/src/modules/rlm_eap/types/rlm_eap_mschapv2/rlm_eap_mschapv2.c
@@ -622,7 +622,7 @@ failure:
 	 *	The MS-Length field is 5 + value_size + length
 	 *	of name, which is put after the response.
 	 */
-	length = (eap_round->response->type.data[2] << 8) | eap_round->response->type.data[3];
+	length = fr_nbo_to_uint16(eap_round->response->type.data + 2);
 	if ((length < (5 + 49)) || (length > (256 + 5 + 49))) {
 		REDEBUG("Response contains contradictory length %zu %d", length, 5 + 49);
 		RETURN_MODULE_INVALID;
diff --git a/src/modules/rlm_radius/rlm_radius_udp.c b/src/modules/rlm_radius/rlm_radius_udp.c
index eb664d2ac5a..6cbb0dbbb08 100644
--- a/src/modules/rlm_radius/rlm_radius_udp.c
+++ b/src/modules/rlm_radius/rlm_radius_udp.c
@@ -2185,7 +2185,7 @@ static void protocol_error_reply(udp_request_t *u, udp_result_t *r, udp_handle_t
 	uint32_t  	response_length = 0;
 	uint8_t const	*attr, *end;
 
-	end = h->buffer + ((h->buffer[2] << 8) | h->buffer[3]);
+	end = h->buffer + fr_nbo_to_uint16(h->buffer + 2);
 
 	for (attr = h->buffer + RADIUS_HEADER_LENGTH;
 	     attr < end;
diff --git a/src/modules/rlm_wimax/rlm_wimax.c b/src/modules/rlm_wimax/rlm_wimax.c
index a729020928c..77a1cbf4f77 100644
--- a/src/modules/rlm_wimax/rlm_wimax.c
+++ b/src/modules/rlm_wimax/rlm_wimax.c
@@ -240,8 +240,7 @@ static unlang_action_t CC_HINT(nonnull) mod_post_auth(rlm_rcode_t *p_result, mod
 	 *	Take the 4 most significant octets.
 	 *	If less than 256, add 256.
 	 */
-	mip_spi = ((mip_rk_1[0] << 24) | (mip_rk_1[1] << 16) |
-		   (mip_rk_1[2] << 8) | mip_rk_1[3]);
+	mip_spi = fr_nbo_to_uint32(mip_rk_1);
 	if (mip_spi < 256) mip_spi += 256;
 
 	REDEBUG2("MIP-RK = 0x%pH", fr_box_octets(mip_rk, rk_len));
diff --git a/src/protocols/radius/base.c b/src/protocols/radius/base.c
index afcc9b92af8..8f7efdc95ad 100644
--- a/src/protocols/radius/base.c
+++ b/src/protocols/radius/base.c
@@ -278,7 +278,7 @@ int fr_radius_sign(uint8_t *packet, uint8_t const *original,
 		   uint8_t const *secret, size_t secret_len)
 {
 	uint8_t		*msg, *end;
-	size_t		packet_len = (packet[2] << 8) | packet[3];
+	size_t		packet_len = fr_nbo_to_uint16(packet + 2);
 
 	/*
 	 *	No real limit on secret length, this is just
@@ -465,7 +465,7 @@ bool fr_radius_ok(uint8_t const *packet, size_t *packet_len_p,
 	 *	i.e. We've received 128 bytes, and the packet header
 	 *	says it's 256 bytes long.
 	 */
-	totallen = (packet[2] << 8) | packet[3];
+	totallen = fr_nbo_to_uint16(packet + 2);
 
 	/*
 	 *	Code of 0 is not understood.
@@ -710,7 +710,7 @@ int fr_radius_verify(uint8_t *packet, uint8_t const *original,
 	bool found_ma;
 	int rcode;
 	uint8_t *msg, *end;
-	size_t packet_len = (packet[2] << 8) | packet[3];
+	size_t packet_len = fr_nbo_to_uint16(packet + 2);
 	uint8_t request_authenticator[RADIUS_AUTH_VECTOR_LENGTH];
 	uint8_t message_authenticator[RADIUS_AUTH_VECTOR_LENGTH];
 
diff --git a/src/protocols/radius/packet.c b/src/protocols/radius/packet.c
index 7cbedb3644a..25a94199a46 100644
--- a/src/protocols/radius/packet.c
+++ b/src/protocols/radius/packet.c
@@ -531,7 +531,7 @@ void _fr_radius_packet_log_hex(fr_log_t const *log, fr_radius_packet_t const *pa
        }
 
        fr_log(log, L_DBG, file, line, "  Id       : %u", packet->data[1]);
-       fr_log(log, L_DBG, file, line, "  Length   : %u", ((packet->data[2] << 8) | (packet->data[3])));
+       fr_log(log, L_DBG, file, line, "  Length   : %u", fr_nbo_to_uint16(packet->data + 2));
        fr_log(log, L_DBG, file, line, "  Vector   : %pH", fr_box_octets(packet->data + 4, RADIUS_AUTH_VECTOR_LENGTH));
 
        if (packet->data_len <= 20) return;
@@ -552,7 +552,7 @@ void _fr_radius_packet_log_hex(fr_log_t const *log, fr_radius_packet_t const *pa
 	       p = buffer + strlen(buffer);
                if ((attr[0] == FR_VENDOR_SPECIFIC) &&
                    (attr[1] > 6)) {
-                       vendor = (attr[2] << 24) | (attr[3] << 16) | (attr[4] << 8) | attr[5];
+                       vendor = fr_nbo_to_uint32(attr + 2);
 
 		       snprintf(p, buffer + sizeof(buffer) - p, "%02x%02x%02x%02x (%u)  ",
 				attr[2], attr[3], attr[4], attr[5], vendor);
diff --git a/src/protocols/radius/tcp.c b/src/protocols/radius/tcp.c
index d63306e4c28..74eea4a40ff 100644
--- a/src/protocols/radius/tcp.c
+++ b/src/protocols/radius/tcp.c
@@ -88,7 +88,7 @@ int fr_tcp_read_packet(fr_radius_packet_t *packet, uint32_t max_attributes, bool
 			return 0;
 		}
 
-		packet_len = (packet->vector[2] << 8) | packet->vector[3];
+		packet_len = fr_nbo_to_uint16(packet->vector + 2);
 
 		if (packet_len < RADIUS_HEADER_LENGTH) {
 			fr_strerror_const("Discarding packet: Smaller than RFC minimum of 20 bytes");
diff --git a/src/protocols/vmps/vmps.c b/src/protocols/vmps/vmps.c
index b0246b65754..7ca5692527d 100644
--- a/src/protocols/vmps/vmps.c
+++ b/src/protocols/vmps/vmps.c
@@ -119,13 +119,12 @@ bool fr_vmps_ok(uint8_t const *packet, size_t *packet_len)
 		 *
 		 *	It's OK for ethernet frames to be longer.
 		 */
-		if ((ptr[3] != 5) &&
-		    ((ptr[4] != 0) || (ptr[5] > 250))) {
+		attrlen = fr_nbo_to_uint16(ptr + 4);
+		if ((ptr[3] != 5) && (attrlen > 250)) {
 			fr_strerror_printf("Packet contains attribute with invalid length %02x %02x", ptr[4], ptr[5]);
 			return false;
 		}
 
-		attrlen = (ptr[4] << 8) | ptr[5];
 		ptr += 6 + attrlen;
 		data_len -= (6 + attrlen);
 	}
@@ -189,8 +188,8 @@ int fr_vmps_decode(TALLOC_CTX *ctx, fr_pair_list_t *out, uint8_t const *data, si
 			return -1;
 		}
 
-		attr = (ptr[2] << 8) | ptr[3];
-		attr_len = (ptr[4] << 8) | ptr[5];
+		attr = fr_nbo_to_uint16(ptr + 2);
+		attr_len = fr_nbo_to_uint16(ptr + 4);
 		ptr += 6;
 
 		/*
@@ -401,7 +400,7 @@ ssize_t fr_vmps_packet_size(uint8_t const *data, size_t data_len)
 		/*
 		 *	Length of the data NOT including the header.
 		 */
-		attr_len = (ptr[4] << 8) | ptr[5];
+		attr_len = fr_nbo_to_uint16(ptr + 4);
 
 		ptr += 6 + attr_len;
 
@@ -490,7 +489,7 @@ void fr_vmps_print_hex(FILE *fp, uint8_t const *packet, size_t packet_len)
 		memcpy(&id, attr, 4);
 		id = ntohl(id);
 
-		length = (attr[4] << 8) | attr[5];
+		length = fr_nbo_to_uint16(attr + 4);
 		if ((attr + length) > end) break;
 
 		fprintf(fp, "\t\t%08x  %04x  ", id, length);
diff --git a/src/tests/util/radius1_test.c b/src/tests/util/radius1_test.c
index 147445114a5..a65a981df4e 100644
--- a/src/tests/util/radius1_test.c
+++ b/src/tests/util/radius1_test.c
@@ -373,7 +373,7 @@ static void master_process(TALLOC_CTX *ctx)
 				goto discard;
 			}
 
-			total_len = (packet[2] << 8) | packet[3];
+			total_len = fr_nbo_to_uint16(packet + 2);
 			if (total_len < 20) {
 				MPRINT1("Master ignoring packet (header length %zu)\n", total_len);
 				goto discard;