Details

Reviewers

cy
rwatson
bcr
delphij

Group Reviewers

secteam
manpages

Commits

rS336547: Automatically run ntpd as non-root when possible.

Summary

Ntpd needs only a subset of full root privileges to do its job. Specifically it needs the ability to manipulate system time, and to re-bind to a privileged UDP port after interface changes. The mac_ntpd(4) policy module (see D16281) can grant these privs.

These changes detect the availability of mac_ntpd(4). If enabled, and if the ntpd configuration is fairly vanilla, it automatically runs ntpd as the non-root user 'ntpd' (uid 123). "Vanilla" means the config doesn't include command line or ntp.conf options changing the location of files or usling any files/dirs likely to be inaccessible to user ntpd.

Note that these changes also address PR 199127 by using the command_args technique suggested in the patch. They also tangentially address PR 113552, which is primarily about inconsistent filenames in documentation, but some of the inconsistancy was caused by old code in rc.d/ntpd which is leftover from the intial import from netbsd. There was code to do chroot setup which required the use of the netbsd clockctl(4) device; that code never had any effect on freebsd, because we lack that device and don't build ntpd with the options that would allow using it.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 18077

Event Timeline

• ian created this revision.Jun 28 2018, 6:18 PM

Herald added a reviewer: manpages. · View Herald TranscriptJun 28 2018, 6:18 PM

Herald added a subscriber: imp. · View Herald Transcript

Harbormaster completed remote builds in B17697: Diff 44586.Jun 28 2018, 6:18 PM

BTW, here's some information that took a while to figure out, so I want to capture it for future reference, somewhere other than a .txt file I'm sure to lose over time.

ntpd file accesses before and after chroot'ing

  ntpd's files...

    pidfile    - open/write/close before chroot
    configfile - open/read/close before chroot
    logfile    - open before chroot, write after, never closed/reopened
    driftfile  - open/read/close before, open/write/close after
    leapfile   - open/read/close before and after
    statsdir   - open/write/close files in this dir after chroot

  /etc files...

    /etc/spwd.db         - open/read/close before chroot
    /etc/nsswitch.conf   - open/read/close before chroot
    /etc/group           - open/read/close before and after
    /etc/services        - open/read/close before and after
    /etc/hosts           - open/read/close before and after
    /etc/resolv.conf     - open/read/close after chroot

etc/rc.d/ntpd
61	Oops, typo here, already fixed locally; it'll be nullfs on the next rev.

Thanks for doing this.

etc/rc.d/ntpd
163	This is not related to this review. I have no objection but it needs to be committed in a separate commit.
199	Why the name change?

This revision now requires changes to proceed.Jun 28 2018, 7:46 PM

• ian added inline comments.Jun 28 2018, 7:54 PM

etc/rc.d/ntpd
163	Ooops, yeah, leftover debugging unrelated to these changes, but I do think I'll commit it on it's own since it's useful.
199	I was getting lost in the maze of similar-named ntp-related rcvars, so I decided to name all the ones that are local to this script (rather than coming from rc.conf) to start with an underbar. I might just commit this alone too, so that it doesn't clutter up the next rev of the main diff.

Thanks indeed for the work. OK from manpages.

cy added inline comments.Jun 28 2018, 8:07 PM

etc/rc.d/ntpd
163	Thanks.
199	Yes, alone so it doesn't confuse this change with unrelated changes. Thanks again.

• ian mentioned this in D15987: Make inside<->outside chroot symlinks for all files on the ntpd command line.Jun 29 2018, 2:37 PM

• ian mentioned this in D16281: Make it possible to run ntpd as a non-root user, add ntpd uid and gid..Jul 15 2018, 8:47 PM

While testing and redeveloping the previous version of the rc.d/ntpd changes, I came to the conclusion that automatically setting up a chroot in an rc.d script is complicated and fraught with potential peril. When I got the point of having a embedded awk program that parsed the ntp.conf file so I could figure out whether we needed to mount a devfs inside the chroot for PPS device access, I realized I was way down a too-complicated path.

So now the change is split into two separate reviews. The new D16281 contains everything related to granting ntpd the limited privs it needs to run as non-root, and this part of the change is just the rc.d changes.

