Skip to content

Commit

Permalink
Fix security issue in _prolog_error().
Browse files Browse the repository at this point in the history
Fix security issue caused by insecure file path handling triggered by
the failure of a Prolog script. To exploit this a user needs to
anticipate or cause the Prolog to fail for their job.

(This commit is slightly different from the fix to the 15.08 branch.)

CVE-2016-10030.
  • Loading branch information
wickberg committed Jan 4, 2017
1 parent b330c05 commit 92362a9
Show file tree
Hide file tree
Showing 2 changed files with 112 additions and 4 deletions.
3 changes: 3 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ documents those changes that are of interest to users and administrators.
-- Fix check for PluginDir within slurmctld to work with multiple directories.
-- Cancel interactive jobs automatically on communication error to launching
srun/salloc process.
-- Fix security issue caused by insecure file path handling triggered by the
failure of a Prolog script. To exploit this a user needs to anticipate or
cause the Prolog to fail for their job. CVE-2016-10030.

* Changes in Slurm 16.05.7
==========================
Expand Down
113 changes: 109 additions & 4 deletions src/slurmd/slurmd/req.c
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ static void _note_batch_job_finished(uint32_t job_id);
static int _prolog_is_running (uint32_t jobid);
static int _step_limits_match(void *x, void *key);
static int _terminate_all_steps(uint32_t jobid, bool batch);
static int _receive_fd(int socket);
static void _rpc_launch_tasks(slurm_msg_t *);
static void _rpc_abort_job(slurm_msg_t *);
static void _rpc_batch_job(slurm_msg_t *msg, bool new_msg);
Expand Down Expand Up @@ -214,6 +215,7 @@ static void _sync_messages_kill(kill_job_msg_t *req);
static int _waiter_init (uint32_t jobid);
static int _waiter_complete (uint32_t jobid);

static void _send_back_fd(int socket, int fd);
static bool _steps_completed_now(uint32_t jobid);
static int _valid_sbcast_cred(file_bcast_msg_t *req, uid_t req_uid,
uint16_t block_no, uint32_t *job_id);
Expand Down Expand Up @@ -1383,6 +1385,111 @@ _rpc_launch_tasks(slurm_msg_t *msg)
send_registration_msg(errnum, false);
}

/*
* Open file based upon permissions of a different user
* IN path_name - name of file to open
* IN uid - User ID to use for file access check
* IN gid - Group ID to use for file access check
* RET -1 on error, file descriptor otherwise
*/
static int _open_as_other(char *path_name, batch_job_launch_msg_t *req)
{
pid_t child;
gids_t *gids;
int pipe[2];
int fd = -1, rc = 0;

if (!(gids = _gids_cache_lookup(req->user_name, req->gid))) {
error("%s: gids_cache_lookup for %s failed",
__func__, req->user_name);
return -1;
}

if ((rc = container_g_create(req->job_id))) {
error("%s: container_g_create(%u): %m", __func__, req->job_id);
_dealloc_gids(gids);
return -1;
}

/* child process will setuid to the user, register the process
* with the container, and open the file for us. */
if (socketpair(AF_UNIX, SOCK_DGRAM, 0, pipe) != 0) {
error("%s: Failed to open pipe: %m", __func__);
_dealloc_gids(gids);
return -1;
}

child = fork();
if (child == -1) {
error("%s: fork failure", __func__);
_dealloc_gids(gids);
close(pipe[0]);
close(pipe[1]);
return -1;
} else if (child > 0) {
close(pipe[0]);
(void) waitpid(child, &rc, 0);
_dealloc_gids(gids);
if (WIFEXITED(rc) && (WEXITSTATUS(rc) == 0))
fd = _receive_fd(pipe[1]);
close(pipe[1]);
return fd;
}

/* child process below here */

close(pipe[1]);

/* container_g_add_pid needs to be called in the
* forked process part of the fork to avoid a race
* condition where if this process makes a file or
* detacts itself from a child before we add the pid
* to the container in the parent of the fork. */
if (container_g_add_pid(req->job_id, getpid(), req->uid)) {
error("%s container_g_add_pid(%u): %m", __func__, req->job_id);
exit(SLURM_ERROR);
}

/* The child actually performs the I/O and exits with
* a return code, do not return! */

/*********************************************************************\
* NOTE: It would be best to do an exec() immediately after the fork()
* in order to help prevent a possible deadlock in the child process
* due to locks being set at the time of the fork and being freed by
* the parent process, but not freed by the child process. Performing
* the work inline is done for simplicity. Note that the logging
* performed by error() should be safe due to the use of
* atfork_install_handlers() as defined in src/common/log.c.
* Change the code below with caution.
\*********************************************************************/

if (setgroups(gids->ngids, gids->gids) < 0) {
error("%s: uid: %u setgroups failed: %m", __func__, req->uid);
exit(errno);
}
_dealloc_gids(gids);

if (setgid(req->gid) < 0) {
error("%s: uid:%u setgid(%u): %m", __func__, req->uid,req->gid);
exit(errno);
}
if (setuid(req->uid) < 0) {
error("%s: getuid(%u): %m", __func__, req->uid);
exit(errno);
}

fd = open(path_name, (O_CREAT|O_APPEND|O_WRONLY), 0644);
if (fd == -1) {
error("%s: uid:%u can't open `%s`: %m",
__func__, req->uid, path_name);
exit(errno);
}
_send_back_fd(pipe[0], fd);
close(fd);
exit(SLURM_SUCCESS);
}

static void
_prolog_error(batch_job_launch_msg_t *req, int rc)
{
Expand Down Expand Up @@ -1415,10 +1522,8 @@ _prolog_error(batch_job_launch_msg_t *req, int rc)
req->work_dir, err_name_ptr);
else
snprintf(path_name, MAXPATHLEN, "/%s", err_name_ptr);

if ((fd = open(path_name, (O_CREAT|O_APPEND|O_WRONLY), 0644)) == -1) {
error("Unable to open %s: %s", path_name,
slurm_strerror(errno));
if ((fd = _open_as_other(path_name, req)) == -1) {
error("Unable to open %s: Permission denied", path_name);
return;
}
snprintf(err_name, sizeof(err_name),
Expand Down

0 comments on commit 92362a9

Please sign in to comment.