Page MenuHomeFreeBSD
Differential D16584

Remove chroot(2) call from dhclient.
ClosedPublic
Actions

Authored by markj on Aug 3 2018, 8:34 PM.
Tags
None
Referenced Files
F114160066: D16584.id46289.diff
Tue, Apr 8, 1:05 PM
Unknown Object (File)
Wed, Mar 26, 9:03 AM
Unknown Object (File)
Sat, Mar 22, 11:31 AM
Unknown Object (File)
Sat, Mar 15, 11:06 PM
Unknown Object (File)
Feb 25 2025, 8:35 PM
Unknown Object (File)
Feb 22 2025, 5:38 PM
Unknown Object (File)
Jan 17 2025, 8:36 PM
Unknown Object (File)
Jan 8 2025, 12:39 AM
View All 80 Files
Subscribers
imp
jonathan
olevole_olevole.ru

Details

Reviewers
emaste
cem
jilles
oshogbo
Group Reviewers
manpages
Commits
rS337382: dhclient: Don't chroot if we are in capability mode.
Summary

dhclient runs in capability mode, so its filesystem access is already
restricted. Moreover, libutil keeps open a directory descriptor for the
pidfile (/var/run by default). If kern.chroot_allow_open_directories is
not set, this descriptor will cause the chroot to fail. To fix this,
stop using chroot(), and store pidfiles under /var/run/dhclient/ instead
to restrict the scope of libutil's descriptor. This is not perfect
since different dhclient instances can still mess with each other's
pidfiles despite the use of capability mode, but I don't think that this
is a serious problem.

Also stop removing rights from the pidfile descriptor after writing the
PID. This interferes with pidfile_verify(), called from
pidfile_close(). We already restrict rights to the pidfile somewhat in
libutil; one possible follow-up enhancement would be drop the
CAP_PWRITE and CAP_TRUNCATE rights in pidfile_write(). Of course, this
would mean that pidfile_write() could only be called once, but that's
most likely fine.

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 18583
Build 18275: arc lint + arc unit

Event Timeline

markj created this revision.Aug 3 2018, 8:34 PM
Harbormaster completed remote builds in B18565: Diff 46267.Aug 3 2018, 8:34 PM
markj edited the summary of this revision. (Show Details)Aug 3 2018, 8:35 PM
markj added reviewers: emaste, cem, jilles, oshogbo.
cem accepted this revision.Aug 3 2018, 9:05 PM
cem added inline comments.
sbin/dhclient/dhclient.c
423

For whatever reason, _PATH_VARRUN has already got a trailing slash. Not that it matters too much for lookup, and I agree this is more clear.

I think spelling it without the %s might also be more clear?

asprintf(..., _PATH_VARRUN "/...", *argv)

The uppercase name implies it is a macro, rather than some extern variable. I'm not sure if that makes it reasonable to expect it to be a string literal or not.

531–534

I might have missed earlier discussion, but why not just wrap this in if (cap_getmode(&dummy) != 0) {? (I.e., kernel does not support CAPABILITY_MODE.)

This revision is now accepted and ready to land.Aug 3 2018, 9:05 PM
olevole_olevole.ru added a subscriber: olevole_olevole.ru.Aug 3 2018, 11:04 PM
markj added inline comments.Aug 3 2018, 11:36 PM
sbin/dhclient/dhclient.c
531–534

Hmm, we still have to handle the original problem then.

... is there any chance capsicum support will become non-optional at some point? :)

cem added a comment.Aug 4 2018, 12:50 AM

(I guess I already marked "approved" so Phab won't let me again. But I just want to be clear that in spite of my line comments, I approve this change as-is.)

sbin/dhclient/dhclient.c
531–534

Given we're already creating /var/run/dhclient for capsicum, could we also use that for the chroot in place of /var/empty? The pid file doesn't seem especially sensitive for an attacker.

But I'm confused. How does chrooted (or capsicum'd) dhclient access its leases file in /var/db today?

