Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Some fixes to exec directory management #12005

Merged
merged 6 commits into from Mar 20, 2019
Merged

Some fixes to exec directory management #12005

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
30ff18d
fs-util: change chmod_and_chown() to not complain if stat data alread…
poettering Mar 14, 2019
edbfeb1
execute: use path_join() where appropriate
poettering Mar 14, 2019
40cd2ec
execute: also do the private/ symlink dance when runtime dir preserva…
poettering Mar 14, 2019
d484580
execute: remove one redundant comparison check
poettering Mar 14, 2019
206e986
core: change ownership/mode of the execution directories also for sta…
poettering Mar 14, 2019
08f6769
execute: generalize uid/gid handling in two cases for any kind of uid…
poettering Mar 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
79 changes: 62 additions & 17 deletions src/basic/fs-util.c
View file
Open in desktop
Expand Up @@ -215,64 +215,109 @@ int readlink_and_make_absolute(const char *p, char **r) {
int chmod_and_chown(const char *path, mode_t mode, uid_t uid, gid_t gid) {
char fd_path[STRLEN("/proc/self/fd/") + DECIMAL_STR_MAX(int) + 1];
_cleanup_close_ int fd = -1;
bool st_valid = false;
struct stat st;
int r;

assert(path);

/* Under the assumption that we are running privileged we first change the access mode and only then hand out
* ownership to avoid a window where access is too open. */
/* Under the assumption that we are running privileged we first change the access mode and only then
* hand out ownership to avoid a window where access is too open. */

fd = open(path, O_PATH|O_CLOEXEC|O_NOFOLLOW); /* Let's acquire an O_PATH fd, as precaution to change mode/owner
* on the same file */
fd = open(path, O_PATH|O_CLOEXEC|O_NOFOLLOW); /* Let's acquire an O_PATH fd, as precaution to change
* mode/owner on the same file */
if (fd < 0)
return -errno;

xsprintf(fd_path, "/proc/self/fd/%i", fd);

if (mode != MODE_INVALID) {

if ((mode & S_IFMT) != 0) {
struct stat st;

if (stat(fd_path, &st) < 0)
return -errno;

if ((mode & S_IFMT) != (st.st_mode & S_IFMT))
return -EINVAL;

st_valid = true;
}

if (chmod(fd_path, mode & 07777) < 0)
return -errno;
if (chmod(fd_path, mode & 07777) < 0) {
r = -errno;

if (!st_valid && stat(fd_path, &st) < 0)
return -errno;

if ((mode & 07777) != (st.st_mode & 07777))
return r;

st_valid = true;
}
}

if (uid != UID_INVALID || gid != GID_INVALID)
if (chown(fd_path, uid, gid) < 0)
return -errno;
if (uid != UID_INVALID || gid != GID_INVALID) {
if (chown(fd_path, uid, gid) < 0) {
r = -errno;

if (!st_valid && stat(fd_path, &st) < 0)
return -errno;

if (uid != UID_INVALID && st.st_uid != uid)
return r;
if (gid != GID_INVALID && st.st_gid != gid)
return r;
}
}

return 0;
}

int fchmod_and_chown(int fd, mode_t mode, uid_t uid, gid_t gid) {
bool st_valid = false;
struct stat st;
int r;

/* Under the assumption that we are running privileged we first change the access mode and only then hand out
* ownership to avoid a window where access is too open. */

if (mode != MODE_INVALID) {

if ((mode & S_IFMT) != 0) {
struct stat st;

if (fstat(fd, &st) < 0)
return -errno;

if ((mode & S_IFMT) != (st.st_mode & S_IFMT))
return -EINVAL;

st_valid = true;
}

if (fchmod(fd, mode & 0777) < 0)
return -errno;
if (fchmod(fd, mode & 07777) < 0) {
r = -errno;

if (!st_valid && fstat(fd, &st) < 0)
return -errno;

if ((mode & 07777) != (st.st_mode & 07777))
return r;

st_valid = true;
}
}

if (uid != UID_INVALID || gid != GID_INVALID)
if (fchown(fd, uid, gid) < 0)
return -errno;
if (fchown(fd, uid, gid) < 0) {
r = -errno;

if (!st_valid && fstat(fd, &st) < 0)
return -errno;

if (uid != UID_INVALID && st.st_uid != uid)
return r;
if (gid != GID_INVALID && st.st_gid != gid)
return r;
}

return 0;
}
Expand Down
65 changes: 36 additions & 29 deletions src/core/execute.c
View file
Open in desktop
Expand Up @@ -2074,7 +2074,7 @@ static int setup_exec_directory(
STRV_FOREACH(rt, context->directories[type].paths) {
_cleanup_free_ char *p = NULL, *pp = NULL;

p = strjoin(params->prefix[type], "/", *rt);
p = path_join(params->prefix[type], *rt);
if (!p) {
r = -ENOMEM;
goto fail;
Expand All @@ -2085,7 +2085,8 @@ static int setup_exec_directory(
goto fail;

if (context->dynamic_user &&
!IN_SET(type, EXEC_DIRECTORY_RUNTIME, EXEC_DIRECTORY_CONFIGURATION)) {
(!IN_SET(type, EXEC_DIRECTORY_RUNTIME, EXEC_DIRECTORY_CONFIGURATION) ||
(type == EXEC_DIRECTORY_RUNTIME && context->runtime_directory_preserve_mode != EXEC_PRESERVE_NO))) {
_cleanup_free_ char *private_root = NULL;

/* So, here's one extra complication when dealing with DynamicUser=1 units. In that case we
Expand All @@ -2110,7 +2111,7 @@ static int setup_exec_directory(
* Also, note that we don't do this for EXEC_DIRECTORY_RUNTIME as that's often used for sharing
* files or sockets with other services. */

private_root = strjoin(params->prefix[type], "/private");
private_root = path_join(params->prefix[type], "private");
if (!private_root) {
r = -ENOMEM;
goto fail;
Expand All @@ -2121,7 +2122,7 @@ static int setup_exec_directory(
if (r < 0)
goto fail;

pp = strjoin(private_root, "/", *rt);
pp = path_join(private_root, *rt);
if (!pp) {
r = -ENOMEM;
goto fail;
Expand Down Expand Up @@ -2156,36 +2157,42 @@ static int setup_exec_directory(
if (r < 0)
goto fail;

/* Lock down the access mode */
if (chmod(pp, context->directories[type].mode) < 0) {
r = -errno;
goto fail;
}
} else {
r = mkdir_label(p, context->directories[type].mode);
if (r < 0 && r != -EEXIST)
goto fail;
if (r == -EEXIST) {
struct stat st;

if (stat(p, &st) < 0) {
r = -errno;
if (r < 0) {
if (r != -EEXIST)
goto fail;
}
if (((st.st_mode ^ context->directories[type].mode) & 07777) != 0)
log_warning("%s \'%s\' already exists but the mode is different. "
"(filesystem: %o %sMode: %o)",
exec_directory_type_to_string(type), *rt,
st.st_mode & 07777, exec_directory_type_to_string(type), context->directories[type].mode & 07777);
if (!context->dynamic_user)

if (type == EXEC_DIRECTORY_CONFIGURATION) {
struct stat st;

/* Don't change the owner/access mode of the configuration directory,
* as in the common case it is not written to by a service, and shall
* not be writable. */

if (stat(p, &st) < 0) {
r = -errno;
goto fail;
}

/* Still complain if the access mode doesn't match */
if (((st.st_mode ^ context->directories[type].mode) & 07777) != 0)
log_warning("%s \'%s\' already exists but the mode is different. "
"(File system: %o %sMode: %o)",
exec_directory_type_to_string(type), *rt,
st.st_mode & 07777, exec_directory_type_to_string(type), context->directories[type].mode & 07777);

continue;
}
}
}

/* Don't change the owner of the configuration directory, as in the common case it is not written to by
* a service, and shall not be writable. */
if (type == EXEC_DIRECTORY_CONFIGURATION)
continue;
/* Lock down the access mode (we use chmod_and_chown() to make this idempotent. We don't
* specifiy UID/GID here, so that path_chown_recursive() can optimize things depending on the
* current UID/GID ownership.) */
r = chmod_and_chown(pp ?: p, context->directories[type].mode, UID_INVALID, GID_INVALID);
if (r < 0)
goto fail;
poettering marked this conversation as resolved.
Show resolved Hide resolved

/* Then, change the ownership of the whole tree, if necessary */
r = path_chown_recursive(pp ?: p, uid, gid);
Expand Down Expand Up @@ -3154,7 +3161,7 @@ static int exec_child(
USER_PROCESS,
username);

if (context->user) {
if (uid_is_valid(uid)) {
r = chown_terminal(STDIN_FILENO, uid);
if (r < 0) {
*exit_status = EXIT_STDIN;
Expand Down Expand Up @@ -3459,7 +3466,7 @@ static int exec_child(
}

if (needs_setuid) {
if (context->user) {
if (uid_is_valid(uid)) {
r = enforce_user(context, uid);
if (r < 0) {
*exit_status = EXIT_USER;
Expand Down