From 3940020c94e24aa7983f9fdd6ad2d37d53ab5e11 Mon Sep 17 00:00:00 2001 From: "Todd C. Miller" Date: Tue, 4 Oct 2022 09:33:44 -0600 Subject: [PATCH] sudo_secure_open_{file,dir}: always check thatreturn value is not -1. Avoids false positives from static analyzers that can't figure out that the fd is always valid when error is SUDO_PATH_SECURE. --- lib/util/sudo_conf.c | 49 +++++++++---------- plugins/sudoers/sudoers.c | 44 +++++++++--------- plugins/sudoers/testsudoers.c | 52 +++++++++++---------- plugins/sudoers/timestamp.c | 88 +++++++++++++++++------------------ 4 files changed, 120 insertions(+), 113 deletions(-) diff --git a/lib/util/sudo_conf.c b/lib/util/sudo_conf.c index 454d071d2..5f046ffa1 100644 --- a/lib/util/sudo_conf.c +++ b/lib/util/sudo_conf.c @@ -681,30 +681,31 @@ sudo_conf_read_v1(const char *conf_file, int conf_types) int error; conf_file = _PATH_SUDO_CONF; fd = sudo_secure_open_file(conf_file, ROOT_UID, -1, &sb, &error); - switch (error) { - case SUDO_PATH_SECURE: - break; - case SUDO_PATH_MISSING: - /* Root should always be able to read sudo.conf. */ - if (errno != ENOENT && geteuid() == ROOT_UID) - sudo_warn(U_("unable to open %s"), conf_file); - goto done; - case SUDO_PATH_BAD_TYPE: - sudo_warnx(U_("%s is not a regular file"), conf_file); - goto done; - case SUDO_PATH_WRONG_OWNER: - sudo_warnx(U_("%s is owned by uid %u, should be %u"), - conf_file, (unsigned int) sb.st_uid, ROOT_UID); - goto done; - case SUDO_PATH_WORLD_WRITABLE: - sudo_warnx(U_("%s is world writable"), conf_file); - goto done; - case SUDO_PATH_GROUP_WRITABLE: - sudo_warnx(U_("%s is group writable"), conf_file); - goto done; - default: - sudo_warnx("%s: internal error, unexpected error %d", - __func__, error); + if (fd == -1) { + switch (error) { + case SUDO_PATH_MISSING: + /* Root should always be able to read sudo.conf. */ + if (errno != ENOENT && geteuid() == ROOT_UID) + sudo_warn(U_("unable to open %s"), conf_file); + break; + case SUDO_PATH_BAD_TYPE: + sudo_warnx(U_("%s is not a regular file"), conf_file); + break; + case SUDO_PATH_WRONG_OWNER: + sudo_warnx(U_("%s is owned by uid %u, should be %u"), + conf_file, (unsigned int) sb.st_uid, ROOT_UID); + break; + case SUDO_PATH_WORLD_WRITABLE: + sudo_warnx(U_("%s is world writable"), conf_file); + break; + case SUDO_PATH_GROUP_WRITABLE: + sudo_warnx(U_("%s is group writable"), conf_file); + break; + default: + sudo_warnx("%s: internal error, unexpected error %d", + __func__, error); + break; + } goto done; } } diff --git a/plugins/sudoers/sudoers.c b/plugins/sudoers/sudoers.c index c079ea3fb..faa5be4f2 100644 --- a/plugins/sudoers/sudoers.c +++ b/plugins/sudoers/sudoers.c @@ -1104,28 +1104,28 @@ open_sudoers(const char *file, bool doedit, bool *keepopen) again: fd = sudo_secure_open_file(file, sudoers_uid, sudoers_gid, &sb, &error); - switch (error) { - case SUDO_PATH_SECURE: - /* - * Make sure we can read the file so we can present the - * user with a reasonable error message (unlike the lexer). - */ - if ((fp = fdopen(fd, "r")) == NULL) { - log_warning(SLOG_SEND_MAIL, N_("unable to open %s"), file); - close(fd); + if (fd != -1) { + /* + * Make sure we can read the file so we can present the + * user with a reasonable error message (unlike the lexer). + */ + if ((fp = fdopen(fd, "r")) == NULL) { + log_warning(SLOG_SEND_MAIL, N_("unable to open %s"), file); + close(fd); + } else { + if (sb.st_size != 0 && fgetc(fp) == EOF) { + log_warning(SLOG_SEND_MAIL, + N_("unable to read %s"), file); + fclose(fp); + fp = NULL; } else { - if (sb.st_size != 0 && fgetc(fp) == EOF) { - log_warning(SLOG_SEND_MAIL, - N_("unable to read %s"), file); - fclose(fp); - fp = NULL; - } else { - /* Rewind fp and set close on exec flag. */ - rewind(fp); - (void) fcntl(fileno(fp), F_SETFD, 1); - } + /* Rewind fp and set close on exec flag. */ + rewind(fp); + (void) fcntl(fileno(fp), F_SETFD, 1); } - break; + } + } else { + switch (error) { case SUDO_PATH_MISSING: /* * If we tried to open sudoers as non-root but got EACCES, @@ -1160,8 +1160,10 @@ again: (unsigned int) sb.st_gid, (unsigned int) sudoers_gid); break; default: - /* NOTREACHED */ + sudo_warnx("%s: internal error, unexpected error %d", + __func__, error); break; + } } if (!restore_perms()) { diff --git a/plugins/sudoers/testsudoers.c b/plugins/sudoers/testsudoers.c index 415c723dc..3af5636d9 100644 --- a/plugins/sudoers/testsudoers.c +++ b/plugins/sudoers/testsudoers.c @@ -448,31 +448,35 @@ open_sudoers(const char *file, bool doedit, bool *keepopen) /* Report errors using the basename for consistent test output. */ base = sudo_basename(file); fd = sudo_secure_open_file(file, sudoers_uid, sudoers_gid, &sb, &error); - switch (error) { - case SUDO_PATH_SECURE: - if ((fp = fdopen(fd, "r")) == NULL) + if (fd != -1) { + if ((fp = fdopen(fd, "r")) == NULL) { + sudo_warn("unable to open %s", base); close(fd); - break; - case SUDO_PATH_MISSING: - sudo_warn("unable to open %s", base); - break; - case SUDO_PATH_BAD_TYPE: - sudo_warnx("%s is not a regular file", base); - break; - case SUDO_PATH_WRONG_OWNER: - sudo_warnx("%s should be owned by uid %u", - base, (unsigned int) sudoers_uid); - break; - case SUDO_PATH_WORLD_WRITABLE: - sudo_warnx("%s is world writable", base); - break; - case SUDO_PATH_GROUP_WRITABLE: - sudo_warnx("%s should be owned by gid %u", - base, (unsigned int) sudoers_gid); - break; - default: - /* NOTREACHED */ - break; + } + } else { + switch (error) { + case SUDO_PATH_MISSING: + sudo_warn("unable to open %s", base); + break; + case SUDO_PATH_BAD_TYPE: + sudo_warnx("%s is not a regular file", base); + break; + case SUDO_PATH_WRONG_OWNER: + sudo_warnx("%s should be owned by uid %u", + base, (unsigned int) sudoers_uid); + break; + case SUDO_PATH_WORLD_WRITABLE: + sudo_warnx("%s is world writable", base); + break; + case SUDO_PATH_GROUP_WRITABLE: + sudo_warnx("%s should be owned by gid %u", + base, (unsigned int) sudoers_gid); + break; + default: + sudo_warnx("%s: internal error, unexpected error %d", + __func__, error); + break; + } } debug_return_ptr(fp); diff --git a/plugins/sudoers/timestamp.c b/plugins/sudoers/timestamp.c index 146fd3b33..825eec6b7 100644 --- a/plugins/sudoers/timestamp.c +++ b/plugins/sudoers/timestamp.c @@ -235,56 +235,56 @@ ts_mkdirs(const char *path, uid_t owner, gid_t group, mode_t mode, static int ts_secure_opendir(const char *path, bool make_it, bool quiet) { - int error, fd = -1; + int error, fd; struct stat sb; debug_decl(ts_secure_opendir, SUDOERS_DEBUG_AUTH); sudo_debug_printf(SUDO_DEBUG_INFO|SUDO_DEBUG_LINENO, "checking %s", path); fd = sudo_secure_open_dir(path, timestamp_uid, timestamp_gid, &sb, &error); - switch (error) { - case SUDO_PATH_SECURE: - break; - case SUDO_PATH_MISSING: - if (make_it) { - fd = ts_mkdirs(path, timestamp_uid, timestamp_gid, S_IRWXU, - S_IRWXU|S_IXGRP|S_IXOTH, quiet); - if (fd != -1) - break; + if (fd == -1) { + switch (error) { + case SUDO_PATH_MISSING: + if (make_it) { + fd = ts_mkdirs(path, timestamp_uid, timestamp_gid, S_IRWXU, + S_IRWXU|S_IXGRP|S_IXOTH, quiet); + if (fd != -1) + break; + } + if (!quiet) + sudo_warn("%s", path); + break; + case SUDO_PATH_BAD_TYPE: + errno = ENOTDIR; + if (!quiet) + sudo_warn("%s", path); + break; + case SUDO_PATH_WRONG_OWNER: + if (!quiet) { + sudo_warnx(U_("%s is owned by uid %u, should be %u"), + path, (unsigned int)sb.st_uid, (unsigned int)timestamp_uid); + } + errno = EACCES; + break; + case SUDO_PATH_WORLD_WRITABLE: + if (!quiet) + sudo_warnx(U_("%s is world writable"), path); + errno = EACCES; + break; + case SUDO_PATH_GROUP_WRITABLE: + if (!quiet) { + sudo_warnx(U_("%s is owned by gid %u, should be %u"), + path, (unsigned int)sb.st_gid, (unsigned int)timestamp_gid); + } + errno = EACCES; + break; + default: + if (!quiet) { + sudo_warnx("%s: internal error, unexpected error %d", + __func__, error); + errno = EINVAL; + } + break; } - if (!quiet) - sudo_warn("%s", path); - break; - case SUDO_PATH_BAD_TYPE: - errno = ENOTDIR; - if (!quiet) - sudo_warn("%s", path); - break; - case SUDO_PATH_WRONG_OWNER: - if (!quiet) { - sudo_warnx(U_("%s is owned by uid %u, should be %u"), - path, (unsigned int)sb.st_uid, (unsigned int)timestamp_uid); - } - errno = EACCES; - break; - case SUDO_PATH_WORLD_WRITABLE: - if (!quiet) - sudo_warnx(U_("%s is world writable"), path); - errno = EACCES; - break; - case SUDO_PATH_GROUP_WRITABLE: - if (!quiet) { - sudo_warnx(U_("%s is owned by gid %u, should be %u"), - path, (unsigned int)sb.st_gid, (unsigned int)timestamp_gid); - } - errno = EACCES; - break; - default: - if (!quiet) { - sudo_warnx("%s: internal error, unexpected error %d", - __func__, error); - errno = EINVAL; - } - break; } debug_return_int(fd);