$ sudo procstat -f $(cat /var/run/dhclient.igb0.pid)
  PID COMM                FD T V FLAGS    REF  OFFSET PRO NAME
  565 dhclient          text v r r-------   -       - -   /sbin/dhclient
  565 dhclient           cwd v d r-------   -       - -   /var/empty
  565 dhclient          root v d r-------   -       - -   /var/empty
  565 dhclient          jail v d r-------   -       - -   /var/empty
  565 dhclient             0 v c rw------   6       0 -   /dev/null
  565 dhclient             1 v c rw------   6       0 -   /dev/null
  565 dhclient             2 v c rw------   6       0 -   /dev/null
  565 dhclient             3 v d r----n--   2       0 -   /var/run
  565 dhclient             4 s - rw---n--   2       0 UDS 0 0 -
  565 dhclient             5 v r -w---n-l   2       0 -   /var/run/dhclient.igb0.pid
  565 dhclient             7 v c r-------   1   12880 -   /dev/bpf
  565 dhclient             8 s - rw------   1       0 ?
  565 dhclient             9 v r -w------   1    5495 -   /var/db/dhclient.leases.igb0
  565 dhclient            11 p - rw------   1       0 -   -

... is there any chance capsicum support will become non-optional at some point? :)

Yeah, that's the question. I expect laptop-class systems and better to have capsicum non-optionally. (I thought that was planned for 12, actually.)

But I am imagining small embedded systems (MIPS/riscv/arm32, e.g., routers) might not want to pay any penalty for capsicum, and still have some protection for dhclient. That said, I don't have any quantified sense of how costly capsicum support is.

I'm willing to assume we'll just require capsicum on everywhere and therefore the chroot can be discarded.

Do you have a good idea of how the capsicum sandboxing interacts with both the pidfile, /dev/bpf, and the lease file today?

emaste added a comment.Aug 4 2018, 12:53 AM

I thought that was planned for 12, actually.

Capsicum is generally enabled in GENERIC config files now (for some time); there is an open question about removing the #ifdefs and kernel config goo.

cem added a comment.Aug 4 2018, 12:57 AM
In D16584#352320, @emaste wrote:

I thought that was planned for 12, actually.

Capsicum is generally enabled in GENERIC config files now (for some time); there is an open question about removing the #ifdefs and kernel config goo.

What would it take to also enable in the myriad non-GENERIC embedded board configs, in a way that doesn't make them useless or annoy their users/developers? I think that's probably a necessary step towards removing the config(8) option and ifdefs entirely? If not adding the option to configs, at least researching what the pain level will be.

markj added a comment.Aug 4 2018, 3:24 PM
In D16584#352320, @emaste wrote:

I thought that was planned for 12, actually.

Capsicum is generally enabled in GENERIC config files now (for some time); there is an open question about removing the #ifdefs and kernel config goo.

FWIW, I've hit this issue before when modifying an application to use Capsicum, and ended up making lack of support a fatal error. The lack of mandatory support makes Capsicum harder to use.

sbin/dhclient/dhclient.c
531–534

Given we're already creating /var/run/dhclient for capsicum, could we also use that for the chroot in place of /var/empty? The pid file doesn't seem especially sensitive for an attacker.

The issue is that we'd have to modify pidfile(3) to close the directory descriptor.

Do you have a good idea of how the capsicum sandboxing interacts with both the pidfile, /dev/bpf, and the lease file today?

For pidfiles, the only requirement today is that you call pidfile_open() before entering capability mode. pidfile_open() keeps rights to both the pidfile and the directory containing the pidfile. The latter is so that we can unlink the pidfile in capability mode when exiting.

dhclient uses some privilege separation and has a non-sandboxed process which can send raw packets. I'm not sure how it manages lease files.

markj updated this revision to Diff 46289.Aug 4 2018, 4:53 PM

