SymbX/systemd
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

xattr-util: rework getxattr_at_malloc()

Browse Source 
Let's return the size in a return parameter instead of the return value.
And if NULL is specified this tells us the caller doesn't care about the
size and expects a NUL terminated string. In that case look for an
embedded NUL byte, and refuse in that case.

This should lock things down a bit, as we'll systematically refuse
embedded NUL strings now when we expect strings.
This commit is contained in:
Lennart Poettering
2025-03-18 07:17:10 +01:00
parent 147511c643
commit 33cbda04eb
15 changed files with 104 additions and 76 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/basic/cgroup-util.c
+2 -2
View File
@@ -723,7 +723,7 @@ int cg_get_xattr(const char *path, const char *name, void *value, size_t size) {
return (int) n;
}
int cg_get_xattr_malloc(const char *path, const char *name, char **ret) {
int cg_get_xattr_malloc(const char *path, const char *name, char **ret, size_t *ret_size) {
_cleanup_free_ char *fs = NULL;
int r;
@@ -734,7 +734,7 @@ int cg_get_xattr_malloc(const char *path, const char *name, char **ret) {
if (r < 0)
return r;
return lgetxattr_malloc(fs, name, ret);
return lgetxattr_malloc(fs, name, ret, ret_size);
}
int cg_get_xattr_bool(const char *path, const char *name) {
src/basic/cgroup-util.h
+1 -1
View File
@@ -264,7 +264,7 @@ int cg_get_owner(const char *path, uid_t *ret_uid);
int cg_set_xattr(const char *path, const char *name, const void *value, size_t size, int flags);
int cg_get_xattr(const char *path, const char *name, void *value, size_t size);
int cg_get_xattr_malloc(const char *path, const char *name, char **ret);
int cg_get_xattr_malloc(const char *path, const char *name, char **ret, size_t *ret_size);
/* Returns negative on error, and 0 or 1 on success for the bool value */
int cg_get_xattr_bool(const char *path, const char *name);
int cg_remove_xattr(const char *path, const char *name);
src/basic/xattr-util.c
+29 -24
View File
@@ -59,7 +59,6 @@ static int normalize_and_maybe_pin_inode(
if (r < 0)
return r;
*ret_opath = r;
*ret_tfd = -EBADF;
return 0;
}
@@ -77,7 +76,7 @@ static int normalize_and_maybe_pin_inode(
return 0;
}
static int getxattr_pinned_internal(
static ssize_t getxattr_pinned_internal(
int fd,
const char *path,
int at_flags,
@@ -104,11 +103,7 @@ static int getxattr_pinned_internal(
return -errno;
assert((size_t) n <= size);
if (n > INT_MAX) /* We couldn't return this as 'int' anymore */
return -E2BIG;
return (int) n;
return n;
}
int getxattr_at_malloc(
@@ -116,7 +111,8 @@ int getxattr_at_malloc(
const char *path,
const char *name,
int at_flags,
char **ret) {
char **ret,
size_t *ret_size) {
_cleanup_close_ int opened_fd = -EBADF;
bool by_procfs;
@@ -154,20 +150,31 @@ int getxattr_at_malloc(
l = MALLOC_ELEMENTSOF(v) - 1;
r = getxattr_pinned_internal(fd, path, at_flags, by_procfs, name, v, l);
if (r >= 0) {
v[r] = 0; /* NUL terminate */
ssize_t n;
n = getxattr_pinned_internal(fd, path, at_flags, by_procfs, name, v, l);
if (n >= 0) {
/* Refuse extended attributes with embedded NUL bytes if the caller isn't interested
* in the size. After all this must mean the caller assumes we return a NUL
* terminated strings, but if there's a NUL byte embedded they are definitely not
* regular strings */
if (!ret_size && n > 1 && memchr(v, 0, n - 1))
return -EBADMSG;
v[n] = 0; /* NUL terminate */
*ret = TAKE_PTR(v);
return r;
if (ret_size)
*ret_size = (size_t) n;
return 0;
}
if (r != -ERANGE)
return r;
if (n != -ERANGE)
return (int) n;
r = getxattr_pinned_internal(fd, path, at_flags, by_procfs, name, NULL, 0);
if (r < 0)
return r;
n = getxattr_pinned_internal(fd, path, at_flags, by_procfs, name, NULL, 0);
if (n < 0)
return (int) n;
l = (size_t) r;
l = (size_t) n;
}
}
@@ -175,13 +182,10 @@ int getxattr_at_bool(int fd, const char *path, const char *name, int at_flags) {
_cleanup_free_ char *v = NULL;
int r;
r = getxattr_at_malloc(fd, path, name, at_flags, &v);
r = getxattr_at_malloc(fd, path, name, at_flags, &v, /* ret_size= */ NULL);
if (r < 0)
return r;
if (memchr(v, 0, r)) /* Refuse embedded NUL byte */
return -EINVAL;
return parse_boolean(v);
}
@@ -408,9 +412,10 @@ int getcrtime_at(
else
a = USEC_INFINITY;
r = getxattr_at_malloc(fd, path, "user.crtime_usec", at_flags, (char**) &le);
size_t le_size;
r = getxattr_at_malloc(fd, path, "user.crtime_usec", at_flags, (char**) &le, &le_size);
if (r >= 0) {
if (r != sizeof(*le))
if (le_size != sizeof(*le))
r = -EIO;
else
r = parse_crtime(*le, &b);
src/basic/xattr-util.h
+8 -7
View File
@@ -1,21 +1,22 @@
/* SPDX-License-Identifier: LGPL-2.1-or-later */
#pragma once
#include <fcntl.h>
#include <stdbool.h>
#include <stddef.h>
#include <sys/types.h>
#include "time-util.h"
int getxattr_at_malloc(int fd, const char *path, const char *name, int at_flags, char **ret);
static inline int getxattr_malloc(const char *path, const char *name, char **ret) {
return getxattr_at_malloc(AT_FDCWD, path, name, AT_SYMLINK_FOLLOW, ret);
int getxattr_at_malloc(int fd, const char *path, const char *name, int at_flags, char **ret, size_t *ret_size);
static inline int getxattr_malloc(const char *path, const char *name, char **ret, size_t *ret_size) {
return getxattr_at_malloc(AT_FDCWD, path, name, AT_SYMLINK_FOLLOW, ret, ret_size);
}
static inline int lgetxattr_malloc(const char *path, const char *name, char **ret) {
return getxattr_at_malloc(AT_FDCWD, path, name, 0, ret);
static inline int lgetxattr_malloc(const char *path, const char *name, char **ret, size_t *ret_size) {
return getxattr_at_malloc(AT_FDCWD, path, name, 0, ret, ret_size);
}
static inline int fgetxattr_malloc(int fd, const char *name, char **ret) {
return getxattr_at_malloc(fd, NULL, name, AT_EMPTY_PATH, ret);
static inline int fgetxattr_malloc(int fd, const char *name, char **ret, size_t *ret_size) {
return getxattr_at_malloc(fd, NULL, name, AT_EMPTY_PATH, ret, ret_size);
}
int getxattr_at_bool(int fd, const char *path, const char *name, int at_flags);
src/core/cgroup.c
+3 -3
View File
@@ -3968,10 +3968,10 @@ int unit_check_oomd_kill(Unit *u) {
r = cg_all_unified();
if (r < 0)
return log_unit_debug_errno(u, r, "Couldn't determine whether we are in all unified mode: %m");
else if (r == 0)
if (r == 0)
return 0;
r = cg_get_xattr_malloc(crt->cgroup_path, "user.oomd_ooms", &value);
r = cg_get_xattr_malloc(crt->cgroup_path, "user.oomd_ooms", &value, /* ret_size= */ NULL);
if (r < 0 && !ERRNO_IS_XATTR_ABSENT(r))
return r;
@@ -3989,7 +3989,7 @@ int unit_check_oomd_kill(Unit *u) {
n = 0;
value = mfree(value);
r = cg_get_xattr_malloc(crt->cgroup_path, "user.oomd_kill", &value);
r = cg_get_xattr_malloc(crt->cgroup_path, "user.oomd_kill", &value, /* ret_size= */ NULL);
if (r >= 0 && !isempty(value))
(void) safe_atou64(value, &n);
src/home/homework-fscrypt.c
+7 -8
View File
@@ -309,9 +309,8 @@ static int fscrypt_setup(
NULSTR_FOREACH(xa, xattr_buf) {
_cleanup_free_ void *salt = NULL, *encrypted = NULL;
_cleanup_free_ char *value = NULL;
size_t salt_size, encrypted_size;
size_t salt_size, encrypted_size, vsize;
const char *nr, *e;
int n;
/* Check if this xattr has the format 'trusted.fscrypt_slot<nr>' where '<nr>' is a 32-bit unsigned integer */
nr = startswith(xa, "trusted.fscrypt_slot");
@@ -320,13 +319,13 @@ static int fscrypt_setup(
if (safe_atou32(nr, NULL) < 0)
continue;
n = fgetxattr_malloc(setup->root_fd, xa, &value);
if (n == -ENODATA) /* deleted by now? */
r = fgetxattr_malloc(setup->root_fd, xa, &value, &vsize);
if (r == -ENODATA) /* deleted by now? */
continue;
if (n < 0)
return log_error_errno(n, "Failed to read %s xattr: %m", xa);
if (r < 0)
return log_error_errno(r, "Failed to read %s xattr: %m", xa);
e = memchr(value, ':', n);
e = memchr(value, ':', vsize);
if (!e)
return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "xattr %s lacks ':' separator.", xa);
@@ -334,7 +333,7 @@ static int fscrypt_setup(
if (r < 0)
return log_error_errno(r, "Failed to decode salt of %s: %m", xa);
r = unbase64mem_full(e + 1, n - (e - value) - 1, /* secure = */ false, &encrypted, &encrypted_size);
r = unbase64mem_full(e + 1, vsize - (e - value) - 1, /* secure = */ false, &encrypted, &encrypted_size);
if (r < 0)
return log_error_errno(r, "Failed to decode encrypted key of %s: %m", xa);
src/journal/journald-client.c
+11 -8
View File
@@ -46,25 +46,26 @@ static int client_parse_log_filter_nulstr(const char *nulstr, size_t len, Set **
}
int client_context_read_log_filter_patterns(ClientContext *c, const char *cgroup) {
char *deny_list_xattr, *xattr_end;
_cleanup_free_ char *xattr = NULL, *unit_cgroup = NULL;
_cleanup_set_free_ Set *allow_list = NULL, *deny_list = NULL;
int r;
assert(c);
_cleanup_free_ char *unit_cgroup = NULL;
r = cg_path_get_unit_path(cgroup, &unit_cgroup);
if (r < 0)
return log_debug_errno(r, "Failed to get the unit's cgroup path for %s: %m", cgroup);
r = cg_get_xattr_malloc(unit_cgroup, "user.journald_log_filter_patterns", &xattr);
_cleanup_free_ char *xattr = NULL;
size_t xattr_size = 0;
r = cg_get_xattr_malloc(unit_cgroup, "user.journald_log_filter_patterns", &xattr, &xattr_size);
if (ERRNO_IS_NEG_XATTR_ABSENT(r)) {
client_set_filtering_patterns(c, NULL, NULL);
client_set_filtering_patterns(c, /* allow_list= */ NULL, /* deny_list= */ NULL);
return 0;
} else if (r < 0)
}
if (r < 0)
return log_debug_errno(r, "Failed to get user.journald_log_filter_patterns xattr for %s: %m", unit_cgroup);
xattr_end = xattr + r;
const char *xattr_end = xattr + xattr_size;
/* We expect '0xff' to be present in the attribute, even if the lists are empty. We expect the
* following:
@@ -76,17 +77,19 @@ int client_context_read_log_filter_patterns(ClientContext *c, const char *cgroup
*
* We do not expect both the allow list and deny list to be empty, as this condition is tested
* before writing to xattr. */
deny_list_xattr = memchr(xattr, (char)0xff, r);
const char *deny_list_xattr = memchr(xattr, (char)0xff, xattr_size);
if (!deny_list_xattr)
return log_debug_errno(SYNTHETIC_ERRNO(EBADMSG),
"Missing delimiter in cgroup user.journald_log_filter_patterns attribute: %m");
_cleanup_set_free_ Set *allow_list = NULL;
r = client_parse_log_filter_nulstr(xattr, deny_list_xattr - xattr, &allow_list);
if (r < 0)
return r;
/* Use 'deny_list_xattr + 1' to skip '0xff'. */
++deny_list_xattr;
_cleanup_set_free_ Set *deny_list = NULL;
r = client_parse_log_filter_nulstr(deny_list_xattr, xattr_end - deny_list_xattr, &deny_list);
if (r < 0)
return r;
src/oom/oomd-util.c
+1 -1
View File
@@ -40,7 +40,7 @@ static int increment_oomd_xattr(const char *path, const char *xattr, uint64_t nu
assert(path);
assert(xattr);
r = cg_get_xattr_malloc(path, xattr, &value);
r = cg_get_xattr_malloc(path, xattr, &value, /* ret_size= */ NULL);
if (r < 0 && !ERRNO_IS_XATTR_ABSENT(r))
return r;
src/oom/test-oomd-util.c
+2 -2
View File
@@ -77,7 +77,7 @@ static void test_oomd_cgroup_kill(void) {
abort();
}
assert_se(cg_get_xattr_malloc(cgroup, "user.oomd_ooms", &v) >= 0);
ASSERT_OK(cg_get_xattr_malloc(cgroup, "user.oomd_ooms", &v, /* ret_size= */ NULL));
assert_se(streq(v, i == 0 ? "1" : "2"));
v = mfree(v);
@@ -85,7 +85,7 @@ static void test_oomd_cgroup_kill(void) {
sleep(2);
assert_se(cg_is_empty(SYSTEMD_CGROUP_CONTROLLER, cgroup) == true);
assert_se(cg_get_xattr_malloc(cgroup, "user.oomd_kill", &v) >= 0);
ASSERT_OK(cg_get_xattr_malloc(cgroup, "user.oomd_kill", &v, /* ret_size= */ NULL));
assert_se(streq(v, i == 0 ? "2" : "4"));
}
}
src/shared/cgroup-show.c
+4 -4
View File
@@ -186,13 +186,13 @@ static int show_cgroup_name(
NULSTR_FOREACH(xa, nl) {
_cleanup_free_ char *x = NULL, *y = NULL, *buf = NULL;
int n;
if (!STARTSWITH_SET(xa, "user.", "trusted."))
continue;
n = fgetxattr_malloc(fd, xa, &buf);
if (n < 0) {
size_t buf_size;
r = fgetxattr_malloc(fd, xa, &buf, &buf_size);
if (r < 0) {
log_debug_errno(r, "Failed to read xattr '%s' off '%s', ignoring: %m", xa, path);
continue;
}
@@ -201,7 +201,7 @@ static int show_cgroup_name(
if (!x)
return -ENOMEM;
y = cescape_length(buf, n);
y = cescape_length(buf, buf_size);
if (!y)
return -ENOMEM;
src/shared/copy.c
+4 -4
View File
@@ -1670,18 +1670,18 @@ int copy_xattr(int df, const char *from, int dt, const char *to, CopyFlags copy_
return r;
NULSTR_FOREACH(p, names) {
_cleanup_free_ char *value = NULL;
if (!FLAGS_SET(copy_flags, COPY_ALL_XATTRS) && !startswith(p, "user."))
continue;
r = getxattr_at_malloc(df, from, p, 0, &value);
_cleanup_free_ char *value = NULL;
size_t value_size;
r = getxattr_at_malloc(df, from, p, 0, &value, &value_size);
if (r == -ENODATA)
continue; /* gone by now */
if (r < 0)
return r;
RET_GATHER(ret, xsetxattr_full(dt, to, /* at_flags = */ 0, p, value, r, /* xattr_flags = */ 0));
RET_GATHER(ret, xsetxattr_full(dt, to, /* at_flags = */ 0, p, value, value_size, /* xattr_flags = */ 0));
}
return ret;
src/shared/dissect-image.c
+2 -2
View File
@@ -3248,7 +3248,7 @@ int verity_settings_load(
* that doesn't exist for /usr */
if (designator < 0 || designator == PARTITION_ROOT) {
r = getxattr_malloc(image, "user.verity.roothash", &text);
r = getxattr_malloc(image, "user.verity.roothash", &text, /* ret_size= */ NULL);
if (r < 0) {
_cleanup_free_ char *p = NULL;
@@ -3277,7 +3277,7 @@ int verity_settings_load(
* `usrhash`, because `usrroothash` or `rootusrhash` would just be too
* confusing. We thus drop the reference to the root of the Merkle tree, and
* just indicate which file system it's about. */
r = getxattr_malloc(image, "user.verity.usrhash", &text);
r = getxattr_malloc(image, "user.verity.usrhash", &text, /* ret_size= */ NULL);
if (r < 0) {
_cleanup_free_ char *p = NULL;
src/shared/smack-util.c
+2 -2
View File
@@ -60,7 +60,7 @@ int mac_smack_read_at(int fd, const char *path, SmackAttr attr, char **ret) {
return 0;
}
return getxattr_at_malloc(fd, path, smack_attr_to_string(attr), /* at_flags = */ 0, ret);
return getxattr_at_malloc(fd, path, smack_attr_to_string(attr), /* at_flags = */ 0, ret, /* ret_size= */ NULL);
}
int mac_smack_apply_at(int fd, const char *path, SmackAttr attr, const char *label) {
@@ -136,7 +136,7 @@ static int smack_fix_fd(
/* If the old label is identical to the new one, suppress any kind of error */
_cleanup_free_ char *old_label = NULL;
if (fgetxattr_malloc(fd, "security.SMACK64", &old_label) >= 0 &&
if (fgetxattr_malloc(fd, "security.SMACK64", &old_label, /* ret_size= */ NULL) >= 0 &&
streq(old_label, label))
return 0;
src/test/test-copy.c
+1 -1
View File
@@ -204,7 +204,7 @@ TEST(copy_tree) {
assert_se(read_full_file(f, &buf, &sz) == 0);
ASSERT_STREQ(buf, "file\n");
k = lgetxattr_malloc(f, "user.testxattr", &c);
k = lgetxattr_malloc(f, "user.testxattr", &c, /* ret_size= */ NULL);
assert_se(xattr_worked < 0 || ((k >= 0) == !!xattr_worked));
if (k >= 0) {
src/test/test-xattr-util.c
+27 -7
View File
@@ -35,25 +35,34 @@ TEST(getxattr_at_malloc) {
return (void) log_tests_skipped_errno(errno, "no xattrs supported on /var/tmp");
assert_se(r >= 0);
assert_se(getxattr_at_malloc(fd, "test", "user.foo", 0, &value) == 3);
ASSERT_OK(getxattr_at_malloc(fd, "test", "user.foo", 0, &value, /* ret_size= */ NULL));
assert_se(memcmp(value, "bar", 3) == 0);
value = mfree(value);
assert_se(getxattr_at_malloc(AT_FDCWD, x, "user.foo", 0, &value) == 3);
ASSERT_OK(getxattr_at_malloc(AT_FDCWD, x, "user.foo", 0, &value, /* ret_size= */ NULL));
assert_se(memcmp(value, "bar", 3) == 0);
value = mfree(value);
safe_close(fd);
fd = open("/", O_RDONLY|O_DIRECTORY|O_CLOEXEC|O_NOCTTY);
assert_se(fd >= 0);
r = getxattr_at_malloc(fd, "usr", "user.idontexist", 0, &value);
r = getxattr_at_malloc(fd, "usr", "user.idontexist", 0, &value, /* ret_size= */ NULL);
assert_se(ERRNO_IS_NEG_XATTR_ABSENT(r));
safe_close(fd);
fd = open(x, O_PATH|O_CLOEXEC);
assert_se(fd >= 0);
assert_se(getxattr_at_malloc(fd, NULL, "user.foo", 0, &value) == 3);
ASSERT_OK(getxattr_at_malloc(fd, NULL, "user.foo", 0, &value, /* ret_size= */ NULL));
ASSERT_STREQ(value, "bar");
value = mfree(value);
ASSERT_OK_ERRNO(setxattr(x, "user.foozu", "bar\0qux\0wal\0\0", 13, /* flags= */ 0));
ASSERT_ERROR(getxattr_at_malloc(fd, /* path= */ NULL, "user.foozu", 0, &value, /* ret_size= */ NULL), EBADMSG);
size_t value_size;
ASSERT_OK(getxattr_at_malloc(fd, /* path= */ NULL, "user.foozu", 0, &value, &value_size));
ASSERT_EQ(value_size, 13U);
ASSERT_EQ(memcmp(value, "bar\0qux\0wal\0\0", 13), 0);
ASSERT_EQ(value[13], 0); /* check extra NUL */
}
TEST(getcrtime) {
@@ -84,7 +93,13 @@ TEST(getcrtime) {
static void verify_xattr(int dfd, const char *expected) {
_cleanup_free_ char *value = NULL;
ASSERT_OK_EQ(getxattr_at_malloc(dfd, "test", "user.foo", 0, &value), (int) strlen(expected));
ASSERT_OK(getxattr_at_malloc(dfd, "test", "user.foo", 0, &value, /* ret_size= */ NULL));
ASSERT_STREQ(value, expected);
value = mfree(value);
size_t size;
ASSERT_OK(getxattr_at_malloc(dfd, "test", "user.foo", 0, &value, &size));
ASSERT_EQ(size, strlen(expected));
ASSERT_STREQ(value, expected);
}
@@ -96,7 +111,12 @@ static void xattr_symlink_test_one(int fd, const char *path) {
ASSERT_ERROR(xsetxattr_full(fd, path, 0, "trusted.bar", "bogus", SIZE_MAX, XATTR_CREATE), EEXIST);
ASSERT_OK(xsetxattr(fd, path, 0, "trusted.test", "schaffen"));
ASSERT_OK_EQ(getxattr_at_malloc(fd, path, "trusted.test", 0, &value), (int) STRLEN("schaffen"));
ASSERT_OK(getxattr_at_malloc(fd, path, "trusted.test", 0, &value, /* ret_size= */ NULL));
ASSERT_STREQ(value, "schaffen");
value = mfree(value);
size_t size;
ASSERT_OK(getxattr_at_malloc(fd, path, "trusted.test", 0, &value, &size));
ASSERT_EQ(size, strlen(value));
ASSERT_STREQ(value, "schaffen");
r = listxattr_at_malloc(fd, path, 0, &list);
@@ -107,7 +127,7 @@ static void xattr_symlink_test_one(int fd, const char *path) {
ASSERT_TRUE(strv_contains(list_split, "trusted.test"));
ASSERT_OK(xremovexattr(fd, path, 0, "trusted.test"));
ASSERT_ERROR(getxattr_at_malloc(fd, path, "trusted.test", 0, &value), ENODATA);
ASSERT_ERROR(getxattr_at_malloc(fd, path, "trusted.test", 0, &value, /* ret_size= */ NULL), ENODATA);
}
TEST(xsetxattr) {