Skip to content

Commit

Permalink
confd: fix audit logs using proper facility
Browse files Browse the repository at this point in the history
The LOG_SECURITY facility was set wrong (1 << 13) instead of (13 << 3), see
https://github.com/kernelkit/sysklogd/blob/0fc6656/src/syslog.h#L120 for
details.  This caused all audit log messages to be logged in LOG_USER.

Also, rename LOG_SECURITY -> LOG_AUDIT and log macro SECURITY() -> AUDIT()
to match RFC5424 terminology.

Similar fix to sysrepo, LOG_AUDIT facility instead of daemon + WARNING.
Additionally, drop the leading [severity] prefix to sysrepo logs.  Only
needed when logging to stdout.

Follow-up to issue #521

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
  • Loading branch information
troglobit committed Aug 30, 2024
1 parent 7d95da7 commit 8a09b3e
Show file tree
Hide file tree
Showing 5 changed files with 77 additions and 45 deletions.
Original file line number Diff line number Diff line change
@@ -1,42 +1,74 @@
From da765b90bca45b91f72fd6525e680040eebd2d4b Mon Sep 17 00:00:00 2001
From d128686fb15833e815ac3dd04bd87d3725c881ac Mon Sep 17 00:00:00 2001
From: Joachim Wiberg <troglobit@gmail.com>
Date: Wed, 21 Aug 2024 16:00:35 +0200
Subject: [PATCH 7/9] Introduce new log level [SEC]
Subject: [PATCH 7/9] Introduce new log level [SEC] for audit trails
Organization: Addiva Elektronik

This adds a new log level for security and audit trail related log
messages. E.g., nacm user applied a change, copied a ds, etc.
messages, see LOG_AUDIT defined in RFC5424. E.g., nacm user applied
a change, copied a datastore, etc.

The new log level is added last to not affect the advertised command
line log levels. A security notice has higher actual priorty than
DBG, of course, so we remap it to WRN. The construct allows us to
have another [label] than [WRN], which might otherwise be read as
a bug/problem rather than just a high-priority-notification.

When logging to syslog() we let delegate labling and filtering to the
system log daemon, dropping any [SEVERITY] prefix. Also, \n is most
often dropped by log daemons.

Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
---
src/log.c | 5 +++++
src/log.h | 1 +
src/sysrepo_types.h | 3 ++-
tests/tcommon.c | 3 +++
4 files changed, 11 insertions(+), 1 deletion(-)
src/log.c | 18 +++++++++++++++++-
src/log.h | 1 +
src/sysrepo_types.h | 3 ++-
tests/tcommon.c | 3 +++
4 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/src/log.c b/src/log.c
index e15055ac..b89ffacf 100644
index e15055ac..25eab8fa 100644
--- a/src/log.c
+++ b/src/log.c
@@ -122,6 +122,11 @@ sr_log_msg(int plugin, sr_log_level_t ll, const char *msg)
@@ -30,6 +30,10 @@

#include "config.h"

+#ifndef LOG_AUDIT
+#define LOG_AUDIT (13<<3) /* Log audit, for audit trails */
+#endif
+
sr_log_level_t sr_stderr_ll = SR_LL_NONE; /**< stderr log level */
sr_log_level_t sr_syslog_ll = SR_LL_NONE; /**< syslog log level */
int syslog_open; /**< Whether syslog was opened */
@@ -122,6 +126,11 @@ sr_log_msg(int plugin, sr_log_level_t ll, const char *msg)
priority = LOG_INFO;
severity = "INF";
break;
+ case SR_LL_SEC:
+ priority = LOG_WARNING;
+ priority = LOG_AUDIT | LOG_NOTICE;
+ severity = "SEC";
+ ll = SR_LL_WRN; /* remap to higher level. */
+ break;
case SR_LL_DBG:
priority = LOG_DEBUG;
severity = "DBG";
@@ -138,7 +147,14 @@ sr_log_msg(int plugin, sr_log_level_t ll, const char *msg)

