maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexey!

Looks good, few comments/suggestions below.
That and my reply to your email (about tests and plugin name).

Just push when you're ready, no need to put it In-Review again.

Thanks!

On Jul 01, Alexey Botchkov wrote:
> revision-id: ed2dcf00fd26e363f34638a841ff17a111b7e324 (mariadb-10.3.6-33-ged2dcf0)
> parent(s): 4b0cedf82d8d8ba582648dcb4a2620c146862a43
> committer: Alexey Botchkov
> timestamp: 2018-07-01 12:23:54 +0400
> message:
> 
> MDEV-15473 Isolate/sandbox PAM modules, so that they can't crash the server.
> 
> The new 'safe' version of the PAM plugin.
> It works using the auth_pam_tool executable. That isolates possible
> crashes in pam modules and, since we can set the +s bit for the tool,
> we can normally run modules that read protected information.
> 
> diff --git a/plugin/auth_pam/CMakeLists.txt b/plugin/auth_pam/CMakeLists.txt
> index 5131752..9c0859c 100644
> --- a/plugin/auth_pam/CMakeLists.txt
> +++ b/plugin/auth_pam/CMakeLists.txt
> @@ -8,6 +8,10 @@ IF(HAVE_PAM_APPL_H)
>    IF(HAVE_STRNDUP)
>      ADD_DEFINITIONS(-DHAVE_STRNDUP)
>    ENDIF(HAVE_STRNDUP)
> +  ADD_DEFINITIONS(-D_GNU_SOURCE)
>    MYSQL_ADD_PLUGIN(auth_pam auth_pam.c LINK_LIBRARIES pam MODULE_ONLY)
> +  MYSQL_ADD_PLUGIN(auth_pam_safe auth_pam_safe.c LINK_LIBRARIES pam dl MODULE_ONLY)

perhaps, rename to auth_pam_new.c (and auth_pam_new.so)?

> +  MYSQL_ADD_EXECUTABLE(auth_pam_tool auth_pam_tool.c DESTINATION  ${INSTALL_PLUGINDIR}/auth_pam_tool_dir COMPONENT Server)
> +  TARGET_LINK_LIBRARIES(auth_pam_tool pam)
>  ENDIF(HAVE_PAM_APPL_H)
>  
> diff --git a/plugin/auth_pam/auth_pam.c b/plugin/auth_pam/auth_pam.c
> index ffc3d6f..c785ba4 100644
> --- a/plugin/auth_pam/auth_pam.c
> +++ b/plugin/auth_pam/auth_pam.c
> @@ -1,5 +1,5 @@
>  /*
> -   Copyright (c) 2011, 2012, Monty Program Ab
> +   Copyright (c) 2018 MariaDB Corporation

should be "2011, 2018" not just "2018"

>     This program is free software; you can redistribute it and/or modify
>     it under the terms of the GNU General Public License as published by
>  static struct st_mysql_auth info =
> diff --git a/plugin/auth_pam/auth_pam_base.c b/plugin/auth_pam/auth_pam_base.c
> new file mode 100644
> index 0000000..30b4e01
> --- /dev/null
> +++ b/plugin/auth_pam/auth_pam_base.c
> @@ -0,0 +1,171 @@
> +/*
> +   Copyright (c) 2018 MariaDB Corporation
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; version 2 of the License.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program; if not, write to the Free Software
> +   Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02111-1301 USA */
> +
> +// struct param {
> +  // unsigned char buf[10240], *ptr;
> +  // MYSQL_PLUGIN_VIO *vio;
> +// };
> +//static int write_packet(struct param *param, const unsigned char *buf,
> +                        //int buf_len)
> +//static int read_packet(struct param *param, unsigned char **pkt)

forgot to remove these comments?

