From: brynet Date: Wed, 28 Jun 2017 13:37:56 +0000 (+0000) Subject: Simplify file(1) by removing the no longer necessary parent/child separation X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=09f2f01e9c82c802fea3a2c6b0c796d4a34d310e;p=openbsd Simplify file(1) by removing the no longer necessary parent/child separation and just drop privileges in the main process. Also allows for a tighter "stdio" pledge. passing regress tests still pass ok nicm@ with helpful feedback --- diff --git a/usr.bin/file/Makefile b/usr.bin/file/Makefile index b126138bf60..7bb8f68bf8f 100644 --- a/usr.bin/file/Makefile +++ b/usr.bin/file/Makefile @@ -1,13 +1,10 @@ -# $OpenBSD: Makefile,v 1.16 2015/10/04 07:25:59 nicm Exp $ +# $OpenBSD: Makefile,v 1.17 2017/06/28 13:37:56 brynet Exp $ PROG= file SRCS= file.c magic-dump.c magic-load.c magic-test.c magic-common.c \ text.c xmalloc.c MAN= file.1 magic.5 -LDADD= -lutil -DPADD= ${LIBUTIL} - CDIAGFLAGS+= -Wno-long-long -Wall -W -Wnested-externs -Wformat=2 CDIAGFLAGS+= -Wmissing-prototypes -Wstrict-prototypes -Wmissing-declarations CDIAGFLAGS+= -Wwrite-strings -Wshadow -Wpointer-arith -Wsign-compare diff --git a/usr.bin/file/file.c b/usr.bin/file/file.c index 6304a38c18f..07082aeadda 100644 --- a/usr.bin/file/file.c +++ b/usr.bin/file/file.c @@ -1,4 +1,4 @@ -/* $OpenBSD: file.c,v 1.59 2017/04/18 14:16:48 nicm Exp $ */ +/* $OpenBSD: file.c,v 1.60 2017/06/28 13:37:56 brynet Exp $ */ /* * Copyright (c) 2015 Nicholas Marriott @@ -29,12 +29,10 @@ #include #include #include -#include #include #include #include #include -#include #include #include #include @@ -43,45 +41,31 @@ #include "magic.h" #include "xmalloc.h" -struct input_msg { - int idx; - - struct stat sb; - int error; - - char link_path[PATH_MAX]; - int link_error; - int link_target; -}; - -struct input_ack { - int idx; -}; - struct input_file { - struct magic *m; - struct input_msg *msg; + struct magic *m; + + const char *path; + struct stat sb; + int fd; + int error; - const char *path; - int fd; + char link_path[PATH_MAX]; + int link_error; + int link_target; - void *base; - size_t size; - int mapped; - char *result; + void *base; + size_t size; + int mapped; + char *result; }; extern char *__progname; __dead void usage(void); -static int prepare_message(struct input_msg *, int, const char *); -static void send_message(struct imsgbuf *, void *, size_t, int); -static int read_message(struct imsgbuf *, struct imsg *, pid_t); +static void prepare_input(struct input_file *, const char *); -static void read_link(struct input_msg *, const char *); - -static __dead void child(int, pid_t, int, char **); +static void read_link(struct input_file *, const char *); static void test_file(struct input_file *, size_t); @@ -99,9 +83,6 @@ static int Lflag; static int sflag; static int Wflag; -static char *magicpath; -static FILE *magicfp; - static struct option longopts[] = { { "brief", no_argument, NULL, 'b' }, { "dereference", no_argument, NULL, 'L' }, @@ -120,14 +101,13 @@ usage(void) int main(int argc, char **argv) { - int opt, pair[2], fd, idx; - char *home; + int opt, idx; + char *home, *magicpath; struct passwd *pw; - struct imsgbuf ibuf; - struct imsg imsg; - struct input_msg msg; - struct input_ack *ack; - pid_t pid, parent; + FILE *magicfp; + struct magic *m; + struct input_file *inf = NULL; + size_t len, width = 0; tzset(); @@ -193,79 +173,72 @@ main(int argc, char **argv) if (magicfp == NULL) err(1, "%s", magicpath); - parent = getpid(); - if (socketpair(AF_UNIX, SOCK_STREAM, PF_UNSPEC, pair) != 0) - err(1, "socketpair"); - switch (pid = fork()) { - case -1: - err(1, "fork"); - case 0: - close(pair[0]); - child(pair[1], parent, argc, argv); + if (!cflag) { + inf = xcalloc(argc, sizeof *inf); + for (idx = 0; idx < argc; idx++) { + len = strlen(argv[idx]) + 1; + if (len > width) + width = len; + prepare_input(&inf[idx], argv[idx]); + } } - close(pair[1]); - fclose(magicfp); - magicfp = NULL; + if (pledge("stdio getpw id", NULL) == -1) + err(1, "pledge"); - if (cflag) - goto wait_for_child; + if (geteuid() == 0) { + pw = getpwnam(FILE_USER); + if (pw == NULL) + errx(1, "unknown user %s", FILE_USER); + if (setgroups(1, &pw->pw_gid) != 0) + err(1, "setgroups"); + if (setresgid(pw->pw_gid, pw->pw_gid, pw->pw_gid) != 0) + err(1, "setresgid"); + if (setresuid(pw->pw_uid, pw->pw_uid, pw->pw_uid) != 0) + err(1, "setresuid"); + } - imsg_init(&ibuf, pair[0]); - for (idx = 0; idx < argc; idx++) { - fd = prepare_message(&msg, idx, argv[idx]); - send_message(&ibuf, &msg, sizeof msg, fd); + if (pledge("stdio", NULL) == -1) + err(1, "pledge"); - if (read_message(&ibuf, &imsg, pid) == 0) - break; - if (imsg.hdr.len != IMSG_HEADER_SIZE + sizeof *ack) - errx(1, "message too small"); - ack = imsg.data; - if (ack->idx != idx) - errx(1, "index not expected"); - imsg_free(&imsg); + m = magic_load(magicfp, magicpath, cflag || Wflag); + if (cflag) { + magic_dump(m); + exit(0); } + fclose(magicfp); -wait_for_child: - close(pair[0]); - while (wait(NULL) == -1 && errno != ECHILD) { - if (errno != EINTR) - err(1, "wait"); + for (idx = 0; idx < argc; idx++) { + inf[idx].m = m; + test_file(&inf[idx], width); } - _exit(0); /* let the child flush */ + exit(0); } -static int -prepare_message(struct input_msg *msg, int idx, const char *path) +static void +prepare_input(struct input_file *inf, const char *path) { int fd, mode, error; - memset(msg, 0, sizeof *msg); - msg->idx = idx; - if (strcmp(path, "-") == 0) { - if (fstat(STDIN_FILENO, &msg->sb) == -1) { - msg->error = errno; - return (-1); + if (fstat(STDIN_FILENO, &inf->sb) == -1) { + inf->error = errno; + inf->fd = -1; } - return (STDIN_FILENO); + inf->fd = STDIN_FILENO; } if (Lflag) - error = stat(path, &msg->sb); + error = stat(path, &inf->sb); else - error = lstat(path, &msg->sb); + error = lstat(path, &inf->sb); if (error == -1) { - msg->error = errno; - return (-1); + inf->error = errno; + inf->fd = -1; } - /* - * pledge(2) doesn't let us pass directory file descriptors around - - * but in fact we don't need them, so just don't open directories or - * symlinks (which could be to directories). - */ - mode = msg->sb.st_mode; + /* We don't need them, so don't open directories or symlinks. */ + mode = inf->sb.st_mode; if (!S_ISDIR(mode) && !S_ISLNK(mode)) { fd = open(path, O_RDONLY|O_NONBLOCK); if (fd == -1 && (errno == ENFILE || errno == EMFILE)) @@ -273,46 +246,13 @@ prepare_message(struct input_msg *msg, int idx, const char *path) } else fd = -1; if (S_ISLNK(mode)) - read_link(msg, path); - return (fd); - + read_link(inf, path); + inf->fd = fd; + inf->path = path; } static void -send_message(struct imsgbuf *ibuf, void *msg, size_t msglen, int fd) -{ - if (imsg_compose(ibuf, -1, -1, 0, fd, msg, msglen) != 1) - err(1, "imsg_compose"); - if (imsg_flush(ibuf) != 0) - err(1, "imsg_flush"); -} - -static int -read_message(struct imsgbuf *ibuf, struct imsg *imsg, pid_t from) -{ - int n; - - while ((n = imsg_read(ibuf)) == -1 && errno == EAGAIN) - /* nothing */ ; - if (n == -1) - err(1, "imsg_read"); - if (n == 0) - return (0); - - if ((n = imsg_get(ibuf, imsg)) == -1) - err(1, "imsg_get"); - if (n == 0) - return (0); - - if ((pid_t)imsg->hdr.pid != from) - errx(1, "PIDs don't match"); - - return (n); - -} - -static void -read_link(struct input_msg *msg, const char *path) +read_link(struct input_file *inf, const char *path) { struct stat sb; char lpath[PATH_MAX]; @@ -322,25 +262,25 @@ read_link(struct input_msg *msg, const char *path) size = readlink(path, lpath, sizeof lpath - 1); if (size == -1) { - msg->link_error = errno; + inf->link_error = errno; return; } lpath[size] = '\0'; if (*lpath == '/') - strlcpy(msg->link_path, lpath, sizeof msg->link_path); + strlcpy(inf->link_path, lpath, sizeof inf->link_path); else { copy = xstrdup(path); root = dirname(copy); if (*root == '\0' || strcmp(root, ".") == 0 || strcmp (root, "/") == 0) - strlcpy(msg->link_path, lpath, sizeof msg->link_path); + strlcpy(inf->link_path, lpath, sizeof inf->link_path); else { - used = snprintf(msg->link_path, sizeof msg->link_path, + used = snprintf(inf->link_path, sizeof inf->link_path, "%s/%s", root, lpath); - if (used < 0 || (size_t)used >= sizeof msg->link_path) { - msg->link_error = ENAMETOOLONG; + if (used < 0 || (size_t)used >= sizeof inf->link_path) { + inf->link_error = ENAMETOOLONG; free(copy); return; } @@ -350,81 +290,7 @@ read_link(struct input_msg *msg, const char *path) } if (!Lflag && stat(path, &sb) == -1) - msg->link_target = errno; -} - -static __dead void -child(int fd, pid_t parent, int argc, char **argv) -{ - struct passwd *pw; - struct magic *m; - struct imsgbuf ibuf; - struct imsg imsg; - struct input_msg *msg; - struct input_ack ack; - struct input_file inf; - int i, idx; - size_t len, width = 0; - - if (pledge("stdio getpw recvfd id", NULL) == -1) - err(1, "pledge"); - - if (geteuid() == 0) { - pw = getpwnam(FILE_USER); - if (pw == NULL) - errx(1, "unknown user %s", FILE_USER); - if (setgroups(1, &pw->pw_gid) != 0) - err(1, "setgroups"); - if (setresgid(pw->pw_gid, pw->pw_gid, pw->pw_gid) != 0) - err(1, "setresgid"); - if (setresuid(pw->pw_uid, pw->pw_uid, pw->pw_uid) != 0) - err(1, "setresuid"); - } - - if (pledge("stdio recvfd", NULL) == -1) - err(1, "pledge"); - - m = magic_load(magicfp, magicpath, cflag || Wflag); - if (cflag) { - magic_dump(m); - exit(0); - } - - for (i = 0; i < argc; i++) { - len = strlen(argv[i]) + 1; - if (len > width) - width = len; - } - - imsg_init(&ibuf, fd); - for (;;) { - if (read_message(&ibuf, &imsg, parent) == 0) - break; - if (imsg.hdr.len != IMSG_HEADER_SIZE + sizeof *msg) - errx(1, "message too small"); - msg = imsg.data; - - idx = msg->idx; - if (idx < 0 || idx >= argc) - errx(1, "index out of range"); - - memset(&inf, 0, sizeof inf); - inf.m = m; - inf.msg = msg; - - inf.path = argv[idx]; - inf.fd = imsg.fd; - - test_file(&inf, width); - - if (imsg.fd != -1) - close(imsg.fd); - imsg_free(&imsg); - - ack.idx = idx; - send_message(&ibuf, &ack, sizeof ack, -1); - } - exit(0); + inf->link_target = errno; } static void * @@ -461,14 +327,14 @@ load_file(struct input_file *inf) { size_t used; - if (inf->msg->sb.st_size == 0 && S_ISREG(inf->msg->sb.st_mode)) + if (inf->sb.st_size == 0 && S_ISREG(inf->sb.st_mode)) return (0); /* empty file */ - if (inf->msg->sb.st_size == 0 || inf->msg->sb.st_size > FILE_READ_SIZE) + if (inf->sb.st_size == 0 || inf->sb.st_size > FILE_READ_SIZE) inf->size = FILE_READ_SIZE; else - inf->size = inf->msg->sb.st_size; + inf->size = inf->sb.st_size; - if (!S_ISREG(inf->msg->sb.st_mode)) + if (!S_ISREG(inf->sb.st_mode)) goto try_read; inf->base = mmap(NULL, inf->size, PROT_READ, MAP_PRIVATE, inf->fd, 0); @@ -491,13 +357,13 @@ try_read: static int try_stat(struct input_file *inf) { - if (inf->msg->error != 0) { + if (inf->error != 0) { xasprintf(&inf->result, "cannot stat '%s' (%s)", inf->path, - strerror(inf->msg->error)); + strerror(inf->error)); return (1); } if (sflag || strcmp(inf->path, "-") == 0) { - switch (inf->msg->sb.st_mode & S_IFMT) { + switch (inf->sb.st_mode & S_IFMT) { case S_IFIFO: if (strcmp(inf->path, "-") != 0) break; @@ -508,29 +374,29 @@ try_stat(struct input_file *inf) } } - if (iflag && (inf->msg->sb.st_mode & S_IFMT) != S_IFREG) { + if (iflag && (inf->sb.st_mode & S_IFMT) != S_IFREG) { xasprintf(&inf->result, "application/x-not-regular-file"); return (1); } - switch (inf->msg->sb.st_mode & S_IFMT) { + switch (inf->sb.st_mode & S_IFMT) { case S_IFDIR: xasprintf(&inf->result, "directory"); return (1); case S_IFLNK: - if (inf->msg->link_error != 0) { + if (inf->link_error != 0) { xasprintf(&inf->result, "unreadable symlink '%s' (%s)", - inf->path, strerror(inf->msg->link_error)); + inf->path, strerror(inf->link_error)); return (1); } - if (inf->msg->link_target == ELOOP) + if (inf->link_target == ELOOP) xasprintf(&inf->result, "symbolic link in a loop"); - else if (inf->msg->link_target != 0) { + else if (inf->link_target != 0) { xasprintf(&inf->result, "broken symbolic link to '%s'", - inf->msg->link_path); + inf->link_path); } else { xasprintf(&inf->result, "symbolic link to '%s'", - inf->msg->link_path); + inf->link_path); } return (1); case S_IFSOCK: @@ -538,13 +404,13 @@ try_stat(struct input_file *inf) return (1); case S_IFBLK: xasprintf(&inf->result, "block special (%ld/%ld)", - (long)major(inf->msg->sb.st_rdev), - (long)minor(inf->msg->sb.st_rdev)); + (long)major(inf->sb.st_rdev), + (long)minor(inf->sb.st_rdev)); return (1); case S_IFCHR: xasprintf(&inf->result, "character special (%ld/%ld)", - (long)major(inf->msg->sb.st_rdev), - (long)minor(inf->msg->sb.st_rdev)); + (long)major(inf->sb.st_rdev), + (long)minor(inf->sb.st_rdev)); return (1); case S_IFIFO: xasprintf(&inf->result, "fifo (named pipe)"); @@ -571,16 +437,16 @@ try_access(struct input_file *inf) { char tmp[256] = ""; - if (inf->msg->sb.st_size == 0 && S_ISREG(inf->msg->sb.st_mode)) + if (inf->sb.st_size == 0 && S_ISREG(inf->sb.st_mode)) return (0); /* empty file */ if (inf->fd != -1) return (0); - if (inf->msg->sb.st_mode & (S_IWUSR|S_IWGRP|S_IWOTH)) + if (inf->sb.st_mode & (S_IWUSR|S_IWGRP|S_IWOTH)) strlcat(tmp, "writable, ", sizeof tmp); - if (inf->msg->sb.st_mode & (S_IXUSR|S_IXGRP|S_IXOTH)) + if (inf->sb.st_mode & (S_IXUSR|S_IXGRP|S_IXOTH)) strlcat(tmp, "executable, ", sizeof tmp); - if (S_ISREG(inf->msg->sb.st_mode)) + if (S_ISREG(inf->sb.st_mode)) strlcat(tmp, "regular file, ", sizeof tmp); strlcat(tmp, "no read permission", sizeof tmp);