laforge has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/43234?usp=email )

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

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(-)

Approvals: laforge: Looks good to me, but someone else must approve osmith: Looks good to me, approved fixeria: Looks good to me, but someone else must approve Jenkins Builder: Verified

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; }