maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexey!

Just a couple of comments (args[] vs stdio and how to prevent
brute-forcing), see below.

Otherwise looks pretty good.

On Jun 06, Alexey Botchkov wrote:
> revision-id: 84c9ec5552bf14bda826912fb045ae36f34a4490 (mariadb-10.3.6-9-g84c9ec5)
> parent(s): 637af7838316a49267e55cde7cf96e23f63e5c9d
> committer: Alexey Botchkov
> timestamp: 2018-06-06 14:13:07 +0400
> message:
> 
> MDEV-15473 Isolate/sandbox PAM modules, so that they can't crash the server.
> 
> Crash-safe version of the PAM plugin added. It runs the auth_pam_tool
> application that actually calls the PAM modules.
>
> diff --git a/plugin/auth_pam/auth_pam_base.c b/plugin/auth_pam/auth_pam_base.c
> new file mode 100644
> index 0000000..885e6f0
> --- /dev/null
> +++ b/plugin/auth_pam/auth_pam_base.c
> @@ -0,0 +1,173 @@
> +/*
> +   Copyright (c) 2011, 2012, Monty Program Ab

could be updated...

> +
> +   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)
> +
> +#define _GNU_SOURCE 1 /* for strndup */
> +
> +#include <stdio.h>
> +#include <string.h>
> +#include <security/pam_appl.h>
> +#include <security/pam_modules.h>
> +
> +/* It least solaris doesn't have strndup */
> +
> +#ifndef HAVE_STRNDUP
> +char *strndup(const char *from, size_t length)
> +{
> +  char *ptr;
> +  size_t max_length= strlen(from);
> +  if (length > max_length)
> +    length= max_length;
> +  if ((ptr= (char*) malloc(length+1)) != 0)
> +  {
> +    memcpy((char*) ptr, (char*) from, length);
> +    ptr[length]=0;
> +  }
> +  return ptr;
> +}
> +#endif
> +
> +#ifndef DBUG_OFF
> +static char pam_debug = 0;
> +#define PAM_DEBUG(X)   do { if (pam_debug) { fprintf X; } } while(0)
> +#else
> +#define PAM_DEBUG(X)   /* no-op */
> +#endif
> +
> +static int conv(int n, const struct pam_message **msg,
> +                struct pam_response **resp, void *data)
> +{
> +  struct param *param = (struct param *)data;
> +  unsigned char *end = param->buf + sizeof(param->buf) - 1;
> +  int i;
> +
> +  *resp = 0;
> +
> +  for (i = 0; i < n; i++)
> +  {
> +    /* if there's a message - append it to the buffer */
> +    if (msg[i]->msg)
> +    {
> +      int len = strlen(msg[i]->msg);
> +      if (len > end - param->ptr)
> +        len = end - param->ptr;
> +      if (len > 0)
> +      {
> +        memcpy(param->ptr, msg[i]->msg, len);
> +        param->ptr+= len;
> +        *(param->ptr)++ = '\n';
> +      }
> +    }
> +    /* if the message style is *_PROMPT_*, meaning PAM asks a question,
> +       send the accumulated text to the client, read the reply */
> +    if (msg[i]->msg_style == PAM_PROMPT_ECHO_OFF ||
> +        msg[i]->msg_style == PAM_PROMPT_ECHO_ON)
> +    {
> +      int pkt_len;
> +      unsigned char *pkt;
> +
> +      /* allocate the response array.
> +         freeing it is the responsibility of the caller */
> +      if (*resp == 0)
> +      {
> +        *resp = calloc(sizeof(struct pam_response), n);
> +        if (*resp == 0)
> +          return PAM_BUF_ERR;
> +      }
> +
> +      /* dialog plugin interprets the first byte of the packet
> +         as the magic number.
> +           2 means "read the input with the echo enabled"
> +           4 means "password-like input, echo disabled"
> +         C'est la vie. */
> +      param->buf[0] = msg[i]->msg_style == PAM_PROMPT_ECHO_ON ? 2 : 4;
> +      PAM_DEBUG((stderr, "PAM: conv: send(%.*s)\n",
> +                (int)(param->ptr - param->buf - 1), param->buf));
> +      if (write_packet(param, param->buf, param->ptr - param->buf - 1))
> +        return PAM_CONV_ERR;
> +
> +      pkt_len = read_packet(param, &pkt);
> +      if (pkt_len < 0)
> +      {
> +        PAM_DEBUG((stderr, "PAM: conv: recv() ERROR\n"));
> +        return PAM_CONV_ERR;
> +      }
> +      PAM_DEBUG((stderr, "PAM: conv: recv(%.*s)\n", pkt_len, pkt));
> +      /* allocate and copy the reply to the response array */
> +      if (!((*resp)[i].resp= strndup((char*) pkt, pkt_len)))
> +        return PAM_CONV_ERR;
> +      param->ptr = param->buf + 1;
> +    }
> +  }
> +  return PAM_SUCCESS;
> +}
> +
> +#define DO(X) if ((status = (X)) != PAM_SUCCESS) goto end
> +
> +#if defined(SOLARIS) || defined(__sun)
> +typedef void** pam_get_item_3_arg;
> +#else
> +typedef const void** pam_get_item_3_arg;
> +#endif
> +
> +static int pam_auth_base(struct param *param, MYSQL_SERVER_AUTH_INFO *info)
> +{
> +  pam_handle_t *pamh = NULL;
> +  int status;
> +  const char *new_username= NULL;
> +  /* The following is written in such a way to make also solaris happy */
> +  struct pam_conv pam_start_arg = { &conv, (char*) param };
> +
> +  /*
> +    get the service name, as specified in
> +
> +     CREATE USER ... IDENTIFIED WITH pam AS "service"
> +  */
> +  const char *service = info->auth_string && info->auth_string[0]
> +                          ? info->auth_string : "mysql";
> +
> +  param->ptr = param->buf + 1;
> +
> +  PAM_DEBUG((stderr, "PAM: pam_start(%s, %s)\n", service, info->user_name));
> +  DO( pam_start(service, info->user_name, &pam_start_arg, &pamh) );
> +
> +  PAM_DEBUG((stderr, "PAM: pam_authenticate(0)\n"));
> +  DO( pam_authenticate (pamh, 0) );
> +
> +  PAM_DEBUG((stderr, "PAM: pam_acct_mgmt(0)\n"));
> +  DO( pam_acct_mgmt(pamh, 0) );
> +
> +  PAM_DEBUG((stderr, "PAM: pam_get_item(PAM_USER)\n"));
> +  DO( pam_get_item(pamh, PAM_USER, (pam_get_item_3_arg) &new_username) );
> +
> +  if (new_username && strcmp(new_username, info->user_name))
> +    strncpy(info->authenticated_as, new_username,
> +            sizeof(info->authenticated_as));
> +  info->authenticated_as[sizeof(info->authenticated_as)-1]= 0;
> +
> +end:
> +  pam_end(pamh, status);
> +  PAM_DEBUG((stderr, "PAM: status = %d user = %s\n", status, info->authenticated_as));
> +  return status == PAM_SUCCESS ? CR_OK : CR_ERROR;
> +}
> +
> diff --git a/plugin/auth_pam/auth_pam_safe.c b/plugin/auth_pam/auth_pam_safe.c
> new file mode 100644
> index 0000000..28c10b8
> --- /dev/null
> +++ b/plugin/auth_pam/auth_pam_safe.c
> @@ -0,0 +1,235 @@
> +/*
> +   Copyright (c) 2011, 2012, Monty Program Ab

could be updated...

> +
> +   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 */
> +
> +#define _GNU_SOURCE 1 /* for strndup */
> +
> +#include <unistd.h>
> +#include <string.h>
> +#include <mysql/plugin_auth.h>
> +#include "auth_pam_tool.h"
> +#include <my_global.h>
> +
> +#ifndef DBUG_OFF
> +static char pam_debug = 0;
> +#define PAM_DEBUG(X)   do { if (pam_debug) { fprintf X; } } while(0)
> +#else
> +#define PAM_DEBUG(X)   /* no-op */
> +#endif
> +
> +static char *opt_plugin_dir; /* To be dynamically linked. */
> +static const char *tool_name= "auth_pam_tool";
> +static const int tool_name_len= 13;
> +
> +static int pam_auth(MYSQL_PLUGIN_VIO *vio, MYSQL_SERVER_AUTH_INFO *info)
> +{
> +  int p_to_c[2], c_to_p[2]; /* Parent-to-child and child-to-parent pipes. */
> +  pid_t proc_id;
> +  int result= CR_ERROR;;

double semicolon :)