/* syslog logging */
if (ll <= sr_syslog_ll) {
- syslog(priority | (plugin ? LOG_DAEMON : 0), "[%s] %s\n", severity, msg);
+ int facility;
+
+ if (priority & ~LOG_PRIMASK)
+ facility = 0; /* audit */
+ else
+ facility = plugin ? LOG_DAEMON : 0;
+
+ syslog(facility | priority, "%s", msg);
}

/* logging callback */
diff --git a/src/log.h b/src/log.h
index d7e65b88..8722e51d 100644
--- a/src/log.h
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
From 7e49f394e0afedc0259d4364f5a9a83296fe2b72 Mon Sep 17 00:00:00 2001
From b5db2b36c06d28918f0d00dda945f7e943b470af Mon Sep 17 00:00:00 2001
From: Joachim Wiberg <troglobit@gmail.com>
Date: Wed, 21 Aug 2024 16:04:43 +0200
Subject: [PATCH 8/9] Add audit trail for high priority system changes
Expand All @@ -18,7 +18,7 @@ Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
1 file changed, 12 insertions(+)

diff --git a/src/sysrepo.c b/src/sysrepo.c
index 86d694e5..c7b97e53 100644
index 86d694e5..dcea51e8 100644
--- a/src/sysrepo.c
+++ b/src/sysrepo.c
@@ -3946,6 +3946,9 @@ store:
Expand All @@ -45,7 +45,7 @@ index 86d694e5..c7b97e53 100644
}
}

+ if (session->nacm_user)
+ if (session->nacm_user && src_datastore != SR_DS_CANDIDATE)
+ SR_LOG_SEC("user \"%s\" copied %s to %s", session->nacm_user, sr_ds2str(src_datastore), sr_ds2str(session->ds));
+
cleanup:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
From 5c4df9f535f6fbdc6dc0573a47c47be2d02f1774 Mon Sep 17 00:00:00 2001
From 13d54101eb2c3506237cebc5be99edef2ad669cd Mon Sep 17 00:00:00 2001
From: Joachim Wiberg <troglobit@gmail.com>
Date: Fri, 23 Aug 2024 12:22:06 +0200
Subject: [PATCH 9/9] On error in sr_shmsub_listen_thread(), exit process
Expand Down
54 changes: 27 additions & 27 deletions src/confd/src/ietf-system.c
Original file line number Diff line number Diff line change
Expand Up @@ -607,9 +607,9 @@ static void add_group(const char *user, const char *group)
return; /* already group member */

if (systemf("adduser %s %s", user, group))
SECURITY("Failed giving user %s UNIX %s permissions.", user, group);
AUDIT("Failed giving user \"%s\" UNIX %s permissions.", user, group);
else
SECURITY("User %s added to UNIX %s group.", user, group);
AUDIT("User \"%s\" added to UNIX \"%s\" group.", user, group);
}

static void del_group(const char *user, const char *group)
Expand All @@ -620,9 +620,9 @@ static void del_group(const char *user, const char *group)
return; /* not member of group */

if (systemf("delgroup %s %s", user, group))
SECURITY("Failed removing user %s from UNIX %s group.", user, group);
AUDIT("Failed removing user \"%s\" from UNIX \"%s\" group.", user, group);
else
SECURITY("User %s removed from UNIX %s group.", user, group);
AUDIT("User \"%s\" removed from UNIX \"%s\" group.", user, group);
}

