savecore: add missing call to cap_openlog when in capabilities mode

[stephane-rochoy-stormshield]

When run in capabilities mode, savecore seems to lack a call to cap_openlog. Notably it break the use of LOG_PERROR.

With the original one:

$ /sbin/savecore -vC
checking for kernel dump on device /dev/gpt/swapfs
No dump exists

With the patched one:

$ ./savecore -vC
checking for kernel dump on device /dev/gpt/swapfs
savecore 1424 - - /dev/gpt/swapfs: Permission denied
No dump exists

[stephane-rochoy-stormshield]

I'm looking to add a test as it should be trivial.

[emaste]

Looks good. CC @oshogbo.

Also @bsdimp do we have advice on rebasing/squashing fixup commits in pull requests?

[stephane-rochoy-stormshield]

Looks good. CC @oshogbo.

Also @bsdimp do we have advice on rebasing/squashing fixup commits in pull requests?

Yes you have ;) I've put this PR in draft mode for now. I'll squash once ready.
I'm discovering atf(7) and kyua(1) to add at least a basic test.

[emaste]

Oh yes I didn't notice the Draft tag. The C change itself looks good :)

[bsdimp]

git rebase -i main
squash all the tests into one commit and all the stuff in the actual binary into one. I think that's just the first one and all the rest squashed.

[stephane-rochoy-stormshield]

Sorry for the initial noise. I cleaned the commits.

But one question remain about the test I propose to add: what is the proper way to test the instance of savecore from the installed build, i.e., not the one on the system? I find it convenient to make world DESTDIR=… but the tests don't seems to be aware of $DESTDIR.

[stephane-rochoy-stormshield]

But one question remain about the test I propose to add: what is the proper way to test the instance of savecore from the installed build, i.e., not the one on the system? I find it convenient to make world DESTDIR=… but the tests don't seems to be aware of $DESTDIR.

@oshogbo Do you have an opinion on this? Do you want me to just ignore $DESTDIR?

[stephane-rochoy-stormshield]

Any news on this topic? Should I just call savecore instead of ${DESTDIR:-''}/sbin/savecore?

[asomers]

Any news on this topic? Should I just call savecore instead of ${DESTDIR:-''}/sbin/savecore?

Yes, you should do that. The test suite is intended to run against an installed system. There aren't any other tests tweaked to work with DESTDIR. However, you can always alter PATH when running this test yourself.

[oshogbo]

Sorry, for late response. LGTM.

[stephane-rochoy-stormshield]

I adjusted copyright to 2025.

[asomers]

While I don't dispute the results of your testing, I don't understand why it works. Shouldn't it be sufficient to run openlog before cap_enter? I don't understand why the program even needs cap_syslog in the first place. IIUC, cap_syslog is only needed if you must open a syslog connection after cap_enter.

[stephane-rochoy-stormshield]

While I don't dispute the results of your testing, …

I'm just starting to discover this API so don't hesitate to dispute anything ;)

… I don't understand why it works. Shouldn't it be sufficient to run openlog before cap_enter? I don't understand why the program even needs cap_syslog in the first place. IIUC, cap_syslog is only needed if you must open a syslog connection after cap_enter.

I have to admit I didn't even try to move the call to openlog somewhere after init_caps because there's some calls to logmsg in between. IIUC, it would means calling vsyslog without LOG_PERROR which is not the intended behavior.

[asomers]

I have to admit I didn't even try to move the call to openlog somewhere after init_caps because there's some calls to logmsg in between. IIUC, it would means calling vsyslog without LOG_PERROR which is not the intended behavior.

Not after, before. openlog ought to be called before init_caps, which it is. However, openlog only affects future calls to syslog and vsyslog, not calls to cap_syslog and cap_vsyslog. I'm wondering if logging will work perfectly if all of the cap_syslog stuff is removed, since openlog is already called early enough.

[stephane-rochoy-stormshield]

Well, the observations that initially led me to think a call to cap_openlog was missing are the following:

Test 1:

1. openlog(…, LOG_PERROR, …)
2. vsyslog(…)
   Result: message is properly written to stderr, no surprise.

Test 2:

1. openlog(…, LOG_PERROR, …)
2. init_caps(…) that include a call to caph_enter_casper
3. cap_vsyslog(…)
   Result: message is not written to stderr.

Test 3:

1. openlog(…, LOG_PERROR, …)
2. init_caps(…) that include a call to caph_enter_casper
3. cap_openlog(…, LOG_PERROR, …)
4. cap_vsyslog(…)
   Result: message is written to stderr.

