From 870c57baf61e75744f4a2be7abb0d77c6d76d311 Mon Sep 17 00:00:00 2001 From: Matthias Gerstner Date: Tue, 8 Aug 2017 15:41:04 +0200 Subject: [PATCH 01/10] pam_script_setenv: simplify putenv code and react on exceeded length --- pam_script.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/pam_script.c b/pam_script.c index 86e6c45..602c78b 100644 --- a/pam_script.c +++ b/pam_script.c @@ -96,11 +96,11 @@ static void pam_script_setenv(const char *key, const char *value) { #elif HAVE_PUTENV char buffer[BUFSIZE], *str; - strncpy(buffer,key,BUFSIZE-2); - strcat(buffer,"="); - strncat(buffer,(value?value:""),BUFSIZE-strlen(buffer)-1); - if ((str = (char *) malloc(strlen(buffer)+1)) != NULL) { - strcpy(str,buffer); + if (snprintf(buffer, BUFSIZE, "%s=%s", key, value ? value : "") > BUFSIZE) { + // insufficient space + return; + } + if ((str = strdup(buffer)) != NULL) { putenv(str); } /* else { untrapped memory error - just do not add to environment @@ -241,7 +241,7 @@ static int pam_script_set_authtok(pam_handle_t *pamh, int flags, { int retval; char *password; - + struct pam_message msg[1],*pmsg[1]; struct pam_response *response; @@ -261,7 +261,7 @@ static int pam_script_set_authtok(pam_handle_t *pamh, int flags, } password = response[0].resp; response[0].resp = NULL; - } + } else return PAM_CONV_ERR; @@ -312,7 +312,7 @@ int pam_sm_authenticate(pam_handle_t *pamh, int flags, int argc, if (!password) { retval = pam_script_set_authtok(pamh, flags, argc, argv, "Password: ", PAM_AUTHTOK); - if (retval != PAM_SUCCESS) + if (retval != PAM_SUCCESS) return retval; } From 6b5cc682b3b2395d1e4dde4ae41d4baed88362d4 Mon Sep 17 00:00:00 2001 From: Matthias Gerstner Date: Tue, 8 Aug 2017 16:51:58 +0200 Subject: [PATCH 02/10] pam_sm_chauthtok: safely clear temporary password buffers after use --- pam_script.c | 31 +++++++++++++++++++++++++------ 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/pam_script.c b/pam_script.c index 602c78b..f2710b5 100644 --- a/pam_script.c +++ b/pam_script.c @@ -345,6 +345,17 @@ int pam_sm_acct_mgmt(pam_handle_t *pamh, int flags, int argc, /* --- password management --- */ +/* + * instead of memset for clearing memory, hopefully not being optimized out + * by the compiler + */ +static void safe_clearmem(char *mem, size_t size) { + size_t i; + for (i = 0; i < size; i++) { + mem[i] = 0; + } +} + PAM_EXTERN int pam_sm_chauthtok(pam_handle_t *pamh, int flags, int argc, const char **argv) @@ -372,7 +383,7 @@ int pam_sm_chauthtok(pam_handle_t *pamh, int flags, int argc, } /* - * Check if PAM_AUTHTOK is set by early pam modules and + * Check if PAM_AUTHTOK is set by early pam modules and * if not ask user for the new password. */ pam_get_item(pamh, PAM_AUTHTOK, (void*) &password); @@ -385,20 +396,28 @@ int pam_sm_chauthtok(pam_handle_t *pamh, int flags, int argc, password = NULL; retval = pam_script_set_authtok(pamh, flags, argc, argv, "New password (again): ", PAM_AUTHTOK); - if (retval != PAM_SUCCESS) + if (retval != PAM_SUCCESS) { + safe_clearmem(new_pass1, sizeof(new_pass1)); return retval; + } retval = pam_get_item(pamh, PAM_AUTHTOK, (void*) &password); snprintf(new_pass2, BUFSIZE, "%s", password); password = NULL; - /* Check if new password's are the same */ + /* Check if new passwords are the same */ if (strcmp(new_pass1, new_pass2) != 0) { retval = pam_script_senderr(pamh, flags, argc, argv, "You must enter the same password twice."); - if (retval != PAM_SUCCESS) - return retval; - return PAM_AUTHTOK_ERR; + + if (retval == PAM_SUCCESS) + retval = PAM_AUTHTOK_ERR; } + + safe_clearmem(new_pass1, sizeof(new_pass1)); + safe_clearmem(new_pass2, sizeof(new_pass2)); + + if (retval != PAM_SUCCESS) + return PAM_AUTHTOK_ERR; } return pam_script_exec(pamh, "password", PAM_SCRIPT_PASSWD, user, PAM_AUTHTOK_ERR, argc, argv); From c8150b43b582bc50c337f140236fb5216394a627 Mon Sep 17 00:00:00 2001 From: Matthias Gerstner Date: Tue, 8 Aug 2017 17:19:32 +0200 Subject: [PATCH 03/10] pam_script_exec: fix check for empty dir= parameter the previous expression always evaluates to true, because the value of the pointer was checked, not what it points to. --- pam_script.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pam_script.c b/pam_script.c index f2710b5..9ecf648 100644 --- a/pam_script.c +++ b/pam_script.c @@ -156,7 +156,7 @@ static int pam_script_exec(pam_handle_t *pamh, "invalid option: %s", argv[i]); } if (strncmp(argv[i],"dir=",4) == 0) { - if (argv[i] + 4) { /* got new scriptdir */ + if (*(argv[i] + 4)) { /* got new scriptdir */ strncpy(cmd,argv[i] + 4, BUFSIZE - 2); } } From 31b02399a5286d7c8457942f717e452510e5129d Mon Sep 17 00:00:00 2001 From: Matthias Gerstner Date: Tue, 8 Aug 2017 17:29:46 +0200 Subject: [PATCH 04/10] pam_script_exec: reject excess length dir= paths --- pam_script.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/pam_script.c b/pam_script.c index 9ecf648..6aa995b 100644 --- a/pam_script.c +++ b/pam_script.c @@ -136,14 +136,12 @@ static int pam_script_exec(pam_handle_t *pamh, int retval = rv, status, i; - char cmd[BUFSIZE]; + char cmd[BUFSIZE] = { '\0' }; char **newargv; struct stat fs; const void *envval = NULL; pid_t child_pid = 0; - strncpy(cmd, PAM_SCRIPT_DIR, BUFSIZE - 1); - /* check for pam.conf options */ for (i = 0; i < argc; i++) { if (strncmp(argv[i],"onerr=",6) == 0) { @@ -156,12 +154,22 @@ static int pam_script_exec(pam_handle_t *pamh, "invalid option: %s", argv[i]); } if (strncmp(argv[i],"dir=",4) == 0) { - if (*(argv[i] + 4)) { /* got new scriptdir */ - strncpy(cmd,argv[i] + 4, BUFSIZE - 2); + const char *new_dir = argv[i] + 4; + const int MAX_DIR_LEN = BUFSIZE - 2; + + if (*new_dir) { /* got new scriptdir */ + if (snprintf(cmd, MAX_DIR_LEN, "%s", new_dir) > MAX_DIR_LEN) { + pam_script_syslog(LOG_ERR,"script dir %s exceeds maximum supported path length", new_dir); + cmd[0] = '\0'; + } } } } + if (cmd[0] == '\0') { + strncpy(cmd, PAM_SCRIPT_DIR, BUFSIZE - 1); + } + /* strip trailing '/' */ if (cmd[strlen(cmd)-1] == '/') cmd[strlen(cmd)-1] = '\0'; strcat(cmd,"/"); From 1931f2f36f33d332fc55b98fd81d73bc4fb048ff Mon Sep 17 00:00:00 2001 From: Matthias Gerstner Date: Tue, 8 Aug 2017 17:41:30 +0200 Subject: [PATCH 05/10] pam_script_exec: reject excess length complete script path --- pam_script.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pam_script.c b/pam_script.c index 6aa995b..637a28a 100644 --- a/pam_script.c +++ b/pam_script.c @@ -173,6 +173,10 @@ static int pam_script_exec(pam_handle_t *pamh, /* strip trailing '/' */ if (cmd[strlen(cmd)-1] == '/') cmd[strlen(cmd)-1] = '\0'; strcat(cmd,"/"); + if (strlen(script) > (BUFSIZE - strlen(cmd) - 1)) { + pam_script_syslog(LOG_ERR,"script path %s/%s exceeds maximum supported path length", cmd, script); + return retval; + } strncat(cmd,script,BUFSIZE-strlen(cmd)-1); /* test for script existence first */ From f52d402ff8e94e813ba594139416551707fdfafd Mon Sep 17 00:00:00 2001 From: Matthias Gerstner Date: Tue, 8 Aug 2017 17:50:20 +0200 Subject: [PATCH 06/10] pam_script_exec: strip all trailing slashes, not only one --- pam_script.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/pam_script.c b/pam_script.c index 637a28a..9eeecce 100644 --- a/pam_script.c +++ b/pam_script.c @@ -171,8 +171,17 @@ static int pam_script_exec(pam_handle_t *pamh, } /* strip trailing '/' */ - if (cmd[strlen(cmd)-1] == '/') cmd[strlen(cmd)-1] = '\0'; + while (1) { + size_t curlen = strlen(cmd); + + if (curlen == 0 || cmd[curlen-1] != '/') + break; + + cmd[curlen - 1] = '\0'; + } + strcat(cmd,"/"); + if (strlen(script) > (BUFSIZE - strlen(cmd) - 1)) { pam_script_syslog(LOG_ERR,"script path %s/%s exceeds maximum supported path length", cmd, script); return retval; From bdad71568c3552a7b5a64b56ea2e25a21208c9ce Mon Sep 17 00:00:00 2001 From: Matthias Gerstner Date: Wed, 9 Aug 2017 10:08:26 +0200 Subject: [PATCH 07/10] pam_script_exec: stop stack execution on failed execve() in the forked process The previous behaviour was to continue running the pam stack in the forked process which is probably not a good thing. --- pam_script.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/pam_script.c b/pam_script.c index 9eeecce..dd0cc21 100644 --- a/pam_script.c +++ b/pam_script.c @@ -221,8 +221,10 @@ static int pam_script_exec(pam_handle_t *pamh, PAM_SCRIPT_SETENV(PAM_OLDAUTHTOK); /* construct newargv */ - if (!(newargv = (char **) calloc(sizeof(char *), argc+2))) - return retval; + if (!(newargv = (char **) calloc(sizeof(char *), argc+2))) { + // for rationale see below + _exit(127); + } newargv[0] = cmd; for (i = 0; i < argc; i++) { newargv[1+i] = (char *) argv[i]; @@ -231,6 +233,17 @@ static int pam_script_exec(pam_handle_t *pamh, /* shouldn't get here, unless an error */ pam_script_syslog(LOG_ALERT, "script %s exec failure", cmd); + /* + * explicitly exit() here to avoid continuing execution of the + * PAM stack in the forked process in an undefined manner. + * + * 127 is typically the exit code when something fundamentally + * went wrong with starting a child process. + * + * use _exit() instead of exit() to avoid execution of any + * cleanup code like atexit() handlers. + */ + _exit(127); return retval; default: /* parent */ From 2d49919b5d0dae1193fa31ad8dd11d20818295d1 Mon Sep 17 00:00:00 2001 From: Matthias Gerstner Date: Wed, 9 Aug 2017 15:29:01 +0200 Subject: [PATCH 08/10] etc/pam_script: apply some basic security checks before running further scripts Sadly there's no way to avoid the race between stat/exec here in the script. But it helps against obvious permission errors. --- etc/pam_script | 31 ++++++++++++++++++++++++++++--- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/etc/pam_script b/etc/pam_script index 4176725..4c23ca2 100755 --- a/etc/pam_script +++ b/etc/pam_script @@ -19,6 +19,33 @@ basedir=`dirname $0` fname=`basename $0` PAMSCRIPTDIR=${PAMSCRIPTDIR:-$basedir/pam-script.d} +runscript () { + script="$1" + shift + + if [ ! -e "$script" ]; then + return 0 + fi + + stat_output=`/usr/bin/stat -c "%A:%u:%g" "$script"` + if [ $? -ne 0 ]; then + echo "$0: Could not stat script $script" 1>&2 + return 1 + fi + + owner=`echo $stat_output | /usr/bin/cut -d ':' -f 2` + group=`echo $stat_output | /usr/bin/cut -d ':' -f 3` + world_write_bit=${stat_output:8:1} + + if [ ${world_write_bit} != "-" -o "$owner" -ne 0 -o "$group" -ne 0 ]; then + echo "$0: Unsafe permissions for script $script; Rejecting execution." 1>&2 + return 1 + fi + + /bin/sh "$script" "$@" + return $? +} + if [ x"$fname" != x"pam_script" ]; then # process files in pam-script.d/ mtype='' @@ -33,9 +60,7 @@ if [ x"$fname" != x"pam_script" ]; then esac for script in $PAMSCRIPTDIR/*$mtype; do - if [ -e $script ]; then - /bin/sh $script "$@" || PAM_SCRIPT_STATUS=1 - fi + runscript "$script" $@ || PAM_SCRIPT_STATUS=1 export PAM_SCRIPT_STATUS done exit $PAM_SCRIPT_STATUS From 19165c11effbb25ebd3f19dc59d82b2f1a4df563 Mon Sep 17 00:00:00 2001 From: Matthias Gerstner Date: Wed, 9 Aug 2017 17:24:29 +0200 Subject: [PATCH 09/10] pam_script_exec(): implement security checks on executed dirs/scripts Similar to the change in the etc/pam_script script the pam_script module itself now checks for secure permissions of the directory containing the scripts and the scripts itself. This also leaves open a race condition between time-of-check and time-of-use that's difficult to solve but hopefully negligible. --- pam_script.c | 71 ++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 58 insertions(+), 13 deletions(-) diff --git a/pam_script.c b/pam_script.c index dd0cc21..283e781 100644 --- a/pam_script.c +++ b/pam_script.c @@ -129,6 +129,58 @@ static int pam_script_get_user(pam_handle_t *pamh, const char **user) { return retval; } +static int check_path_perms(const char *path) { + + struct stat fs; + const mode_t ALL_EXEC_MASK = (S_IXUSR|S_IXGRP|S_IXOTH); + + /* + * note: checking security properties like this leaves us with a race + * condition, because the stat()/execve() execution is not atomic. + * + * likely candidates for overcoming this would have been fexecve() or + * execveat(). But both suffer from a side effect (at least on + * Linux/glibc implementation) that causes scripts to be called like + * + * "bash /proc/self/fd/ ..." + * + * This would break existing scripts that can't make out their + * basename/dirname any more to base decisions on them. + * + * An explicit environment variable could be added that carries the + * "real" basename but that would complicate things even more. + */ + + /* test for script existence first */ + if (stat(path, &fs) < 0) { + /* stat failure */ + pam_script_syslog(LOG_ERR,"can not stat %s", path); + return 1; + } + + if ((fs.st_mode & ALL_EXEC_MASK) != ALL_EXEC_MASK) { + /* script not executable at all levels */ + pam_script_syslog(LOG_ALERT, + "path %s not fully executable", path); + return 1; + } + else if ((fs.st_mode & S_IWOTH) != 0) { + /* script is world writeable, probably not a good idea */ + pam_script_syslog(LOG_ALERT, + "path %s is world-writeable, rejecting for " + "security reasons", path); + return 1; + } + else if (fs.st_uid != 0 || fs.st_gid != 0) { + /* script should be owned by root:root */ + pam_script_syslog(LOG_ALERT, + "path %s is not owned by root:root, rejecting for security reasons", path); + return 1; + } + + return 0; +} + static int pam_script_exec(pam_handle_t *pamh, const char *type, const char *script, const char *user, int rv, int argc, const char **argv) { @@ -138,7 +190,6 @@ static int pam_script_exec(pam_handle_t *pamh, i; char cmd[BUFSIZE] = { '\0' }; char **newargv; - struct stat fs; const void *envval = NULL; pid_t child_pid = 0; @@ -180,6 +231,10 @@ static int pam_script_exec(pam_handle_t *pamh, cmd[curlen - 1] = '\0'; } + /* check the base directory permissions */ + if (check_path_perms(cmd) != 0) + return retval; + strcat(cmd,"/"); if (strlen(script) > (BUFSIZE - strlen(cmd) - 1)) { @@ -188,19 +243,9 @@ static int pam_script_exec(pam_handle_t *pamh, } strncat(cmd,script,BUFSIZE-strlen(cmd)-1); - /* test for script existence first */ - if (stat(cmd, &fs) < 0) { - /* stat failure */ - pam_script_syslog(LOG_ERR,"can not stat %s", cmd); + /* check the script permissions */ + if (check_path_perms(cmd) != 0) return retval; - } - if ((fs.st_mode & (S_IXUSR|S_IXGRP|S_IXOTH)) - != (S_IXUSR|S_IXGRP|S_IXOTH)) { - /* script not executable at all levels */ - pam_script_syslog(LOG_ALERT, - "script %s not fully executable", cmd); - return retval; - } /* Execute external program */ /* fork process */ From c91fb1451dc73d98046d19ba00dab2fa411286c3 Mon Sep 17 00:00:00 2001 From: Matthias Gerstner Date: Wed, 9 Aug 2017 17:35:31 +0200 Subject: [PATCH 10/10] etc/pam_script: also apply security checks to the script directory --- etc/pam_script | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/etc/pam_script b/etc/pam_script index 4c23ca2..4d7d0c6 100755 --- a/etc/pam_script +++ b/etc/pam_script @@ -19,17 +19,11 @@ basedir=`dirname $0` fname=`basename $0` PAMSCRIPTDIR=${PAMSCRIPTDIR:-$basedir/pam-script.d} -runscript () { - script="$1" - shift - - if [ ! -e "$script" ]; then - return 0 - fi - - stat_output=`/usr/bin/stat -c "%A:%u:%g" "$script"` +goodperms () { + local path="$1" + stat_output=`/usr/bin/stat -c "%A:%u:%g" "$path"` if [ $? -ne 0 ]; then - echo "$0: Could not stat script $script" 1>&2 + echo "$0: Could not stat path $path" 1>&2 return 1 fi @@ -38,9 +32,20 @@ runscript () { world_write_bit=${stat_output:8:1} if [ ${world_write_bit} != "-" -o "$owner" -ne 0 -o "$group" -ne 0 ]; then - echo "$0: Unsafe permissions for script $script; Rejecting execution." 1>&2 + echo "$0: Unsafe permissions for path $path; Rejecting execution." 1>&2 return 1 fi +} + +runscript () { + script="$1" + shift + + if [ ! -e "$script" ]; then + return 0 + fi + + goodperms "$script" || return 1 /bin/sh "$script" "$@" return $? @@ -59,6 +64,8 @@ if [ x"$fname" != x"pam_script" ]; then *_ses_close) mtype='_ses_close' ;; esac + goodperms "$PAMSCRIPTDIR" || exit $PAM_SCRIPT_STATUS + for script in $PAMSCRIPTDIR/*$mtype; do runscript "$script" $@ || PAM_SCRIPT_STATUS=1 export PAM_SCRIPT_STATUS