On Tue, Jun 27, 2017 at 12:26:08AM -0400, Bryan Steele wrote:
> OpenBSD's file(1) implementation was written by nicm@, first introduced
> in 5.8, the inital design included a privileged parent process which
> forked an unprivileged child which would handle potentially unsafe
> file parsing.
>
> It also had 'sandboxing' using systrace(4), which required complex
> parent/child monitoring (SIGSTOP/START) to attach a policy to the
> child process.
>
> The goal was to make running file(1) safter, as it is often blindly
> run as root by users and build scripts alike.
>
> Today, file(1) uses pledge(2) in the unprivileged child, and the
> parent handles the initial opening and passing of fds using imsg, but
> otherwise it just wait(4)'s until the process exits.
>
> The diff below attempts to simplify the design, removing the
> parent/child abstractions entirely and dropping privs in the parent
> after opening the magic(5) patterns and input.
>
> This was brought up during awolk@'s #openbsd-daily readthrough.
>
> Make sense, or unnecessary churn? :-)
>
> -Bryan.
Some unintentional changes crept in, here's another diff..
Index: Makefile
===================================================================
RCS file: /cvs/src/usr.bin/file/Makefile,v
retrieving revision 1.16
diff -u -p -u -r1.16 Makefile
--- Makefile 4 Oct 2015 07:25:59 -0000 1.16
+++ Makefile 27 Jun 2017 05:17:07 -0000
@@ -5,9 +5,6 @@ SRCS= file.c magic-dump.c magic-load.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
Index: file.c
===================================================================
RCS file: /cvs/src/usr.bin/file/file.c,v
retrieving revision 1.59
diff -u -p -u -r1.59 file.c
--- file.c 18 Apr 2017 14:16:48 -0000 1.59
+++ file.c 27 Jun 2017 05:17:07 -0000
@@ -29,12 +29,10 @@
#include <errno.h>
#include <fcntl.h>
#include <getopt.h>
-#include <imsg.h>
#include <libgen.h>
#include <limits.h>
#include <pwd.h>
#include <stdlib.h>
-#include <stdlib.h>
#include <string.h>
#include <time.h>
#include <unistd.h>
@@ -43,27 +41,16 @@
#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;
const char *path;
- int fd;
+ struct stat sb;
+ int fd, error;
+
+ char link_path[PATH_MAX];
+ int link_error;
+ int link_target;
void *base;
size_t size;
@@ -75,15 +62,13 @@ 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 void read_link(struct input_file *, const char *);
-static __dead void child(int, pid_t, int, char **);
+static void privdrop(void);
-static void test_file(struct input_file *, size_t);
+static void test_file(struct input_file *, struct magic *, size_t);
static int try_stat(struct input_file *);
static int try_empty(struct input_file *);
@@ -120,14 +105,12 @@ usage(void)
int
main(int argc, char **argv)
{
- int opt, pair[2], fd, idx;
+ int opt, idx;
char *home;
struct passwd *pw;
- struct imsgbuf ibuf;
- struct imsg imsg;
- struct input_msg msg;
- struct input_ack *ack;
- pid_t pid, parent;
+ struct magic *m;
+ struct input_file *inf;
+ size_t len, width = 0;
tzset();
@@ -193,71 +176,48 @@ 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);
+ m = magic_load(magicfp, magicpath, cflag || Wflag);
+ if (cflag) {
+ magic_dump(m);
+ exit(0);
}
- close(pair[1]);
- fclose(magicfp);
- magicfp = NULL;
+ inf = xcalloc(argc, sizeof *inf);
+ for (idx = 0; idx < argc; idx++)
+ prepare_input(&inf[idx], argv[idx]);
- if (cflag)
- goto wait_for_child;
+ privdrop();
- 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 (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);
- }
-
-wait_for_child:
- close(pair[0]);
- while (wait(NULL) == -1 && errno != ECHILD) {
- if (errno != EINTR)
- err(1, "wait");
+ len = strlen(argv[idx]) + 1;
+ if (len > width)
+ width = len;
+ test_file(&inf[idx], m, width);
}
- _exit(0); /* let the child flush */
+ fclose(magicfp);
+ magicfp = NULL;
}
-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;
}
/*
@@ -265,7 +225,7 @@ prepare_message(struct input_msg *msg, i
* 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;
+ 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 +233,13 @@ prepare_message(struct input_msg *msg, i
} else
fd = -1;
if (S_ISLNK(mode))
- read_link(msg, path);
- return (fd);
-
-}
-
-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);
-
+ read_link(inf, path);
+ inf->fd = fd;
+ inf->path = path;
}
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 +249,25 @@ read_link(struct input_msg *msg, const c
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,23 +277,15 @@ read_link(struct input_msg *msg, const c
}
if (!Lflag && stat(path, &sb) == -1)
- msg->link_target = errno;
+ inf->link_target = errno;
}
-static __dead void
-child(int fd, pid_t parent, int argc, char **argv)
+static void
+privdrop(void)
{
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)
+ if (pledge("stdio getpw id", NULL) == -1)
err(1, "pledge");
if (geteuid() == 0) {
@@ -381,50 +300,9 @@ child(int fd, pid_t parent, int argc, ch
err(1, "setresuid");
}
- if (pledge("stdio recvfd", NULL) == -1)
+ if (pledge("stdio", 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);
}
static void *
@@ -461,14 +339,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 +369,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 +386,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 +416,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 +449,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);
@@ -653,10 +531,12 @@ try_unknown(struct input_file *inf)
}
static void
-test_file(struct input_file *inf, size_t width)
+test_file(struct input_file *inf, struct magic *m, size_t width)
{
char *label;
int stop;
+
+ inf->m = m;
stop = 0;
if (!stop)
