[patch] ftpd: pledge(2)

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

[patch] ftpd: pledge(2)

Fritjof Bornebusch
Hi,

this adds pledge(2) to ftpd(8).

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");
 
  (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");
+
  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");
+
  return (1);
  }
 

Reply | Threaded
Open this post in threaded view
|

Re: [patch] ftpd: pledge(2)

Sebastien Marie-2
On Sat, Apr 02, 2016 at 04:38:10PM +0200, [hidden email] 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

Reply | Threaded
Open this post in threaded view
|

Re: [patch] ftpd: pledge(2)

Fritjof Bornebusch
In reply to this post by Fritjof Bornebusch
On Sat, Apr 02, 2016 at 04:38:10PM +0200, [hidden email] wrote:
> Hi,
>
> this adds pledge(2) to ftpd(8).
>
> --f.
>

With help from semarie@ the original diff was changed a little
bit.

The following processes are pledged:
- [priv post-auth]
- User-privileged slave
- Unprivileged slave

As I don't cover all use cases, please send me your feedback.

--f.

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 3 Apr 2016 15:42:21 -0000
@@ -193,6 +193,10 @@
 
  endpwent();
  close(fd_slave);
+
+ if (pledge("stdio", NULL) == -1)
+ fatalx("pledge");
+
  return (1);
  }
 
@@ -302,6 +306,11 @@
  case AUTH_SLAVE:
  /* User-privileged slave */
  debugmsg("user-privileged slave started");
+
+ if (pledge("stdio rpath getpw proc wpath cpath inet ioctl sendfd recvfd",
+   NULL) == -1) {
+ fatalx("pledge");
+ }
  return;
  /* NOTREACHED */
  case AUTH_MONITOR:
@@ -311,6 +320,11 @@
  setproctitle("%s: [priv post-auth]",
     remotehost);
  slavequit = 1;
+
+ if (pledge("stdio proc dns inet sendfd",
+   NULL) == -1) {
+ fatalx("pledge");
+ }
 
  send_data(fd_slave, &slavequit,
     sizeof(slavequit));

Reply | Threaded
Open this post in threaded view
|

Re: [patch] ftpd: pledge(2)

Sebastien Marie-2
On Sun, Apr 03, 2016 at 06:09:21PM +0200, [hidden email] wrote:

> On Sat, Apr 02, 2016 at 04:38:10PM +0200, [hidden email] wrote:
> > Hi,
> >
> > this adds pledge(2) to ftpd(8).
> >
> > --f.
> >
>
> With help from semarie@ the original diff was changed a little
> bit.
>
> The following processes are pledged:
> - [priv post-auth]
> - User-privileged slave
> - Unprivileged slave
>
> As I don't cover all use cases, please send me your feedback.
>
> --f.
>
> 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 3 Apr 2016 15:42:21 -0000
> @@ -193,6 +193,10 @@
>  
>   endpwent();
>   close(fd_slave);
> +
> + if (pledge("stdio", NULL) == -1)
> + fatalx("pledge");
> +
>   return (1);
>   }
>  
> @@ -302,6 +306,11 @@
>   case AUTH_SLAVE:
>   /* User-privileged slave */
>   debugmsg("user-privileged slave started");
> +
> + if (pledge("stdio rpath getpw proc wpath cpath inet ioctl sendfd recvfd",
> +   NULL) == -1) {
> + fatalx("pledge");
> + }

whoa, still a big list of promises, and some are a bit unexpected for
me. could you explain the need for them ?

I mean, if "rpath wpath cpath" are expected for a daemon that serve
files, "ioctl" for example is more questionable. could you explain
quickly why or where ftpd needs them ?

thanks.

>   return;
>   /* NOTREACHED */
>   case AUTH_MONITOR:
> @@ -311,6 +320,11 @@
>   setproctitle("%s: [priv post-auth]",
>      remotehost);
>   slavequit = 1;
> +
> + if (pledge("stdio proc dns inet sendfd",
> +   NULL) == -1) {
> + fatalx("pledge");
> + }
>  
>   send_data(fd_slave, &slavequit,
>      sizeof(slavequit));
>
>

