List:Commits« Previous MessageNext Message »
From:Mats Kindahl Date:December 1 2010 12:54pm
Subject:bzr commit into mysql-5.1-bugteam branch (mats.kindahl:3508) Bug#58246
View as plain text  
#At file:///home/bzr/bugs/b58246-5.1-bugteam/ based on revid:davi.arnaut@stripped

 3508 Mats Kindahl	2010-12-01
      BUG#58246: INSTALL PLUGIN not secure & crashable
      
      When installing plugins, there is a missing check
      for slash (/) in the path on Windows. Note that on
      Windows, both / and \ can be used to separate
      directories.
      
      This patch fixes the issue by:
      - Adding a FN_DIRSEP symbol for all platforms
        consisting of a string of legal directory
        separators.
      - Adding a charset-aware version of strcspn().
      - Adding a check_valid_path() function that uses
        my_strcspn() to check if any FN_DIRSEP character
        is in the supplied string.
      - Using the check_valid_path() function in
        sql_plugin.cc and sql_udf.cc (which means
        replacing the existing test there).
     @ include/config-netware.h
        Adding FN_DIRSEP
        ******
        Adding FN_DIRSEP
     @ include/config-win.h
        Adding FN_DIRSEP
        ******
        Adding FN_DIRSEP
     @ include/m_ctype.h
        Adding my_strspn() and my_strcspn().
        
        ******
        Adding my_strspn() and my_strcspn().
     @ include/my_global.h
        Adding FN_DIRSEP
        ******
        Adding FN_DIRSEP
     @ mysql-test/t/plugin_not_embedded.test
        Adding test that file names containing / is
        disallowed on *all* platforms.
        ******
        Adding test that file names containing / is
        disallowed on *all* platforms.
     @ sql/sql_plugin.cc
        Introducing check_if_path() function for
        checking if filename is a path to include
        / on Windows.
        ******
        Introducing check_if_path() function for
        checking if filename is a path to include
        / on Windows.
     @ sql/sql_udf.cc
        Switching to use check_if_path() function.
        ******
        Switching to use check_if_path() function.
     @ strings/my_strchr.c
        Adding my_strspn() and my_strcspn().
        ******
        Adding my_strspn() and my_strcspn().

    modified:
      include/config-netware.h
      include/config-win.h
      include/m_ctype.h
      include/my_global.h
      mysql-test/r/plugin_not_embedded.result
      mysql-test/t/plugin_not_embedded.test
      sql/sql_plugin.cc
      sql/sql_plugin.h
      sql/sql_udf.cc
      strings/my_strchr.c
