Page MenuHomeFreeBSD

Limit access to system accounting files
ClosedPublic

Authored by ian on Jul 7 2019, 5:56 PM.

Details

Summary

In 2013 the security chapter of the Handbook was updated in r42501 to suggest limiting access to the system accounting file [*1] by creating the initial file with a mode of 0600. This was in part based on a discussion in the forums [*2]. Unfortunately, this advice is overridden by the fact that a new file is created as part of periodic daily processing, and the file mode is set by the rc.d/accounting script.

These changes update the accounting script to create the directory with mode 0750 if it doesn't already exist, and to create the daily file with mode 0640. This limits write access to root only, read access to root and members of wheel, and eliminates world access completely. For admins who want to prevent even members of wheel from accessing the files, the mode of the /var/account directory can be manually changed to 0700, because the script never creates or changes that directory if it already exists.

The accounting_rotate_log() function now also handles the error cases of no existing log file to rotate, and attempting to rotate the file multiple times (.0 file already exists).

Another small change here eliminates the complexity of the mktemp/chmod/mv sequence for creating a new acct file by just using touch with the appropriate umask to directly create the file with the desired modes. That allows coalescing two separate if checkyesno accounting_enable blocks into one.

These changes were inspired by my investigation of PR 202203.

[1] https://www.freebsd.org/doc/handbook/security-accounting.html
[2] http://forums.freebsd.org/showthread.php?t=41059

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ian created this revision.Jul 7 2019, 5:56 PM
imp added a comment.Jul 7 2019, 6:07 PM

I like these changes.
I had a minor gut reactions that the mode should be settable, but then realized no, it shouldn't.
But what's the group the file gets created as? Or is that adequately covered with the sticky bit in /var/account that we don't mess with that the system manager sets up?

cem added a subscriber: cem.Jul 7 2019, 6:10 PM
cem added inline comments.
libexec/rc/rc.d/accounting
37 ↗(On Diff #59514)

Could use install -m 0640 -g wheel -o root ... insead of umask to explicitly set owner/group to root/wheel.

ian updated this revision to Diff 59515.Jul 7 2019, 6:37 PM

Use install to create the acct file with correct ownership and permissions instead of umask+touch.

ian marked an inline comment as done.Jul 7 2019, 6:39 PM
ian added inline comments.
libexec/rc/rc.d/accounting
37 ↗(On Diff #59514)

Oooo, I like it. Diff updated (and yes, I checked that /usr/bin will be mounted before rc.d/accounting runs).

cem accepted this revision.Jul 7 2019, 6:41 PM

Looks good to me.

libexec/rc/rc.d/accounting
35 ↗(On Diff #59515)

This could be install -d ... as well.

This revision is now accepted and ready to land.Jul 7 2019, 6:41 PM
eadler accepted this revision as: eadler.Jul 10 2019, 4:58 AM
rwatson accepted this revision.Jul 12 2019, 5:46 PM
rwatson added a subscriber: rwatson.

Although this is not a code review (best not to rely on my sh skills), I think this is generally a laudable change. I wonder if any man-page update is due to lastcomm, acct, etc., do mention the need for wheel group to access the files?

ian marked an inline comment as done.Jul 13 2019, 4:07 PM
This revision was automatically updated to reflect the committed changes.