--
Sebastien Marie

Reply | Threaded
Open this post in threaded view
|

Re: [patch] ftpd: pledge(2)

Sebastien Marie-2
On Sun, Apr 03, 2016 at 06:28:21PM +0200, Sebastien Marie wrote:

> > +
> > + if (pledge("stdio rpath getpw proc wpath cpath inet ioctl sendfd recvfd",
> > +   NULL) == -1) {
> > + fatalx("pledge");
> > + }
>
> whoa, still a big list of promises, and some are a bit unexpected for
> me. could you explain the need for them ?
>
> I mean, if "rpath wpath cpath" are expected for a daemon that serve
> files, "ioctl" for example is more questionable. could you explain
> quickly why or where ftpd needs them ?
>

I started to search:
  - rpath wpath cpath : file manipulation
  - inet : ftp passive (so initiate a socket connection)
  - proc : forking for invoking ls
  - getpw tty : builtin ls (call ls_main from src/bin/ls)
  I would prefer the use of "tty" instead of "ioctl"

for sendfd and recvfd, I am still unsure for now.
--
Sebastien Marie

Reply | Threaded
Open this post in threaded view
|

Re: [patch] ftpd: pledge(2)

Theo Buehler
In reply to this post by Sebastien Marie-2
> > + if (pledge("stdio rpath getpw proc wpath cpath inet ioctl sendfd recvfd",
> > +   NULL) == -1) {
> > + fatalx("pledge");
> > + }
>
> whoa, still a big list of promises, and some are a bit unexpected for
> me. could you explain the need for them ?
>
> I mean, if "rpath wpath cpath" are expected for a daemon that serve
> files, "ioctl" for example is more questionable. could you explain
> quickly why or where ftpd needs them ?

Pretty sure that "ioctl" promise can be replaced with "tty".

retrieve() -> ftpd_popen() -> ls_main()

/usr/src/bin/ls/ls_main.c:121 contains a call to
"ioctl(STDOUT_FILENO, TIOCGWINSZ, &win)".

I'm a bit worried about this execv() call in popen.c:143 in ftpd_popen()

Are you sure this can't be reached?

Otherwise an "exec" promise would probably also be needed.

Reply | Threaded
Open this post in threaded view
|

Re: [patch] ftpd: pledge(2)

Fritjof Bornebusch
In reply to this post by Sebastien Marie-2
On Sun, Apr 03, 2016 at 06:28:21PM +0200, Sebastien Marie wrote:

> On Sun, Apr 03, 2016 at 06:09:21PM +0200, [hidden email] wrote:
> > On Sat, Apr 02, 2016 at 04:38:10PM +0200, [hidden email] wrote:
> > > Hi,
> > >
> > > this adds pledge(2) to ftpd(8).
> > >
> > > --f.
> > >
> >
> > With help from semarie@ the original diff was changed a little
> > bit.
> >
> > The following processes are pledged:
> > - [priv post-auth]
> > - User-privileged slave
> > - Unprivileged slave
> >
> > As I don't cover all use cases, please send me your feedback.
> >
> > --f.
> >
> > 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 3 Apr 2016 15:42:21 -0000
> > @@ -193,6 +193,10 @@
> >  
> >   endpwent();
> >   close(fd_slave);
> > +
> > + if (pledge("stdio", NULL) == -1)
> > + fatalx("pledge");
> > +
> >   return (1);
> >   }
> >  
> > @@ -302,6 +306,11 @@
> >   case AUTH_SLAVE:
> >   /* User-privileged slave */
> >   debugmsg("user-privileged slave started");
> > +
> > + if (pledge("stdio rpath getpw proc wpath cpath inet ioctl sendfd recvfd",
> > +   NULL) == -1) {
> > + fatalx("pledge");
> > + }
>
> whoa, still a big list of promises, and some are a bit unexpected for
> me. could you explain the need for them ?
>
> I mean, if "rpath wpath cpath" are expected for a daemon that serve
> files, "ioctl" for example is more questionable. could you explain
> quickly why or where ftpd needs them ?
>

