From 2dbee9361e74d03727a8b618ba80a5e28c006011 Mon Sep 17 00:00:00 2001 From: Jakub Grajciar Date: Fri, 7 Feb 2020 11:30:26 +0100 Subject: api: improve api string safety - Remove vl_api_from_api_string to prevent use of not nul-terminated strings. - Rename vl_api_from_api_to_vec -> vl_api_from_api_to_new_vec to imply a new vector is created. NOT nul terminated. - Add vl_api_from_api_to_new_c_string. Returns nul terminated string in a new vector. - Add vl_api_c_string_to_api_string. Convert nul terminated string to vl_api_string_t - Add vl_api_vec_to_api_string. Convert NON nul terminated vector to vl_api_string_t Type: fix Signed-off-by: Jakub Grajciar Change-Id: Iadd59b612c0d960a34ad0dd07a9d17f56435c6ea Signed-off-by: Jakub Grajciar --- src/plugins/http_static/http_static_test.c | 7 +--- src/plugins/ioam/lib-pot/pot_api.c | 4 +- src/plugins/ioam/lib-pot/pot_test.c | 4 +- src/plugins/memif/memif_test.c | 7 +--- src/tools/vppapigen/vppapigen_c.py | 5 +-- src/vat/api_format.c | 19 ++++----- src/vlibapi/api_shared.c | 34 ++++++++++----- src/vlibapi/api_types.h | 13 ++++-- src/vnet/devices/tap/tapv2_api.c | 2 +- src/vnet/interface_api.c | 2 +- src/vnet/ip/punt_api.c | 4 +- src/vpp/api/api.c | 13 +++--- src/vpp/api/custom_dump.c | 6 +-- test/ext/vapi_c_test.c | 67 ++++++++++++++++++++++++++++++ test/test_punt.py | 2 + 15 files changed, 133 insertions(+), 56 deletions(-) diff --git a/src/plugins/http_static/http_static_test.c b/src/plugins/http_static/http_static_test.c index 0b46d23dce5..3503a1b0812 100644 --- a/src/plugins/http_static/http_static_test.c +++ b/src/plugins/http_static/http_static_test.c @@ -111,11 +111,8 @@ api_http_static_enable (vat_main_t * vam) /* Construct the API message */ M (HTTP_STATIC_ENABLE, mp); - vl_api_to_api_string (strnlen ((const char *) www_root, 256), - (const char *) www_root, - (vl_api_string_t *) & mp->www_root); - vl_api_to_api_string (strnlen ((const char *) uri, 256), (const char *) uri, - (vl_api_string_t *) & mp->uri); + strncpy_s ((char *) mp->www_root, 256, (const char *) www_root, 256); + strncpy_s ((char *) mp->uri, 256, (const char *) uri, 256); mp->fifo_size = ntohl (fifo_size); mp->cache_size_limit = ntohl (cache_size_limit); mp->prealloc_fifos = ntohl (prealloc_fifos); diff --git a/src/plugins/ioam/lib-pot/pot_api.c b/src/plugins/ioam/lib-pot/pot_api.c index 2ecfc51d97a..31ddf9da528 100644 --- a/src/plugins/ioam/lib-pot/pot_api.c +++ b/src/plugins/ioam/lib-pot/pot_api.c @@ -43,7 +43,7 @@ static void vl_api_pot_profile_add_t_handler pot_profile *profile = NULL; u8 *name = 0; - name = vl_api_from_api_to_vec(&mp->list_name); + name = vl_api_from_api_to_new_vec(&mp->list_name); pot_profile_list_init(name); id = mp->id; @@ -121,7 +121,7 @@ static void vl_api_pot_profile_activate_t_handler u8 id; u8 *name = NULL; - name = vl_api_from_api_to_vec(&mp->list_name); + name = vl_api_from_api_to_new_vec(&mp->list_name); if (!pot_profile_list_is_enabled(name)) { rv = -1; } else { diff --git a/src/plugins/ioam/lib-pot/pot_test.c b/src/plugins/ioam/lib-pot/pot_test.c index 90cff23888b..b30b21e9471 100644 --- a/src/plugins/ioam/lib-pot/pot_test.c +++ b/src/plugins/ioam/lib-pot/pot_test.c @@ -88,7 +88,7 @@ static int api_pot_profile_add (vat_main_t *vam) M2(POT_PROFILE_ADD, mp, sizeof(vl_api_string_t) + vec_len(name)); - vl_api_to_api_string(vec_len(name), (const char *)name, &mp->list_name); + vl_api_vec_to_api_string(name, &mp->list_name); mp->secret_share = clib_host_to_net_u64(secret_share); mp->polynomial_public = clib_host_to_net_u64(poly2); mp->lpc = clib_host_to_net_u64(lpc); @@ -142,7 +142,7 @@ static int api_pot_profile_activate (vat_main_t *vam) } M2(POT_PROFILE_ACTIVATE, mp, sizeof(vl_api_string_t) + vec_len(name)); - vl_api_to_api_string(vec_len(name), (const char *)name, &mp->list_name); + vl_api_vec_to_api_string(name, &mp->list_name); mp->id = id; S(mp); diff --git a/src/plugins/memif/memif_test.c b/src/plugins/memif/memif_test.c index 05f5c29b957..1ec6703d135 100644 --- a/src/plugins/memif/memif_test.c +++ b/src/plugins/memif/memif_test.c @@ -112,9 +112,7 @@ api_memif_socket_filename_add_del (vat_main_t * vam) mp->is_add = is_add; mp->socket_id = htonl (socket_id); char *p = (char *) &mp->socket_filename; - p += - vl_api_to_api_string (strlen ((char *) socket_filename), - (char *) socket_filename, (vl_api_string_t *) p); + p += vl_api_vec_to_api_string (socket_filename, (vl_api_string_t *) p); vec_free (socket_filename); @@ -218,8 +216,7 @@ api_memif_create (vat_main_t * vam) if (secret != 0) { char *p = (char *) &mp->secret; - p += vl_api_to_api_string (strlen ((char *) secret), (char *) secret, - (vl_api_string_t *) p); + p += vl_api_vec_to_api_string (secret, (vl_api_string_t *) p); vec_free (secret); } memcpy (mp->hw_addr, hw_addr, 6); diff --git a/src/tools/vppapigen/vppapigen_c.py b/src/tools/vppapigen/vppapigen_c.py index a893b3a89bc..46f60dea843 100644 --- a/src/tools/vppapigen/vppapigen_c.py +++ b/src/tools/vppapigen/vppapigen_c.py @@ -136,10 +136,9 @@ class Printfun(): if o.modern_vla: write(' if (vl_api_string_len(&a->{f}) > 0) {{\n' .format(f=o.fieldname)) - write(' s = format(s, "\\n%U{f}: %.*s", ' + write(' s = format(s, "\\n%U{f}: %U", ' 'format_white_space, indent, ' - 'vl_api_string_len(&a->{f}) - 1, ' - 'vl_api_from_api_string(&a->{f}));\n'.format(f=o.fieldname)) + 'vl_api_format_string, (&a->{f}));\n'.format(f=o.fieldname)) write(' } else {\n') write(' s = format(s, "\\n%U{f}:", ' 'format_white_space, indent);\n'.format(f=o.fieldname)) diff --git a/src/vat/api_format.c b/src/vat/api_format.c index acb173042db..f85e94a50b7 100644 --- a/src/vat/api_format.c +++ b/src/vat/api_format.c @@ -1127,18 +1127,12 @@ vl_api_cli_inband_reply_t_handler (vl_api_cli_inband_reply_t * mp) { vat_main_t *vam = &vat_main; i32 retval = ntohl (mp->retval); - u32 length = vl_api_string_len (&mp->reply); vec_reset_length (vam->cmd_reply); vam->retval = retval; if (retval == 0) - { - vec_validate (vam->cmd_reply, length); - clib_memcpy ((char *) (vam->cmd_reply), - vl_api_from_api_string (&mp->reply), length); - vam->cmd_reply[length] = 0; - } + vam->cmd_reply = vl_api_from_api_to_new_vec (&mp->reply); vam->result_ready = 1; } @@ -1147,16 +1141,18 @@ vl_api_cli_inband_reply_t_handler_json (vl_api_cli_inband_reply_t * mp) { vat_main_t *vam = &vat_main; vat_json_node_t node; + u8 *reply = 0; /* reply vector */ + reply = vl_api_from_api_to_new_vec (&mp->reply); vec_reset_length (vam->cmd_reply); vat_json_init_object (&node); vat_json_object_add_int (&node, "retval", ntohl (mp->retval)); - vat_json_object_add_string_copy (&node, "reply", - vl_api_from_api_string (&mp->reply)); + vat_json_object_add_string_copy (&node, "reply", reply); vat_json_print (vam->ofp, &node); vat_json_free (&node); + vec_free (reply); vam->retval = ntohl (mp->retval); vam->result_ready = 1; @@ -5638,9 +5634,8 @@ exec_inband (vat_main_t * vam) * must be a vector ending in \n, not a C-string ending * in \n\0. */ - u32 len = vec_len (vam->input->buffer); - M2 (CLI_INBAND, mp, len); - vl_api_to_api_string (len - 1, (const char *) vam->input->buffer, &mp->cmd); + M2 (CLI_INBAND, mp, vec_len (vam->input->buffer)); + vl_api_vec_to_api_string (vam->input->buffer, &mp->cmd); S (mp); W (ret); diff --git a/src/vlibapi/api_shared.c b/src/vlibapi/api_shared.c index aba853dc997..28e9f481650 100644 --- a/src/vlibapi/api_shared.c +++ b/src/vlibapi/api_shared.c @@ -1091,15 +1091,19 @@ vl_msg_pop_heap (void *oldheap) vl_msg_pop_heap_w_region (am->vlib_rp, oldheap); } +/* Must be nul terminated */ int -vl_api_to_api_string (u32 len, const char *buf, vl_api_string_t * str) +vl_api_c_string_to_api_string (const char *buf, vl_api_string_t * str) { - if (len) + /* copy without nul terminator */ + u32 len = strlen (buf); + if (len > 0) clib_memcpy_fast (str->buf, buf, len); str->length = htonl (len); return len + sizeof (u32); } +/* Must NOT be nul terminated */ int vl_api_vec_to_api_string (const u8 * vec, vl_api_string_t * str) { @@ -1109,13 +1113,6 @@ vl_api_vec_to_api_string (const u8 * vec, vl_api_string_t * str) return len + sizeof (u32); } -/* Return a pointer to the API string (not nul terminated */ -u8 * -vl_api_from_api_string (vl_api_string_t * astr) -{ - return astr->buf; -} - u32 vl_api_string_len (vl_api_string_t * astr) { @@ -1132,15 +1129,32 @@ vl_api_format_string (u8 * s, va_list * args) /* * Returns a new vector. Remember to free it after use. + * NOT nul terminated. */ u8 * -vl_api_from_api_to_vec (vl_api_string_t * astr) +vl_api_from_api_to_new_vec (vl_api_string_t * astr) { u8 *v = 0; vec_add (v, astr->buf, clib_net_to_host_u32 (astr->length)); return v; } +/* + * Returns a new vector. Remember to free it after use. + * Nul terminated. + */ +char * +vl_api_from_api_to_new_c_string (vl_api_string_t * astr) +{ + char *v = 0; + if (clib_net_to_host_u32 (astr->length) > 0) + { + vec_add (v, astr->buf, clib_net_to_host_u32 (astr->length)); + vec_add1 (v, 0); + } + return v; +} + void vl_api_set_elog_main (elog_main_t * m) { diff --git a/src/vlibapi/api_types.h b/src/vlibapi/api_types.h index 8cd47bb770f..21fcde53b0e 100644 --- a/src/vlibapi/api_types.h +++ b/src/vlibapi/api_types.h @@ -38,11 +38,18 @@ typedef struct u8 buf[0]; } __attribute__ ((packed)) vl_api_string_t; -extern int vl_api_to_api_string (u32 len, const char *buf, vl_api_string_t * str); +/* Nul terminated string to vl_api_string_t */ +extern int vl_api_c_string_to_api_string (const char *buf, vl_api_string_t * str); +/* NON nul terminated vector to vl_api_string_t */ extern int vl_api_vec_to_api_string (const u8 *vec, vl_api_string_t * str); -extern u8 * vl_api_from_api_string (vl_api_string_t * astr); + extern u32 vl_api_string_len (vl_api_string_t * astr); -extern u8 * vl_api_from_api_to_vec (vl_api_string_t *astr); + +/* Returns new vector. NON nul terminated */ +extern u8 * vl_api_from_api_to_new_vec (vl_api_string_t *astr); +/* Returns new vector. Nul terminated */ +extern char * vl_api_from_api_to_new_c_string (vl_api_string_t *astr); + extern u8 *vl_api_format_string (u8 *s, va_list *args); #ifdef __cplusplus diff --git a/src/vnet/devices/tap/tapv2_api.c b/src/vnet/devices/tap/tapv2_api.c index e0121a83c2f..3b66bf0d6ec 100644 --- a/src/vnet/devices/tap/tapv2_api.c +++ b/src/vnet/devices/tap/tapv2_api.c @@ -134,7 +134,7 @@ vl_api_tap_create_v2_t_handler (vl_api_tap_create_v2_t * mp) /* If a tag was supplied... */ if (vl_api_string_len (&mp->tag)) { - u8 *tag = format (0, "%s%c", vl_api_from_api_string (&mp->tag), 0); + u8 *tag = vl_api_from_api_to_new_vec (&mp->tag); vnet_set_sw_interface_tag (vnm, tag, ap->sw_if_index); } diff --git a/src/vnet/interface_api.c b/src/vnet/interface_api.c index fe3426cb827..7086c5c4c7b 100644 --- a/src/vnet/interface_api.c +++ b/src/vnet/interface_api.c @@ -358,7 +358,7 @@ vl_api_sw_interface_dump_t_handler (vl_api_sw_interface_dump_t * mp) if (mp->name_filter_valid) { - filter = vl_api_from_api_to_vec (&mp->name_filter); + filter = vl_api_from_api_to_new_vec (&mp->name_filter); vec_add1 (filter, 0); /* Ensure it's a C string for strcasecmp() */ } diff --git a/src/vnet/ip/punt_api.c b/src/vnet/ip/punt_api.c index 6ed62a1873c..077b1ac3a69 100644 --- a/src/vnet/ip/punt_api.c +++ b/src/vnet/ip/punt_api.c @@ -347,7 +347,7 @@ punt_reason_dump_walk_cb (vlib_punt_reason_t id, const u8 * name, void *args) mp->context = ctx->context; mp->reason.id = clib_host_to_net_u32 (id); - vl_api_to_api_string (vec_len (name), (char *) name, &mp->reason.name); + vl_api_vec_to_api_string (name, &mp->reason.name); vl_api_send_msg (ctx->reg, (u8 *) mp); @@ -366,7 +366,7 @@ vl_api_punt_reason_dump_t_handler (vl_api_punt_reason_dump_t * mp) punt_reason_dump_walk_ctx_t ctx = { .reg = reg, .context = mp->context, - .name = vl_api_from_api_to_vec (&mp->reason.name), + .name = vl_api_from_api_to_new_vec (&mp->reason.name), }; punt_reason_walk (punt_reason_dump_walk_cb, &ctx); diff --git a/src/vpp/api/api.c b/src/vpp/api/api.c index f652205d583..01527a565f3 100644 --- a/src/vpp/api/api.c +++ b/src/vpp/api/api.c @@ -212,7 +212,7 @@ vl_api_cli_inband_t_handler (vl_api_cli_inband_t * mp) vlib_main_t *vm = vlib_get_main (); unformat_input_t input; u8 *out_vec = 0; - u32 len = 0; + u8 *cmd_vec = 0; if (vl_msg_api_get_msg_length (mp) < vl_api_string_len (&mp->cmd) + sizeof (*mp)) @@ -221,20 +221,21 @@ vl_api_cli_inband_t_handler (vl_api_cli_inband_t * mp) goto error; } - unformat_init_string (&input, (char *) vl_api_from_api_string (&mp->cmd), + cmd_vec = vl_api_from_api_to_new_vec (&mp->cmd); + + unformat_init_string (&input, (char *) cmd_vec, vl_api_string_len (&mp->cmd)); rv = vlib_cli_input (vm, &input, inband_cli_output, (uword) & out_vec); - len = vec_len (out_vec); - error: /* *INDENT-OFF* */ - REPLY_MACRO3(VL_API_CLI_INBAND_REPLY, len, + REPLY_MACRO3(VL_API_CLI_INBAND_REPLY, vec_len (out_vec), ({ - vl_api_to_api_string(len, (const char *)out_vec, &rmp->reply); + vl_api_vec_to_api_string(out_vec, &rmp->reply); })); /* *INDENT-ON* */ vec_free (out_vec); + vec_free (cmd_vec); } static void diff --git a/src/vpp/api/custom_dump.c b/src/vpp/api/custom_dump.c index c155039a4a6..806fee69ddb 100644 --- a/src/vpp/api/custom_dump.c +++ b/src/vpp/api/custom_dump.c @@ -1783,7 +1783,7 @@ static void *vl_api_sw_interface_dump_t_print if (mp->name_filter_valid) { - u8 *v = vl_api_from_api_to_vec (&mp->name_filter); + u8 *v = vl_api_from_api_to_new_vec (&mp->name_filter); s = format (s, "name_filter %v ", v); vec_free (v); } @@ -1841,10 +1841,8 @@ static void *vl_api_cli_inband_t_print { u8 *s; u8 *cmd = 0; - u32 length = vl_api_string_len (&mp->cmd); - vec_validate (cmd, length); - clib_memcpy (cmd, vl_api_from_api_string (&mp->cmd), length); + cmd = vl_api_from_api_to_new_vec (&mp->cmd); s = format (0, "SCRIPT: exec %v ", cmd); diff --git a/test/ext/vapi_c_test.c b/test/ext/vapi_c_test.c index a9572ae8716..51fc572746f 100644 --- a/test/ext/vapi_c_test.c +++ b/test/ext/vapi_c_test.c @@ -29,6 +29,9 @@ #include #include +#include +#include + DEFINE_VAPI_MSG_IDS_VPE_API_JSON; DEFINE_VAPI_MSG_IDS_INTERFACE_API_JSON; DEFINE_VAPI_MSG_IDS_L2_API_JSON; @@ -911,6 +914,66 @@ START_TEST (test_unsupported) END_TEST; +START_TEST (test_api_strings) +{ + printf ("--- Invalid api strings ---\n"); + + /* test string 'TEST' + * size = 5 + * length = 4 + */ + const char str[] = "TEST"; + u8 *vec_str = 0, *vstr = 0; + char *cstr; + + vapi_msg_sw_interface_dump *dump = + malloc (sizeof (vapi_msg_sw_interface_dump) + strlen (str)); + clib_mem_init (0, 1 << 20); + + vl_api_c_string_to_api_string (str, &dump->payload.name_filter); + /* Assert nul terminator NOT present */ + ck_assert_int_eq (vl_api_string_len (&dump->payload.name_filter), + strlen (str)); + + cstr = vl_api_from_api_to_new_c_string (&dump->payload.name_filter); + ck_assert_ptr_ne (cstr, NULL); + /* Assert nul terminator present */ + ck_assert_int_eq (vec_len (cstr), sizeof (str)); + ck_assert_int_eq (strlen (str), strlen (cstr)); + vec_free (cstr); + + vstr = vl_api_from_api_to_new_vec (&dump->payload.name_filter); + ck_assert_ptr_ne (vstr, NULL); + /* Assert nul terminator NOT present */ + ck_assert_int_eq (vec_len (vstr), strlen (str)); + vec_free (vstr); + + /* vector conaining NON nul terminated string 'TEST' */ + vec_add (vec_str, str, strlen (str)); + clib_memset (dump->payload.name_filter.buf, 0, strlen (str)); + dump->payload.name_filter.length = 0; + + vl_api_vec_to_api_string (vec_str, &dump->payload.name_filter); + /* Assert nul terminator NOT present */ + ck_assert_int_eq (vl_api_string_len (&dump->payload.name_filter), + vec_len (vec_str)); + + cstr = vl_api_from_api_to_new_c_string (&dump->payload.name_filter); + ck_assert_ptr_ne (cstr, NULL); + /* Assert nul terminator present */ + ck_assert_int_eq (vec_len (cstr), sizeof (str)); + ck_assert_int_eq (strlen (str), strlen (cstr)); + vec_free (cstr); + + vstr = vl_api_from_api_to_new_vec (&dump->payload.name_filter); + ck_assert_ptr_ne (vstr, NULL); + /* Assert nul terminator NOT present */ + ck_assert_int_eq (vec_len (vstr), strlen (str)); + vec_free (vstr); +} + +END_TEST; + Suite * test_suite (void) { @@ -957,6 +1020,10 @@ test_suite (void) tcase_add_test (tc_unsupported, test_unsupported); suite_add_tcase (s, tc_unsupported); + TCase *tc_dynamic = tcase_create ("Dynamic message size"); + tcase_add_test (tc_dynamic, test_api_strings); + suite_add_tcase (s, tc_dynamic); + return s; } diff --git a/test/test_punt.py b/test/test_punt.py index 8ebf44767f3..ecd34f6eea7 100644 --- a/test/test_punt.py +++ b/test/test_punt.py @@ -811,6 +811,8 @@ class TestExceptionPuntSocket(TestPuntSocket): rs = self.vapi.punt_reason_dump() for key in cfgs: for r in rs: + print(r.reason.name) + print(key) if r.reason.name == key: cfgs[key]['id'] = r.reason.id cfgs[key]['vpp'] = copy.deepcopy( -- cgit 1.2.3-korg