/* Users with a valid shell are also allowed CLI access */
Expand Down Expand Up @@ -786,7 +786,7 @@ static int sys_call_adduser(sr_session_ctx_t *sess, char *name, uid_t uid, gid_t
char **args;
int err;

DEBUG("Adding new user %s, cleaning up any stale group.", name);
DEBUG("Adding new user \"%s\", cleaning up any stale group.", name);
systemf("delgroup %s 2>/dev/null", name);

/* reusing existing uid:gid from $HOME */
Expand Down Expand Up @@ -840,15 +840,15 @@ static int sys_add_user(sr_session_ctx_t *sess, char *name)
/* Verify IDs aren't already used, like BusyBox adduser */
if (getpwuid(st.st_uid) || getgrgid(st.st_uid) || getgrgid(st.st_gid)) {
/* Exists but owned by someone else. */
SECURITY("Failed mapping user %s to /home/%s, uid:gid (%d:%d) already exists.",
AUDIT("Failed mapping user \"%s\" to /home/%s, uid:gid (%d:%d) already exists.",
name, name, st.st_uid, st.st_gid);
err = sys_call_adduser(sess, name, 0, 0);
} else {
SECURITY("Reusing uid:gid %d:%d and /home/%s for new user %s",
AUDIT("Reusing uid:gid %d:%d and /home/%s for new user \"%s\"",
st.st_uid, st.st_gid, name, name);
err = sys_call_adduser(sess, name, st.st_uid, st.st_gid);
if (err) {
SECURITY("Failed reusing uid:gid from /home/%s, retrying create user ...", name);
AUDIT("Failed reusing uid:gid from /home/%s, retrying create user ...", name);
err = sys_call_adduser(sess, name, 0, 0);
} else
reused = true;
Expand All @@ -857,11 +857,11 @@ static int sys_add_user(sr_session_ctx_t *sess, char *name)
err = sys_call_adduser(sess, name, 0, 0);

if (err) {
SECURITY("Failed creating new user \"%s\"", name);
AUDIT("Failed creating new user \"%s\"", name);
return SR_ERR_SYS;
}

SECURITY("User \"%s\" created%s.", name, reused ? ", mapped to existing home directory" : "");
AUDIT("User \"%s\" created%s.", name, reused ? ", mapped to existing home directory" : "");

/*
* OpenSSH in Infix has been set up to use /var/run/sshd/%s.keys
Expand Down Expand Up @@ -930,7 +930,7 @@ static int set_shell(const char *user, const char *shell)

if (!strcmp(pw->pw_name, user)) {
if (strcmp(pw->pw_shell, shell))
NOTE("Updating login shell for user %s to %s", user, shell);
AUDIT("Updating login shell for user \"%s\" to %s", user, shell);

upw = *pw;
upw.pw_shell = (char *)shell;
Expand All @@ -953,7 +953,7 @@ static int set_shell(const char *user, const char *shell)
if (fp)
fclose(fp);
endpwent();
ERRNO("Failed setting user %s shell %s", user, shell);
ERRNO("Failed setting user \"%s\" shell %s", user, shell);

return -1;
}
Expand All @@ -971,7 +971,7 @@ static int set_password(const char *user, const char *hash, bool lock)

fp = fopen(_PATH_SHADOW "+", "w");
if (!fp) {
ERRNO("Failed opening %s+ for %s", _PATH_SHADOW, user);
ERRNO("Failed opening %s+ for user \"%s\"", _PATH_SHADOW, user);
goto fail;
}
fd = fileno(fp);
Expand Down Expand Up @@ -1016,7 +1016,7 @@ static int set_password(const char *user, const char *hash, bool lock)
endspent();
ulckpwdf();
exit:
SECURITY("Failed setting password for %s", user);
AUDIT("Failed setting password for user \"%s\"", user);

return -1;
}
Expand Down Expand Up @@ -1045,7 +1045,7 @@ static const char *is_valid_hash(struct confd *confd, const char *user, const ch

pwd = json_object_get(confd->root, "factory-password-hash");
if (!json_is_string(pwd)) {
EMERG("Cannot find factory-default password hash for user %s!", user);
EMERG("Cannot find factory-default password hash for user \"%s\"!", user);
return NULL;
}

Expand All @@ -1072,7 +1072,7 @@ static sr_error_t handle_sr_passwd_update(sr_session_ctx_t *, struct confd *conf
assert(change->new);

if (change->new->type != SR_STRING_T) {
SECURITY("Internal error, expected user %s password to be string type.", user);
AUDIT("Internal error, expected user \"%s\" password to be string type.", user);
err = SR_ERR_INTERNAL;
break;
}
Expand All @@ -1090,17 +1090,17 @@ static sr_error_t handle_sr_passwd_update(sr_session_ctx_t *, struct confd *conf
if (set_password(user, hash, lock))
err = SR_ERR_SYS;
else if (lock)
NOTE("User account %s locked.", user);
NOTE("User account \"%s\" locked.", user);
else if (!strcmp(hash, "*"))
NOTE("Password login disabled for user %s", user);
NOTE("Password login disabled for user \"%s\"", user);
else
SECURITY("Password updated for user %s", user);
AUDIT("Password updated for user \"%s\"", user);
break;
case SR_OP_DELETED:
if (set_password(user, "*", false))
err = SR_ERR_SYS;
else
NOTE("Password login disabled for user %s", user);
NOTE("Password login disabled for user \"%s\"", user);
break;
case SR_OP_MOVED:
break;
Expand All @@ -1125,10 +1125,10 @@ static sr_error_t handle_sr_shell_update(sr_session_ctx_t *sess, struct confd *c

shell = sys_find_usable_shell(sess, (char *)user, is_admin_user(sess, user));
if (set_shell(user, shell)) {
SECURITY("Failed updating shell to %s for user %s", shell, user);
AUDIT("Failed updating shell to %s for user \"%s\"", shell, user);
err = SR_ERR_SYS;
} else {
SECURITY("Login shell updated for user %s", user);
AUDIT("Login shell updated for user \"%s\"", user);
err = SR_ERR_OK;
}
free(shell);
Expand All @@ -1148,7 +1148,7 @@ static sr_error_t check_sr_user_update(sr_session_ctx_t *, struct confd *, struc

name = sr_xpath_key_value(val->xpath, "user", "name", &state);
if (!is_valid_username(name)) {
SECURITY("Invalid username \"%s\"", name);
AUDIT("Invalid username \"%s\"", name);
return SR_ERR_VALIDATION_FAILED;
}
NOTE("Username \"%s\" is valid", name);
Expand Down Expand Up @@ -1232,7 +1232,7 @@ static sr_error_t generate_auth_keys(sr_session_ctx_t *session, const char *xpat

fp = fopenf("w", "/var/run/sshd/%s.keys", username);
if (!fp) {
ERROR("failed opening %s authorized_keys file: %s", username, strerror(errno));
ERROR("failed opening user \"%s\" authorized_keys file: %s", username, strerror(errno));
continue;
}

Expand Down Expand Up @@ -1361,7 +1361,7 @@ static sr_error_t change_auth_done(struct confd *confd, sr_session_ctx_t *sessio

err = generate_auth_keys(session, XPATH_AUTH_"/user//.");
if (err) {
SECURITY("failed saving authorized-key data.");
AUDIT("failed saving authorized-key data.");
goto cleanup;
}

Expand Down Expand Up @@ -1432,11 +1432,11 @@ static int change_nacm(sr_session_ctx_t *session, uint32_t sub_id, const char *m
for (size_t i = 0; i < user_count; i++) {
const char *user = users[i].data.string_val;
bool is_admin = is_admin_user(session, user);
char *shell;
const char *shell;

shell = sys_find_usable_shell(session, (char *)user, is_admin);
if (set_shell(user, shell))
SECURITY("Failed adjusting shell for user %s", user);
AUDIT("Failed adjusting shell for user \"%s\"", user);

if (is_admin)
add_group(user, "wheel");
Expand Down
6 changes: 3 additions & 3 deletions src/libsrx/src/common.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@
extern int debug;

/* In IETF referred to LOG_AUDIT */
#ifndef LOG_SECURITY
#define LOG_SECURITY (1 << 13)
#ifndef LOG_AUDIT
#define LOG_AUDIT (13<<3) /* Log audit, for audit trails */
#endif

#ifndef HAVE_VASPRINTF
Expand All @@ -33,6 +33,6 @@ int asprintf(char **strp, const char *fmt, ...);
#define EMERG(fmt, ...) syslog(LOG_EMERG, fmt, ##__VA_ARGS__)
#define ERROR(fmt, ...) syslog(LOG_ERR, fmt, ##__VA_ARGS__)
#define ERRNO(fmt, ...) syslog(LOG_ERR, fmt ": %s", ##__VA_ARGS__, strerror(errno))
#define SECURITY(fmt, ...) syslog(LOG_SECURITY | LOG_NOTICE, fmt, ##__VA_ARGS__)
#define AUDIT(fmt, ...) syslog(LOG_AUDIT | LOG_NOTICE, fmt, ##__VA_ARGS__)

#endif /* CONFD_COMMON_H_ */

0 comments on commit 8a09b3e

Please sign in to comment.