osmith has submitted this change. (
https://gerrit.osmocom.org/c/osmo-hlr/+/42888?usp=email )
Change subject: mslookup: fix size_t underflow in mDNS decode
......................................................................
mslookup: fix size_t underflow in mDNS decode
osmo_mdns_rfc_record_decode() computes the strnlen() scan bound as
'data_len - 10' where data_len is a size_t. osmo_mdns_msg_answer_decode()
loops "while (data_len)" with no minimum-length guard, so it can call
record_decode() with data_len in 1..9. 'data_len - 10' then underflows
to ~SIZE_MAX and strnlen() scans far past the receive buffer until it
finds a NUL, and data[name_len] reads further still, before the trailing
"name_len + 10 + rdlength > data_len" check ever runs.
A record must contain at least one name byte plus the 10 fixed trailing
bytes (type, class, ttl, rdlength); reject anything shorter so the
subtraction cannot underflow.
Additionally, an unterminated name still let the fixed-field loads read
one byte past the buffer: strnlen() can return its bound, making name_len
equal to data_len - 9, so osmo_load16be(data + name_len + 8) touched
data[data_len] before the trailing length check ran. Require room for the
name plus the 10 fixed trailing bytes before reading any of them.
Change-Id: I1bf5bade953217b1a998f91679711a6170a886a8
---
M src/mslookup/mdns_rfc.c
1 file changed, 13 insertions(+), 0 deletions(-)
Approvals:
pespin: Looks good to me, but someone else must approve
osmith: Looks good to me, approved
Jenkins Builder: Verified
diff --git a/src/mslookup/mdns_rfc.c b/src/mslookup/mdns_rfc.c
index 8a8cdec..886b4f6 100644
--- a/src/mslookup/mdns_rfc.c
+++ b/src/mslookup/mdns_rfc.c
@@ -156,10 +156,23 @@
struct osmo_mdns_rfc_record *ret;
size_t name_len;
+ /* A record needs at least one name byte plus the 10 fixed trailing
+ * bytes (type, class, ttl, rdlength). Reject anything shorter, so the
+ * unsigned 'data_len - 10' below cannot underflow to ~SIZE_MAX. */
+ if (data_len < 11)
+ return NULL;
+
/* name length: represented as a series of labels, and terminated by a
* label with zero length (RFC 1035 3.3). A label with zero length is a
* NUL byte. */
name_len = strnlen((const char *)data, data_len - 10) + 1;
+ /* If no label terminator was found within the scanned range, strnlen()
+ * returns its bound, so name_len can be as large as data_len - 9. The
+ * fixed-field loads below read up to data[name_len + 9], which would
then
+ * be one byte past the buffer. Require room for the name plus the 10
+ * fixed trailing bytes before touching any of them. */
+ if (name_len + 10 > data_len)
+ return NULL;
if (data[name_len])
return NULL;
--
To view, visit https://gerrit.osmocom.org/c/osmo-hlr/+/42888?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: merged
Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-Change-Id: I1bf5bade953217b1a998f91679711a6170a886a8
Gerrit-Change-Number: 42888
Gerrit-PatchSet: 4
Gerrit-Owner: n0k0 <[email protected]>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <[email protected]>
Gerrit-Reviewer: neels <[email protected]>
Gerrit-Reviewer: osmith <[email protected]>
Gerrit-Reviewer: pespin <[email protected]>
Gerrit-CC: laforge <[email protected]>
