pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/43251?usp=email )
Change subject: gsm: Reject BSSMAP Encryption Information IE with key length != 8 ...................................................................... gsm: Reject BSSMAP Encryption Information IE with key length != 8 >From early TS 08.08 to current 3GPP TS 48.008 release 19, 3.2.2.10 Encryption Information IE clearly states that the key size should be 8 if present. This also in turn fixes a potential write buffer overflow if an IE with len>253 was passed to the decoding function. This patch fixes the problem by rewriting the function, since it actually was way too convoluted/complex for the kind of checks it had to do. This way also a call to memcpy is avoided if no key is present. Related: OS#7054 Reported-By: Adam Bedard <[email protected]> Change-Id: I9fd70875f35cebb923278869e1260c5bf3047439 --- M src/gsm/gsm0808_utils.c M tests/gsm0808/gsm0808_test.c 2 files changed, 38 insertions(+), 7 deletions(-) Approvals: laforge: Looks good to me, approved osmith: Looks good to me, approved Jenkins Builder: Verified diff --git a/src/gsm/gsm0808_utils.c b/src/gsm/gsm0808_utils.c index 1abfbc8..d7bc275 100644 --- a/src/gsm/gsm0808_utils.c +++ b/src/gsm/gsm0808_utils.c @@ -831,7 +831,6 @@ uint8_t perm_algo; unsigned int i; unsigned int perm_algo_len = 0; - const uint8_t *old_elem = elem; if (!elem) return -EINVAL; @@ -854,14 +853,18 @@ } } ei->perm_algo_len = perm_algo_len; - - /* FIXME: 48.008 3.2.2.10 Encryption Information says: - * "When present, the key shall be 8 octets long." */ ei->key_len = len - 1; - memcpy(ei->key, elem, ei->key_len); - elem+=ei->key_len; - return (int)(elem - old_elem); + /* No key present, done */ + if (ei->key_len == 0) + return len; + + /* "When present, the key shall be 8 octets long." */ + if (ei->key_len != 8) + return -EINVAL; + OSMO_ASSERT(sizeof(ei->key) >= ei->key_len); + memcpy(ei->key, elem, ei->key_len); + return len; } /*! Encode TS 48.008 Kc128 IE. diff --git a/tests/gsm0808/gsm0808_test.c b/tests/gsm0808/gsm0808_test.c index 497de6c..8ebbd4b 100644 --- a/tests/gsm0808/gsm0808_test.c +++ b/tests/gsm0808/gsm0808_test.c @@ -1282,6 +1282,25 @@ rc_dec = gsm0808_dec_encrypt_info(&dec_ei, msg->data + 2, msg->len - 2); OSMO_ASSERT(rc_dec == 9); OSMO_ASSERT(memcmp(&enc_ei, &dec_ei, sizeof(enc_ei)) == 0); + + + /* Test decoding IE with No Encryption and hence with no Key */ + struct gsm0808_encrypt_info enc_ei_no_encryption = { + .perm_algo = { GSM0808_ALG_ID_A5_0 }, + .perm_algo_len = 1, + .key = { 0 }, + .key_len = 0, + }; + uint8_t ei_enc_no_encryption_expected[] = { GSM0808_IE_ENCRYPTION_INFORMATION, 0x01, 0x01 }; + + msg = msgb_alloc(1024, "output buffer"); + rc_enc = gsm0808_enc_encrypt_info(msg, &enc_ei_no_encryption); + OSMO_ASSERT(rc_enc == 3); + OSMO_ASSERT(memcmp(ei_enc_no_encryption_expected, msg->data, msg->len) == 0); + + rc_dec = gsm0808_dec_encrypt_info(&dec_ei, msg->data + 2, msg->len - 2); + OSMO_ASSERT(rc_dec == 1); + OSMO_ASSERT(memcmp(&enc_ei_no_encryption, &dec_ei, sizeof(enc_ei_no_encryption)) == 0); msgb_free(msg); /* Test decoding of malformed IE with no algo selected: */ @@ -1289,6 +1308,15 @@ rc_dec = gsm0808_dec_encrypt_info(&dec_ei, &ei_enc_no_algo[2], sizeof(ei_enc_no_algo) - 2); OSMO_ASSERT(rc_dec == -EINVAL); + /* Test decoding of malformed IE with wrong key length: */ + uint8_t ei_enc_wrong_key_len[256] = { GSM0808_IE_ENCRYPTION_INFORMATION, 0xfd, 0x03 }; + rc_dec = gsm0808_dec_encrypt_info(&dec_ei, &ei_enc_wrong_key_len[2], 0xfd); + OSMO_ASSERT(rc_dec == -EINVAL); + /* test with another invalid key length: */ + ei_enc_wrong_key_len[1] = 0x03; + rc_dec = gsm0808_dec_encrypt_info(&dec_ei, &ei_enc_wrong_key_len[2], 0x03); + OSMO_ASSERT(rc_dec == -EINVAL); + } static void test_gsm0808_dec_cell_id_list_srvcc(void) -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/43251?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I9fd70875f35cebb923278869e1260c5bf3047439 Gerrit-Change-Number: 43251 Gerrit-PatchSet: 3 Gerrit-Owner: pespin <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <[email protected]> Gerrit-Reviewer: osmith <[email protected]> Gerrit-Reviewer: pespin <[email protected]>
