Bug 223327 - dhclient: close the pidfile before calling chroot(2)
Summary: dhclient: close the pidfile before calling chroot(2)
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Some People
Assignee: Mark Johnston
URL:
Keywords: patch
Depends on:
Blocks: 228911
  Show dependency treegraph
 
Reported: 2017-10-30 18:38 UTC by olevole
Modified: 2018-08-06 16:27 UTC (History)
8 users (show)

See Also:


Attachments
forces the dhclient to work in the vnet-jail again on FreeBSD-CURRENT (389 bytes, patch)
2017-10-30 18:38 UTC, olevole
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description olevole 2017-10-30 18:38:14 UTC
Created attachment 187588 [details]
forces the dhclient to work in the vnet-jail again on FreeBSD-CURRENT

At the moment dhclient(8) does not work in vnet jail under FreeBSD 12-CURRENT. 
If you try to execute dhclient in jail, it will return with the following error:
--
chroot
exiting.
--
and NOPERM in errno.

This behavior occurs when you try to execute a chroot with an open to the outside environment descriptor

kern.chroot_allow_open_directories can affect this behavior, but apparently in dhclient it is not necessary to keep fd open, because all operations on it occur before chroot.

This patch forces the dhclient to work in the jail again.

How to reproduce problem (have fresh FreeBSD 12-CURRENT, e.g. 325104+):
--
1) prepare base for chroot
% mkdir /tmp/base
% cd /tmp/base
% wget http://ftp.freebsd.org/pub/FreeBSD/snapshots/amd64/12.0-CURRENT/base.txz
% tar xfz base.txz
% ifconfig epair0 create


2) Wrote /tmp/jail.conf:

jail1 {
    path = /tmp/base;
    devfs_ruleset="99";
    allow.mount;
    vnet = new;
    vnet.interface = epair0a;
    mount.devfs;
    interface = vlan1;
    allow.raw_sockets;
    allow.sysvipc;
    exec.start = "/bin/sh /etc/rc";
    exec.stop = "/bin/sh /etc/rc.shutdown";
}

3) Create jail:
jail -c -f /jail1.conf

4) Try to dhclient where 4 is jail ID:
jexec 4 dhclient epair0a
--
Comment 1 Kristof Provost freebsd_committer 2017-11-18 11:58:03 UTC
Hmm. This is quite interesting. I think the problem started with r322369 which changed libutil / pidfile_open() to keep the directory fd (rather than the pidfile fd) so it could unlink the pidfile in capability mode.
That appears to conflict with chroot now, because you can't chroot if you've got a directory fd open to somewhere outside the new root.

I don't think this patch is right, because it'll break the pidfile_remove() in routehandler().

I'm not at all sure how this should be fixed though.
Comment 2 Jilles Tjoelker freebsd_committer 2017-11-18 22:06:52 UTC
Although kern.chroot_allow_open_directories can be bypassed trivially via Unix domain socket file descriptor passing, it does serve a purpose in pointing out open chroots and jails like this one. The open directory allows full access to the / that dhclient was started from, defeating its chroot to /var/empty. In capability mode where ".." is disallowed, there is still full access to /var/run.

It looks like the status quo is that the pidfile will not be removed when dhclient aborts after chrooting. The pidfile_remove() call will always fail. Before r322369 this was the case because of the chroot, and after r322369 this was the case because dhclient limits the pidfile descriptor to no rights (so that pidfile_verify() will fail).

If this status quo is acceptable, the fix is to close the directory file descriptor using a newly added pidfile(3) function.

