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(a)mariadb.org
