Skip to content

linux: treats empty path to safe_openat as root #1753

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

Merged
merged 8 commits into from
May 27, 2025
Merged

linux: treats empty path to safe_openat as root #1753

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
040cb2e
linux: include errno check in UNLIKELY macro
giuseppe May 20, 2025
9ddddfb
test: fix mount to test
giuseppe May 20, 2025
372446d
tests: fix unused variable
giuseppe May 20, 2025
953a8c4
utils: crun_safe_ensure_at opens empty paths
giuseppe May 20, 2025
e9d159f
linux: store rootfsfd under private data only
giuseppe May 20, 2025
2e210bd
linux: use rootfsfd directly from container data
giuseppe May 20, 2025
7de03e6
linux: safe_openat reopens root
giuseppe May 20, 2025
64a2e0e
linux: Update rootfsfd when rootfs is replaced
giuseppe May 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 42 additions & 35 deletions src/libcrun/linux.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,9 @@ cleanup_private_data (void *private_data)
if (p->dev_fds)
cleanup_close_mapp (&(p->dev_fds));

if (p->rootfsfd >= 0)
close (p->rootfsfd);

free (p->unified_cgroup_path);
free (p->host_notify_socket_path);
free (p->container_notify_socket_path);
Expand Down Expand Up @@ -1027,7 +1030,6 @@ do_masked_or_readonly_path (libcrun_container_t *container, const char *rel_path
{
unsigned long mount_flags = 0;
const char *rootfs = get_private_data (container)->rootfs;
int rootfsfd = get_private_data (container)->rootfsfd;
cleanup_close int pathfd = -1;
struct statfs sfs;
int ret;
Expand All @@ -1036,7 +1038,7 @@ do_masked_or_readonly_path (libcrun_container_t *container, const char *rel_path
if (rel_path[0] == '/')
rel_path++;

pathfd = safe_openat (rootfsfd, rootfs, rel_path, O_PATH | O_CLOEXEC, 0, err);
pathfd = safe_openat (get_private_data (container)->rootfsfd, rootfs, rel_path, O_PATH | O_CLOEXEC, 0, err);
if (UNLIKELY (pathfd < 0))
{
if (errno != ENOENT && errno != EACCES)
Expand Down Expand Up @@ -1225,6 +1227,17 @@ do_mount (libcrun_container_t *container, const char *source, int targetfd,
if (UNLIKELY (fd < 0))
return fd;

/* We are replacing the rootfs, reopen it. */
if (is_empty_string (target))
{
int tmp = dup (fd);
if (UNLIKELY (tmp < 0))
return crun_make_error (err, errno, "dup");

TEMP_FAILURE_RETRY (close (get_private_data (container)->rootfsfd));
get_private_data (container)->rootfsfd = tmp;
}

#ifdef HAVE_FGETXATTR
if (label_how == LABEL_XATTR)
{
Expand Down Expand Up @@ -1585,7 +1598,6 @@ libcrun_create_dev (libcrun_container_t *container, int devfd, int srcfd,
mode_t type = (device->type[0] == 'b') ? S_IFBLK : ((device->type[0] == 'p') ? S_IFIFO : S_IFCHR);
const char *fullname = device->path;
cleanup_close int fd = -1;
int rootfsfd = get_private_data (container)->rootfsfd;
const char *rootfs = get_private_data (container)->rootfs;
if (is_empty_string (fullname))
return crun_make_error (err, EINVAL, "device path is empty");
Expand Down Expand Up @@ -1616,7 +1628,7 @@ libcrun_create_dev (libcrun_container_t *container, int devfd, int srcfd,
{
const char *rel_path = consume_slashes (normalized_path);

fd = crun_safe_create_and_open_ref_at (false, rootfsfd, rootfs, rel_path, 0755, err);
fd = crun_safe_create_and_open_ref_at (false, get_private_data (container)->rootfsfd, rootfs, rel_path, 0755, err);
if (UNLIKELY (fd < 0))
return fd;
}
Expand Down Expand Up @@ -1681,18 +1693,18 @@ libcrun_create_dev (libcrun_container_t *container, int devfd, int srcfd,

if (dirname[0] == '\0')
{
dirfd = dup (rootfsfd);
dirfd = dup (get_private_data (container)->rootfsfd);
if (UNLIKELY (dirfd < 0))
return crun_make_error (err, errno, "dup fd for `%s`", rootfs);
}
else
{
dirfd = safe_openat (rootfsfd, rootfs, dirname, O_DIRECTORY | O_PATH | O_CLOEXEC, 0, err);
dirfd = safe_openat (get_private_data (container)->rootfsfd, rootfs, dirname, O_DIRECTORY | O_PATH | O_CLOEXEC, 0, err);
if (dirfd < 0 && ensure_parent_dir)
{
crun_error_release (err);

dirfd = crun_safe_create_and_open_ref_at (true, rootfsfd, rootfs, dirname, 0755, err);
dirfd = crun_safe_create_and_open_ref_at (true, get_private_data (container)->rootfsfd, rootfs, dirname, 0755, err);
}
if (UNLIKELY (dirfd < 0))
return dirfd;
Expand Down Expand Up @@ -1748,13 +1760,12 @@ create_missing_devs (libcrun_container_t *container, bool binds, libcrun_error_t
cleanup_close int devfd = -1;
runtime_spec_schema_config_schema *def = container->container_def;
const char *rootfs = get_private_data (container)->rootfs;
int rootfsfd = get_private_data (container)->rootfsfd;
cleanup_close_map struct libcrun_fd_map *dev_fds = NULL;

dev_fds = get_private_data (container)->dev_fds;
get_private_data (container)->dev_fds = NULL;

devfd = openat (rootfsfd, "dev", O_CLOEXEC | O_PATH | O_DIRECTORY);
devfd = openat (get_private_data (container)->rootfsfd, "dev", O_CLOEXEC | O_PATH | O_DIRECTORY);
if (UNLIKELY (devfd < 0))
return crun_make_error (err, errno, "open `/dev` directory in `%s`", rootfs);

Expand Down Expand Up @@ -1909,7 +1920,6 @@ static int
append_tmpfs_mode_if_missing (libcrun_container_t *container, runtime_spec_schema_defs_mount *mount, char **data, libcrun_error_t *err)
{
const char *rootfs = get_private_data (container)->rootfs;
int rootfsfd = get_private_data (container)->rootfsfd;
bool empty_data = is_empty_string (*data);
cleanup_close int fd = -1;
struct stat st;
Expand All @@ -1918,7 +1928,7 @@ append_tmpfs_mode_if_missing (libcrun_container_t *container, runtime_spec_schem
if (*data != NULL && strstr (*data, "mode="))
return 0;

fd = safe_openat (rootfsfd, rootfs, mount->destination, O_CLOEXEC | O_RDONLY, 0, err);
fd = safe_openat (get_private_data (container)->rootfsfd, rootfs, mount->destination, O_CLOEXEC | O_RDONLY, 0, err);
if (fd < 0)
{
if (crun_error_get_errno (err) != ENOENT)
Expand Down Expand Up @@ -2045,13 +2055,13 @@ get_force_cgroup_v1_annotation (libcrun_container_t *container)
}

static int
do_mounts (libcrun_container_t *container, int rootfsfd, const char *rootfs, libcrun_error_t *err)
do_mounts (libcrun_container_t *container, const char *rootfs, libcrun_error_t *err)
{
size_t i;
int ret;
runtime_spec_schema_config_schema *def = container->container_def;
const char *systemd_cgroup_v1 = get_force_cgroup_v1_annotation (container);
cleanup_close_map struct libcrun_fd_map *mount_fds = NULL;
size_t i;
int ret;

mount_fds = get_private_data (container)->mount_fds;
get_private_data (container)->mount_fds = NULL;
Expand Down Expand Up @@ -2133,7 +2143,7 @@ do_mounts (libcrun_container_t *container, int rootfsfd, const char *rootfs, lib
if (UNLIKELY (len < 0))
return len;

ret = safe_create_symlink (rootfsfd, rootfs, target, def->mounts[i]->destination, err);
ret = safe_create_symlink (get_private_data (container)->rootfsfd, rootfs, target, def->mounts[i]->destination, err);
if (UNLIKELY (ret < 0))
return ret;

Expand All @@ -2142,20 +2152,20 @@ do_mounts (libcrun_container_t *container, int rootfsfd, const char *rootfs, lib
else if (is_sysfs_or_proc)
{
/* Enforce sysfs and proc to be mounted on a regular directory. */
ret = openat (rootfsfd, target, O_CLOEXEC | O_NOFOLLOW | O_DIRECTORY);
ret = openat (get_private_data (container)->rootfsfd, target, O_CLOEXEC | O_NOFOLLOW | O_DIRECTORY);
if (UNLIKELY (ret < 0))
{
if (errno == ENOENT)
{
if (strchr (target, '/'))
return crun_make_error (err, 0, "invalid target `%s`: it must be mounted at the root", target);

ret = mkdirat (rootfsfd, target, 0755);
ret = mkdirat (get_private_data (container)->rootfsfd, target, 0755);
if (UNLIKELY (ret < 0))
return crun_make_error (err, errno, "mkdirat `%s`", target);

/* Try opening it again. */
ret = openat (rootfsfd, target, O_CLOEXEC | O_NOFOLLOW | O_DIRECTORY);
ret = openat (get_private_data (container)->rootfsfd, target, O_CLOEXEC | O_NOFOLLOW | O_DIRECTORY);
}
else if (errno == ENOTDIR)
return crun_make_error (err, errno, "the target `/%s` is invalid", target);
Expand All @@ -2171,7 +2181,7 @@ do_mounts (libcrun_container_t *container, int rootfsfd, const char *rootfs, lib
bool is_dir = S_ISDIR (src_mode);

/* Make sure any other directory/file is created and take a O_PATH reference to it. */
ret = crun_safe_create_and_open_ref_at (is_dir, rootfsfd, rootfs, target, is_dir ? 01755 : 0755, err);
ret = crun_safe_create_and_open_ref_at (is_dir, get_private_data (container)->rootfsfd, rootfs, target, is_dir ? 01755 : 0755, err);
if (UNLIKELY (ret < 0))
return ret;

Expand Down Expand Up @@ -2244,7 +2254,7 @@ do_mounts (libcrun_container_t *container, int rootfsfd, const char *rootfs, lib
{
int destfd, tmpfd;

destfd = safe_openat (rootfsfd, rootfs, target, O_CLOEXEC | O_DIRECTORY, 0, err);
destfd = safe_openat (get_private_data (container)->rootfsfd, rootfs, target, O_CLOEXEC | O_DIRECTORY, 0, err);
if (UNLIKELY (destfd < 0))
return crun_error_wrap (err, "open `%s` to write for tmpcopyup", target);

Expand All @@ -2261,7 +2271,7 @@ do_mounts (libcrun_container_t *container, int rootfsfd, const char *rootfs, lib
const bool is_dir = S_ISDIR (src_mode);
cleanup_close int dfd = -1;

dfd = safe_openat (rootfsfd, rootfs, target, O_RDONLY | O_PATH | O_CLOEXEC | (is_dir ? O_DIRECTORY : 0), 0, err);
dfd = safe_openat (get_private_data (container)->rootfsfd, rootfs, target, O_RDONLY | O_PATH | O_CLOEXEC | (is_dir ? O_DIRECTORY : 0), 0, err);
if (UNLIKELY (dfd < 0))
return crun_make_error (err, errno, "open mount target `/%s`", target);

Expand All @@ -2282,7 +2292,6 @@ do_mounts (libcrun_container_t *container, int rootfsfd, const char *rootfs, lib
int
libcrun_container_do_bind_mount (libcrun_container_t *container, char *mount_source, char *mount_destination, char **mount_options, size_t mount_options_len, libcrun_error_t *err)
{
int ret, rootfsfd;
const char *target = consume_slashes (mount_destination);
cleanup_free char *data = NULL;
unsigned long flags = 0;
Expand All @@ -2292,9 +2301,9 @@ libcrun_container_do_bind_mount (libcrun_container_t *container, char *mount_sou
uint64_t rec_clear = 0;
uint64_t rec_set = 0;
const char *rootfs = get_private_data (container)->rootfs;
rootfsfd = get_private_data (container)->rootfsfd;
int ret;

if ((rootfsfd < 0) || (rootfs == NULL))
if ((get_private_data (container)->rootfsfd < 0) || (rootfs == NULL))
return crun_make_error (err, 0, "invalid rootfs state while performing bind mount from external plugin or handler");

if (mount_options == NULL)
Expand All @@ -2320,7 +2329,7 @@ libcrun_container_do_bind_mount (libcrun_container_t *container, char *mount_sou
}

/* Make sure any other directory/file is created and take a O_PATH reference to it. */
ret = crun_safe_create_and_open_ref_at (is_dir, rootfsfd, rootfs, target, is_dir ? 01755 : 0755, err);
ret = crun_safe_create_and_open_ref_at (is_dir, get_private_data (container)->rootfsfd, rootfs, target, is_dir ? 01755 : 0755, err);
if (UNLIKELY (ret < 0))
return ret;

Expand Down Expand Up @@ -2576,9 +2585,7 @@ int
libcrun_set_mounts (struct container_entrypoint_s *entrypoint_args, libcrun_container_t *container, const char *rootfs, set_mounts_cb_t cb, void *cb_data, libcrun_error_t *err)
{
runtime_spec_schema_config_schema *def = container->container_def;
cleanup_close int rootfsfd_cleanup = -1;
unsigned long rootfs_propagation = 0;
int rootfsfd = -1;
int cgroup_mode;
int is_user_ns = 0;
int ret = 0;
Expand Down Expand Up @@ -2609,12 +2616,12 @@ libcrun_set_mounts (struct container_entrypoint_s *entrypoint_args, libcrun_cont
return ret;
}

rootfsfd = rootfsfd_cleanup = open (rootfs, O_PATH | O_CLOEXEC);
if (UNLIKELY (rootfsfd < 0))
ret = open (rootfs, O_PATH | O_CLOEXEC);
if (UNLIKELY (ret < 0))
return crun_make_error (err, errno, "open `%s`", rootfs);

get_private_data (container)->rootfsfd = ret;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): Potential file descriptor leak on early return

Open the fd into a temporary variable (e.g. via cleanup_close) and assign it to private_data only after all operations succeed, or close/reset it on every error path.

get_private_data (container)->rootfs = rootfs;
get_private_data (container)->rootfsfd = rootfsfd;

// configure handler mounts
ret = libcrun_container_notify_handler (entrypoint_args, HANDLER_CONFIGURE_MOUNTS, container, rootfs, err);
Expand All @@ -2627,7 +2634,7 @@ libcrun_set_mounts (struct container_entrypoint_s *entrypoint_args, libcrun_cont
unsigned long remount_flags = MS_REMOUNT | MS_BIND | MS_RDONLY;
int fd;

fd = dup (rootfsfd);
fd = dup (get_private_data (container)->rootfsfd);
if (UNLIKELY (fd < 0))
return crun_make_error (err, errno, "dup fd for `%s`", rootfs);

Expand Down Expand Up @@ -2655,7 +2662,7 @@ libcrun_set_mounts (struct container_entrypoint_s *entrypoint_args, libcrun_cont
if (UNLIKELY (ret < 0))
return ret;

ret = do_mounts (container, rootfsfd, rootfs, err);
ret = do_mounts (container, rootfs, err);
if (UNLIKELY (ret < 0))
return ret;

Expand Down Expand Up @@ -2691,7 +2698,7 @@ libcrun_set_mounts (struct container_entrypoint_s *entrypoint_args, libcrun_cont
libcrun_error_t tmp_err = NULL;
const char *rel_cwd = consume_slashes (def->process->cwd);
/* Ignore errors here and let it fail later. */
(void) crun_safe_ensure_directory_at (rootfsfd, rootfs, rel_cwd, 0755, &tmp_err);
(void) crun_safe_ensure_directory_at (get_private_data (container)->rootfsfd, rootfs, rel_cwd, 0755, &tmp_err);
crun_error_release (&tmp_err);
}

Expand All @@ -2708,7 +2715,7 @@ libcrun_set_mounts (struct container_entrypoint_s *entrypoint_args, libcrun_cont
if (UNLIKELY (ret < 0))
return crun_make_error (err, errno, "failed configuring mounts for handler at phase: HANDLER_CONFIGURE_AFTER_MOUNTS");

get_private_data (container)->rootfsfd = -1;
close_and_reset (&(get_private_data (container)->rootfsfd));

return 0;
}
Expand Down Expand Up @@ -4303,7 +4310,7 @@ prepare_and_send_dev_mounts (libcrun_container_t *container, int sync_socket_hos
return ret;

ret = mkdir (devs_path, 0700);
if (UNLIKELY (ret < 0) && errno != EEXIST)
if (UNLIKELY (ret < 0 && errno != EEXIST))
return crun_make_error (err, errno, "mkdir `%s`", devs_path);

current_mountns = open ("/proc/self/ns/mnt", O_RDONLY | O_CLOEXEC);
Expand Down
50 changes: 45 additions & 5 deletions src/libcrun/utils.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,25 @@ crun_ensure_directory_at (int dirfd, const char *path, int mode, bool nofollow,
return 0;
}

static int
check_fd_is_path (const char *path, int fd, const char *fdname, libcrun_error_t *err)
{
proc_fd_path_t fdpath;
size_t path_len = strlen (path);
char link[PATH_MAX];
int ret;

get_proc_self_fd_path (fdpath, fd);
ret = TEMP_FAILURE_RETRY (readlink (fdpath, link, sizeof (link)));
if (UNLIKELY (ret < 0))
return crun_make_error (err, errno, "readlink `%s`", fdname);

if (((size_t) ret) != path_len || memcmp (link, path, path_len))
return crun_make_error (err, 0, "target `%s` does not point to the directory `%s`", fdname, path);

return 0;
}

static int
check_fd_under_path (const char *rootfs, size_t rootfslen, int fd, const char *fdname, libcrun_error_t *err)
{
Expand Down Expand Up @@ -377,6 +396,23 @@ safe_openat (int dirfd, const char *rootfs, const char *path, int flags, int mod
static bool openat2_supported = true;
int ret;

if (is_empty_string (path))
{
cleanup_close int fd = -1;

fd = open (rootfs, flags, mode);
if (UNLIKELY (fd < 0))
return crun_make_error (err, errno, "open `%s`", rootfs);

ret = check_fd_is_path (rootfs, fd, path, err);
if (UNLIKELY (ret < 0))
return ret;

ret = fd;
fd = -1;
return ret;
}

if (openat2_supported)
{
repeat:
Expand Down Expand Up @@ -449,7 +485,11 @@ crun_safe_ensure_at (bool do_open, bool dir, int dirfd, const char *dirpath,

/* Empty path, nothing to do. */
if (*path == '\0')
return 0;
{
if (do_open)
return open (dirpath, O_CLOEXEC | O_PATH, 0);
return 0;
}

npath = xstrdup (path);

Expand Down Expand Up @@ -577,12 +617,12 @@ crun_safe_ensure_at (bool do_open, bool dir, int dirfd, const char *dirpath,
int
crun_safe_create_and_open_ref_at (bool dir, int dirfd, const char *dirpath, const char *path, int mode, libcrun_error_t *err)
{
int fd;
int ret;

/* If the file/dir already exists, just open it. */
fd = safe_openat (dirfd, dirpath, path, O_PATH | O_CLOEXEC, 0, err);
if (LIKELY (fd >= 0))
return fd;
ret = safe_openat (dirfd, dirpath, path, O_PATH | O_CLOEXEC, 0, err);
if (LIKELY (ret >= 0))
return ret;

crun_error_release (err);
return crun_safe_ensure_at (true, dir, dirfd, dirpath, path, mode, MAX_READLINKS, err);
Expand Down
Loading