Fall back to a chroot if we are not sandboxed after calling
caph_enter_casper().

Document the problem with chroot_allow_open_directories in
the code and man page. Because capsicum is enabled in most
kernel configurations and chroot_allow_open_directories defaults
to 1, the scope of the problem reported in PR 223327 is
reduced significantly. If Capsicum is made non-optional,
the problem goes away.

This revision now requires review to proceed.Aug 4 2018, 4:53 PM
Herald added a subscriber: jonathan. · View Herald TranscriptAug 4 2018, 4:53 PM
Harbormaster completed remote builds in B18583: Diff 46289.Aug 4 2018, 4:53 PM
cem accepted this revision.Aug 4 2018, 5:37 PM
This revision is now accepted and ready to land.Aug 4 2018, 5:37 PM
oshogbo accepted this revision.Aug 5 2018, 10:22 PM
oshogbo added inline comments.
sbin/dhclient/dhclient.c
543

I would add some comment here because this is not obvious why after entering cap_mode we check the mode.

Closed by commit rS337382: dhclient: Don't chroot if we are in capability mode. (authored by markj). · Explain WhyAug 6 2018, 4:22 PM
This revision was automatically updated to reflect the committed changes.
markj marked an inline comment as done.
Herald added a reviewer: manpages. · View Herald TranscriptAug 6 2018, 4:22 PM
Herald added a subscriber: imp. · View Herald Transcript

Revision Contents
Changeset List

PathSize
etc/
mtree/
2 lines
sbin/
dhclient/
17 lines
26 lines
init/
rc.d/
2 lines

Diff 46289

View Options

etc/mtree/BSD.var.dist

PropertyOld ValueNew Value
svn:eol-stylenullnative
\ No newline at end of property
svn:keywordsnullFreeBSD=%H
\ No newline at end of property
svn:mime-typenulltext/plain
\ No newline at end of property
# $FreeBSD$
PORTNAME= pyfda
PORTVERSION= 0.1
DISTVERSIONSUFFIX= rc6
CATEGORIES= cad python devel
MASTER_SITES= CHEESESHOP
PKGNAMEPREFIX= ${PYTHON_PKGNAMEPREFIX}
MAINTAINER= yuri@FreeBSD.org
COMMENT= GUI tool for designing and analysing discrete time filters
LICENSE= MIT
LICENSE_FILE= ${WRKSRC}/LICENSE
RUN_DEPENDS= ${PYTHON_PKGNAMEPREFIX}docutils>0:textproc/py-docutils@${FLAVOR} \
${PYTHON_PKGNAMEPREFIX}matplotlib>0:math/py-matplotlib@${FLAVOR} \
${PYNUMPY} \
${PYTHON_PKGNAMEPREFIX}scipy>0:science/py-scipy@${FLAVOR}
USES= python:3.4+ pyqt:5
USE_PYTHON= distutils autoplist noflavors
matUnsubmitted
Not Done
Inline Actions

Also, setting noflavors (which is weird, no reason to do it) means that the FLAVOR variable is empty, thus, it makes this port broken because the dependencies line end with @ with no flavor.

mat: Also, setting noflavors (which is weird, no reason to do it) means that the FLAVOR variable is…
yuriAuthorUnsubmitted
Not Done
Inline Actions

But this is an app, not a library. Shouldn't python apps all be noflavors?

yuri: But this is an app, not a library. Shouldn't python apps all be `noflavors`?
matUnsubmitted
Not Done
Inline Actions

this has a PKGNAMEPREFIX, nothing against it having noflavors.

