auditd: support loading plugin configs from symlinks

[LordGrimmauld]

auditd: support loading plugin configs from symlinks

auditd previously did not support loading plugin configurations from
symlinked config files. This is problematic on systems such as NixOS,
which constructs basically the entirety of /etc using symlinks.

I considered why symlinks were not supported, and concluded the reason was
simplicity. While having a symlink point to a writable location would be
insecure, a user putting an insecure symlink to trigger this behavior could
also immediately do worse things.

There also were edge cases if the config file is replaced between the file
type check and the actual read. This is because load_plugin_conf uses
path based logic to check whether a file is a regular file or not, and then
asses the file path to load_pconfig. This means audispd would already
load symlinked configs if a regular file was replaced by a symlink at
precisely the right time in execution.

load_pconfig opens the supplied config file path using open. Crucially,
it does not set O_NOFOLLOW, meaning load_pconfig already supports
loading plugin configs from symlinks. The check in load_pconfig also
already uses the file-descriptor based fstat call, which mitigates the
replacement problems: file descriptors are stable.

This means, to support symlinks, it is sufficient to remove the check for
regular files from load_plugin_conf. This does change internal API: It
now is the responsibility of load_pconfig to make sure a plugin config
file is a regular file. This API change is purely internal, neither
load_pconfig nor load_plugin_conf are part of the public headers.

This change has been tested against auditd 4.0.3 and 4.0.5 in a NixOS VM.
The plugin config files af_unix.conf, au-remote.conf, filter.conf, syslog.conf
all successfully loaded through symlink.

[Cropi]

Hello,
Is the load_pconfig function unable to process symbolic links by default? I'm also wondering if it's necessary to pass the entire path to it.
I ask because it appears that if a problem occurs and the reason variable is set, rp will not be freed.

[LordGrimmauld]

load_pconfig should actually support symlinks. And indeed if (e->d_type != DT_REG && e->d_type != DT_LNK) would have been the trivial "fix". However, i didn't follow that route as it changes behavior: If the symlink points to an irregular file, the responsibility of detecting that then falls to load_pconfig instead. It does support that detection already, so it should be fine - but it is a change of api responsibility none the less.

audit-userspace/audisp/audispd-pconfig.c

Lines 182 to 187 in 263414b

	if (!S_ISREG(st.st_mode)) {
	audit_msg(LOG_ERR, "Error - %s is not a regular file",
	file);
	close(fd);
	return 1;
	}

load_pconfig itself actually checks the file mode, and the open does not take O_NOFOLLOW, so we probably could use if (e->d_type != DT_REG && e->d_type != DT_LNK) - or ignore that check completely, considering there is TOCTOU weirdness here anyways (check happens on the direntry, but later the name is actually opened and rechecked, the actual file might have changed since).

I see three options:

• fix the free (only really valid if we absolutely want to keep old behavior of skipping the attempt if its not regular file/symlink instead of having load_pconfig fail later)
• use if (e->d_type != DT_REG && e->d_type != DT_LNK) condition (mostly keeps old behavior, has load_pconfig fail later if symlink points to irregular file)
• drop that if completely (instead of skipping irregular files, load_pconfig will then always fail later)

I tried to keep behavior the same, but this might not be the best approach. Of the three options, tell me which you prefer and i'll update this PR accordingly.

[LordGrimmauld]

I just ran both of the alternative candidates through my VM tests:

diff --git a/audisp/audispd.c b/audisp/audispd.c
index 2113d4f9..871e0aad 100644
--- a/audisp/audispd.c
+++ b/audisp/audispd.c
@@ -118,7 +118,7 @@ static void load_plugin_conf(conf_llist *plugin)
 			char fname[PATH_MAX];
 			const char *ext, *reason = NULL;
 
-			if (e->d_type != DT_REG)
+			if (e->d_type != DT_REG && e->d_type != DT_LNK)
 				reason = "not a regular file";
 			else if (e->d_name[0] == '.')
 				reason = "hidden file";

diff --git a/audisp/audispd.c b/audisp/audispd.c
index 2113d4f9..70a8a298 100644
--- a/audisp/audispd.c
+++ b/audisp/audispd.c
@@ -118,9 +118,7 @@ static void load_plugin_conf(conf_llist *plugin)
 			char fname[PATH_MAX];
 			const char *ext, *reason = NULL;
 
