- developers - lists.mariadb.org

Re: [Maria-developers] 1a2748c: MDEV-15473 Isolate/sandbox PAM modules, so that they can't crash the server.
by Sergei Golubchik 10 Jul '18

10 Jul '18

Hi, Alexey! On Jul 10, Alexey Botchkov wrote: > > > Did you test that it works? > Of course i tested the mysql_install_db script a lot. > Didn't test the .rpm and .deb though. As far as i can see they plainly run > mysql_install_db --user=.. under root. could you please test with rpm and deb too? should only take 10-20 minutes in a buildbot VM. ok to push after that (if all goes well :)) Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 1a2748c: MDEV-15473 Isolate/sandbox PAM modules, so that they can't crash the server.
by Sergei Golubchik 09 Jul '18

09 Jul '18

Hi, Alexey! On Jul 09, Alexey Botchkov wrote: > revision-id: 1a2748ceb0e30770eebc97d82d7baa885b332c49 (mariadb-10.3.6-45-g1a2748c) > parent(s): aa01f51bdef9cc38d8e0a75ea9e2788651e41d16 > committer: Alexey Botchkov > timestamp: 2018-07-09 13:20:42 +0400 > message: > > MDEV-15473 Isolate/sandbox PAM modules, so that they can't crash the server. > > Scripts added to set safe permissions for the auth_pam_tool and it's > directory. > > diff --git a/scripts/mysql_install_db.sh b/scripts/mysql_install_db.sh > index ad7c028..fd26228 100644 > --- a/scripts/mysql_install_db.sh > +++ b/scripts/mysql_install_db.sh > @@ -308,6 +308,7 @@ then > srcpkgdatadir="$srcdir/scripts" > buildpkgdatadir="$builddir/scripts" > plugindir="$builddir/plugin/auth_socket" > + pamtooldir="$builddir/plugin/auth_pam" > elif test -n "$basedir" > then > bindir="$basedir/bin" # only used in the help text > @@ -337,6 +338,7 @@ then > exit 1 > fi > plugindir=`find_in_dirs --dir auth_socket.so $basedir/lib*/plugin $basedir/lib*/mysql/plugin` > + pamtooldir=`find_in_dirs --dir auth_pam.so $basedir/lib*/plugin $basedir/lib*/mysql/plugin` Why not just pamtooldir=$plugindir ? > else > basedir="@prefix@" > bindir="@bindir@" > @@ -445,6 +448,13 @@ done > > if test -n "$user" > then > + chown $user "$pamtooldir/auth_pam_tool_dir" > + if test $? -ne 0 > + then > + echo "Cannot change ownership of the '$pamtooldir\auth_pam_tool_dir' directory" > + echo " to the '$user' user. Check that you have the necessary permissions and try again." > + exit 1 > + fi Looks ok. Did you test that it works? I mean, a test woud be to install from a tarball and run mysql_install_db or install from rpm or deb (they run the install script automatically) and then check that all permissions are correct. Also, I suspect that at additional chown root "$pamtooldir/auth_pam_tool_dir/auth_pam_tool could also be useful here. But not when the script is running in the builddir. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] [Commits] 765ba2a: MDEV-15473 Isolate/sandbox PAM modules, so that they can't crash the server.
by Sergei Golubchik 06 Jul '18

06 Jul '18

Hi, Alexey! On Jul 05, Alexey Botchkov wrote: > revision-id: 765ba2ac76dab984183bf829dc3407713a4d5d9b (mariadb-10.3.6-40-g765ba2a) > parent(s): 7e704a2308e25953b5f8fb154eb325df3e25c2ec > committer: Alexey Botchkov > timestamp: 2018-07-05 17:00:47 +0400 > message: > > MDEV-15473 Isolate/sandbox PAM modules, so that they can't crash the server. > > Proper access permissions for the auth_pam_tool_dir and auth_pam_tool. > > diff --git a/plugin/auth_pam/CMakeLists.txt b/plugin/auth_pam/CMakeLists.txt > index 4943d57..b9313de 100644 > --- a/plugin/auth_pam/CMakeLists.txt > +++ b/plugin/auth_pam/CMakeLists.txt > @@ -11,7 +11,13 @@ IF(HAVE_PAM_APPL_H) > ADD_DEFINITIONS(-D_GNU_SOURCE) > MYSQL_ADD_PLUGIN(auth_pam_v1 auth_pam_v1.c LINK_LIBRARIES pam MODULE_ONLY) > MYSQL_ADD_PLUGIN(auth_pam auth_pam.c LINK_LIBRARIES pam dl MODULE_ONLY) > - MYSQL_ADD_EXECUTABLE(auth_pam_tool auth_pam_tool.c DESTINATION ${INSTALL_PLUGINDIR}/auth_pam_tool_dir COMPONENT Server) > + 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) > + INSTALL(CODE "EXECUTE_PROCESS( > + COMMAND chmod u=rwx,g=,o= auth_pam_tool_dir > + COMMAND chmod u=rwx,g=rx,o=rx auth_pam_tool_dir/auth_pam_tool > + COMMAND chmod +s auth_pam_tool_dir/auth_pam_tool > + WORKING_DIRECTORY \$ENV{DESTDIR}\${CMAKE_INSTALL_PREFIX}/${INSTALL_PLUGINDIR}/)" > + COMPONENT Server) I think it's generally ok. Two comments: minor: you can combine two chmods on auth_pam_tool in one, like u=rwxs major: you still need to make auth_pam_tool_dir to be owned by mysql user. I'm afraid the only way to do it is from a post-install scriptlet or from mysql_install_db. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] [Commits] af15686: MDEV-16242: MyRocks: parallel slave on a table without PK can stop with ER_KEY_NOT_FOUND
by Kristian Nielsen 05 Jul '18

