neels has submitted this change. (
https://gerrit.osmocom.org/c/libosmo-pfcp/+/29039 )
Change subject: gtlv: check memory bounds 1/3: encoding TLV
......................................................................
gtlv: check memory bounds 1/3: encoding TLV
Introduce a maximum bound of memory access to the osmo_gtlv API.
Properly pass const-ness within the gtlv implementation. This patch adds
membof_const(). The following patch will add the non-const membof()
equivalent, which is not needed in this patch, yet.
Coverity CID#275417 drew my attention to the fact that the gtlv decoding
and encoding does not actually guard against access past the end of the
decoded struct.
We have not yet officially released libosmo-gtlv; also, osmo-upf and
osmo-hnbgw so far only use the libosmo-pfcp API, which "hides" the gtlv
API. Hence just change the API without a backwards compat shim.
Related: CID#275417
Related: SYS#5599
Change-Id: Id8d997c9d5e655ff1842ec69eab6c073875c6330
---
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, 33 insertions(+), 14 deletions(-)
Approvals:
neels: Looks good to me, approved
diff --git a/include/osmocom/gtlv/gtlv_dec_enc.h
b/include/osmocom/gtlv/gtlv_dec_enc.h
index 132239f..cb62fe3 100644
--- a/include/osmocom/gtlv/gtlv_dec_enc.h
+++ b/include/osmocom/gtlv/gtlv_dec_enc.h
@@ -186,9 +186,9 @@
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,
unsigned int obj_ofs,
- 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,
+ void *err_cb_data, const struct value_string *iei_strs);
int osmo_gtlvs_encode_to_str_buf(char *buf, size_t buflen, const void
*decoded_struct, unsigned int obj_ofs,
const struct osmo_gtlv_coding *ie_coding, const
struct value_string *iei_strs);
diff --git a/src/libosmo-gtlv/gtlv_dec_enc.c b/src/libosmo-gtlv/gtlv_dec_enc.c
index c3e45ea..be3002f 100644
--- a/src/libosmo-gtlv/gtlv_dec_enc.c
+++ b/src/libosmo-gtlv/gtlv_dec_enc.c
@@ -49,6 +49,19 @@
return RC; \
} while (0)
+/* Reverse offsetof(): return the address of the struct member for a given
parent struct and member offset value.
+ * \param parent Pointer to the parent struct, e.g. decoded_struct as passed
to osmo_gtlvs_decode().
+ * \param parent_size sizeof(*parent), for memory bounds checking, e.g.
decoded_struct_size.
+ * \param memb_ofs byte offset to reach a member (e.g.
osmo_gtlv_coding.memb_ofs, .presence_flag_ofs, ...). */
+static const void *membof_const(const void *parent, size_t parent_size,
unsigned int memb_ofs)
+{
+ const uint8_t *p = parent;
+ const uint8_t *end = p + parent_size;
+ const uint8_t *memb = p + memb_ofs;
+ OSMO_ASSERT(memb < end);
+ OSMO_ASSERT(memb >= p);
+ return memb;
+}
/*! 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
@@ -378,19 +391,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.
*/
-int osmo_gtlvs_encode(struct osmo_gtlv_put *gtlv, const void *decoded_struct,
unsigned int obj_ofs,
- 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,
+ void *err_cb_data, const struct value_string *iei_strs)
{
- void *obj = MEMB(decoded_struct, obj_ofs);
+ const void *obj = membof_const(decoded_struct, decoded_struct_size,
obj_ofs);
+ size_t obj_maxlen = decoded_struct_size - obj_ofs;
if (!ie_coding)
return -ENOTSUP;
for (; !osmo_gtlv_coding_end(ie_coding); ie_coding++) {
int rc;
- bool *presence_flag_p = ie_coding->has_presence_flag ?
MEMB(obj, ie_coding->presence_flag_ofs) : NULL;
- unsigned int *multi_count_p = ie_coding->has_count ? MEMB(obj,
ie_coding->count_ofs) : NULL;
+ const bool *presence_flag_p = ie_coding->has_presence_flag ?
+ membof_const(obj, obj_maxlen,
ie_coding->presence_flag_ofs) : NULL;
+ const unsigned int *multi_count_p = ie_coding->has_count ?
+ membof_const(obj, obj_maxlen, ie_coding->count_ofs) :
NULL;
unsigned int n;
unsigned int i;
@@ -423,13 +439,14 @@
.cfg = ie_coding->nested_ies_cfg ? :
gtlv->cfg,
.dst = gtlv->dst,
};
- rc = osmo_gtlvs_encode(&nested_tlv,
decoded_struct, obj_ofs + memb_ofs,
- ie_coding->nested_ies,
err_cb, err_cb_data, iei_strs);
+ rc = osmo_gtlvs_encode(&nested_tlv,
decoded_struct, decoded_struct_size,
+ obj_ofs + memb_ofs,
ie_coding->nested_ies,
+ err_cb, err_cb_data,
iei_strs);
if (rc)
RETURN_ERROR(rc, ie_coding->ti,
"Error while encoding TLV
structure nested inside this IE");
} else {
- rc = ie_coding->enc_func(gtlv, decoded_struct,
MEMB(obj, memb_ofs));
+ rc = ie_coding->enc_func(gtlv, decoded_struct,
membof_const(obj, obj_maxlen, memb_ofs));
if (rc)
RETURN_ERROR(rc, ie_coding->ti, "Error
while encoding this IE");
}
diff --git a/src/libosmo-gtlv/gtlv_gen.c b/src/libosmo-gtlv/gtlv_gen.c
index fd3fbd9..9fe4b0c 100644
--- a/src/libosmo-gtlv/gtlv_gen.c
+++ b/src/libosmo-gtlv/gtlv_gen.c
@@ -388,7 +388,8 @@
"int %s_ies_encode(struct osmo_gtlv_put *gtlv, const union
%s_ies *src,\n"
" %s message_type, osmo_gtlv_err_cb err_cb, void
*err_cb_data, const struct value_string *iei_strs)\n"
"{\n"
- " return osmo_gtlvs_encode(gtlv, src, 0,
%s_get_msg_coding(message_type), err_cb, err_cb_data, iei_strs);\n"
+ " return osmo_gtlvs_encode(gtlv, src, sizeof(*src), 0,
%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 e02a2e5..63de266 100644
--- a/tests/libosmo-gtlv/gtlv_dec_enc_test.c
+++ b/tests/libosmo-gtlv/gtlv_dec_enc_test.c
@@ -387,7 +387,8 @@
.cfg = cfg,
.dst = msgb_alloc(1024, __func__),
};
- rc = osmo_gtlvs_encode(&put, (void *)orig, 0, msg_ie_coding,
err_cb, &verify_err_cb_data, tag_names);
+ rc = osmo_gtlvs_encode(&put, (void *)orig, sizeof(*orig), 0,
msg_ie_coding,
+ err_cb, &verify_err_cb_data, tag_names);
printf("osmo_gtlvs_encode() rc = %d\n", rc);
printf("%s.\n", osmo_hexdump(put.dst->data, put.dst->len));
null--
To view, visit https://gerrit.osmocom.org/c/libosmo-pfcp/+/29039
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-pfcp
Gerrit-Branch: master
Gerrit-Change-Id: Id8d997c9d5e655ff1842ec69eab6c073875c6330
Gerrit-Change-Number: 29039
Gerrit-PatchSet: 5
Gerrit-Owner: neels <nhofm...@sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <lafo...@osmocom.org>
Gerrit-Reviewer: neels <nhofm...@sysmocom.de>
Gerrit-Reviewer: pespin <pes...@sysmocom.de>
Gerrit-MessageType: merged
