libmemif: fix insecure uses of strncpy

A calling patterm of "strncpy(dst, src, strlen(src))" invites a lot of troubles.

However, even using the target size may result in a problem if the string is
longer, since then the termination is not done.

Use strlcpy(dst, src, sizeof(dst)), which will always null-terminate
the string.

Change-Id: I8ddaf3dc8380a78af08914e81849279dae7ab24a
Type: fix
Signed-off-by: Andrew Yourtchenko <ayourtch@gmail.com>
Signed-off-by: Jakub Grajciar <jgrajcia@cisco.com>

diff --git a/extras/libmemif/src/CMakeLists.txt b/extras/libmemif/src/CMakeLists.txt
index aced550..ddb8a52 100644 (file)
--- a/extras/libmemif/src/CMakeLists.txt
+++ b/extras/libmemif/src/CMakeLists.txt
@@ -34,6 +34,13 @@ include_directories(${HEADERS_DIR})
 
 add_library(memif SHARED ${MEMIF_SOURCES})
 target_link_libraries(memif ${CMAKE_THREAD_LIBS_INIT})
+
+find_library(LIB_BSD bsd)
+if(LIB_BSD)
+  add_compile_definitions(HAS_LIB_BSD)
+  target_link_libraries(memif ${LIB_BSD})
+endif()
+
 foreach(file ${MEMIF_HEADERS})
   get_filename_component(dir ${file} DIRECTORY)
      install(

diff --git a/extras/libmemif/src/main.c b/extras/libmemif/src/main.c
index d7345d6..1eb6929 100644 (file)
--- a/extras/libmemif/src/main.c
+++ b/extras/libmemif/src/main.c
@@ -158,14 +158,11 @@ memif_strerror (int err_code)
 {
   if (err_code >= ERRLIST_LEN)
     {
-      strncpy (memif_buf, MEMIF_ERR_UNDEFINED, strlen (MEMIF_ERR_UNDEFINED));
-      memif_buf[strlen (MEMIF_ERR_UNDEFINED)] = '\0';
+      strlcpy (memif_buf, MEMIF_ERR_UNDEFINED, sizeof (memif_buf));
     }
   else
     {
-      strncpy (memif_buf, memif_errlist[err_code],
-              strlen (memif_errlist[err_code]));
-      memif_buf[strlen (memif_errlist[err_code])] = '\0';
+      strlcpy (memif_buf, memif_errlist[err_code], sizeof (memif_buf));
     }
   return memif_buf;
 }
@@ -532,14 +529,12 @@ memif_init (memif_control_fd_update_t * on_control_fd_update, char *app_name,
 
   if (app_name != NULL)
     {
-      uint8_t len = (strlen (app_name) > MEMIF_NAME_LEN)
-       ? strlen (app_name) : MEMIF_NAME_LEN;
-      strncpy ((char *) lm->app_name, app_name, len);
+      strlcpy ((char *) lm->app_name, app_name, sizeof (lm->app_name));
     }
   else
     {
-      strncpy ((char *) lm->app_name, MEMIF_DEFAULT_APP_NAME,
-              strlen (MEMIF_DEFAULT_APP_NAME));
+      strlcpy ((char *) lm->app_name, MEMIF_DEFAULT_APP_NAME,
+              sizeof (lm->app_name));
     }
 
   lm->poll_cancel_fd = -1;
@@ -699,14 +694,12 @@ memif_per_thread_init (memif_per_thread_main_handle_t * pt_main,
   /* set app name */
   if (app_name != NULL)
     {
-      uint8_t len = (strlen (app_name) > MEMIF_NAME_LEN)
-       ? strlen (app_name) : MEMIF_NAME_LEN;
-      strncpy ((char *) lm->app_name, app_name, len);
+      strlcpy ((char *) lm->app_name, app_name, MEMIF_NAME_LEN);
     }
   else
     {
-      strncpy ((char *) lm->app_name, MEMIF_DEFAULT_APP_NAME,
-              strlen (MEMIF_DEFAULT_APP_NAME));
+      strlcpy ((char *) lm->app_name, MEMIF_DEFAULT_APP_NAME,
+              sizeof (lm->app_name));
     }
 
   lm->poll_cancel_fd = -1;
@@ -885,8 +878,7 @@ memif_socket_start_listening (memif_socket_t * ms)
 
   DBG ("socket %d created", ms->fd);
   un.sun_family = AF_UNIX;
-  strncpy ((char *) un.sun_path, (char *) ms->filename,
-          sizeof (un.sun_path) - 1);
+  strlcpy ((char *) un.sun_path, (char *) ms->filename, sizeof (un.sun_path));
   if (setsockopt (ms->fd, SOL_SOCKET, SO_PASSCRED, &on, sizeof (on)) < 0)
     {
       err = memif_syscall_error_handler (errno);
@@ -963,7 +955,7 @@ memif_create_socket (memif_socket_handle_t * sock, const char *filename,
       goto error;
     }
   memset (ms->filename, 0, strlen (filename) + sizeof (char));
-  strncpy ((char *) ms->filename, filename, strlen (filename));
+  strlcpy ((char *) ms->filename, filename, sizeof (ms->filename));
 
   ms->type = MEMIF_SOCKET_TYPE_NONE;
 
@@ -1047,7 +1039,7 @@ memif_per_thread_create_socket (memif_per_thread_main_handle_t pt_main,
       goto error;
     }
   memset (ms->filename, 0, strlen (filename) + sizeof (char));
-  strncpy ((char *) ms->filename, filename, strlen (filename));
+  strlcpy ((char *) ms->filename, filename, sizeof (ms->filename));
 
   ms->type = MEMIF_SOCKET_TYPE_NONE;
 
@@ -1150,12 +1142,12 @@ memif_create (memif_conn_handle_t * c, memif_conn_args_t * args,
   conn->private_ctx = private_ctx;
   memset (&conn->run_args, 0, sizeof (memif_conn_run_args_t));
 
-  uint8_t l = strlen ((char *) args->interface_name);
-  strncpy ((char *) conn->args.interface_name, (char *) args->interface_name,
-          l);
+  strlcpy ((char *) conn->args.interface_name, (char *) args->interface_name,
+          sizeof (conn->args.interface_name));
 
-  if ((l = strlen ((char *) args->secret)) > 0)
-    strncpy ((char *) conn->args.secret, (char *) args->secret, l);
+  if ((strlen ((char *) args->secret)) > 0)
+    strlcpy ((char *) conn->args.secret, (char *) args->secret,
+            sizeof (conn->args.secret));
 
   if (args->socket != NULL)
     conn->args.socket = args->socket;
@@ -1260,7 +1252,7 @@ memif_request_connection (memif_conn_handle_t c)
 
   sun.sun_family = AF_UNIX;
 
-  strncpy (sun.sun_path, (char *) ms->filename, sizeof (sun.sun_path) - 1);
+  strlcpy (sun.sun_path, (char *) ms->filename, sizeof (sun.sun_path));
 
   if (connect (sockfd, (struct sockaddr *) &sun,
               sizeof (struct sockaddr_un)) == 0)

diff --git a/extras/libmemif/src/memif_private.h b/extras/libmemif/src/memif_private.h
index dd58d62..59899fd 100644 (file)
--- a/extras/libmemif/src/memif_private.h
+++ b/extras/libmemif/src/memif_private.h
@@ -66,6 +66,33 @@ _Static_assert (strlen (MEMIF_DEFAULT_APP_NAME) <= MEMIF_NAME_LEN,
 #define DBG(...)
 #endif /* MEMIF_DBG */
 
+#ifndef HAS_LIB_BSD
+static inline size_t
+strlcpy (char *dest, const char *src, size_t len)
+{
+  const char *s = src;
+  size_t n = len;
+
+  while (--n > 0)
+    {
+      if ((*dest++ = *s++) == '\0')
+       break;
+    }
+
+  if (n == 0)
+    {
+      if (len != 0)
+       *dest = '\0';
+      while (*s++)
+       ;
+    }
+
+  return (s - src - 1);
+}
+#else
+#include <bsd/string.h>
+#endif
+
 typedef enum
 {
   MEMIF_SOCKET_TYPE_NONE = 0,  /* unassigned, not used by any interface */

diff --git a/extras/libmemif/src/socket.c b/extras/libmemif/src/socket.c
index 2454616..b801cac 100644 (file)
--- a/extras/libmemif/src/socket.c
+++ b/extras/libmemif/src/socket.c
@@ -111,8 +111,7 @@ memif_msg_send_hello (libmemif_main_t * lm, int fd)
   h->max_region = MEMIF_MAX_REGION;
   h->max_log2_ring_size = MEMIF_MAX_LOG2_RING_SIZE;
 
-  strncpy ((char *) h->name, (char *) lm->app_name,
-          strlen ((char *) lm->app_name));
+  strlcpy ((char *) h->name, (char *) lm->app_name, sizeof (h->name));
 
   /* msg hello is not enqueued but sent directly,
      because it is the first msg to be sent */
@@ -139,8 +138,7 @@ memif_msg_enq_init (memif_connection_t * c)
   i->id = c->args.interface_id;
   i->mode = c->args.mode;
 
-  strncpy ((char *) i->name, (char *) lm->app_name,
-          strlen ((char *) lm->app_name));
+  strlcpy ((char *) i->name, (char *) lm->app_name, sizeof (i->name));
   if (strlen ((char *) c->args.secret) > 0)
     strncpy ((char *) i->secret, (char *) c->args.secret, sizeof (i->secret));
 
@@ -260,8 +258,8 @@ memif_msg_enq_connect (memif_connection_t * c)
 
   e->msg.type = MEMIF_MSG_TYPE_CONNECT;
   e->fd = -1;
-  strncpy ((char *) cm->if_name, (char *) c->args.interface_name,
-          strlen ((char *) c->args.interface_name));
+  strlcpy ((char *) cm->if_name, (char *) c->args.interface_name,
+          sizeof (cm->if_name));
 
   e->next = NULL;
   if (c->msg_queue == NULL)
@@ -295,8 +293,8 @@ memif_msg_enq_connected (memif_connection_t * c)
 
   e->msg.type = MEMIF_MSG_TYPE_CONNECTED;
   e->fd = -1;
-  strncpy ((char *) cm->if_name, (char *) c->args.interface_name,
-          strlen ((char *) c->args.interface_name));
+  strlcpy ((char *) cm->if_name, (char *) c->args.interface_name,
+          sizeof (cm->if_name));
 
   e->next = NULL;
   if (c->msg_queue == NULL)
@@ -327,12 +325,12 @@ memif_msg_send_disconnect (int fd, uint8_t * err_string, uint32_t err_code)
   msg.type = MEMIF_MSG_TYPE_DISCONNECT;
   d->code = err_code;
   uint16_t l = strlen ((char *) err_string);
-  if (l > 96)
+  if (l > sizeof (d->string) - 1)
     {
-      DBG ("Disconnect string too long. Sending first 96 characters.");
-      l = 96;
+      DBG ("Disconnect string too long. Sending the first %d characters.",
+          sizeof (d->string) - 1);
     }
-  strncpy ((char *) d->string, (char *) err_string, l);
+  strlcpy ((char *) d->string, (char *) err_string, sizeof (d->string));
 
   return memif_msg_send (fd, &msg, -1);
 }
@@ -356,8 +354,7 @@ memif_msg_receive_hello (memif_connection_t * c, memif_msg_t * msg)
   c->run_args.log2_ring_size = memif_min (h->max_log2_ring_size,
                                          c->args.log2_ring_size);
   c->run_args.buffer_size = c->args.buffer_size;
-  strncpy ((char *) c->remote_name, (char *) h->name,
-          strlen ((char *) h->name));
+  strlcpy ((char *) c->remote_name, (char *) h->name, sizeof (c->remote_name));
 
   return MEMIF_ERR_SUCCESS;    /* 0 */
 }
@@ -420,8 +417,7 @@ memif_msg_receive_init (memif_socket_t * ms, int fd, memif_msg_t * msg)
       goto error;
     }
 
-  strncpy ((char *) c->remote_name, (char *) i->name,
-          strlen ((char *) i->name));
+  strlcpy ((char *) c->remote_name, (char *) i->name, sizeof (c->remote_name));
 
   if (strlen ((char *) c->args.secret) > 0)
     {
@@ -588,8 +584,8 @@ memif_msg_receive_connect (memif_connection_t * c, memif_msg_t * msg)
   if (err != MEMIF_ERR_SUCCESS)
     return err;
 
-  strncpy ((char *) c->remote_if_name, (char *) cm->if_name,
-          strlen ((char *) cm->if_name));
+  strlcpy ((char *) c->remote_if_name, (char *) cm->if_name,
+          sizeof (c->remote_if_name));
 
   int i;
   if (c->on_interrupt != NULL)
@@ -625,7 +621,7 @@ memif_msg_receive_connected (memif_connection_t * c, memif_msg_t * msg)
     return err;
 
   strncpy ((char *) c->remote_if_name, (char *) cm->if_name,
-          strlen ((char *) cm->if_name));
+          sizeof (c->remote_if_name));
 
   int i;
   if (c->on_interrupt != NULL)
@@ -650,7 +646,7 @@ memif_msg_receive_disconnect (memif_connection_t * c, memif_msg_t * msg)
   memset (c->remote_disconnect_string, 0,
          sizeof (c->remote_disconnect_string));
   strncpy ((char *) c->remote_disconnect_string, (char *) d->string,
-          strlen ((char *) d->string));
+          sizeof (c->remote_disconnect_string));
 
   /* on returning error, handle function will call memif_disconnect () */
   DBG ("disconnect received: %s, mode: %d",