-			if (e->d_type != DT_REG)
-				reason = "not a regular file";
-			else if (e->d_name[0] == '.')
+			if (e->d_name[0] == '.')
 				reason = "hidden file";
 			else if (count_dots(e->d_name) > 1)
 				reason = "backup file";

Both work perfectly fine.

[LordGrimmauld]

Okay, audispd.c is the only caller of that method, and neither load_pconfig nor load_plugin_conf are part of public API, so the stability consideration is mostly mute. I will just remove that check completely.

[LordGrimmauld]

Alright, with these considerations, its probably better to just remove that check completely and call it good.

[LordGrimmauld]

That said:

audit-userspace/audisp/audispd-pconfig.c

Line 162 in 263414b

* not symlink.

this comment is inaccurate, otherwise it should be O_NOFOLLOW/lstatat

[stevegrubb]

That test was added to make sure we have a real file. We recently had an issue filed because a fifo was in a directory and opening it deadlocked the program. So, if we need to allow symlinks, we need to check that it is a either a regular file or a symlink that resolves to a regular file to pass that first check.

[LordGrimmauld]

Afaik open syscall should "just work" for FIFO, unless you set O_RDWR (which you don't). So i am a bit confused.... Is there an issue link to read up on that? Or a repro i can test?

[LordGrimmauld]

Ah, you need to set O_NONBLOCK for fifo to not stall indefinitely.
That said, if there is a block/char device that does not support non-blocking open, the issue would come back.... Hmm...

[LordGrimmauld]

I suppose i can do the realpath+stat again (what i had before the force push), but that still allows a potential attacker to "race" and replace a regular file with a blocking char device (at the exact moment of execution after the type check and before the open) to infinitely stall the audit daemon.... Not ideal either.

[LordGrimmauld]

Ah, i know how to fix it! Hold on, will put an actual clean fix that isn't TOCTOU nor runs into the blocking issues.

[LordGrimmauld]

There, no unnecessary stat, no TOCTOU issues (that i can see), symlink support, and the commits are in an order where at no point there'd be unsafe code, even between changes.

[LordGrimmauld]

Tough challenge... O_PATH won't work - it returns a stable fd, but that fd can't be reopened for reading, making it somewhat useless. the stat would work, but then there'd be issues of TOCTOU between stat/read.

O_NONBLOCK will work with FIFO and most device files. This will however lock up if someone is mad enough to symlink /dev/ttyX into the plugin directory.

[LordGrimmauld]

i also switched to openat, that does at least fix some of the time of check/time of use issues. Notably, it will now correctly deal with the entire plugin directory being replaced during plugin load, which it previously also did not.

[LordGrimmauld]

Okay, now i am happy. no race-y stat/open, fixed the race with the dirfd, and only actually open the file for reading once all the stat succeeds. This should be good now.

[LordGrimmauld]

Oh, does this need an entry in the changelog? Sorry, there is no contribution guidelines, so i missed that.

[LordGrimmauld]

@stevegrubb whats the status here, what do i need to do to move this forward? Do you need me to do specific testing, more explanations, some code changes?

we need to check that it is a either a regular file or a symlink that resolves to a regular file

This is now the case, and it won't lock up anymore even without that first preliminary check present anymore. So i am not sure what else i need to do here.

Side note: I won't claim there is anything wrong with checking file type, but why exactly are we protecting root from doing something stupid here? It wasn't that much effort and i got an excuse to read more man pages, but it does seem a bit silly to try and accomodate root "accidentially" symlinking /dev/tty into the plugin config directory...

[stevegrubb]

Basically, it needed my time to come back to this. I had a mega patch trying to land. This seems more complicated than it should be, but maybe that's the problem domain. Thanks for the patch!

[LordGrimmauld]

Thanks for getting this merged, and i do appreciate your work! Indeed this feels on the complicated side, i tried to keep it as simple as possible while properly fixing all edge cases (including TOCTOU and questionable symlinks). Chances are, this could be simpler given more assumptions about what is allowed in the config directory, but alas i was trying to write a thorough patch without imposing any additional assumptions.

Now its time to clean this up on our (nixos) side, i am excited to see more security features gaining support. Thanks for your work, not just here but also with libcap_ng, these are quite useful tools!