laforge submitted this change.

View Change

iuup: Avoid stack buffer-overflow rx IuUP with payload >1024 bytes

While verifying the IuUP payload CRC, osmo_pbit2ubit() is called where
an out buffer of fixed size is passed and written to.
Since there was no check validating the received IuUP payload would fit
in that buffer, it could happen that osmo_pbit2ubit() would write past
the stack buffer.

* Make sure the PDU payload size is validated to be smaller than the buf
* Increase a the buf size to 1500 to make sure all UDP/RTP/IuUP payloads
  on regular ethernet frames can fit just well.

Related: OS#7052
Reported-By: Adam Bedard <adam.bedard@gmail.com>
Change-Id: Id71d3963649f8f711b1584daec3508951ebf462d
---
M src/core/bits.c
M src/gsm/iuup.c
2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/src/core/bits.c b/src/core/bits.c
index 8616d32..a4c0970 100644
--- a/src/core/bits.c
+++ b/src/core/bits.c
@@ -139,6 +139,8 @@
  *  \param[in] in input buffer of packed bits
  *  \param[in] num_bits number of bits
  *  \return number of bytes used in \ref out
+ *
+ *  Note: size of out array is expected to be ">= num_bits" bytes.
  */
 int osmo_pbit2ubit(ubit_t *out, const pbit_t *in, unsigned int num_bits)
 {
diff --git a/src/gsm/iuup.c b/src/gsm/iuup.c
index 0952c5e..622302a 100644
--- a/src/gsm/iuup.c
+++ b/src/gsm/iuup.c
@@ -70,9 +70,12 @@
 
 int osmo_iuup_compute_payload_crc(const uint8_t *iuup_pdu, unsigned int pdu_len)
 {
-	ubit_t buf[1024*8];
+	/* Assume no IuUP payloads bigger than a regular ethernet frame: */
+	const unsigned int max_supported_iuup_payload_len_bytes = 1500;
+	ubit_t buf[max_supported_iuup_payload_len_bytes * 8];
 	uint8_t pdu_type;
-	int offset, payload_len_bytes;
+	int offset;
+	unsigned int payload_len_bytes;
 
 	if (pdu_len < 1)
 		return -1;
@@ -91,6 +94,11 @@
 		return -1;
 
 	payload_len_bytes = pdu_len - offset;
+
+	/* Guard against osmo_pbit2ubit writing past buf: */
+	if (payload_len_bytes > max_supported_iuup_payload_len_bytes)
+		return -1;
+
 	osmo_pbit2ubit(buf, iuup_pdu+offset, payload_len_bytes*8);
 	return osmo_crc16gen_compute_bits(&iuup_data_crc_code, buf, payload_len_bytes*8);
 }
@@ -887,6 +895,8 @@
 		t0h = (struct iuup_pdutype0_hdr *) data;
 		payload_crc = ((uint16_t)t0h->payload_crc_hi << 8) | t0h->payload_crc_lo;
 		payload_crc_computed = osmo_iuup_compute_payload_crc(data, len);
+		if (payload_crc_computed < 0)
+			goto payload_crc_err;
 		if (payload_crc != payload_crc_computed)
 			goto payload_crc_err;
 		break;
@@ -901,6 +911,8 @@
 		if (t14h->ack_nack == IUUP_AN_PROCEDURE) {
 			payload_crc = ((uint16_t)t14h->payload_crc_hi << 8) | t14h->payload_crc_lo;
 			payload_crc_computed = osmo_iuup_compute_payload_crc(data, len);
+			if (payload_crc_computed < 0)
+				goto payload_crc_err;
 			if (payload_crc != payload_crc_computed)
 				goto payload_crc_err;
 		}

To view, visit change 43234. To unsubscribe, or for help writing mail filters, visit settings.