9:12 p.m.
When building with -D_FORTIFY_SOURCE=2 and -W-unused-return, there are a
number of warnings caused by return values of functions marked with the
warn_unused_result attribute being ignored. The audit codebase makes an
attempt to suppress these warnings by casting the return value to void, but
that does not work when D_FORITY_SOURCE is in use.
Here's an explanation of how this patch fixes the warnings and how the
potential error conditions are handled:
Errors writing to the auditd pid file should be logged since errors opening
the pid file are logged. These write() errors aren't treated as fatal.
Problems adjusting auditd's out of memory score should be logged, if simply
to catch a change to the kernel interface. These errors aren't treated as
fatal.
Auditd refuses to start when nice() fails during initialization, so it should
take disk_error_action whenever nice() fails during a reconfigure.
Failure to chdir("/") while daemonizing should be logged and treated as fatal
since errors while redirecting stdin, stdout, and stderr are logged and
considered fatal.
All nice() return values are handled sufficiently by relying on errno.
However, they still throw warnings when D_FORTIFY_SOURCE is used. This patch
quiets those warnings by capturing the return value and using it and errno to
determine if nice() failed.
Failure to adjust audit log file owner (fchown) and permissions (fchmod) are
logged and considered fatal when opening the log file for the first time. They
are not treated as fatal when the operations fail on during log rotation since
we made sure that they file owner and permissions were correct when originally
opening the log file.
Signed-off-by: Tyler Hicks <tyhicks(a)canonical.com>
---
audisp/audispd.c | 6 ++++--
src/auditd-event.c | 45 +++++++++++++++++++++++++++++++++++++--------
src/auditd.c | 52 +++++++++++++++++++++++++++++++++-------------------
src/autrace.c | 6 +++++-
4 files changed, 79 insertions(+), 30 deletions(-)
diff --git a/audisp/audispd.c b/audisp/audispd.c
index 749128f..0ef4b8e 100644
--- a/audisp/audispd.c
+++ b/audisp/audispd.c
@@ -369,9 +369,11 @@ int main(int argc, char *argv[])
/* Now boost priority to make sure we are getting time slices */
if (daemon_config.priority_boost != 0) {
+ int rc;
+
errno = 0;
- (void) nice((int)-daemon_config.priority_boost);
- if (errno) {
+ rc = nice((int)-daemon_config.priority_boost);
+ if (rc == -1 && errno) {
syslog(LOG_ERR, "Cannot change priority (%s)",
strerror(errno));
/* Stay alive as this is better than stopping */
diff --git a/src/auditd-event.c b/src/auditd-event.c
index acf5aa1..2965be3 100644
--- a/src/auditd-event.c
+++ b/src/auditd-event.c
@@ -708,10 +708,18 @@ static void rotate_logs(struct auditd_consumer_data *data,
if (data->config->num_logs < 2)
return;
- /* Close audit file */
- fchmod(data->log_fd,
- data->config->log_group ? S_IRUSR|S_IRGRP : S_IRUSR);
- fchown(data->log_fd, 0, data->config->log_group);
+ /* Close audit file. fchmod and fchown errors are not fatal because we
+ * already adjusted log file permissions and ownership when opening the
+ * log file. */
+ if (fchmod(data->log_fd, data->config->log_group ? S_IRUSR|S_IRGRP :
+ S_IRUSR) < 0) {
+ audit_msg(LOG_NOTICE, "Couldn't change permissions while "
+ "rotating log file (%s)", strerror(errno));
+ }
+ if (fchown(data->log_fd, 0, data->config->log_group) < 0) {
+ audit_msg(LOG_NOTICE, "Couldn't change ownership while "
+ "rotating log file (%s)", strerror(errno));
+ }
fclose(data->log_file);
/* Rotate */
@@ -924,9 +932,20 @@ retry:
return 1;
}
}
- fchmod(lfd, data->config->log_group ? S_IRUSR|S_IWUSR|S_IRGRP :
- S_IRUSR|S_IWUSR);
- fchown(lfd, 0, data->config->log_group);
+ if (fchmod(lfd, data->config->log_group ? S_IRUSR|S_IWUSR|S_IRGRP :
+ S_IRUSR|S_IWUSR) < 0) {
+ audit_msg(LOG_ERR,
+ "Couldn't change permissions of log file (%s)",
+ strerror(errno));
+ close(lfd);
+ return 1;
+ }
+ if (fchown(lfd, 0, data->config->log_group) < 0) {
+ audit_msg(LOG_ERR, "Couldn't change ownership of log file (%s)",
+ strerror(errno));
+ close(lfd);
+ return 1;
+ }
data->log_fd = lfd;
data->log_file = fdopen(lfd, "a");
@@ -1089,8 +1108,18 @@ static void reconfigure(struct auditd_consumer_data *data)
// priority boost
if (oconf->priority_boost != nconf->priority_boost) {
+ int rc;
+
oconf->priority_boost = nconf->priority_boost;
- nice(-oconf->priority_boost);
+ errno = 0;
+ rc = nice(-oconf->priority_boost);
+ if (rc == -1 && errno) {
+ int saved_errno = errno;
+ audit_msg(LOG_NOTICE, "Cannot change priority in "
+ "reconfigure (%s)", strerror(errno));
+ do_disk_error_action("reconfig", data->config,
+ saved_errno);
+ }
}
// log format
diff --git a/src/auditd.c b/src/auditd.c
index 84fdea2..52ec5df 100644
--- a/src/auditd.c
+++ b/src/auditd.c
@@ -240,7 +240,7 @@ int send_audit_event(int type, const char *str)
static int write_pid_file(void)
{
- int pidfd, len;
+ int pidfd, len, rc;
char val[16];
len = snprintf(val, sizeof(val), "%u\n", getpid());
@@ -256,29 +256,38 @@ static int write_pid_file(void)
pidfile = 0;
return 1;
}
- (void)write(pidfd, val, (unsigned int)len);
+ if (write(pidfd, val, (unsigned int)len) != len) {
+ audit_msg(LOG_ERR, "Unable to write pidfile (%s)",
+ strerror(errno));
+ close(pidfd);
+ pidfile = 0;
+ return 1;
+ }
close(pidfd);
return 0;
}
static void avoid_oom_killer(void)
{
- int oomfd;
+ int oomfd, len, rc;
+ char *score = NULL;
/* New kernels use different technique */
- oomfd = open("/proc/self/oom_score_adj", O_NOFOLLOW | O_WRONLY);
- if (oomfd >= 0) {
- (void)write(oomfd, "-1000", 5);
- close(oomfd);
- return;
- }
- oomfd = open("/proc/self/oom_adj", O_NOFOLLOW | O_WRONLY);
- if (oomfd >= 0) {
- (void)write(oomfd, "-17", 3);
- close(oomfd);
- return;
- }
- // Old style kernel...perform another action here
+ if ((oomfd = open("/proc/self/oom_score_adj",
+ O_NOFOLLOW | O_WRONLY)) >= 0) {
+ score = "-1000";
+ } else if ((oomfd = open("/proc/self/oom_adj",
+ O_NOFOLLOW | O_WRONLY)) >= 0) {
+ score = "-17";
+ } else
+ audit_msg(LOG_NOTICE, "Cannot open out of memory adjuster");
+
+ len = strlen(score);
+ rc = write(oomfd, score, len);
+ if (rc != len)
+ audit_msg(LOG_NOTICE, "Unable to adjust out of memory score");
+
+ close(oomfd);
}
/*
@@ -328,7 +337,12 @@ static int become_daemon(void)
close(fd);
/* Change to '/' */
- chdir("/");
+ rc = chdir("/");
+ if (rc < 0) {
+ audit_msg(LOG_ERR,
+ "Cannot change working directory to /");
+ return -1;
+ }
/* Become session/process group leader */
setsid();
@@ -540,8 +554,8 @@ int main(int argc, char *argv[])
if (config.priority_boost != 0) {
errno = 0;
- (void) nice((int)-config.priority_boost);
- if (errno) {
+ rc = nice((int)-config.priority_boost);
+ if (rc == -1 && errno) {
audit_msg(LOG_ERR, "Cannot change priority (%s)",
strerror(errno));
return 1;
diff --git a/src/autrace.c b/src/autrace.c
index e124660..47c6c4f 100644
--- a/src/autrace.c
+++ b/src/autrace.c
@@ -245,7 +245,11 @@ int main(int argc, char *argv[])
exit(1);
}
sleep(1);
- (void)write(fd[1],"1", 1);
+ if (write(fd[1],"1", 1) != 1) {
+ kill(pid,SIGTERM);
+ (void)delete_all_rules(audit_fd);
+ exit(1);
+ }
waitpid(pid, NULL, 0);
close(fd[1]);
puts("Cleaning up...");
--
1.7.10.4