On Sat, Apr 02, 2016 at 04:38:10PM +0200, [email protected] wrote: > Hi, > > this adds pledge(2) to ftpd(8).
thanks for your interest. I haven't tested your diff, but just some comments below. > Unfortunately, the main process and its children "[priv pre-auth]" need > a lot of rights. The unprivileged process is better to pledge, as it > does not need that much rights. But, I try to find other strategic points > where pledge(2) could take place. > > Maybe it's possible to avoid some rights, with a solution like this one: > http://marc.info/?l=openbsd-tech&m=145954965514619&w=2 > > If anyone could test this diff in different constellations, I'm happy > to get feedback as I don't cover them all. > > --f. > > Index: ftpd.c > =================================================================== > RCS file: /cvs/src/libexec/ftpd/ftpd.c,v > retrieving revision 1.213 > diff -u -r1.213 ftpd.c > --- ftpd.c 16 Mar 2016 15:41:10 -0000 1.213 > +++ ftpd.c 2 Apr 2016 14:14:02 -0000 > @@ -80,6 +80,7 @@ > #include <bsd_auth.h> > #include <ctype.h> > #include <dirent.h> > +#include <err.h> > #include <errno.h> > #include <fcntl.h> > #include <glob.h> > @@ -375,6 +376,9 @@ > syslog(LOG_ERR, "options 'U' and 'W' are mutually exclusive"); > exit(1); > } > + > + if (pledge("stdio wpath cpath getpw proc rpath dns inet id ioctl sendfd > recvfd", NULL) == -1) > + err(1, "pledge"); I have some concerns with the placement of the pledge(2) call: you put it near the beginning of main(): in fact, the first instructions are arguments parsing, and just next the pledge(2) call you added. pledge(2) suits better after initialisation, not before. it could explains a part of the huge list of promises. another concern is chroot(2). If -current code permit the use of chroot(2) with pledge(2), there are discussion for removing it from allowed syscalls under pledge(2). Currently only few programs in base use it while pledged (dhcpd, ntpd, rebound and spamd). is it possible to use pledge(2) as part of privdrop ? I only readed the ftpd code, and it seems that handle_cmds() in monitor.c could be a good place for that, here we have (if I correctly understand) : - pre-auth monitor process - post-auth monitor process - user-privileged slave process (after successful authentication) and keeping a process unpledged isn't a problem per se. > (void) freopen(_PATH_DEVNULL, "w", stderr); > > Index: monitor.c > =================================================================== > RCS file: /cvs/src/libexec/ftpd/monitor.c,v > retrieving revision 1.23 > diff -u -r1.23 monitor.c > --- monitor.c 16 Nov 2015 17:31:14 -0000 1.23 > +++ monitor.c 2 Apr 2016 14:14:02 -0000 > @@ -21,6 +21,7 @@ > #include <sys/wait.h> > #include <netinet/in.h> > > +#include <err.h> > #include <errno.h> > #include <fcntl.h> > #include <paths.h> > @@ -169,6 +170,9 @@ > fatalx("fork of unprivileged slave failed"); > if (slave_pid == 0) { > /* Unprivileged slave */ > + if (pledge("stdio proc getpw id rpath", NULL) == -1) > + err(1, "pledge"); > + this pledge(2) calls is a bit useless as you have another pledge(2) call just after the privdrop initialisation (chroot + ... + setuid). > set_slave_signals(); > > if ((pw = getpwnam(FTPD_PRIVSEP_USER)) == NULL) > @@ -193,6 +197,10 @@ > > endpwent(); > close(fd_slave); > + > + if (pledge("stdio", NULL) == -1) > + err(1, "pledge"); > + I like this one :) > return (1); > } > Thanks ! -- Sebastien Marie