> +
> +#include <stdio.h>
> +#include <string.h>
> +#include <security/pam_appl.h>
> +#include <security/pam_modules.h>
> +
> diff --git a/plugin/auth_pam/auth_pam_safe.c b/plugin/auth_pam/auth_pam_safe.c
> new file mode 100644
> index 0000000..9dac90d
> --- /dev/null
> +++ b/plugin/auth_pam/auth_pam_safe.c
> @@ -0,0 +1,235 @@
...
> +    PAM_DEBUG((stderr, "PAM: Child process prepares pipes.\n"));
> +    
> +    if (close(p_to_c[1]) < 0 ||
> +        close(c_to_p[0]) < 0 ||
> +        dup2(p_to_c[0], 0) < 0 || /* Parent's pipe to STDIN. */
> +        dup2(c_to_p[1], 1) < 0)   /* Sandbox's pipe to STDOUT. */
> +    {
> +      exit(-1);
> +    }
> +
> +    PAM_DEBUG((stderr, "PAM: check tool directory: %s, %s.\n",
> +                       opt_plugin_dir, tool_name));
> +    plugin_dir_len= strlen(opt_plugin_dir);
> +    if (plugin_dir_len + tool_name_len + 2 > sizeof(toolpath))
> +    {
> +      /* Tool path too long. */
> +      exit(-1);
> +    }
> +
> +    memcpy(toolpath, opt_plugin_dir, plugin_dir_len);
> +    if (plugin_dir_len && toolpath[plugin_dir_len-1] != FN_LIBCHAR)
> +      toolpath[plugin_dir_len++]= FN_LIBCHAR;
> +    memcpy(toolpath+plugin_dir_len, tool_name, tool_name_len+1);

check the length before copying?
like, toolpath+plugin_dir_len+tool_name_len+1 < sizeof(toolpath) ?

or simply

  strnxfrm(toolpath, sizeof(toolpath), opt_plugin_dir, "/", tool_name, NULL);

> +
> +    PAM_DEBUG((stderr, "PAM: execute pam sandbox [%s].\n", toolpath));
> +    (void) execl(toolpath, toolpath, NULL);
> +    PAM_DEBUG((stderr, "PAM: exec() failed.\n"));
> +    exit(-1);
> +  }
...
> +static char use_cleartext_plugin;
> +static MYSQL_SYSVAR_BOOL(use_cleartext_plugin, use_cleartext_plugin,
> +       PLUGIN_VAR_NOCMDARG | PLUGIN_VAR_READONLY,
> +       "Use mysql_cleartext_plugin on the client side instead of the dialog "
> +       "plugin. This may be needed for compatibility reasons, but it only "
> +       "supports simple PAM policies that don't require anything besides "
> +       "a password", NULL, NULL, 0);
> +
> +#ifndef DBUG_OFF
> +static MYSQL_SYSVAR_BOOL(debug, pam_debug, PLUGIN_VAR_OPCMDARG,
> +       "Log all PAM activity", NULL, NULL, 0);
> +#endif

still a lot of similar code in auth_pam.c and auth_pam_safe.c. could be
moved to auth_pam_common.c?

> +
> +
> diff --git a/plugin/auth_pam/auth_pam_tool.c b/plugin/auth_pam/auth_pam_tool.c
> new file mode 100644
> index 0000000..5f19d29
> --- /dev/null
> +++ b/plugin/auth_pam/auth_pam_tool.c
> @@ -0,0 +1,132 @@
> +
> +int main(int argc, char **argv)
> +{
> +  struct param param;
> +  MYSQL_SERVER_AUTH_INFO info;
> +  unsigned char field;
> +  int res;
> +  char a_buf[MYSQL_USERNAME_LENGTH + 1 + 1024];
> +
> +  if (read(0, &field, 1) < 1)
> +    return -1;
> +#ifndef DBUG_OFF
> +  pam_debug= field;
> +#endif
> +  
> +  PAM_DEBUG((stderr, "PAM: sandbox started [%s].\n", argv[0]));
> +
> +  info.user_name= a_buf;
> +  if ((res= read_string(0, info.user_name, sizeof(a_buf))) < 0)
> +    return -1;
> +  PAM_DEBUG((stderr, "PAM: sandbox username [%s].\n", info.user_name));
> +
> +#ifndef DBUG_OFF
> +  /* Produce the crash for testing purposes. */
> +  if ((res == 14) &&
> +      memcmp(info.user_name, "crash_pam_tool", 14) == 0)
> +  {
> +    info.user_name= 0;
> +    memcpy(info.user_name, a_buf, sizeof(a_buf));
> +    return -1;
> +  }

now I think it's better to put it into pam_mariadb_mtr.c, sorry :(
then it doesn't need to be in debug-only builds

> +#endif
> +
> +  info.auth_string= info.user_name + res + 1;
> +  if (read_string(0, info.auth_string, sizeof(a_buf) - 1 - res) < 0)
> +    return -1;
> +
> +  PAM_DEBUG((stderr, "PAM: sandbox auth string [%s].\n", info.auth_string));

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx