CVE-2020-5208: Buffer Copy without Checking Size of Input ('Classic Buffer Overflow') (CWE-120)
value: It's been found that multiple functions in ipmitool before 1.8.19 neglect proper checking of the data received from a remote LAN party, which may lead to buffer overflows and potentially to remote code execution on the ipmitool side. This is especially dangerous if ipmitool is run as a privileged user. This problem is fixed in version 1.8.19.
CWE description: The product copies an input buffer to an output buffer without verifying that the size of the input buffer is less than the size of the output buffer, leading to a buffer overflow.
CVSS v3 base score: 8.8
Published: 2020-02-05T14:15Z
Repository: https://github.com/ipmitool/ipmitool
Language: C
Files changed: lib/ipmi_fru.c
Commit date: 2020-02-04 14:58:06 +0300
Commit message: fru: Fix buffer overflow vulnerabilities

Partial fix for CVE-2020-5208, see
https://github.com/ipmitool/ipmitool/security/advisories/GHSA-g659-9qxw-p7cp

The `read_fru_area_section` function only performs size validation of
requested read size, and falsely assumes that the IPMI message will not
respond with more than the requested amount of data; it uses the
unvalidated response size to copy into `frubuf`. If the response is
larger than the request, this can result in overflowing the buffer.

The same issue affects the `read_fru_area` function.