- sendfd / recvfd are for active ftp
- ioctl is e.g. used for "ls" after ftp(1) established a connection
  I'm not exactly sure why, as there is no ioctl(2) call, but maybe
  in one underlaying library.
- proc for fork e.g. (popen_ftpd.c)
- getpwnam for getpwname(3) e.g. in ftpd.c    

> thanks.
>
> >   return;
> >   /* NOTREACHED */
> >   case AUTH_MONITOR:
> > @@ -311,6 +320,11 @@
> >   setproctitle("%s: [priv post-auth]",
> >      remotehost);
> >   slavequit = 1;
> > +
> > + if (pledge("stdio proc dns inet sendfd",
> > +   NULL) == -1) {
> > + fatalx("pledge");
> > + }
> >  
> >   send_data(fd_slave, &slavequit,
> >      sizeof(slavequit));
> >
> >
>
> --
> Sebastien Marie
>

Reply | Threaded
Open this post in threaded view
|

Re: [patch] ftpd: pledge(2)

Fritjof Bornebusch
In reply to this post by Theo Buehler
On Sun, Apr 03, 2016 at 06:51:47PM +0200, Theo Buehler wrote:

> > > + if (pledge("stdio rpath getpw proc wpath cpath inet ioctl sendfd recvfd",
> > > +   NULL) == -1) {
> > > + fatalx("pledge");
> > > + }
> >
> > whoa, still a big list of promises, and some are a bit unexpected for
> > me. could you explain the need for them ?
> >
> > I mean, if "rpath wpath cpath" are expected for a daemon that serve
> > files, "ioctl" for example is more questionable. could you explain
> > quickly why or where ftpd needs them ?
>
> Pretty sure that "ioctl" promise can be replaced with "tty".
>

Jep, works with "tty".

> retrieve() -> ftpd_popen() -> ls_main()
>
> /usr/src/bin/ls/ls_main.c:121 contains a call to
> "ioctl(STDOUT_FILENO, TIOCGWINSZ, &win)".
>

Ahh, this was the ioctl(2) call I was looking for.
I saw the link to ls_main, but overlooked the ioctl(2) call.

> I'm a bit worried about this execv() call in popen.c:143 in ftpd_popen()
>
> Are you sure this can't be reached?
>
> Otherwise an "exec" promise would probably also be needed.
>

Reply | Threaded
Open this post in threaded view
|

Re: [patch] ftpd: pledge(2)

Fritjof Bornebusch
In reply to this post by Fritjof Bornebusch
On Sun, Apr 03, 2016 at 07:07:27PM +0200, [hidden email] wrote:
> - sendfd / recvfd are for active ftp
> - ioctl is e.g. used for "ls" after ftp(1) established a connection
>   I'm not exactly sure why, as there is no ioctl(2) call, but maybe
>   in one underlaying library.
> - proc for fork e.g. (popen_ftpd.c)
> - getpwnam for getpwname(3) e.g. in ftpd.c    
>

Sorry, missed your mail while writing mine. :/

Reply | Threaded
Open this post in threaded view
|

Re: [patch] ftpd: pledge(2)

Fritjof Bornebusch
In reply to this post by Theo Buehler
On Sun, Apr 03, 2016 at 06:51:47PM +0200, Theo Buehler wrote:

> > > + if (pledge("stdio rpath getpw proc wpath cpath inet ioctl sendfd recvfd",
> > > +   NULL) == -1) {
> > > + fatalx("pledge");
> > > + }
> >
> > whoa, still a big list of promises, and some are a bit unexpected for
> > me. could you explain the need for them ?
> >
> > I mean, if "rpath wpath cpath" are expected for a daemon that serve
> > files, "ioctl" for example is more questionable. could you explain
> > quickly why or where ftpd needs them ?
>
> Pretty sure that "ioctl" promise can be replaced with "tty".
>
> retrieve() -> ftpd_popen() -> ls_main()
>
> /usr/src/bin/ls/ls_main.c:121 contains a call to
> "ioctl(STDOUT_FILENO, TIOCGWINSZ, &win)".
>
> I'm a bit worried about this execv() call in popen.c:143 in ftpd_popen()
>
> Are you sure this can't be reached?
>
> Otherwise an "exec" promise would probably also be needed.
>

No I'm not, see updated diff below. :)
Thanks for the hint.

--f.


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 3 Apr 2016 18:44:50 -0000
@@ -193,6 +193,10 @@
 
  endpwent();
  close(fd_slave);
+
+ if (pledge("stdio", NULL) == -1)
+ fatalx("pledge");
+
  return (1);
  }
 
@@ -302,6 +306,11 @@
  case AUTH_SLAVE:
  /* User-privileged slave */
  debugmsg("user-privileged slave started");
+
+ if (pledge("stdio rpath wpath cpath inet getpw sendfd recvfd tty proc exec",
+   NULL) == -1) {
+ fatalx("pledge");
+ }
  return;
  /* NOTREACHED */
  case AUTH_MONITOR:
@@ -311,6 +320,11 @@
  setproctitle("%s: [priv post-auth]",
     remotehost);
  slavequit = 1;
+
+ if (pledge("stdio inet dns inet sendfd proc",
+   NULL) == -1) {
+ fatalx("pledge");
+ }
 
  send_data(fd_slave, &slavequit,
     sizeof(slavequit));

Reply | Threaded
Open this post in threaded view
|

Re: [patch] ftpd: pledge(2)

Fritjof Bornebusch
In reply to this post by Theo Buehler
On Sun, Apr 03, 2016 at 06:51:47PM +0200, Theo Buehler wrote:

> > > + if (pledge("stdio rpath getpw proc wpath cpath inet ioctl sendfd recvfd",
> > > +   NULL) == -1) {
> > > + fatalx("pledge");
> > > + }
> >
> > whoa, still a big list of promises, and some are a bit unexpected for
> > me. could you explain the need for them ?
> >
> > I mean, if "rpath wpath cpath" are expected for a daemon that serve
> > files, "ioctl" for example is more questionable. could you explain
> > quickly why or where ftpd needs them ?
>
> Pretty sure that "ioctl" promise can be replaced with "tty".
>
> retrieve() -> ftpd_popen() -> ls_main()
>
> /usr/src/bin/ls/ls_main.c:121 contains a call to
> "ioctl(STDOUT_FILENO, TIOCGWINSZ, &win)".
>
> I'm a bit worried about this execv() call in popen.c:143 in ftpd_popen()
>
> Are you sure this can't be reached?
>
> Otherwise an "exec" promise would probably also be needed.
>

Removed double "inet" call.

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 3 Apr 2016 18:58:10 -0000
@@ -193,6 +193,10 @@
 
  endpwent();
  close(fd_slave);
+
+ if (pledge("stdio", NULL) == -1)
+ fatalx("pledge");
+
  return (1);
  }
 
@@ -302,6 +306,11 @@
  case AUTH_SLAVE:
  /* User-privileged slave */
  debugmsg("user-privileged slave started");
+
+ if (pledge("stdio rpath wpath cpath inet getpw sendfd recvfd tty proc exec",
+   NULL) == -1) {
+ fatalx("pledge");
+ }
  return;
  /* NOTREACHED */
  case AUTH_MONITOR:
@@ -311,6 +320,11 @@
  setproctitle("%s: [priv post-auth]",
     remotehost);
  slavequit = 1;
+
+ if (pledge("stdio dns inet sendfd proc",
+   NULL) == -1) {
+ fatalx("pledge");
+ }
 
  send_data(fd_slave, &slavequit,
     sizeof(slavequit));