> +
> +  PAM_DEBUG((stderr, "PAM: opening pipes.\n"));
> +  if (pipe(p_to_c) < 0 || pipe(c_to_p) < 0)
> +  {
> +    /* Error creating pipes. */
> +    return CR_ERROR;
> +  }
> +  PAM_DEBUG((stderr, "PAM: forking.\n"));
> +  if ((proc_id= fork()) < 0)
> +  {
> +    /* Error forking. */
> +    close(p_to_c[0]);
> +    close(c_to_p[1]);
> +    goto error_ret;
> +  }
> +
> +  if (proc_id == 0)
> +  {
> +    /* The 'sandbox' process started. */
> +    const char *arg[5];
> +    char toolpath[FN_REFLEN];
> +    size_t plugin_dir_len;
> +
> +    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);
> +
> +    arg[0]= toolpath;
> +    arg[1]= info->user_name;
> +    arg[2]= info->auth_string;
> +    if (pam_debug)
> +    {
> +      arg[3]= "DEBUG";
> +      arg[4]= 0;
> +    }
> +    else
> +      arg[3]= 0;

I'd rather passed that via the pipe, like, in the first three lines.
so that it wouldn't be seen in `ps` output.
May be it's harmless, but better to avoid this argument altogether.
Just do, like

   fprintf(p_to_c[0], "%s\n%s\n%d\n", info->user_name, info->auth_string,
                      pam_debug);

