From: Jakub Grajciar <jgrajcia@cisco.com>
Date: Fri, 7 Feb 2020 10:30:26 +0000 (+0100)
Subject: api: improve api string safety
X-Git-Tag: v20.09-rc0~503
X-Git-Url: https://gerrit.fd.io/r/gitweb?p=vpp.git;a=commitdiff_plain;h=2dbee9361e74d03727a8b618ba80a5e28c006011

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 <jgrajcia@cisco.com>
Change-Id: Iadd59b612c0d960a34ad0dd07a9d17f56435c6ea
Signed-off-by: Jakub Grajciar <jgrajcia@cisco.com>
---

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 <vapi/l2.api.vapi.h>
 #include <fake.api.vapi.h>
 
+#include <vppinfra/vec.h>
+#include <vppinfra/mem.h>
+
 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(