=== modified file 'include/config-netware.h'
--- a/include/config-netware.h	2009-07-31 19:28:15 +0000
+++ b/include/config-netware.h	2010-12-01 12:54:50 +0000
@@ -122,6 +122,7 @@ extern "C" {
 #define CANT_DELETE_OPEN_FILES 1
 
 #define FN_LIBCHAR '\\'
+#define FN_DIRSEP  "/\\"              /* Valid directory separators */
 #define FN_ROOTDIR "\\"
 #define FN_DEVCHAR ':'
 

=== modified file 'include/config-win.h'
--- a/include/config-win.h	2009-12-20 18:02:15 +0000
+++ b/include/config-win.h	2010-12-01 12:54:50 +0000
@@ -332,6 +332,7 @@ inline ulonglong double2ulonglong(double
 /* File name handling */
 
 #define FN_LIBCHAR	'\\'
+#define FN_DIRSEP       "/\\"               /* Valid directory separators */
 #define FN_ROOTDIR	"\\"
 #define FN_DEVCHAR	':'
 #define FN_NETWORK_DRIVES	/* Uses \\ to indicate network drives */

=== modified file 'include/m_ctype.h'
--- a/include/m_ctype.h	2009-06-10 08:59:49 +0000
+++ b/include/m_ctype.h	2010-12-01 12:54:50 +0000
@@ -464,6 +464,8 @@ extern my_bool my_parse_charset_xml(cons
 				    int (*add)(CHARSET_INFO *cs));
 extern char *my_strchr(CHARSET_INFO *cs, const char *str, const char *end,
                        pchar c);
+extern size_t my_strcspn(CHARSET_INFO *cs, const char *str, const char *end,
+                         const char *accept);
 
 my_bool my_propagate_simple(CHARSET_INFO *cs, const uchar *str, size_t len);
 my_bool my_propagate_complex(CHARSET_INFO *cs, const uchar *str, size_t len);

=== modified file 'include/my_global.h'
--- a/include/my_global.h	2010-07-14 19:39:40 +0000
+++ b/include/my_global.h	2010-12-01 12:54:50 +0000
@@ -758,6 +758,7 @@ typedef SOCKET_SIZE_TYPE size_socket;
 
 #ifndef FN_LIBCHAR
 #define FN_LIBCHAR	'/'
+#define FN_DIRSEP       "/"     /* Valid directory separators */
 #define FN_ROOTDIR	"/"
 #endif
 #define MY_NFILE	64	/* This is only used to save filenames */

=== modified file 'mysql-test/r/plugin_not_embedded.result'
--- a/mysql-test/r/plugin_not_embedded.result	2010-03-13 21:32:42 +0000
+++ b/mysql-test/r/plugin_not_embedded.result	2010-12-01 12:54:50 +0000
@@ -8,3 +8,5 @@ ERROR 42000: DELETE command denied to us
 GRANT DELETE ON mysql.plugin TO bug51770@localhost;
 UNINSTALL PLUGIN example;
 DROP USER bug51770@localhost;
+INSTALL PLUGIN example SONAME '../ha_example.so';
+ERROR HY000: No paths allowed for shared library

=== modified file 'mysql-test/t/plugin_not_embedded.test'
--- a/mysql-test/t/plugin_not_embedded.test	2010-03-14 11:16:59 +0000
+++ b/mysql-test/t/plugin_not_embedded.test	2010-12-01 12:54:50 +0000
@@ -18,3 +18,14 @@ UNINSTALL PLUGIN example;
 disconnect con1;
 connection default;
 DROP USER bug51770@localhost;
+
+# 
+# BUG#58246: INSTALL PLUGIN not secure & crashable
+#
+# The bug consisted of not recognizing / on Windows, so checking / on
+# all platforms should cover this case.
+
+let $path = `select CONCAT_WS('/', '..', $HA_EXAMPLE_SO)`;
+--error ER_UDF_NO_PATHS
+eval INSTALL PLUGIN example SONAME '$path';
+

=== modified file 'sql/sql_plugin.cc'
--- a/sql/sql_plugin.cc	2010-08-05 12:10:24 +0000
+++ b/sql/sql_plugin.cc	2010-12-01 12:54:50 +0000
@@ -231,6 +231,26 @@ extern bool check_if_table_exists(THD *t
 #endif /* EMBEDDED_LIBRARY */
 
 
+/**
+   Check if the provided path is valid in the sense that it does cause
+   a relative reference outside the directory.
+
+   @note Currently, this function only check if there are any
+   characters in FN_DIRSEP in the string, but it might change in the
+   future.
+
+   @code
+   check_valid_path("../foo.so") -> true
+   check_valid_path("foo.so") -> false
+   @endcode
+ */
+bool check_valid_path(const char *path, size_t len)
+{
+  size_t prefix= my_strcspn(files_charset_info, path, path + len, FN_DIRSEP);
+  return  prefix < len;
+}
+
+
 /****************************************************************************
   Value type thunks, allows the C world to play in the C++ world
 ****************************************************************************/
@@ -354,13 +374,14 @@ static st_plugin_dl *plugin_dl_add(const
   struct st_plugin_dl *tmp, plugin_dl;
   void *sym;
   DBUG_ENTER("plugin_dl_add");
+  DBUG_PRINT("enter", ("dl->str: '%s', dl->length: %d", dl->str, dl->length));
   plugin_dir_len= strlen(opt_plugin_dir);
   /*
     Ensure that the dll doesn't have a path.
     This is done to ensure that only approved libraries from the
     plugin directory are used (to make this even remotely secure).
   */
-  if (my_strchr(files_charset_info, dl->str, dl->str + dl->length, FN_LIBCHAR) ||
+  if (check_valid_path(dl->str, dl->length) ||
       check_string_char_length((LEX_STRING *) dl, "", NAME_CHAR_LEN,
                                system_charset_info, 1) ||
       plugin_dir_len + dl->length + 1 >= FN_REFLEN)

=== modified file 'sql/sql_plugin.h'
--- a/sql/sql_plugin.h	2009-05-14 12:03:33 +0000
+++ b/sql/sql_plugin.h	2010-12-01 12:54:50 +0000
@@ -131,6 +131,7 @@ extern bool mysql_uninstall_plugin(THD *
 extern bool plugin_register_builtin(struct st_mysql_plugin *plugin);
 extern void plugin_thdvar_init(THD *thd);
 extern void plugin_thdvar_cleanup(THD *thd);
+extern bool check_valid_path(const char *path, size_t length);
 
 typedef my_bool (plugin_foreach_func)(THD *thd,
                                       plugin_ref plugin,

=== modified file 'sql/sql_udf.cc'
--- a/sql/sql_udf.cc	2010-01-25 02:55:05 +0000
+++ b/sql/sql_udf.cc	2010-12-01 12:54:50 +0000
@@ -173,10 +173,7 @@ void udf_init()
 
       On windows we must check both FN_LIBCHAR and '/'.
     */
-    if (my_strchr(files_charset_info, dl_name,
-                  dl_name + strlen(dl_name), FN_LIBCHAR) ||
-        IF_WIN(my_strchr(files_charset_info, dl_name,
-                         dl_name + strlen(dl_name), '/'), 0) ||
+    if (check_valid_path(dl_name, strlen(dl_name)) ||
         check_string_char_length(&name, "", NAME_CHAR_LEN,
                                  system_charset_info, 1))
     {
@@ -416,13 +413,8 @@ int mysql_create_function(THD *thd,udf_f
     Ensure that the .dll doesn't have a path
     This is done to ensure that only approved dll from the system
     directories are used (to make this even remotely secure).
-
-    On windows we must check both FN_LIBCHAR and '/'.
   */
-  if (my_strchr(files_charset_info, udf->dl,
-                udf->dl + strlen(udf->dl), FN_LIBCHAR) ||
-      IF_WIN(my_strchr(files_charset_info, udf->dl,
-                       udf->dl + strlen(udf->dl), '/'), 0))
+  if (check_valid_path(udf->dl, strlen(udf->dl)))
   {
     my_message(ER_UDF_NO_PATHS, ER(ER_UDF_NO_PATHS), MYF(0));
     DBUG_RETURN(1);

=== modified file 'strings/my_strchr.c'
--- a/strings/my_strchr.c	2007-02-23 11:13:55 +0000
+++ b/strings/my_strchr.c	2010-12-01 12:54:50 +0000
@@ -13,6 +13,45 @@
    along with this program; if not, write to the Free Software
    Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA */
 
+#include <my_global.h>
+#include "m_string.h"
+#include "m_ctype.h"
+
+#define NEQ(A, B) ((A) != (B))
+#define EQU(A, B) ((A) == (B))
+
+/**
+  Macro for the body of the string scanning.
+
+  @param CS  The character set of the string
+  @param STR Pointer to beginning of string
+  @param END Pointer to one-after-end of string
+  @param ACC Pointer to beginning of accept (or reject) string
+  @param LEN Length of accept (or reject) string
+  @param CMP is a function-like for doing the comparison of two characters.
+ */
+
+#define SCAN_STRING(CS, STR, END, ACC, LEN, CMP)                        \
+  do {                                                                  \
+    uint mbl;                                                           \
+    const char *ptr_str, *ptr_acc;                                      \
+    const char *acc_end= (ACC) + (LEN);                                 \
+    for (ptr_str= (STR) ; ptr_str < (END) ; ptr_str+= mbl)              \
+    {                                                                   \
+      mbl= my_mbcharlen((CS), *(uchar*)ptr_str);                        \
+      if (mbl < 2)                                                      \
+      {                                                                 \
+        DBUG_ASSERT(mbl == 1);                                          \
+        for (ptr_acc= (ACC) ; ptr_acc < acc_end ; ++ptr_acc)            \
+          if (CMP(*ptr_acc, *ptr_str))                                  \
+            goto end;                                                   \
+      }                                                                 \
+    }                                                                   \
+end:                                                                    \
+    return (size_t) (ptr_str - (STR));                                  \
+  } while (0)
+
+
 /*
   my_strchr(cs, str, end, c) returns a pointer to the first place in
   str where c (1-byte character) occurs, or NULL if c does not occur
@@ -21,11 +60,6 @@
   frequently.
 */
 
-#include <my_global.h>
-#include "m_string.h"
-#include "m_ctype.h"
-
-
 char *my_strchr(CHARSET_INFO *cs, const char *str, const char *end,
                 pchar c)
 {
@@ -45,3 +79,26 @@ char *my_strchr(CHARSET_INFO *cs, const 
   return(0);
 }
 
+/**
+  Calculate the length of the initial segment of 'str' which consists
+  entirely of characters not in 'reject'.
+
+  @note The reject string points to single-byte characters so it is
+  only possible to find the first occurrence of a single-byte
+  character.  Multi-byte characters in 'str' are treated as not
+  matching any character in the reject string.
+
+  @todo should be moved to CHARSET_INFO if it's going to be called
+  frequently.
+
+  @internal The implementation builds on the assumption that 'str' is long,
+  while 'reject' is short. So it compares each character in string
+  with the characters in 'reject' in a tight loop over the characters
+  in 'reject'.
+*/
+
+size_t my_strcspn(CHARSET_INFO *cs, const char *str, const char *str_end,
+                  const char *reject)
+{
+  SCAN_STRING(cs, str, str_end, reject, strlen(reject), EQU);
+}


Attachment: [text/bzr-bundle] bzr/mats.kindahl@oracle.com-20101201125450-b82p91vtwymmjxh6.bundle
Thread
bzr commit into mysql-5.1-bugteam branch (mats.kindahl:3508) Bug#58246Mats Kindahl1 Dec
Re: bzr commit into mysql-5.1-bugteam branch (mats.kindahl:3508)Bug#58246Alexander Barkov6 Dec