and in the tool, fscanf("%s\n%s\n%d\n");
Hmm. Taking into account that the tool is installed SUID,
you need to take extra care to avoid buffer overflows,
assuming someone can invoke the tool directly.
So, a little bit more complex than fscanf("%s\n%s\n%d\n"), sorry

> +
> +    PAM_DEBUG((stderr, "PAM: execute pam sandbox:[%s] [%s] [%s] [%s].\n",
> +                       arg[0], arg[1], arg[2],
> +                       arg[3] ? arg[3] : "No debug"));
> +    (void) execv(arg[0], (char **) arg);
> +    PAM_DEBUG((stderr, "PAM: exec() failed.\n"));
> +    exit(-1);
> +  }
> +
> +  /* Parent process continues. */
> +
> +  PAM_DEBUG((stderr, "PAM: parent continues.\n"));
> +  if (close(p_to_c[0]) < 0 ||
> +      close(c_to_p[1]) < 0)
> +    goto error_ret;
> +
> +  for (;;)
> +  {
> +    unsigned char field;
> +
> +    PAM_DEBUG((stderr, "PAM: listening to the sandbox.\n"));
> +    if (read(c_to_p[0], &field, 1) < 1)
> +    {
> +      PAM_DEBUG((stderr, "PAM: read failed.\n"));
> +      goto error_ret;
> +    }
> +
> +    if (field == AP_EOF)
> +    {
> +      PAM_DEBUG((stderr, "PAM: auth OK returned.\n"));
> +      break;
> +    }
> +
> +    switch (field)
> +    {
> +    case AP_AUTHENTICATED_AS:
> +      PAM_DEBUG((stderr, "PAM: reading authenticated_as string.\n"));
> +      if (read_string(c_to_p[0], info->authenticated_as,
> +                      sizeof(info->authenticated_as)) < 0)
> +        goto error_ret;
> +      break;
> +
> +    case AP_CONV:
> +      {
> +        unsigned char buf[10240];
> +        int buf_len;
> +        unsigned char *pkt;
> +
> +        PAM_DEBUG((stderr, "PAM: getting CONV string.\n"));
> +        if ((buf_len= read_string(c_to_p[0], (char *) buf, sizeof(buf))) < 0)
> +          goto error_ret;
> +
> +        PAM_DEBUG((stderr, "PAM: sending CONV string.\n"));
> +        if (vio->write_packet(vio, buf, buf_len))
> +          goto error_ret;
> +
> +        PAM_DEBUG((stderr, "PAM: reading CONV answer.\n"));
> +        if ((buf_len= vio->read_packet(vio, &pkt)) < 0)
> +          goto error_ret;
> +
> +        PAM_DEBUG((stderr, "PAM: answering CONV.\n"));
> +        if (write_string(p_to_c[1], pkt, buf_len))
> +          goto error_ret;
> +      }
> +      break;
> +
> +    default:
> +      PAM_DEBUG((stderr, "PAM: unknown sandbox field.\n"));
> +      goto error_ret;
> +    }
> +  }
> +  result= CR_OK;
> +
> +error_ret:
> +  close(p_to_c[1]);
> +  close(c_to_p[0]);
> +
> +  PAM_DEBUG((stderr, "PAM: auth result %d.\n", result));
> +  return result;
> +}
> +
> +static struct st_mysql_auth info =
> +{
> +  MYSQL_AUTHENTICATION_INTERFACE_VERSION,
> +  "dialog",
> +  pam_auth
> +};
> +
> +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
> +
> +
> +static struct st_mysql_sys_var* vars[] = {
> +  MYSQL_SYSVAR(use_cleartext_plugin),
> +#ifndef DBUG_OFF
> +  MYSQL_SYSVAR(debug),
> +#endif
> +  NULL
> +};
> +
> +
> +static int init(void *p __attribute__((unused)))
> +{
> +  if (use_cleartext_plugin)
> +    info.client_auth_plugin= "mysql_clear_password";
> +  if (!(opt_plugin_dir= dlsym(RTLD_DEFAULT, "opt_plugin_dir")))
> +    return 1;
> +
> +  return 0;
> +}
> +
> +maria_declare_plugin(pam)
> +{
> +  MYSQL_AUTHENTICATION_PLUGIN,
> +  &info,
> +  "pam",
> +  "MariaDB Corp",
> +  "PAM based authentication (safe)",

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.

> +  PLUGIN_LICENSE_GPL,
> +  init,
> +  NULL,
> +  0x0100,
> +  NULL,
> +  vars,
> +  "1.0",
> +  MariaDB_PLUGIN_MATURITY_STABLE

Let's start from beta and change it to stable later :)

> +}
> +maria_declare_plugin_end;
> diff --git a/plugin/auth_pam/auth_pam_tool.c b/plugin/auth_pam/auth_pam_tool.c
> new file mode 100644
> index 0000000..e4addd3
> --- /dev/null
> +++ b/plugin/auth_pam/auth_pam_tool.c
> @@ -0,0 +1,115 @@
> +/*
> +   Copyright (c) 2011, 2012, Monty Program Ab

could be updated...

> +
> +   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 */
> +
> +#define _GNU_SOURCE 1 /* for strndup */