Diff: diff --git a/lib/ipmi_fru.c b/lib/ipmi_fru.c
index c2a139d..2e323ff 100644
--- a/lib/ipmi_fru.c
+++ b/lib/ipmi_fru.c
@@ -661,11 +661,14 @@ write_fru_area(struct ipmi_intf * intf, struct fru_info *fru, uint8_t id,
 */
 int
 read_fru_area(struct ipmi_intf * intf, struct fru_info *fru, uint8_t id,
 			uint32_t offset, uint32_t length, uint8_t *frubuf)
 {
-	uint32_t off = offset, tmp, finish;
+	uint32_t off = offset;
+	uint32_t tmp;
+	uint32_t finish;
+	uint32_t size_left_in_buffer;
 	struct ipmi_rs * rsp;
 	struct ipmi_rq req;
 	uint8_t msg_data[4];
 
 	if (offset > fru->size) {
@@ -674,14 +677,16 @@ read_fru_area(struct ipmi_intf * intf, struct fru_info *fru, uint8_t id,
 		return -1;
 	}
 
 	finish = offset + length;
 	if (finish > fru->size) {
+		memset(frubuf + fru->size, 0, length - fru->size);
 		finish = fru->size;
 		lprintf(LOG_NOTICE, "Read FRU Area length %d too large, "
 			"Adjusting to %d",
 			offset + length, finish - offset);
+		length = finish - offset;
 	}
 
 	memset(&req, 0, sizeof(req));
 	req.msg.netfn = IPMI_NETFN_STORAGE;
 	req.msg.cmd = GET_FRU_DATA;
@@ -713,10 +718,11 @@ read_fru_area(struct ipmi_intf * intf, struct fru_info *fru, uint8_t id,
 		if (fru->access) {
 			fru->max_read_size &= ~1;
 		}
 	}
 
+	size_left_in_buffer = length;
 	do {
 		tmp = fru->access ? off >> 1 : off;
 		msg_data[0] = id;
 		msg_data[1] = (uint8_t)(tmp & 0xff);
 		msg_data[2] = (uint8_t)(tmp >> 8);
@@ -754,13 +760,22 @@ read_fru_area(struct ipmi_intf * intf, struct fru_info *fru, uint8_t id,
 				val2str(rsp->ccode, completion_code_vals));
 			break;
 		}
 
 		tmp = fru->access ? rsp->data[0] << 1 : rsp->data[0];
+		if(rsp->data_len < 1
+		   || tmp > rsp->data_len - 1
+		   || tmp > size_left_in_buffer)
+		{
+			printf(" Not enough buffer size");
+			return -1;
+		}
+
 		memcpy(frubuf, rsp->data + 1, tmp);
 		off += tmp;
 		frubuf += tmp;
+		size_left_in_buffer -= tmp;
 		/* sometimes the size returned in the Info command
 		* is too large.  return 0 so higher level function
 		* still attempts to parse what was returned */
 		if (tmp == 0 && off < finish) {
 			return 0;
@@ -789,11 +804,13 @@ read_fru_area(struct ipmi_intf * intf, struct fru_info *fru, uint8_t id,
 int
 read_fru_area_section(struct ipmi_intf * intf, struct fru_info *fru, uint8_t id,
 			uint32_t offset, uint32_t length, uint8_t *frubuf)
 {
 	static uint32_t fru_data_rqst_size = 20;
-	uint32_t off = offset, tmp, finish;
+	uint32_t off = offset;
+	uint32_t tmp, finish;
+	uint32_t size_left_in_buffer;
 	struct ipmi_rs * rsp;
 	struct ipmi_rq req;
 	uint8_t msg_data[4];
 
 	if (offset > fru->size) {
@@ -802,14 +819,16 @@ read_fru_area_section(struct ipmi_intf * intf, struct fru_info *fru, uint8_t id,
 		return -1;
 	}
 
 	finish = offset + length;
 	if (finish > fru->size) {
+		memset(frubuf + fru->size, 0, length - fru->size);
 		finish = fru->size;
 		lprintf(LOG_NOTICE, "Read FRU Area length %d too large, "
 			"Adjusting to %d",
 			offset + length, finish - offset);
+		length = finish - offset;
 	}
 
 	memset(&req, 0, sizeof(req));
 	req.msg.netfn = IPMI_NETFN_STORAGE;
 	req.msg.cmd = GET_FRU_DATA;
@@ -820,10 +839,12 @@ read_fru_area_section(struct ipmi_intf * intf, struct fru_info *fru, uint8_t id,
 	if (fru_data_rqst_size > 16)
 #else
 	if (fru->access && fru_data_rqst_size > 16)
 #endif
 		fru_data_rqst_size = 16;
+
+	size_left_in_buffer = length;
 	do {
 		tmp = fru->access ? off >> 1 : off;
 		msg_data[0] = id;
 		msg_data[1] = (uint8_t)(tmp & 0xff);
 		msg_data[2] = (uint8_t)(tmp >> 8);
@@ -851,12 +872,20 @@ read_fru_area_section(struct ipmi_intf * intf, struct fru_info *fru, uint8_t id,
 				val2str(rsp->ccode, completion_code_vals));
 			break;
 		}
 
 		tmp = fru->access ? rsp->data[0] << 1 : rsp->data[0];
+		if(rsp->data_len < 1
+		   || tmp > rsp->data_len - 1
+		   || tmp > size_left_in_buffer)
+		{
+			printf(" Not enough buffer size");
+			return -1;
+		}
 		memcpy((frubuf + off)-offset, rsp->data + 1, tmp);
 		off += tmp;
+		size_left_in_buffer -= tmp;
 
 		/* sometimes the size returned in the Info command
 		* is too large.  return 0 so higher level function
 		* still attempts to parse what was returned */
 		if (tmp == 0 && off < finish)

Vulnerable code: 	uint32_t off = offset, tmp, finish;
	uint32_t off = offset, tmp, finish;
Fixed code: 	uint32_t off = offset;
	uint32_t tmp;
	uint32_t finish;
	uint32_t size_left_in_buffer;
		memset(frubuf + fru->size, 0, length - fru->size);
		length = finish - offset;
	size_left_in_buffer = length;
		if(rsp->data_len < 1
		   || tmp > rsp->data_len - 1
		   || tmp > size_left_in_buffer)
		{
			printf(" Not enough buffer size");
			return -1;
		}

		size_left_in_buffer -= tmp;
	uint32_t off = offset;
	uint32_t tmp, finish;
	uint32_t size_left_in_buffer;
		memset(frubuf + fru->size, 0, length - fru->size);
		length = finish - offset;

	size_left_in_buffer = length;
		if(rsp->data_len < 1
		   || tmp > rsp->data_len - 1
		   || tmp > size_left_in_buffer)
		{
			printf(" Not enough buffer size");
			return -1;
		}
		size_left_in_buffer -= tmp;