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@mariadb.org