If the status quo is not acceptable, it could be fixed by adding a not chrooted non-capmode intermediate process to do the remove or by moving the dhclient pidfile into its own directory and fixing the rights on the pidfile and directory descriptors. The latter also requires an addition to the pidfile(3) API.
Comment 3 Conrad Meyer freebsd_committer 2017-11-18 22:30:07 UTC
(In reply to Jilles Tjoelker from comment #2)
> In capability mode where ".." is disallowed, there is still full access to
> /var/run.

Nitpicking a little bit: .. *is* allowed in capability mode, as long as it modifies a path below the reference descriptor.
Comment 4 Mark Johnston freebsd_committer 2017-11-20 03:24:31 UTC
I realize that this doesn't address the general problem, but what's the reason for chrooting in the first place now that dhclient runs in capability mode?

(In reply to Jilles Tjoelker from comment #2)
I don't quite follow the comment about pidfile_verify() - we do retain the CAP_FSTAT capability on the pidfile descriptor (see r322370).
Comment 5 Mark Johnston freebsd_committer 2017-11-20 03:26:24 UTC
(In reply to Mark Johnston from comment #4)
Err, of course dhclient might run on a kernel compiled without capsicum support.
Comment 6 Jilles Tjoelker freebsd_committer 2017-11-25 11:56:14 UTC
(In reply to Mark Johnston from comment #4)
Although the pidfile library retains the rights, dhclient itself does not: around line 2435 of sbin/dhclient/dhclient.c it removes all rights from the descriptor after writing the pidfile.
Comment 7 Goran Mekić 2018-05-19 06:29:03 UTC
With the 12 code clash being relatively close, is there a way to fix this? Given the comments before this one, I would expect at least one more capsicumed app had the same problem?
Comment 8 Ed Maste freebsd_committer 2018-08-03 18:10:37 UTC
(In reply to Goran Mekić from comment #7)
> I would expect at least one more capsicumed app had the same problem?

I think dhclient may be the only program that uses all of pidfile, capsicum, and chroot.
Comment 9 Ed Maste freebsd_committer 2018-08-03 18:15:28 UTC
(In reply to Mark Johnston from comment #4)
> I realize that this doesn't address the general problem, but what's
> the reason for chrooting in the first place now that dhclient runs
> in capability mode?

Perhaps we could enter capability mode, and iff that fails try chroot?
Comment 10 Mark Johnston freebsd_committer 2018-08-03 20:03:16 UTC
(In reply to Ed Maste from comment #9)
In addition, how about we also keep dhclient pidfiles under /var/run/dhclient so that dhclient doesn't have access to /var/run/* via the directory descriptor?
Comment 11 Mark Johnston freebsd_committer 2018-08-03 20:38:40 UTC
I think that this represents the best compromise, and is relatively simple: https://reviews.freebsd.org/D16584
Comment 12 commit-hook freebsd_committer 2018-08-06 16:22:17 UTC
A commit references this bug:

Author: markj
Date: Mon Aug  6 16:22:02 UTC 2018
New revision: 337382
URL: https://svnweb.freebsd.org/changeset/base/337382

Log:
  dhclient: Don't chroot if we are in capability mode.

  The main dhclient process is Capsicumized but also chroots to
  restrict filesystem access.  With r322369, pidfile(3) maintains a
  directory descriptor for the pidfile, which can cause the chroot
  to fail in certain cases.  To minimize the problem, only chroot
  if we fail to enter capability mode, and store dhclient pidfiles
  in a subdirectory of /var/run, thus restricting access via
  pidfile(3)'s directory descriptor.

  PR:		223327
  Reviewed by:	cem, oshogbo
  Sponsored by:	The FreeBSD Foundation
  Differential Revision:	https://reviews.freebsd.org/D16584

Changes:
  head/etc/mtree/BSD.var.dist
  head/sbin/dhclient/dhclient.8
  head/sbin/dhclient/dhclient.c
  head/sbin/init/rc.d/dhclient
Comment 13 Mark Johnston freebsd_committer 2018-08-06 16:27:30 UTC
Committed the workaround in r337382.  The general problem still exists, and I'm not sure what the best solution is.  Anything that uses pidfile(3) and chroot(2) is susceptible to the problem.  I think r337382 is a desirable change regardless: there's no point in chrooting if we're in capability mode, and there might be multiple dhclient pidfiles so it's nice to keep them in their own subdirectory.