fixeria has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/43235?usp=email )
Change subject: gsm0480: fix out-of-bounds reads in parse_ss_{invoke,return_result}() ......................................................................
gsm0480: fix out-of-bounds reads in parse_ss_{invoke,return_result}()
Both Facility component parsers checked 'offset' against 'length' one (or two) bytes short of the index they went on to dereference, and parse_ss_invoke() never re-validated 'offset' after skipping the optional Linked ID, whose skip length is attacker-controlled. The same off-by-one on the operation-code path also let "length - offset - 3" underflow a uint16_t, handing parse_process_uss_req() a bogus, oversized length that bypassed its own bounds check and grew the over-read into a memcpy() into req->ussd_data/req->ussd_text.
Tighten each guard to cover the index actually dereferenced, and re-check 'offset' against 'length' after the Linked ID skip and after the post-SEQUENCE-tag increment in parse_ss_return_result().
Change-Id: I59fe4df8045dbe1e2b9509330527408b84abf2e4 Reported-By: Adam Bedard adam.bedard@gmail.com Fixes: OS#7051 --- M src/gsm/gsm0480.c 1 file changed, 15 insertions(+), 6 deletions(-)
Approvals: laforge: Looks good to me, but someone else must approve pespin: Looks good to me, but someone else must approve Jenkins Builder: Verified fixeria: Looks good to me, approved
diff --git a/src/gsm/gsm0480.c b/src/gsm/gsm0480.c index a8eac6b..5db5bed 100644 --- a/src/gsm/gsm0480.c +++ b/src/gsm/gsm0480.c @@ -548,17 +548,24 @@ offset = invoke_data[1] + 2; req->invoke_id = invoke_data[2];
- /* look ahead once */ - if (offset + 1 > length) + /* look ahead once: need invoke_data[offset] and, if it turns out to be + * the optional Linked ID tag, invoke_data[offset+1] as well */ + if (offset + 2 > length) return 0;
/* optional part */ - if (invoke_data[offset] == GSM0480_COMPIDTAG_LINKED_ID) + if (invoke_data[offset] == GSM0480_COMPIDTAG_LINKED_ID) { offset += invoke_data[offset+1] + 2; /* skip over it */
+ /* offset moved by an attacker-controlled amount: re-validate */ + if (offset >= length) + return 0; + } + /* mandatory part */ if (invoke_data[offset] == GSM0480_OPERATION_CODE) { - if (offset + 2 > length) + /* need invoke_data[offset+2] below, and length - offset - 3 must not underflow */ + if (offset + 3 > length) return 0; uint8_t operation_code = invoke_data[offset+2]; req->opcode = operation_code; @@ -624,10 +631,12 @@ if (rr_data[offset] != GSM_0480_SEQUENCE_TAG) return 0;
- if (offset + 2 > length) + offset += 2; + + /* need rr_data[offset+2] below, and length - offset - 3 must not underflow */ + if (offset + 3 > length) return 0;
- offset += 2; operation_code = rr_data[offset + 2]; req->opcode = operation_code;