From acdf044438d5f4d1031684088114366a2d309813 Mon Sep 17 00:00:00 2001
From: ian <ian@FreeBSD.org>
Date: Sat, 13 Jul 2019 16:07:38 +0000
Subject: [PATCH] Limit access to system accounting files.

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 using install(1) with the flags
needed to directly create the file with the desired ownership and
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

PR:		202203
Differential Revision:	https://reviews.freebsd.org/D20876
---
 libexec/rc/rc.d/accounting | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/libexec/rc/rc.d/accounting b/libexec/rc/rc.d/accounting
index 0c7ac36c0e81..dd33c2f1f07b 100755
--- a/libexec/rc/rc.d/accounting
+++ b/libexec/rc/rc.d/accounting
@@ -21,23 +21,27 @@ start_cmd="accounting_start"
 stop_cmd="accounting_stop"
 rotate_log_cmd="accounting_rotate_log"
 
+create_accounting_file()
+{
+	install -o root -g wheel -m 0640 /dev/null "${accounting_file}"
+}
+
 accounting_start()
 {
 	local _dir
 
 	_dir="${accounting_file%/*}"
 	if [ ! -d "$_dir" ]; then
-		if ! mkdir -p "$_dir"; then
+		if ! mkdir -p -m 0750 "$_dir"; then
 			err 1 "Could not create $_dir."
 		fi
 	fi
 
 	if [ ! -e "$accounting_file" ]; then
 		echo -n "Creating accounting file ${accounting_file}"
-		touch "$accounting_file"
+		create_accounting_file
 		echo '.'
 	fi
-	chmod 644 "$accounting_file"
 
 	echo "Turning on accounting."
 	${accounting_command} ${accounting_file}
@@ -51,21 +55,24 @@ accounting_stop()
 
 accounting_rotate_log()
 {
-	local _dir _file
+	# Note that this function must handle being called as "onerotate_log"
+	# (by the periodic scripts) when accounting is disabled, and handle
+	# being called multiple times (by an admin making mistakes) without
+	# anything having actually rotated the old .0 file out of the way.
 
-	_dir="${accounting_file%/*}"
-	cd $_dir
+	if [ -e "${accounting_file}.0" ]; then
+		err 1 "Cannot rotate accounting log, ${accounting_file}.0 already exists."
+	fi
 
-	if checkyesno accounting_enable; then
-		_file=`mktemp newacct-XXXXX`
-		chmod 644 $_file
-		${accounting_command} ${_dir}/${_file}
+	if [ ! -e "${accounting_file}" ]; then
+		err 1 "Cannot rotate accounting log, ${accounting_file} does not exist."
 	fi
 
 	mv ${accounting_file} ${accounting_file}.0
 
 	if checkyesno accounting_enable; then
-		mv $_file ${accounting_file}
+		create_accounting_file
+		${accounting_command} "${accounting_file}"
 	fi
 }