On Thu, Jul 26, 2018 at 10:22:28AM -0600, Theo de Raadt wrote:
> This mail includes a large diff of userland which demonstrates how
> unveil() will be used in base.
>
[...]
>
> And here is the userland diff, applying to about 37 programs. There will
> be weaknesses and errors in here. This is not perfect yet.
Here, a quick review of some possible problems, unrelated diff hunks or
others comments.
I didn't check for the accuracy of the unveiled paths.
> Index: libexec/fingerd/fingerd.c
> ===================================================================
> RCS file: /cvs/src/libexec/fingerd/fingerd.c,v
> retrieving revision 1.39
> diff -u -p -u -r1.39 fingerd.c
> --- libexec/fingerd/fingerd.c 13 Nov 2015 01:26:33 -0000 1.39
> +++ libexec/fingerd/fingerd.c 12 Jul 2018 16:18:13 -0000
> @@ -68,7 +68,7 @@ main(int argc, char *argv[])
> char **ap, *av[ENTRIES + 1], line[8192], *lp, *hname;
> char hostbuf[HOST_NAME_MAX+1];
>
> - if (pledge("stdio inet dns proc exec", NULL) == -1)
> + if (pledge("stdio unveil inet dns proc exec", NULL) == -1)
> err(1, "pledge");
>
> prog = _PATH_FINGER;
> @@ -110,6 +110,9 @@ main(int argc, char *argv[])
> default:
> usage();
> }
> +
> + if (unveil(_PATH_FINGER, "x") == -1)
> + err(1, "unveil");
>
> if (logging) {
> struct sockaddr_storage ss;
unveil(2) call (with static name _PATH_FINGER) should be before
pledge(2). alternatively another call of pledge(2) without "unveil"
should be done after.
> Index: libexec/spamd-setup/spamd-setup.c
> ===================================================================
> RCS file: /cvs/src/libexec/spamd-setup/spamd-setup.c,v
> retrieving revision 1.50
> diff -u -p -u -r1.50 spamd-setup.c
> --- libexec/spamd-setup/spamd-setup.c 7 Jul 2017 00:10:15 -0000 1.50
> +++ libexec/spamd-setup/spamd-setup.c 12 Jul 2018 16:18:13 -0000
> @@ -851,13 +851,24 @@ main(int argc, char *argv[])
> spamd_uid = pw->pw_uid;
> spamd_gid = pw->pw_gid;
>
> - if (pledge("stdio rpath inet proc exec id", NULL) == -1)
> + if (pledge("stdio unveil rpath inet proc exec id", NULL) == -1)
> err(1, "pledge");
>
> if (daemonize)
> daemon(0, 0);
> else if (chdir("/") != 0)
> err(1, "chdir(\"/\")");
> +
> + if (unveil(PATH_FTP, "x") == -1)
> + err(1, "unveil");
> + if (unveil(PATH_PFCTL, "x") == -1)
> + err(1, "unveil");
> + if (unveil(PATH_SPAMD_CONF, "r") == -1)
> + err(1, "unveil");
> + if (unveil(_PATH_SERVICES, "r") == -1)
> + err(1, "unveil");
> + if (pledge("stdio rpath inet proc exec id", NULL) == -1)
> + err(1, "pledge");
>
> if ((ent = getservbyname("spamd-cfg", "tcp")) == NULL)
> errx(1, "cannot find service \"spamd-cfg\" in /etc/services");
I would perfer to see all unveil(2) calls before pledge(2), instead of
calling pledge(2) twice (here it is possible: all unveiled paths are
known in advance).
> Index: sbin/scan_ffs/scan_ffs.c
> ===================================================================
> RCS file: /cvs/src/sbin/scan_ffs/scan_ffs.c,v
> retrieving revision 1.22
> diff -u -p -u -r1.22 scan_ffs.c
> --- sbin/scan_ffs/scan_ffs.c 26 Apr 2018 15:55:14 -0000 1.22
> +++ sbin/scan_ffs/scan_ffs.c 12 Jul 2018 16:18:13 -0000
> @@ -139,7 +139,7 @@ main(int argc, char *argv[])
> daddr_t beg = 0, end = -1;
> const char *errstr;
>
> - if (pledge("stdio rpath disklabel", NULL) == -1)
> + if (pledge("stdio unveil rpath disklabel", NULL) == -1)
> err(1, "pledge");
>
> while ((ch = getopt(argc, argv, "lsvb:e:")) != -1)
hum. Makefile doesn't show it would search source code somewhere else.
The change (adding "unveil" promise without unveil(2) calls) is
suspirious.
> Index: usr.bin/doas/doas.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/doas/doas.c,v
> retrieving revision 1.72
> diff -u -p -u -r1.72 doas.c
> --- usr.bin/doas/doas.c 27 May 2017 09:51:07 -0000 1.72
> +++ usr.bin/doas/doas.c 12 Jul 2018 16:18:13 -0000
> @@ -239,6 +239,30 @@ good:
> }
> }
>
> +void
> +pledgecommands(const char *ipath, const char *cmd)
cosmetic: unveilcommands() would be a better name :)
> +{
> + char *path;
> + char buf[PATH_MAX];
> + char *p, *cp;
> +
> + path = strdup(ipath);
> + if (!path)
> + err(1, "copying path");
> +
> + for (p = path; p && *p;) {
> + cp = strsep(&p, ":");
> + if (cp) {
> + int r = snprintf(buf, sizeof buf, "%s/%s", cp, cmd);
> + if (r == -1 || r >= sizeof buf)
> + errx(1, "snprintf");
> + if (unveil(buf, "x") == -1)
> + err(1, "unveil");
> + }
> + }
> + free(path);
> +}
> +
> int
> main(int argc, char **argv)
> {
> @@ -364,6 +388,7 @@ main(int argc, char **argv)
> authuser(myname, login_style, rule->options & PERSIST);
> }
>
> + pledgecommands(safepath, cmd);
> if (pledge("stdio rpath getpw exec id", NULL) == -1)
> err(1, "pledge");
>
> Index: usr.bin/ftp/main.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/ftp/main.c,v
> retrieving revision 1.120
> diff -u -p -u -r1.120 main.c
> --- usr.bin/ftp/main.c 10 Feb 2018 06:25:16 -0000 1.120
> +++ usr.bin/ftp/main.c 12 Jul 2018 16:18:13 -0000
> @@ -273,6 +273,7 @@ main(volatile int argc, char *argv[])
> }
>
> ttyout = stdout;
> +// espie: fw_update, ^T not working. due to no TERM= while in rc script?
> if (isatty(fileno(ttyout)) && !dumb_terminal && foregroundproc())
> progress = 1; /* progress bar on if tty is usable */
>
unrelated to unveil(2)
> Index: usr.bin/top/top.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/top/top.c,v
> retrieving revision 1.89
> diff -u -p -u -r1.89 top.c
> --- usr.bin/top/top.c 15 Mar 2017 04:24:14 -0000 1.89
> +++ usr.bin/top/top.c 12 Jul 2018 16:18:13 -0000
> @@ -412,6 +412,8 @@ main(int argc, char *argv[])
> sigprocmask(SIG_BLOCK, &mask, &oldmask);
> if (interactive)
> init_screen();
> + if (pledge("stdio getpw tty proc ps vminfo", NULL) == -1)
> + err(1, "pledge");
> (void) signal(SIGINT, leave);
> siginterrupt(SIGINT, 1);
> (void) signal(SIGQUIT, leave);
unrelated to unveil(2)
> Index: usr.sbin/bgpctl/bgpctl.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
> retrieving revision 1.209
> diff -u -p -u -r1.209 bgpctl.c
> --- usr.sbin/bgpctl/bgpctl.c 22 Jul 2018 17:07:53 -0000 1.209
> +++ usr.sbin/bgpctl/bgpctl.c 25 Jul 2018 17:04:18 -0000
> @@ -192,7 +192,9 @@ main(int argc, char *argv[])
> break;
> }
>
> - if (pledge("stdio rpath wpath unix", NULL) == -1)
> + if (unveil(sockname, "r") == -1)
> + err(1, "unveil");
> + if (pledge("stdio unix", NULL) == -1)
> err(1, "pledge");
>
> if ((fd = socket(AF_UNIX, SOCK_STREAM, 0)) == -1)
suspirious change in pledge(2)
- before: "stdio rpath wpath unix"
- after: "stdio unix"
> Index: usr.sbin/portmap/portmap.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/portmap/portmap.c,v
> retrieving revision 1.48
> diff -u -p -u -r1.48 portmap.c
> --- usr.sbin/portmap/portmap.c 14 Oct 2015 13:32:44 -0000 1.48
> +++ usr.sbin/portmap/portmap.c 12 Jul 2018 16:18:13 -0000
> @@ -609,7 +609,7 @@ callit(struct svc_req *rqstp, SVCXPRT *x
> return;
> }
>
> - if (pledge("stdio rpath inet", NULL) == -1)
> + if (pledge("stdio inet", NULL) == -1)
> err(1, "pledge");
>
> port = pml->pml_map.pm_port;
suspirious change in pledge(2)
- before: "stdio rpath inet"
- after: "stdio inet"
(and diff unrelated to unveil(2))
> Index: usr.sbin/vmctl/main.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/vmctl/main.c,v
> retrieving revision 1.39
> diff -u -p -u -r1.39 main.c
> --- usr.sbin/vmctl/main.c 12 Jul 2018 14:53:37 -0000 1.39
> +++ usr.sbin/vmctl/main.c 25 Jul 2018 17:04:20 -0000
> @@ -158,9 +166,14 @@ parse(int argc, char *argv[])
> res.action = ctl->action;
> res.ctl = ctl;
>
> + if (unveil(SOCKET_NAME, "r") == -1)
> + err(1, "unveil");
> +
> if (!ctl->has_pledge) {
> /* pledge(2) default if command doesn't have its own pledge */
> - if (pledge("stdio rpath exec unix getpw", NULL) == -1)
> + if (unveil(VMCTL_CU, "x") == -1)
> + err(1, "unveil");
> + if (pledge("stdio rpath exec unix getpw paths", NULL) == -1)
> err(1, "pledge");
"paths" isn't a know promise name (it should be "unveil")
> }
> if (ctl->main(&res, argc, argv) != 0)
--
Sebastien Marie
