neels has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-pfcp/+/29040 ) Change subject: gtlv: check memory bounds 2/3: decoding TLV ...................................................................... gtlv: check memory bounds 2/3: decoding TLV See Id8d997c9d5e655ff1842ec69eab6c073875c6330 Related: CID#275417 Related: SYS#5599 Change-Id: I841da89112ccf70fcd0f60eb902445fb1712eb48 --- M include/osmocom/gtlv/gtlv_dec_enc.h M src/libosmo-gtlv/gtlv_dec_enc.c M src/libosmo-gtlv/gtlv_gen.c M tests/libosmo-gtlv/gtlv_dec_enc_test.c 4 files changed, 50 insertions(+), 30 deletions(-) diff --git a/include/osmocom/gtlv/gtlv_dec_enc.h b/include/osmocom/gtlv/gtlv_dec_enc.h index cb62fe3..2126beb 100644 --- a/include/osmocom/gtlv/gtlv_dec_enc.h +++ b/include/osmocom/gtlv/gtlv_dec_enc.h @@ -182,9 +182,10 @@ */ typedef void (*osmo_gtlv_err_cb)(void *data, void *decoded_struct, const char *file, int line, const char *fmt, ...); -int osmo_gtlvs_decode(void *decoded_struct, unsigned int obj_ofs, struct osmo_gtlv_load *gtlv, bool tlv_ordered, - const struct osmo_gtlv_coding *ie_coding, - osmo_gtlv_err_cb err_cb, void *err_cb_data, const struct value_string *iei_strs); +int osmo_gtlvs_decode(void *decoded_struct, size_t decoded_struct_size, + unsigned int obj_ofs, struct osmo_gtlv_load *gtlv, bool tlv_ordered, + const struct osmo_gtlv_coding *ie_coding, + osmo_gtlv_err_cb err_cb, void *err_cb_data, const struct value_string *iei_strs); int osmo_gtlvs_encode(struct osmo_gtlv_put *gtlv, const void *decoded_struct, size_t decoded_struct_size, unsigned int obj_ofs, const struct osmo_gtlv_coding *ie_coding, osmo_gtlv_err_cb err_cb, diff --git a/src/libosmo-gtlv/gtlv_dec_enc.c b/src/libosmo-gtlv/gtlv_dec_enc.c index be3002f..e1e6d45 100644 --- a/src/libosmo-gtlv/gtlv_dec_enc.c +++ b/src/libosmo-gtlv/gtlv_dec_enc.c @@ -63,6 +63,11 @@ return memb; } +static void *membof(void *parent, size_t parent_size, unsigned int memb_ofs) +{ + return (void *)membof_const((const void *)parent, parent_size, memb_ofs); +} + /*! Decode a TLV structure from raw data to a decoded struct, for unordered TLV IEs. * How to decode IE values and where to place them in the decoded struct, is defined by ie_coding, an array terminated * by a '{}' entry. @@ -78,11 +83,13 @@ * \param[in] iei_strs value_string array to give IEI names in error messages passed to err_cb(), or NULL. * \return 0 on success, negative on error. */ -static int osmo_gtlvs_decode_unordered(void *decoded_struct, unsigned int obj_ofs, struct osmo_gtlv_load *gtlv, - const struct osmo_gtlv_coding *ie_coding, - osmo_gtlv_err_cb err_cb, void *err_cb_data, const struct value_string *iei_strs) +static int osmo_gtlvs_decode_unordered(void *decoded_struct, size_t decoded_struct_size, + unsigned int obj_ofs, struct osmo_gtlv_load *gtlv, + const struct osmo_gtlv_coding *ie_coding, + osmo_gtlv_err_cb err_cb, void *err_cb_data, const struct value_string *iei_strs) { - void *obj = MEMB(decoded_struct, obj_ofs); + void *obj = membof(decoded_struct, decoded_struct_size, obj_ofs); + size_t obj_maxlen = decoded_struct_size - obj_ofs; const struct osmo_gtlv_coding *iec; unsigned int *multi_count_p = NULL; @@ -136,8 +143,10 @@ ie_max_allowed_count += iec->has_count ? iec->count_max : 1; /* Was this iec instance already decoded? Then skip to the next one, if any. */ - presence_flag_p = iec->has_presence_flag ? MEMB(obj, iec->presence_flag_ofs) : NULL; - multi_count_p = iec->has_count ? MEMB(obj, iec->count_ofs) : NULL; + presence_flag_p = iec->has_presence_flag ? + membof(obj, obj_maxlen, iec->presence_flag_ofs) : NULL; + multi_count_p = iec->has_count ? + membof(obj, obj_maxlen, iec->count_ofs) : NULL; if ((presence_flag_p && *presence_flag_p) || (multi_count_p && *multi_count_p >= iec->count_max)) continue; @@ -187,15 +196,16 @@ default: OSMO_ASSERT(0); } - rc = osmo_gtlvs_decode(decoded_struct, obj_ofs + memb_ofs, &inner_tlv, ordered, iec->nested_ies, - err_cb, err_cb_data, iei_strs); + rc = osmo_gtlvs_decode(decoded_struct, decoded_struct_size, + obj_ofs + memb_ofs, &inner_tlv, ordered, iec->nested_ies, + err_cb, err_cb_data, iei_strs); if (rc) RETURN_ERROR(rc, gtlv->ti, "Error while decoding TLV structure nested inside this IE"); } else { /* Normal IE, decode the specific IE data. */ if (!iec->dec_func) RETURN_ERROR(-EIO, gtlv->ti, "IE definition lacks a dec_func()"); - rc = iec->dec_func(decoded_struct, MEMB(obj, memb_ofs), gtlv); + rc = iec->dec_func(decoded_struct, membof(obj, obj_maxlen, memb_ofs), gtlv); if (rc) RETURN_ERROR(rc, gtlv->ti, "Error while decoding this IE"); } @@ -218,7 +228,7 @@ for (iec = ie_coding; !osmo_gtlv_coding_end(iec); iec++) { if (iec->has_presence_flag) continue; - multi_count_p = iec->has_count ? MEMB(obj, iec->count_ofs) : NULL; + multi_count_p = iec->has_count ? membof(obj, obj_maxlen, iec->count_ofs) : NULL; if (multi_count_p) { if (*multi_count_p < iec->count_mandatory) RETURN_ERROR(-EINVAL, iec->ti, "%u instances of this IE are mandatory, got %u", @@ -247,18 +257,22 @@ * \param[in] iei_strs value_string array to give IEI names in error messages passed to err_cb(), or NULL. * \return 0 on success, negative on error. */ -static int osmo_gtlvs_decode_ordered(void *decoded_struct, unsigned int obj_ofs, struct osmo_gtlv_load *gtlv, - const struct osmo_gtlv_coding *ie_coding, - osmo_gtlv_err_cb err_cb, void *err_cb_data, const struct value_string *iei_strs) +static int osmo_gtlvs_decode_ordered(void *decoded_struct, size_t decoded_struct_size, + unsigned int obj_ofs, struct osmo_gtlv_load *gtlv, + const struct osmo_gtlv_coding *ie_coding, + osmo_gtlv_err_cb err_cb, void *err_cb_data, const struct value_string *iei_strs) { - void *obj = MEMB(decoded_struct, obj_ofs); + void *obj = membof(decoded_struct, decoded_struct_size, obj_ofs); + size_t obj_maxlen = decoded_struct_size - obj_ofs; osmo_gtlv_load_start(gtlv); for (; !osmo_gtlv_coding_end(ie_coding); ie_coding++) { int rc; - bool *presence_flag = ie_coding->has_presence_flag ? MEMB(obj, ie_coding->presence_flag_ofs) : NULL; - unsigned int *multi_count = ie_coding->has_count ? MEMB(obj, ie_coding->count_ofs) : NULL; + bool *presence_flag = ie_coding->has_presence_flag ? + membof(obj, obj_maxlen, ie_coding->presence_flag_ofs) : NULL; + unsigned int *multi_count = ie_coding->has_count ? + membof(obj, obj_maxlen, ie_coding->count_ofs) : NULL; struct osmo_gtlv_tag_inst peek_ti; rc = osmo_gtlv_load_next_by_tag_inst(gtlv, &ie_coding->ti); @@ -308,8 +322,9 @@ default: OSMO_ASSERT(0); } - rc = osmo_gtlvs_decode(decoded_struct, obj_ofs + memb_ofs, &inner_tlv, ordered, - ie_coding->nested_ies, err_cb, err_cb_data, iei_strs); + rc = osmo_gtlvs_decode(decoded_struct, decoded_struct_size, + obj_ofs + memb_ofs, &inner_tlv, ordered, ie_coding->nested_ies, + err_cb, err_cb_data, iei_strs); if (rc) RETURN_ERROR(rc, ie_coding->ti, "Error while decoding TLV structure nested inside this IE"); @@ -317,7 +332,7 @@ /* Normal IE, decode the specific IE data. */ if (!ie_coding->dec_func) RETURN_ERROR(-EIO, ie_coding->ti, "IE definition lacks a dec_func()"); - rc = ie_coding->dec_func(decoded_struct, MEMB(obj, memb_ofs), gtlv); + rc = ie_coding->dec_func(decoded_struct, membof(obj, obj_maxlen, memb_ofs), gtlv); if (rc) RETURN_ERROR(rc, ie_coding->ti, "Error while decoding this IE"); } @@ -365,17 +380,19 @@ * \param[in] iei_strs value_string array to give IEI names in error messages passed to err_cb(), or NULL. * \return 0 on success, negative on error. */ -int osmo_gtlvs_decode(void *decoded_struct, unsigned int obj_ofs, struct osmo_gtlv_load *gtlv, bool tlv_ordered, - const struct osmo_gtlv_coding *ie_coding, - osmo_gtlv_err_cb err_cb, void *err_cb_data, const struct value_string *iei_strs) +int osmo_gtlvs_decode(void *decoded_struct, size_t decoded_struct_size, + unsigned int obj_ofs, struct osmo_gtlv_load *gtlv, bool tlv_ordered, + const struct osmo_gtlv_coding *ie_coding, + osmo_gtlv_err_cb err_cb, void *err_cb_data, const struct value_string *iei_strs) { if (!ie_coding) return -ENOTSUP; if (tlv_ordered) - return osmo_gtlvs_decode_ordered(decoded_struct, obj_ofs, gtlv, ie_coding, err_cb, err_cb_data, iei_strs); + return osmo_gtlvs_decode_ordered(decoded_struct, decoded_struct_size, obj_ofs, gtlv, ie_coding, + err_cb, err_cb_data, iei_strs); else - return osmo_gtlvs_decode_unordered(decoded_struct, obj_ofs, gtlv, ie_coding, err_cb, err_cb_data, - iei_strs); + return osmo_gtlvs_decode_unordered(decoded_struct, decoded_struct_size, obj_ofs, gtlv, ie_coding, + err_cb, err_cb_data, iei_strs); } /*! Encode a TLV structure from decoded struct to raw data. diff --git a/src/libosmo-gtlv/gtlv_gen.c b/src/libosmo-gtlv/gtlv_gen.c index 9fe4b0c..3b2b1f0 100644 --- a/src/libosmo-gtlv/gtlv_gen.c +++ b/src/libosmo-gtlv/gtlv_gen.c @@ -381,7 +381,8 @@ " %s message_type,\n" " osmo_gtlv_err_cb err_cb, void *err_cb_data, const struct value_string *iei_strs)\n" "{\n" - " return osmo_gtlvs_decode(dst, 0, gtlv, tlv_ordered, %s_get_msg_coding(message_type), err_cb, err_cb_data, iei_strs);\n" + " return osmo_gtlvs_decode(dst, sizeof(*dst), 0, gtlv, tlv_ordered, %s_get_msg_coding(message_type),\n" + " err_cb, err_cb_data, iei_strs);\n" "}\n", g_cfg->proto_name, g_cfg->proto_name, g_cfg->message_type_enum ? : "int", g_cfg->proto_name); printf("\n" diff --git a/tests/libosmo-gtlv/gtlv_dec_enc_test.c b/tests/libosmo-gtlv/gtlv_dec_enc_test.c index 63de266..a542cfa 100644 --- a/tests/libosmo-gtlv/gtlv_dec_enc_test.c +++ b/tests/libosmo-gtlv/gtlv_dec_enc_test.c @@ -396,7 +396,8 @@ .cfg = cfg, .src = { put.dst->data, put.dst->len }, }; - rc = osmo_gtlvs_decode(&parsed, 0, &load, ordered, msg_ie_coding, err_cb, &verify_err_cb_data, tag_names); + rc = osmo_gtlvs_decode(&parsed, sizeof(parsed), 0, &load, ordered, msg_ie_coding, + err_cb, &verify_err_cb_data, tag_names); printf("osmo_gtlvs_decode() rc = %d\n", rc); printf("decoded: %s\n", decoded_msg_to_str(&parsed)); if (strcmp(decoded_msg_to_str(orig), decoded_msg_to_str(&parsed))) { null-- To view, visit https://gerrit.osmocom.org/c/libosmo-pfcp/+/29040 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-pfcp Gerrit-Branch: master Gerrit-Change-Id: I841da89112ccf70fcd0f60eb902445fb1712eb48 Gerrit-Change-Number: 29040 Gerrit-PatchSet: 5 Gerrit-Owner: neels &lt;nhofmeyr(a)sysmocom.de&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-MessageType: merged