05 Jul '18

psergey(a)askmonty.org (Sergei Petrunia) writes: > MDEV-16242: MyRocks: parallel slave on a table without PK can stop with ER_KEY_NOT_FOUND > > DRAFT: If RBR event applier uses a secondary key or a full table scan > to locate a row, force waiting for prior commit to complete. I think if you wait_for_prior_commit() also for secondary key case, you should definitely restrict it to be only for conservative parallel replication, not for optimistic/agressive. Using a secondary key seems generally fine (as opposed to full table scan), and this problem should affect only conservative mode, no need to penalise optimistic/aggressive. (But maybe that is what is meant with "DRAFT"). - Kristian.

2 2

Re: [Maria-developers] b88e98df9da: C++11 experiment: check if all builders support it
by Sergei Golubchik 04 Jul '18

04 Jul '18

Hi, Sergey! > Current status is: all staging builders pass, except for Ubuntu Precise. But we no longer build on precise, do we? Any followup MDEVs after this one is pushed? Like migrate atomics ops to C++11 or something? > revision-id: b88e98df9daa586bd6ed1f5908ededaa65a6e464 (mariadb-10.3.6-16-gb88e98df9da) > parent(s): 9988a423d229730b252942b7fa066d1591b7eca9 > author: Sergey Vojtovich > committer: Sergey Vojtovich > timestamp: 2018-06-04 12:41:18 +0400 > message: > > C++11 experiment: check if all builders support it > > diff --git a/CMakeLists.txt b/CMakeLists.txt > index 76b0817f8c7..efeae3d1ca5 100644 > --- a/CMakeLists.txt > +++ b/CMakeLists.txt > @@ -89,6 +89,14 @@ ELSE() > ENDIF() > PROJECT(${MYSQL_PROJECT_NAME}) > > +IF(CMAKE_VERSION VERSION_LESS "3.1") > + IF(CMAKE_CXX_COMPILER_ID STREQUAL "GNU") > + SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=gnu++11") Why gnu++11 and not c++11 ? And why not for clang? > + ENDIF() > +ELSE() > + SET(CMAKE_CXX_STANDARD 11) I'd expect this to mean c++11, not gnu++11. > +ENDIF() > + > SET(CPACK_PACKAGE_NAME "MariaDB") > SET(CPACK_PACKAGE_DESCRIPTION_SUMMARY "MariaDB: a very fast and robust SQL database server") > SET(CPACK_PACKAGE_URL "http://mariadb.org") > diff --git a/storage/tokudb/PerconaFT/portability/toku_crash.cc b/storage/tokudb/PerconaFT/portability/toku_crash.cc > index 0af85342a99..297cc29d9ca 100644 > --- a/storage/tokudb/PerconaFT/portability/toku_crash.cc > +++ b/storage/tokudb/PerconaFT/portability/toku_crash.cc > @@ -70,7 +70,7 @@ run_gdb(pid_t parent_pid, const char *gdb_path) { > "-ex", "thread apply all bt", > "-ex", "thread apply all bt full", > exe_buf, pid_buf, > - NULL); > + (char*) NULL); Why's that? Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

3 3

[Maria-developers] MDEV-13995 MAX(timestamp) returns a wrong result near DST change
by Alexander Barkov 03 Jul '18

03 Jul '18

Hi Sergei, Please review a patch for MDEV-13995. Thanks!

1 0

[Maria-developers] FOR loop documentation
by Alexander Barkov 03 Jul '18

03 Jul '18

Hi Ian, can you please document the FOR loop? I've created an MDEV for it: MDEV-16674 Document FOR loop with links to all relevant tasks. Thanks!

1 0

Re: [Maria-developers] ed2dcf0: MDEV-15473 Isolate/sandbox PAM modules, so that they can't crash the server.
by Sergei Golubchik 01 Jul '18

01 Jul '18

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

1 0

Re: [Maria-developers] 84c9ec5: MDEV-15473 Isolate/sandbox PAM modules, so that they can't crash the server.
by Sergei Golubchik 01 Jul '18

01 Jul '18

Hi, Alexey! On Jun 30, Alexey Botchkov wrote: > > not sure about calling it "safe". I think it's more of a side > > effect, the main feature it that it works, while old pam plugin > > simply doesn't :) unless mysqld is run as root. > > > > and not totally sure about calling it also pam. it means one won't > > be able to load it and the old pam plugin at the same time. I > > suspect it'll still ok and benefits overweight it. > > Well i'm open to any ideas here :) > We can swap 'safe' with somethin else. 'box' for instance. > Or we can instead rename the original version like 'fast' or 'old'. > Finally we can build only one version of the plugin. The 'safe' > whenever possible. I'd rather rename the old version to (old) or (requires root) or something. > I also have a question about the testing. > Now pam*.test-s rely on some mariadb_mtr setup for PAM. How can i see what > is in the expected pam.d configuration file, and what pam modules are used? See plugin/auth_pam/testing/pam_mariadb_mtr.c Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] FYI: 10.3 master branch head builds OK Travis-CI
by Otto Kekäläinen 30 Jun '18

30 Jun '18

Hello! I am happy to announce that of this morning, Travis-CI for MariaDB 10.3 shows all green again! [1] Developers: please rebase your 10.3 branches on top of the latest 10.3 to get the Travis-CI benefits, and please don't break Travis. Last time it was all green in January it took only a few weeks until it was broken again. We don't have protected branches enabled yet, so we rely on all developers to be diligent and not push changes that break the all-green-status on Travis-CI for 10.3. Otto [1] https://travis-ci.org/MariaDB/server/builds [2} https://mariadb.org/protected-branches-ensuring-code-quality-git/

1 0