mat: this has a PKGNAMEPREFIX, nothing against it having noflavors.
USE_PYQT= core_run gui_run widgets_run
NO_ARCH= yes
post-extract:
@${MV} ${WRKSRC}/pyfda/pyfda_log.conf ${WRKSRC}/pyfda/pyfda_log.conf.sample
@${MV} ${WRKSRC}/pyfda/pyfda_log_debug.conf ${WRKSRC}/pyfda/pyfda_log_debug.conf.sample
post-patch:
@${REINPLACE_CMD} -e " \
s|package_data={'pyfda': \['pyfda_log.conf', 'pyfda_log_debug.conf',|package_data={'pyfda': [|; \
s|data_files = \[|data_files = [('${PREFIX}/etc', ['pyfda/pyfda_log.conf.sample', 'pyfda/pyfda_log_debug.conf.sample'])|" \
${WRKSRC}/setup.py
@${REINPLACE_CMD} -e " \
s|logging.config.fileConfig(os.path.join(base_dir, rc.log_config_file))|logging.config.fileConfig(os.path.join('${PREFIX}/etc', rc.log_config_file))|" \
${WRKSRC}/pyfda/pyfdax.py
@${REINPLACE_CMD} -e " \
s|'pyfda.log'|'/tmp/pyfda.log'|" \
${WRKSRC}/pyfda/*.conf.sample
post-stage:
@${REINPLACE_CMD} -E " \
s|(.*\.sample)$$|@sample \1|" \
${WRKDIR}/.PLIST.pymodtmp
.include <bsd.port.mk>
View Options

sbin/dhclient/dhclient.8

PropertyOld ValueNew Value
svn:eol-stylenullnative
\ No newline at end of property
svn:keywordsnullFreeBSD=%H
\ No newline at end of property
svn:mime-typenulltext/plain
\ No newline at end of property
# $FreeBSD$
PORTNAME= pyfda
PORTVERSION= 0.1
DISTVERSIONSUFFIX= rc6
CATEGORIES= cad python devel
MASTER_SITES= CHEESESHOP
PKGNAMEPREFIX= ${PYTHON_PKGNAMEPREFIX}
MAINTAINER= yuri@FreeBSD.org
COMMENT= GUI tool for designing and analysing discrete time filters
LICENSE= MIT
LICENSE_FILE= ${WRKSRC}/LICENSE
RUN_DEPENDS= ${PYTHON_PKGNAMEPREFIX}docutils>0:textproc/py-docutils@${FLAVOR} \
${PYTHON_PKGNAMEPREFIX}matplotlib>0:math/py-matplotlib@${FLAVOR} \
${PYNUMPY} \
${PYTHON_PKGNAMEPREFIX}scipy>0:science/py-scipy@${FLAVOR}
USES= python:3.4+ pyqt:5
USE_PYTHON= distutils autoplist noflavors
matUnsubmitted
Not Done
Inline Actions

Also, setting noflavors (which is weird, no reason to do it) means that the FLAVOR variable is empty, thus, it makes this port broken because the dependencies line end with @ with no flavor.

mat: Also, setting noflavors (which is weird, no reason to do it) means that the FLAVOR variable is…
yuriAuthorUnsubmitted
Not Done
Inline Actions

But this is an app, not a library. Shouldn't python apps all be noflavors?

yuri: But this is an app, not a library. Shouldn't python apps all be `noflavors`?
matUnsubmitted
Not Done
Inline Actions

this has a PKGNAMEPREFIX, nothing against it having noflavors.

mat: this has a PKGNAMEPREFIX, nothing against it having noflavors.
USE_PYQT= core_run gui_run widgets_run
NO_ARCH= yes
post-extract:
@${MV} ${WRKSRC}/pyfda/pyfda_log.conf ${WRKSRC}/pyfda/pyfda_log.conf.sample
@${MV} ${WRKSRC}/pyfda/pyfda_log_debug.conf ${WRKSRC}/pyfda/pyfda_log_debug.conf.sample
post-patch:
@${REINPLACE_CMD} -e " \
s|package_data={'pyfda': \['pyfda_log.conf', 'pyfda_log_debug.conf',|package_data={'pyfda': [|; \
s|data_files = \[|data_files = [('${PREFIX}/etc', ['pyfda/pyfda_log.conf.sample', 'pyfda/pyfda_log_debug.conf.sample'])|" \
${WRKSRC}/setup.py
@${REINPLACE_CMD} -e " \
s|logging.config.fileConfig(os.path.join(base_dir, rc.log_config_file))|logging.config.fileConfig(os.path.join('${PREFIX}/etc', rc.log_config_file))|" \
${WRKSRC}/pyfda/pyfdax.py
@${REINPLACE_CMD} -e " \
s|'pyfda.log'|'/tmp/pyfda.log'|" \
${WRKSRC}/pyfda/*.conf.sample
post-stage:
@${REINPLACE_CMD} -E " \
s|(.*\.sample)$$|@sample \1|" \
${WRKDIR}/.PLIST.pymodtmp
.include <bsd.port.mk>
View Options

sbin/dhclient/dhclient.c

PropertyOld ValueNew Value
svn:eol-stylenullnative
\ No newline at end of property
svn:keywordsnullFreeBSD=%H
\ No newline at end of property
svn:mime-typenulltext/plain
\ No newline at end of property
# $FreeBSD$
PORTNAME= pyfda
PORTVERSION= 0.1
DISTVERSIONSUFFIX= rc6
CATEGORIES= cad python devel
MASTER_SITES= CHEESESHOP
PKGNAMEPREFIX= ${PYTHON_PKGNAMEPREFIX}
MAINTAINER= yuri@FreeBSD.org
COMMENT= GUI tool for designing and analysing discrete time filters
LICENSE= MIT
LICENSE_FILE= ${WRKSRC}/LICENSE
RUN_DEPENDS= ${PYTHON_PKGNAMEPREFIX}docutils>0:textproc/py-docutils@${FLAVOR} \
${PYTHON_PKGNAMEPREFIX}matplotlib>0:math/py-matplotlib@${FLAVOR} \
${PYNUMPY} \
${PYTHON_PKGNAMEPREFIX}scipy>0:science/py-scipy@${FLAVOR}
USES= python:3.4+ pyqt:5
USE_PYTHON= distutils autoplist noflavors
matUnsubmitted
Not Done
Inline Actions

Also, setting noflavors (which is weird, no reason to do it) means that the FLAVOR variable is empty, thus, it makes this port broken because the dependencies line end with @ with no flavor.

mat: Also, setting noflavors (which is weird, no reason to do it) means that the FLAVOR variable is…
yuriAuthorUnsubmitted
Not Done
Inline Actions

But this is an app, not a library. Shouldn't python apps all be noflavors?

yuri: But this is an app, not a library. Shouldn't python apps all be `noflavors`?
matUnsubmitted
Not Done
Inline Actions

this has a PKGNAMEPREFIX, nothing against it having noflavors.

mat: this has a PKGNAMEPREFIX, nothing against it having noflavors.
USE_PYQT= core_run gui_run widgets_run
NO_ARCH= yes
post-extract:
@${MV} ${WRKSRC}/pyfda/pyfda_log.conf ${WRKSRC}/pyfda/pyfda_log.conf.sample
@${MV} ${WRKSRC}/pyfda/pyfda_log_debug.conf ${WRKSRC}/pyfda/pyfda_log_debug.conf.sample
post-patch:
@${REINPLACE_CMD} -e " \
s|package_data={'pyfda': \['pyfda_log.conf', 'pyfda_log_debug.conf',|package_data={'pyfda': [|; \
s|data_files = \[|data_files = [('${PREFIX}/etc', ['pyfda/pyfda_log.conf.sample', 'pyfda/pyfda_log_debug.conf.sample'])|" \
${WRKSRC}/setup.py
@${REINPLACE_CMD} -e " \
s|logging.config.fileConfig(os.path.join(base_dir, rc.log_config_file))|logging.config.fileConfig(os.path.join('${PREFIX}/etc', rc.log_config_file))|" \
${WRKSRC}/pyfda/pyfdax.py
@${REINPLACE_CMD} -e " \
s|'pyfda.log'|'/tmp/pyfda.log'|" \
${WRKSRC}/pyfda/*.conf.sample
post-stage:
@${REINPLACE_CMD} -E " \
s|(.*\.sample)$$|@sample \1|" \
${WRKDIR}/.PLIST.pymodtmp
.include <bsd.port.mk>
View Options

sbin/init/rc.d/dhclient

PropertyOld ValueNew Value
svn:eol-stylenullnative
\ No newline at end of property
svn:keywordsnullFreeBSD=%H
\ No newline at end of property
svn:mime-typenulltext/plain
\ No newline at end of property
# $FreeBSD$
PORTNAME= pyfda
PORTVERSION= 0.1
DISTVERSIONSUFFIX= rc6
CATEGORIES= cad python devel
MASTER_SITES= CHEESESHOP
PKGNAMEPREFIX= ${PYTHON_PKGNAMEPREFIX}
MAINTAINER= yuri@FreeBSD.org
COMMENT= GUI tool for designing and analysing discrete time filters
LICENSE= MIT
LICENSE_FILE= ${WRKSRC}/LICENSE
RUN_DEPENDS= ${PYTHON_PKGNAMEPREFIX}docutils>0:textproc/py-docutils@${FLAVOR} \
${PYTHON_PKGNAMEPREFIX}matplotlib>0:math/py-matplotlib@${FLAVOR} \
${PYNUMPY} \
${PYTHON_PKGNAMEPREFIX}scipy>0:science/py-scipy@${FLAVOR}
USES= python:3.4+ pyqt:5
USE_PYTHON= distutils autoplist noflavors
matUnsubmitted
Not Done
Inline Actions

Also, setting noflavors (which is weird, no reason to do it) means that the FLAVOR variable is empty, thus, it makes this port broken because the dependencies line end with @ with no flavor.

mat: Also, setting noflavors (which is weird, no reason to do it) means that the FLAVOR variable is…
yuriAuthorUnsubmitted
Not Done
Inline Actions

But this is an app, not a library. Shouldn't python apps all be noflavors?

yuri: But this is an app, not a library. Shouldn't python apps all be `noflavors`?
matUnsubmitted
Not Done
Inline Actions

this has a PKGNAMEPREFIX, nothing against it having noflavors.

mat: this has a PKGNAMEPREFIX, nothing against it having noflavors.
USE_PYQT= core_run gui_run widgets_run
NO_ARCH= yes
post-extract:
@${MV} ${WRKSRC}/pyfda/pyfda_log.conf ${WRKSRC}/pyfda/pyfda_log.conf.sample
@${MV} ${WRKSRC}/pyfda/pyfda_log_debug.conf ${WRKSRC}/pyfda/pyfda_log_debug.conf.sample
post-patch:
@${REINPLACE_CMD} -e " \
s|package_data={'pyfda': \['pyfda_log.conf', 'pyfda_log_debug.conf',|package_data={'pyfda': [|; \
s|data_files = \[|data_files = [('${PREFIX}/etc', ['pyfda/pyfda_log.conf.sample', 'pyfda/pyfda_log_debug.conf.sample'])|" \
${WRKSRC}/setup.py
@${REINPLACE_CMD} -e " \
s|logging.config.fileConfig(os.path.join(base_dir, rc.log_config_file))|logging.config.fileConfig(os.path.join('${PREFIX}/etc', rc.log_config_file))|" \
${WRKSRC}/pyfda/pyfdax.py
@${REINPLACE_CMD} -e " \
s|'pyfda.log'|'/tmp/pyfda.log'|" \
${WRKSRC}/pyfda/*.conf.sample
post-stage:
@${REINPLACE_CMD} -E " \
s|(.*\.sample)$$|@sample \1|" \
${WRKDIR}/.PLIST.pymodtmp
.include <bsd.port.mk>