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.
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 03:19:51 -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 03:19:51 -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 *);
static int try_stat(struct input_file *);
static int try_empty(struct input_file *);
@@ -120,14 +105,11 @@ usage(void)
int
main(int argc, char **argv)
{
- int opt, pair[2], fd, idx;
+ int opt, idx, nargs;
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;
tzset();
@@ -193,71 +175,44 @@ 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;
- if (cflag)
- goto wait_for_child;
+ inf = xcalloc(argc, sizeof *inf);
+ for (idx = 0; idx < argc; idx++)
+ prepare_input(&inf[idx], argv[idx]);
- 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);
+ privdrop();
- 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");
- }
- _exit(0); /* let the child flush */
+ for (idx = 0; idx < argc; idx++)
+ test_file(&inf[idx], m);
+ 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 +220,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 +228,13 @@ prepare_message(struct input_msg *msg, i
} 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 +244,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 +272,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 +295,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 +334,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 +364,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 +381,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 +411,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 +444,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,11 +526,13 @@ 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)
{
char *label;
int stop;
+ inf->m = m;
+
stop = 0;
if (!stop)
stop = try_stat(inf);
@@ -681,7 +556,7 @@ test_file(struct input_file *inf, size_t
xasprintf(&label, "/dev/stdin:");
else
xasprintf(&label, "%s:", inf->path);
- printf("%-*s %s\n", (int)width, label, inf->result);
+ printf("%s %s\n", label, inf->result);
free(label);
}
free(inf->result);
