Fix audit records #1188
Fix audit records #1188
Conversation
The audit system only recognizes key=value word pairs. It uses the first
whitespace after the '=' to determine the end of the value associated
with the field name. The op field contains multiple words describing what
operation is being performed on the user account. However, due to white
space between the words, the audit parser cannot get the whole operation
description as intended.
This patch is the least invasive way to fix the problem. What it does is
replace white space in the op field with dashes soo that the parser keeps
all of the words togther. Below are before and after events:
type=ADD_GROUP msg=audit(01/18/2025 13:50:27.903:685) : pid=116430 uid=root
auid=sgrubb ses=3 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
msg='op=adding group acct=test exe=/home/sgrubb/shadow-utils/src/useradd
hostname=x2 addr=? terminal=pts/1 res=success'
type=ADD_GROUP msg=audit(01/18/2025 13:56:45.031:709) : pid=107681 uid=root
auid=sgrubb ses=3 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023
msg='op=adding-group acct=test1 exe=/home/sgrubb/shadow-utils/src/useradd
hostname=x2 addr=? terminal=pts/1 res=success'
To show the effect using auformat which captures just the field asked for:
Before:
ausearch --start 13:50 -m ADD_USER --format raw | \
/home/sgrubb/test/auformat "%OP\n"
adding
After:
ausearch --start 13:55 -m ADD_USER --format raw | \
/home/sgrubb/test/auformat "%OP\n"
adding-user
lib/audit_help.c
Outdated
| */ | ||
| char *fixed_op = strreplace (strdup (op), ' ', '-'); | ||
| audit_log_acct_message (audit_fd, type, NULL, | ||
| fixed_op ? fixed_op : op, name, |
There was a problem hiding this comment.
I don't think it's a good idea to fall back to op on allocation error. This means that two formats can coexist in the same file, and software (and humans) must expect both (but one will be rare enough that humans will forget to search for it).
I think I would rather report the error to the caller (and maybe abort the program).
There was a problem hiding this comment.
If you make sure that op is always passed as a string literal, maybe you could use strdupa(3).
To make sure that an argument is a string literal, you could use this code:
https://github.com/shadow-maint/shadow/pull/1189/files
(we can merge it before this one, so that you can use it in your patch.) Cc: @hallyn, @ikerexxe
For now, you can cherry-pick that commit in your branch.
Then, you could define this:
#define audit_logger(..., op, ...) audit_logger_(..., STRDUPA(op), ...)
void audit_logger_(...)
{...}
lib/audit_help.c
Outdated
| /* | ||
| * This takes a string and replaces the old character with the new. | ||
| */ | ||
| static char *strreplace (char *str, char old, char new) |
There was a problem hiding this comment.
I'd put this API under lib/string/strtr/. We already have an extense string library:
$ find lib/string/
lib/string/
lib/string/strftime.c
lib/string/strtok
lib/string/strtok/stpsep.c
lib/string/strtok/stpsep.h
lib/string/strftime.h
lib/string/strchr
lib/string/strchr/strchrcnt.h
lib/string/strchr/stpspn.h
lib/string/strchr/strrspn.h
lib/string/strchr/strnul.h
lib/string/strchr/strrspn.c
lib/string/strchr/strnul.c
lib/string/strchr/strchrcnt.c
lib/string/strchr/stpspn.c
lib/string/memset
lib/string/memset/memzero.c
lib/string/memset/memzero.h
lib/string/strcmp
lib/string/strcmp/streq.h
lib/string/strcmp/streq.c
lib/string/strcpy
lib/string/strcpy/stpecpy.c
lib/string/strcpy/strtcpy.h
lib/string/strcpy/strncpy.c
lib/string/strcpy/stpecpy.h
lib/string/strcpy/strncat.c
lib/string/strcpy/strtcpy.c
lib/string/strcpy/strncpy.h
lib/string/strcpy/strncat.h
lib/string/sprintf
lib/string/sprintf/xasprintf.h
lib/string/sprintf/xasprintf.c
lib/string/sprintf/stpeprintf.c
lib/string/sprintf/snprintf.h
lib/string/sprintf/snprintf.c
lib/string/sprintf/stpeprintf.h
lib/string/strdup
lib/string/strdup/xstrndup.c
lib/string/strdup/strndupa.h
lib/string/strdup/xstrdup.c
lib/string/strdup/xstrdup.h
lib/string/strdup/strndupa.c
lib/string/strdup/xstrndup.h
Made the following changes based on feedback: Changed the function name to strtr, drop strdup/free in favor of alloca, changed while loop to a for loop, and added missing inline attribute.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
I made a few changes. Changed the function name to strtr, drop strdup/free in favor of alloca, changed while loop to a for loop, and added missing inline attribute. I would not suggest adding a 1 use function to a library. If something else uses, sure. I also wonder if putting string functions in a library will lose the FORTIFY_SOURCE protection they have if used directly? I also like keeping C code readable without having to look up macros. (I worked on Xinetd back in the day and you cannot understand that code without digging through many header files.) MUSL C also does not have strdupa. So, I work around that with alloca + strcpy. Nothing should fail now. I also don't like the idea of aborting in the middle of a multi-step process. You have a user half installed or deleted. This should work and is readable. |
Thanks!
That's just an internal library. Don't worry. I prefer having it more organized there. Anyway, we make those APIs inline, so in the end the compiler will inline it. But in the library it's more organized, and we know we have the API just by looking at the library file structure.
I don't think so. glibc is a library, and code calling it is still fortified. Anyway, we inline all of our string APIs, so it's as if it had been defined in the translation units.
We have a quite consistent rule in this project that macros with upper-case names that are called like a function are thin wrappers that only add type checks. In many cases, we also implicitly calculate a size: $ grepc STRNCPY .
./lib/string/strcpy/strncpy.h:#define STRNCPY(dst, src) strncpy(dst, src, NITEMS(dst))This STRDUPA() is consistent with this pattern, so I'd find it easy to read.
|
lib/audit_help.c
Outdated
| * be replaced with dashes so that parsers get the whole | ||
| * field. Not all C libraries have strdupa. | ||
| */ | ||
| char *tmp_op = alloca (strlen (op) + 1); |
There was a problem hiding this comment.
strlen() + 1 doesn't enforce a string literal.
Using alloca(3) with arbitrary input is dangerous.
Please add a macro that validates the input and calls alloca().
There was a problem hiding this comment.
These are hard-coded strings embedded in the shadow-utils code. They cannot be attacker influenced.
There was a problem hiding this comment.
Yeah, but we might forget and change some of those in the future. That's why I wanted a STRDUPA() macro that enforces that the input is a string literal.
Don't check for NULL string being passed, put returned type on a separate line, use streq, remove some curly braces, use strdupa, and constify the returned pointer.
|
New updates pushed. |
lib/audit_help.c
Outdated
| static inline const char * | ||
| strtr(char *str, char old, char new) | ||
| { | ||
| for (char *p = str; streq(p, ""); p++) { |
There was a problem hiding this comment.
Oops, sorry, I should have said !streq(p, ""), that is, while the string is not equal to the empty string, iterate over it.
Here are cases where we use it:
$ grep -rn for.*streq
lib/string/strchr/strchrcnt.h:26: for (; !streq(s, ""); s++) {
lib/fputsx.c:50: for (i = 0; !streq(s, ""); i++, s++) {
lib/port.c:218: for (j = 0; !streq(cp, "") && (j < PORT_TIMES); j++) {
lib/getdate.y:630: for (p = buff; !streq(p, ""); p++)
lib/getdate.y:723: for (i = 0, p = q = buff; !streq(q, ""); q++)
lib/fields.c:51: for (cp = field; !streq(cp, ""); cp++) {
lib/gshadow.c:45: for (i = 0; s != NULL && !streq(s, ""); i++)
lib/obscure.c:85: for (cp = string; !streq(cp, ""); cp++) {| * field. | ||
| */ | ||
| const char *fixed_op = strtr(strdupa(op), ' ', '-'); | ||
| audit_log_acct_message(audit_fd, type, NULL, |
There was a problem hiding this comment.
I've been thinking...
If this is a string literal that we absolutely control, and we only use it for this, why not just change the literals accordingly?
If the string literal is exactly the same as what is logged, the users will be able to grep the source code for the string that they see in the logs.
There was a problem hiding this comment.
This reason it was done this way is because there are about 200 places to change. This patch is an attempt to fix the issue with the least amount of code churn. Long term that is the fix. But I don't have the time to do the bigger fix any time soon.
There was a problem hiding this comment.
You could probably start with this:
$ find -type f \
| grep '\.c$' \
| xargs pcre2grep -hM '(?s)\baudit_logger *\([^{};]*\);' \
| sed 's/^\s*//' \
| tr '\n' ' ' \
| sed 's/; /;\n/g' \
| cut -d, -f3 \
| sort \
| uniq;
""
"adding SELinux user mapping"
"adding group to /etc/group"
"adding group to /etc/gshadow"
"adding group"
"adding home directory"
"adding shadow password"
"adding user to /etc/passwd"
"adding user to /etc/shadow"
"adding user to group"
"adding user to shadow group"
"adding user"
"change age"
"change all aging information"
"change inactive days"
"change last change date"
"change max age"
"change min age"
"change passwd expiration"
"change passwd warning"
"changing admin name in shadow group"
"changing comment"
"changing expiration date"
"changing group member"
"changing home directory owner"
"changing home directory"
"changing inactive days"
"changing mail file name"
"changing mail file owner"
"changing member in shadow group"
"changing name"
"changing password"
"changing primary group"
"changing uid"
"changing user shell"
"changing useradd defaults"
"changing"
"clearing-lastlog"
"deleting group"
"deleting home directory"
"deleting mail file"
"deleting shadow group"
"deleting user entries"
"deleting user from group"
"deleting user from shadow group"
"deleting user logged in"
"deleting user not found"
"deleting user"
"display aging info"
"locking group file"
"locking password file"
"locking shadow group file"
"locking shadow password file"
"locking subordinate group file"
"locking subordinate user file"
"modifying User mapping "
"modifying account"
"modifying group"
"moving home directory"
"opening group file"
"opening password file"
"opening shadow group file"
"opening shadow password file"
"opening subordinate group file"
"opening subordinate user file"
"refreshing-lastlog"
"removing SELinux user mapping"
"removing group from /etc/group"
"removing group from /etc/gshadow"
"removing group member"
"removing user from shadow group"
"unlocking group file"
"unlocking gshadow file"
"unlocking passwd file"
"unlocking shadow file"
"unlocking subordinate group file"
"unlocking subordinate user file"
"unlocking-group-file"
"unlocking-gshadow-file"
"updating passwd"
"updating password"
audit_buf
buf
info->audit_msg
info_group.audit_msg
info_gshadow.audit_msg
info_passwd.audit_msgAnd then try a script to replace them (please make sure to paste the script verbatim in the commit message if you do.
On the other hand, this shows that a few cases are not string literals. We should check that and see what they are.
There was a problem hiding this comment.
Steve I'm thinking that we could open a PR to include the downstream patch in shadow. I don't think it would take a great deal of time to do it since I already had to update it for shadow 4.17.0 and I don't think the code has changed much since that was released.
There was a problem hiding this comment.
@ikerexxe feel free to do that. I can close this if a better patch is accepted.
The audit system only recognizes key=value word pairs. It uses the first whitespace after the '=' to determine the end of the value associated with the field name. The op field contains multiple words describing what operation is being performed on the user account. However, due to white space between the words, the audit parser cannot get the whole operation description as intended.
This patch is the least invasive way to fix the problem. What it does is replace white space in the op field with dashes soo that the parser keeps all of the words togther. Below are before and after events:
type=ADD_GROUP msg=audit(01/18/2025 13:50:27.903:685) : pid=116430 uid=root auid=sgrubb ses=3 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 msg='op=adding group acct=test exe=/home/sgrubb/shadow-utils/src/useradd hostname=x2 addr=? terminal=pts/1 res=success'
type=ADD_GROUP msg=audit(01/18/2025 13:56:45.031:709) : pid=107681 uid=root auid=sgrubb ses=3 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 msg='op=adding-group acct=test1 exe=/home/sgrubb/shadow-utils/src/useradd hostname=x2 addr=? terminal=pts/1 res=success'
To show the effect using auformat which captures just the field asked for:
Before:
ausearch --start 13:50 -m ADD_USER --format raw |
/home/sgrubb/test/auformat "%OP\n"
adding
After:
ausearch --start 13:55 -m ADD_USER --format raw |
/home/sgrubb/test/auformat "%OP\n"
adding-user