you could just set _GNU_SOURCE once in CMakeLists.txt instead of
doing it in every file

> +
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <mysql/plugin_auth_common.h>
> +
> +struct param {
> +  unsigned char buf[10240], *ptr;
> +};
> +
> +
> +#include "auth_pam_tool.h"
> +
> +
> +static int write_packet(struct param *param  __attribute__((unused)),
> +                        const unsigned char *buf, int buf_len)
> +{
> +  unsigned char b=  AP_CONV;
> +  return write(1, &b, 1) < 1 ||
> +         write_string(1, buf, buf_len);
> +}
> +
> +
> +static int read_packet(struct param *param, unsigned char **pkt)
> +{
> +  *pkt= (unsigned char *) param->buf;
> +  return read_string(0, (char *) param->buf, (int) sizeof(param->buf)) - 1;
> +}
> +
> +
> +typedef struct st_mysql_server_auth_info
> +{
> +  /**
> +    User name as sent by the client and shown in USER().
> +    NULL if the client packet with the user name was not received yet.
> +  */
> +  char *user_name;
> +
> +  /**
> +    A corresponding column value from the mysql.user table for the
> +    matching account name
> +  */
> +  char *auth_string;
> +
> +  /**
> +    Matching account name as found in the mysql.user table.
> +    A plugin can override it with another name that will be
> +    used by MySQL for authorization, and shown in CURRENT_USER()
> +  */
> +  char authenticated_as[MYSQL_USERNAME_LENGTH+1]; 
> +} MYSQL_SERVER_AUTH_INFO;
> +
> +
> +#include "auth_pam_base.c"
> +
> +
> +int main(int argc, char **argv)
> +{
> +  struct param param;
> +  MYSQL_SERVER_AUTH_INFO info;
> +  unsigned char field;
> +  int res;
> +
> +  if (argc < 3)
> +    return -1;
> +
> +  info.user_name= argv[1];
> +  info.auth_string= argv[2];
> +
> +  if (argc >= 4)
> +    pam_debug= 1;

As we discussed, there should be some ways to prevent it being used
to brute-force passwords. It'll be suid root, so it cannot be
made executable only by "mysql" user.

I see two approaches here:

 1. getuid() and check that the user name is "mysql"
 2. getppid() and check that it matches the value in a pid_file

second is heavier (one might need to read my.cnf to find the location
of a pid_file). first hard-codes the user name.

oh, a third approach. create a new directory under plugin-dir, say
auth_tool with permissions r-x------ and owned by "mysql".
Put the actual executable there, so nobody besides "mysql" user
will be able to get to it.

Yes, this seems the easiest, fastest, and most flexible too :)

> +
> +  PAM_DEBUG((stderr, "PAM: sandbox started [%s] [%s] [%s] [%s].\n", argv[0],
> +                      info.user_name, info.auth_string,
> +                      pam_debug ? argv[3] : "No debug"));

Hmm, where will it go? messages written by the tool to its stderr

> +
> +  if ((res= pam_auth_base(&param, &info)) != CR_OK)
> +  {
> +    PAM_DEBUG((stderr, "PAM: auth failed, sandbox closed.\n"));
> +    return -1;
> +  }
> +
> +  if (info.authenticated_as[0])
> +  {
> +    PAM_DEBUG((stderr, "PAM: send authenticated_as field.\n"));
> +    field= AP_AUTHENTICATED_AS;
> +    if (write(1, &field, 1) < 1 ||
> +        write_string(1, (unsigned char *) info.authenticated_as,
> +                     strlen(info.authenticated_as)))
> +      return -1;
> +  }
> +  
> +  PAM_DEBUG((stderr, "PAM: send OK result.\n"));
> +  field= AP_EOF;
> +  write(1, &field, 1);
> +
> +  PAM_DEBUG((stderr, "PAM: sandbox closed.\n"));
> +  return 0;
> +}

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx