Skip to content

Commit

Permalink
checkpath: use fchown and fchmod to handle ownership and mode changes
Browse files Browse the repository at this point in the history 
This is related to #195.

This is an attempt to shorten the window for the first two issues
discussed by using a file descriptor which does not follow symbolic
links and using the fchmod and fchown calls instead of chown and chmod.
with.
  • Loading branch information
@williamh
williamh committed Jan 23, 2018
1 parent 87c98eb commit 1771bc2
Showing 1 changed file with 79 additions and 45 deletions.
124 changes: 79 additions & 45 deletions src/rc/checkpath.c
View file
Expand Up @@ -73,33 +73,42 @@ static int do_check(char *path, uid_t uid, gid_t gid, mode_t mode,
inode_t type, bool trunc, bool chowner, bool selinux_on)
{
struct stat st;
int fd, flags;
int fd;
int flags;
int r;
int readfd;
int readflags;
int u;

memset(&st, 0, sizeof(st));
if (lstat(path, &st) || trunc) {
if (type == inode_file) {
einfo("%s: creating file", path);
if (!mode) /* 664 */
mode = S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH;
flags = O_CREAT|O_NDELAY|O_WRONLY|O_NOCTTY;
flags = O_CREAT|O_NDELAY|O_WRONLY|O_NOCTTY;
readflags = O_NDELAY|O_NOCTTY|O_RDONLY;
#ifdef O_CLOEXEC
flags |= O_CLOEXEC;
flags |= O_CLOEXEC;
readflags |= O_CLOEXEC;
#endif
#ifdef O_NOFOLLOW
flags |= O_NOFOLLOW;
flags |= O_NOFOLLOW;
readflags |= O_NOFOLLOW;
#endif
if (trunc)
flags |= O_TRUNC;
if (trunc)
flags |= O_TRUNC;
readfd = open(path, readflags);
if (readfd == -1 || (type == inode_file && trunc)) {
if (type == inode_file) {
einfo("%s: creating file", path);
if (!mode) /* 664 */
mode = S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH;
u = umask(0);
fd = open(path, flags, mode);
umask(u);
if (fd == -1) {
eerror("%s: open: %s", applet, strerror(errno));
return -1;
}
close (fd);
if (readfd != -1 && trunc)
close(readfd);
readfd = fd;
} else if (type == inode_dir) {
einfo("%s: creating directory", path);
if (!mode) /* 775 */
Expand All @@ -113,7 +122,12 @@ static int do_check(char *path, uid_t uid, gid_t gid, mode_t mode,
strerror (errno));
return -1;
}
mode = 0;
readfd = open(path, readflags);
if (readfd == -1) {
eerror("%s: unable to open directory: %s", applet,
strerror(errno));
return -1;
}
} else if (type == inode_fifo) {
einfo("%s: creating fifo", path);
if (!mode) /* 600 */
Expand All @@ -126,56 +140,76 @@ static int do_check(char *path, uid_t uid, gid_t gid, mode_t mode,
strerror (errno));
return -1;
}
readfd = open(path, readflags);
if (readfd == -1) {
eerror("%s: unable to open fifo: %s", applet,
strerror(errno));
return -1;
}
}
} else {
}
if (fstat(readfd, &st) != -1) {
if (type != inode_dir && S_ISDIR(st.st_mode)) {
eerror("%s: is a directory", path);
close(readfd);
return 1;
}
if (type != inode_file && S_ISREG(st.st_mode)) {
eerror("%s: is a file", path);
close(readfd);
return 1;
}
if (type != inode_fifo && S_ISFIFO(st.st_mode)) {
eerror("%s: is a fifo", path);
close(readfd);
return -1;
}
}

if (mode && (st.st_mode & 0777) != mode) {
if ((type != inode_dir) && (st.st_nlink > 1)) {
eerror("%s: chmod: %s %s", applet, "Too many hard links to", path);
return -1;
}
if (S_ISLNK(st.st_mode)) {
eerror("%s: chmod: %s %s", applet, path, " is a symbolic link");
return -1;
}
einfo("%s: correcting mode", path);
if (chmod(path, mode)) {
eerror("%s: chmod: %s", applet, strerror(errno));
return -1;
if (mode && (st.st_mode & 0777) != mode) {
if ((type != inode_dir) && (st.st_nlink > 1)) {
eerror("%s: chmod: %s %s", applet, "Too many hard links to", path);
close(readfd);
return -1;
}
if (S_ISLNK(st.st_mode)) {
eerror("%s: chmod: %s %s", applet, path, " is a symbolic link");
close(readfd);
return -1;
}
einfo("%s: correcting mode", path);
if (fchmod(readfd, mode)) {
eerror("%s: chmod: %s", applet, strerror(errno));
close(readfd);
return -1;
}
}
}

if (chowner && (st.st_uid != uid || st.st_gid != gid)) {
if ((type != inode_dir) && (st.st_nlink > 1)) {
eerror("%s: chown: %s %s", applet, "Too many hard links to", path);
return -1;
}
if (S_ISLNK(st.st_mode)) {
eerror("%s: chown: %s %s", applet, path, " is a symbolic link");
return -1;
}
einfo("%s: correcting owner", path);
if (lchown(path, uid, gid)) {
eerror("%s: lchown: %s", applet, strerror(errno));
return -1;
if (chowner && (st.st_uid != uid || st.st_gid != gid)) {
if ((type != inode_dir) && (st.st_nlink > 1)) {
eerror("%s: chown: %s %s", applet, "Too many hard links to", path);
close(readfd);
return -1;
}
if (S_ISLNK(st.st_mode)) {
eerror("%s: chown: %s %s", applet, path, " is a symbolic link");
close(readfd);
return -1;
}
einfo("%s: correcting owner", path);
if (fchown(readfd, uid, gid)) {
eerror("%s: chown: %s", applet, strerror(errno));
close(readfd);
return -1;
}
}
if (selinux_on)
selinux_util_label(path);
} else {
eerror(fstat: %s: %s", path, strerror(errno));

This comment has been minimized.

Sign in to view

Copy link
@ageis

ageis Jan 24, 2018

@williamh hey there, this line is a bug/improper syntax, you're missing a quotation.

caught it upon building, since the commit is so recent I figured I'd make a comment to you here instead of opening an issue or PR.
close(readfd);
return -1;
}

if (selinux_on)
selinux_util_label(path);
close(readfd);

return 0;
}
Expand Down

0 comments on commit 1771bc2

Please sign in to comment.