And I've just done an additional test:

1. openlog(…, LOG_PERROR, …)
2. init_caps(…) that include a call to caph_enter_casper
3. cap_openlog(…, LOG_PERROR, …)
4. openlog(…, LOG_PERROR, …)
5. cap_vsyslog(…)
   Result: message is not written to stderr.

. I'm wondering if logging will work perfectly if all of the cap_syslog stuff is removed, since openlog is already called early enough.

I guess it is. The following sequence show capabilities don't interfere with syslog:

1. openlog(…, LOG_PERROR, …)
2. syslog(…, "first")
3. init_caps(…) that include a call to caph_enter_casper
4. cap_openlog(…, LOG_PERROR, …)
5. openlog(…, LOG_PERROR, …)
6. cap_vsyslog(…, "second")
7. syslog(…, "third")
   Result: only the "first" and "third" messages are written to stderr.

[asomers]

It sounds like we can elminate cap_openlog/cap_syslog and go back to plain openlog/syslog, then. Could you please do that?

[oshogbo]

@asomers Sorry, I'm just jumping in. It's not like syslog might want to reopen the file or connect again to syslogd z(for example when the daemon was restarted)? Or savecorealways log only tostderr`?

[asomers]

@asomers Sorry, I'm just jumping in. It's not like syslog might want to reopen the file or connect again to syslogd z(for example when the daemon was restarted)? Or savecorealways log only tostderr`?

Since savecore is a short-lived program, running for only a few seconds, I don't think we need to worry about syslogd restarts. Are there any other reasons why syslog(3) might reconnect?

[oshogbo]

Yeah, that's a valid point.
On the other hand, saving the core might take some time, as dumping a couple of GBs can be a time-consuming task.

I don't have a strong opinion on this.

I'm not sure what happens when the log file is rotated.

[markjdb]

syslogd handles log rotation transparently to loggers. syslog(3) users don't need to reopen sockets after a log rotation. I can't see any reason not to use the plain syslog() and openlog() functions.

[stephane-rochoy-stormshield]

syslogd handles log rotation transparently to loggers. syslog(3) users don't need to reopen sockets after a log rotation. I can't see any reason not to use the plain syslog() and openlog() functions.

So the question is why do we have cap_ variants for them exactly?

[asomers]

So the question is why do we have cap_ variants for them exactly?

I think it was due to an oversight in d7fffd0 . The author replaced several other services with their capsicumized versions, where that's really needed. I think he just didn't realize that the non-capsicum syslog would work fine.

[markjdb]

So the question is why do we have cap_ variants for them exactly?

I think it was due to an oversight in d7fffd0 . The author replaced several other services with their capsicumized versions, where that's really needed. I think he just didn't realize that the non-capsicum syslog would work fine.

Yes, I think it was probably me being overlay paranoid.

As was already noted, savecore is a short-lived program, it's unlikely that it'll hit the conditions under which syslog(3) needs to reconnect (syslogd restarts, or its logging buffer is full). So, plain openlog()+syslog() should work fine in practice.

OTOH, I suspect the patch in this PR is also fine as-is. Maybe let's merge it and consider removing cap_syslog separately, given that we're just trying to fix a simple bug here? Either approach is ok with me.

[stephane-rochoy-stormshield]

So the short-term plan is to add the missing call to cap_openlog to fix savecore's behavior regarding LOG_PERROR.

But I'm not sure to understand the long-term plan: is it to just drop the use of cap_openlog and cap_vsyslog? Is looks like dropping the Casper's system.syslog service so could someone explain the purpose of this service in the first place?

[markjdb]

So the short-term plan is to add the missing call to cap_openlog to fix savecore's behavior regarding LOG_PERROR.

But I'm not sure to understand the long-term plan: is it to just drop the use of cap_openlog and cap_vsyslog? Is looks like dropping the Casper's system.syslog service so could someone explain the purpose of this service in the first place?

It's to handle the rare case where syslog(3) has to reconnect to syslogd's socket. In capability mode, connect() is not allowed, so we use a casper service. This is mostly suitable for long-running programs, where the likelihood of needing to reconnect at some point is high.

I tend to think that syslog(3) could instead just open /var/run (limiting rights to CAP_CONNECTAT, CAP_LOOKUP, CAP_WRITE, ...?) and use connectat(2) when needed. @oshogbo do you see any problem with this alternative? It means that we hold an extra capability for /var/run, but I think that's a small downside.