From 68ef4931cb8bcbc53c0d2b6078946d836da7b5d9 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Wed, 28 Sep 2022 14:33:18 +1000 Subject: Clean up parsing in conf_runas() conf_runas() handles several of the different possible cases for the --runas argument in a slightly odd order. Although it can parse both numeric UIDs/GIDs and user/group names, it can't parse a numeric UID combined with a group name or vice versa. That's not obviously useful, but it's slightly surprising gap to have. Rework the parsing to be more systematic: first split the option into user and (optional) group parts, then separately parse each part as either numeric or a name. As a bonus this removes some clang-tidy warnings. While we're there also add cppcheck suppressions for getpwnam() and getgrnam(). It complains about those because they're not reentrant. passt is single threaded though, and is always likely to be during this initialization code, even if we multithread later. There were some existing suppressions for these in the cppcheck invocation but they're no longer up to date. Replace them with inline suppressions which, being next to the code, are more likely to stay correct. Signed-off-by: David Gibson Signed-off-by: Stefano Brivio --- Makefile | 3 +- conf.c | 95 ++++++++++++++++++++++++++++++++++------------------------------ 2 files changed, 51 insertions(+), 47 deletions(-) diff --git a/Makefile b/Makefile index 59967eb..e0933f3 100644 --- a/Makefile +++ b/Makefile @@ -282,12 +282,12 @@ cppcheck: $(SRCS) $(HEADERS) $(SYSTEM_INCLUDES:%=--config-exclude=%) \ $(SYSTEM_INCLUDES:%=--suppress=*:%/*) \ $(SYSTEM_INCLUDES:%=--suppress=unmatchedSuppression:%/*) \ + --inline-suppr \ --suppress=objectIndex:tcp.c --suppress=objectIndex:udp.c \ --suppress=va_list_usedBeforeStarted:util.c \ --suppress=unusedFunction \ --suppress=knownConditionTrueFalse:conf.c \ --suppress=strtokCalled:conf.c --suppress=strtokCalled:qrap.c \ - --suppress=getpwnamCalled:passt.c \ --suppress=localtimeCalled:pcap.c \ --suppress=unusedStructMember:pcap.c \ --suppress=funcArgNamesDifferent:util.h \ @@ -295,7 +295,6 @@ cppcheck: $(SRCS) $(HEADERS) \ --suppress=unmatchedSuppression:conf.c \ --suppress=unmatchedSuppression:dhcp.c \ - --suppress=unmatchedSuppression:passt.c \ --suppress=unmatchedSuppression:pcap.c \ --suppress=unmatchedSuppression:qrap.c \ --suppress=unmatchedSuppression:tcp.c \ diff --git a/conf.c b/conf.c index dbc8864..c668eea 100644 --- a/conf.c +++ b/conf.c @@ -859,46 +859,50 @@ dns6: * * Return: 0 on success, negative error code on failure */ -static int conf_runas(const char *opt, unsigned int *uid, unsigned int *gid) +static int conf_runas(char *opt, unsigned int *uid, unsigned int *gid) { - char ubuf[LOGIN_NAME_MAX], gbuf[LOGIN_NAME_MAX], *endptr; - struct passwd *pw; - struct group *gr; + const char *uopt, *gopt = NULL; + char *sep = strchr(opt, ':'); + char *endptr; - /* NOLINTNEXTLINE(cert-err34-c): 2 if conversion succeeds */ - if (sscanf(opt, "%u:%u", uid, gid) == 2 && *uid && *gid) - return 0; - - *uid = strtol(opt, &endptr, 0); - if (!*endptr && (*gid = *uid)) - return 0; - -#ifdef GLIBC_NO_STATIC_NSS - (void)ubuf; - (void)gbuf; - (void)pw; - (void)gr; - - return -EINVAL; -#else - /* NOLINTNEXTLINE(cert-err34-c): 2 if conversion succeeds */ - if (sscanf(opt, "%" STR(LOGIN_NAME_MAX) "[^:]:" - "%" STR(LOGIN_NAME_MAX) "s", ubuf, gbuf) == 2) { - if (!(pw = getpwnam(ubuf)) || !(*uid = pw->pw_uid)) - return -ENOENT; + if (sep) { + *sep = '\0'; + gopt = sep + 1; + } + uopt = opt; - if (!(gr = getgrnam(gbuf)) || !(*gid = gr->gr_gid)) + *gid = *uid = strtol(uopt, &endptr, 0); + if (*endptr) { +#ifndef GLIBC_NO_STATIC_NSS + /* Not numeric, look up as a username */ + struct passwd *pw; + /* cppcheck-suppress getpwnamCalled */ + if (!(pw = getpwnam(uopt)) || !(*uid = pw->pw_uid)) return -ENOENT; + *gid = pw->pw_gid; +#else + return -EINVAL; +#endif + } + if (!gopt) return 0; - } - pw = getpwnam(ubuf); - if (!pw || !(*uid = pw->pw_uid) || !(*gid = pw->pw_gid)) - return -ENOENT; + *gid = strtol(gopt, &endptr, 0); + if (*endptr) { +#ifndef GLIBC_NO_STATIC_NSS + /* Not numeric, look up as a group name */ + struct group *gr; + /* cppcheck-suppress getgrnamCalled */ + if (!(gr = getgrnam(gopt))) + return -ENOENT; + *gid = gr->gr_gid; +#else + return -EINVAL; +#endif + } return 0; -#endif /* !GLIBC_NO_STATIC_NSS */ } /** @@ -909,10 +913,9 @@ static int conf_runas(const char *opt, unsigned int *uid, unsigned int *gid) * * Return: 0 on success, negative error code on failure */ -static int conf_ugid(const char *runas, uid_t *uid, gid_t *gid) +static int conf_ugid(char *runas, uid_t *uid, gid_t *gid) { const char root_uid_map[] = " 0 0 4294967295"; - struct passwd *pw; char buf[BUFSIZ]; int ret; int fd; @@ -951,21 +954,23 @@ static int conf_ugid(const char *runas, uid_t *uid, gid_t *gid) /* ...otherwise use nobody:nobody */ warn("Don't run as root. Changing to nobody..."); + { #ifndef GLIBC_NO_STATIC_NSS - pw = getpwnam("nobody"); - if (!pw) { - perror("getpwnam"); - exit(EXIT_FAILURE); - } + struct passwd *pw; + /* cppcheck-suppress getpwnamCalled */ + pw = getpwnam("nobody"); + if (!pw) { + perror("getpwnam"); + exit(EXIT_FAILURE); + } - *uid = pw->pw_uid; - *gid = pw->pw_gid; + *uid = pw->pw_uid; + *gid = pw->pw_gid; #else - (void)pw; - - /* Common value for 'nobody', not really specified */ - *uid = *gid = 65534; + /* Common value for 'nobody', not really specified */ + *uid = *gid = 65534; #endif + } return 0; } @@ -1032,8 +1037,8 @@ void conf(struct ctx *c, int argc, char **argv) struct fqdn *dnss = c->dns_search; uint32_t *dns4 = c->ip4.dns; int name, ret, mask, b, i; - const char *runas = NULL; unsigned int ifi = 0; + char *runas = NULL; uid_t uid; gid_t gid; -- cgit v1.2.3