On Mon, 22 May 2023 15:41:17 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:On Mon, May 22, 2023 at 01:41:56AM +0200, Stefano Brivio wrote:Ah, yes, I thought of doing that, then I also thought that with 4294967295 as range size in the first line, there can't be other lines. Then I (clumsily) went back to the original idea, because... go figure, let's say it gets extended with something one day... But now it's inconsistent (and probably confusing): the buffer is long enough, but I don't use it: const char root_uid_map[] = " 0 0 4294967295"; 32 bytes string length, sizeof() is 33 char buf[sizeof(root_uid_map) + 1]; 34 bytes (could be 33) read(fd, buf, sizeof(root_uid_map)) read at most 33 bytes != sizeof(root_uid_map) || isn't 33 bytes (should be 32) strncmp(buf, root_uid_map, sizeof(root_uid_map) - 1)) or the first 32 bytes, up to "5", differ (we should compare one more) and, by the way, uid_map has _lines_: $ hexdump -C /proc/self/uid_map 00000000 20 20 20 20 20 20 20 20 20 30 20 20 20 20 20 20 | 0 | 00000010 20 20 20 20 30 20 34 32 39 34 39 36 37 32 39 35 | 0 4294967295| 00000020 0a |.| so it kind of works, but not for the reason I intended. Respinning now... -- StefanoWe'll need this in isolate_initial(). While at it, don't rely on BUFSIZ: the earlier issue we had with musl reminded me it's not a magic "everything will fit" value. Size the read buffer to what we actually need from uid_map. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au> Although...--- conf.c | 16 +--------------- util.c | 25 +++++++++++++++++++++++++ util.h | 2 ++ 3 files changed, 28 insertions(+), 15 deletions(-) diff --git a/conf.c b/conf.c index 447b000..984c3ce 100644 --- a/conf.c +++ b/conf.c @@ -1096,10 +1096,6 @@ static int conf_runas(char *opt, unsigned int *uid, unsigned int *gid) */ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid) { - const char root_uid_map[] = " 0 0 4294967295"; - char buf[BUFSIZ]; - int fd; - /* If user has specified --runas, that takes precedence... */ if (runas) { if (conf_runas(runas, uid, gid)) @@ -1116,18 +1112,8 @@ static void conf_ugid(char *runas, uid_t *uid, gid_t *gid) return; /* ...or at least not root in the init namespace... */ - if ((fd = open("/proc/self/uid_map", O_RDONLY | O_CLOEXEC)) < 0) { - die("Can't determine if we're in init namespace: %s", - strerror(errno)); - } - - if (read(fd, buf, BUFSIZ) != sizeof(root_uid_map) || - strncmp(buf, root_uid_map, sizeof(root_uid_map) - 1)) { - close(fd); + if (!ns_is_init()) return; - } - - close(fd); /* ...otherwise use nobody:nobody */ warn("Don't run as root. Changing to nobody..."); diff --git a/util.c b/util.c index c3e3471..5ec8a6c 100644 --- a/util.c +++ b/util.c @@ -390,6 +390,31 @@ int ns_enter(const struct ctx *c) return 0; } +/** + * ns_is_init() - Is the caller running in the "init" user namespace? + * + * Return: true if caller is in init, false otherwise, won't return on failure + */ +bool ns_is_init(void) +{ + const char root_uid_map[] = " 0 0 4294967295"; + char buf[sizeof(root_uid_map) + 1]; + bool ret = true; + int fd; + + if ((fd = open("/proc/self/uid_map", O_RDONLY | O_CLOEXEC)) < 0) { + die("Can't determine if we're in init namespace: %s", + strerror(errno)); + } + + if (read(fd, buf, sizeof(root_uid_map)) != sizeof(root_uid_map) ||I don't think it can go bad in practice, but I think you want to pass a slightly larger buffer than root_uid_map[], otherwise this test will succeed if the uid_map contains the expected thing for init, followed by something else.