isa/sudo
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

sudo_secure_open_{file,dir}: always check thatreturn value is not -1.

Browse Source 
Avoids false positives from static analyzers that can't figure out
that the fd is always valid when error is SUDO_PATH_SECURE.
This commit is contained in:
Todd C. Miller
2022-10-04 09:33:44 -06:00
parent 59765dd360
commit 3940020c94
4 changed files with 120 additions and 113 deletions
Download Patch File Download Diff File Expand all files Collapse all files

49
lib/util/sudo_conf.c
View File

@@ -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;
}
}

44
plugins/sudoers/sudoers.c
View File

@@ -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()) {

52
plugins/sudoers/testsudoers.c
View File

@@ -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);

88
plugins/sudoers/timestamp.c
View File

@@ -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);