It turns out a chroot isn't really necessary to get ntpd running as non-root. In fact, all that's needed to make it do so is to remove a check in ntpd that makes it exit with an error if you run it as non-root. It's much less complicated to automatically set up to run as non-root in rc.d/ntpd if populating a chroot is no longer part of the problem, so that's the approach taken in this latest revision.

The ntpd precmd function now checks whether the admin used any command line or ntp.conf options that affect files and directories, because if these point to non-standard locations, they may not be accessible if running as non-root. If the setup is fairly vanilla, it automatically runs the daemon as user ntpd, otherwise it runs as it always has.

Even non-vanilla setups can easily run as non-root, but the admin will need to make sure that user ntpd has access to the files and directories. The admin can do so, then set ntpd_user=ntpd in rc.conf to run as non-root. I'll look into putting together an update for the ntp chapter of the handbook to document this (as a separate change/review).

Harbormaster completed remote builds in B18053: Diff 45335.Jul 15 2018, 9:16 PM

• ian marked 6 inline comments as done.Jul 15 2018, 9:18 PM

Fix a typo (s/driftopt-/driftopt=/) that made it through my initial testing.

Harbormaster completed remote builds in B18054: Diff 45338.Jul 15 2018, 9:28 PM

Except for a couple of logger -s statements, looks good.

etc/rc.d/ntpd
66	I think a message to syslog to inform the sysadmin that directory permissions have changed should address any POLA concerns. What do you think of daemon.notice? Also to stderr, logger -s.
90	Similarly, inform the sysadmin that files are being moved.

This revision now requires changes to proceed.Jul 16 2018, 5:08 AM

Add requested logging when moving/changing files and dirs.

Add a missing 'then' in a series of if/elif.

Fix a || conditional that should have been && when checking for file keywords in ntp.conf.

Harbormaster completed remote builds in B18077: Diff 45380.Jul 16 2018, 5:58 PM

• ian marked 2 inline comments as done.Jul 16 2018, 5:58 PM

cy accepted this revision.Jul 17 2018, 4:06 AM

This revision is now accepted and ready to land.Jul 17 2018, 4:06 AM

delphij added a subscriber: delphij.Jul 18 2018, 4:38 AM

delphij added inline comments.

etc/rc.d/ntpd
56	If MAC is not present, should it be treated as a fail (e.g. else return 1;;)?

Fix the logic for handling mac(4)-not-in-kernel and mac_ntpd policy not-enabled. The last round of refactoring changed all this stuff to 'early return to run as root' and the logic for these checks got pasted from the old code without change.

This revision now requires review to proceed.Jul 18 2018, 2:29 PM

Harbormaster completed remote builds in B18119: Diff 45461.Jul 18 2018, 2:29 PM

• ian marked an inline comment as done.Jul 18 2018, 2:31 PM

• ian added inline comments.

etc/rc.d/ntpd
56	Oh, good catch. This got screwed up on the latest refactoring. Fixed now, and this time tested as well. :)

LGTM, thanks!

This revision is now accepted and ready to land.Jul 19 2018, 7:10 AM

Closed by commit rS336547: Automatically run ntpd as non-root when possible. (authored by • ian). · Explain WhyJul 20 2018, 1:59 PM

This revision was automatically updated to reflect the committed changes.

• ian marked an inline comment as done.

bdrewery added a subscriber: bdrewery.May 28 2021, 11:35 PM

bdrewery added inline comments.

head/etc/rc.d/ntpd
17–24 ↗	(On Diff #45603)	Why is the pidfile not in /var/run? It persisting through reboot in /var/db means there is a chance it won't start if the value in there is picked up by a new process. Plus removing the -p flag from the rc vars (without something like `ntpd_pidfile`) means users can't fix this mistake.

Automatically run ntpd as non-root when possible.
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 45380

etc/defaults/rc.conf

etc/rc.d/ntpd

Automatically run ntpd as non-root when possible.ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 45380

etc/defaults/rc.conf

etc/rc.d/ntpd

Automatically run ntpd as non-root when possible.
ClosedPublic
Actions

Revision Contents
Changeset List