Unbreak syslogging with fabricated PID

[eugen_grosbein.net]

Summary

Long running shell script can utilize the following command to tag its log with its own PID:
logger -t "ident[$$]" -p user.notice "test"

As documented with logger(1) manual page, the logger does not tag the entry with its PID without -i flag, so the script may fabricate PID part of the entry. And this worked for years (until FreeBSD 12+) including matching resulted entries by ident in syslog.conf:

!ident
*.* /var/log/ident.log

This was broken with FreeBSD 12+ due to incompatible libc changes: now PID of called process added forcibly despite of PID already added to the ident. Proposed change restores compatibility while keeping implicit addition of PID for processes that do not call openlog(3) with omitted LOG_PID and fabricated PID within the ident.

Test Plan

For unpatched FreeBSD 12+:

$ logger -t 'ident[123]' -p user.notice test1
Aug  1 07:36:58 hostname ident[123][86483]: test1

Same with the flag -i.

For FreeBSD versions upto 11.4 and patched recent ones:

Aug  1 07:39:40 hostname ident[123]: test1

Also, ident matching works again.

The change does not affect applications using openlog() with LOG_PID. It does not affect applications that do not embed fabricated PID into the ident.

[eugen_grosbein.net]

Add another check to parse_tag() for "ident[]" (empty PID).

[danfe]

Admittedly, first forcibly adding the PID and now providing non-trivial gimmicks to parse the string (what can possibly go wrong, this is C, right? ;-), assuming some particular format, looks pretty bad. I'd much rather respect the LOG_PID and fix those applications that are important enough for us to care, instead of depriving ourselves from flexibility when it's actually needed.

libc/gen/syslog.c

219

Funny thing: the comment suggests exactly what should be fixed (buggy applications), not the syslog(3). Forcing PID logging had always seemed wrong to me.

[eugen_grosbein.net]

In D36005#817519, @danfe wrote:

Admittedly, first forcibly adding the PID and now providing non-trivial gimmicks to parse the string (what can possibly go wrong, this is C, right? ;-), assuming some particular format, looks pretty bad. I'd much rather respect the LOG_PID and fix those applications that are important enough for us to care, instead of depriving ourselves from flexibility when it's actually needed.

I believe there is no API for an application to submit fabricated PID.

[eugen_grosbein.net]

In D36005#817519, @danfe wrote:

Admittedly, first forcibly adding the PID and now providing non-trivial gimmicks to parse the string (what can possibly go wrong, this is C, right? ;-), assuming some particular format, looks pretty bad. I'd much rather respect the LOG_PID and fix those applications that are important enough for us to care, instead of depriving ourselves from flexibility when it's actually needed.

Also note that "some particular format" is from https://datatracker.ietf.org/doc/html/rfc3164#section-5.3
After all, the change is about restoring compatibility with working code.

[eugen_grosbein.net]

Add another check into parse_tag() for empty ident.

[eugen_grosbein.net]

Add more commentaries to parse_tag().

[glebius]

I would aruge that not being able to fabricate PID is a good thing, and the opposite is a security problem. I don't have any strong opinion on this enough to block this change, but I'd like the security people to be aware of it.

[eugen_grosbein.net]

In D36005#818242, @glebius wrote:

I would aruge that not being able to fabricate PID is a good thing, and the opposite is a security problem. I don't have any strong opinion on this enough to block this change, but I'd like the security people to be aware of it.

I would aruge that security problem would be trust in non-signed text logs including received from remote source.

[eugen_grosbein.net]

In D36005#818242, @glebius wrote:

I would aruge that not being able to fabricate PID is a good thing, and the opposite is a security problem. I don't have any strong opinion on this enough to block this change, but I'd like the security people to be aware of it.

Also note this is library code. Malicious application can produce arbitraty logs anyway, not using libc's syslog(3) function.

[eugen_grosbein.net]

I will commit the change next week unless other objections raised.

[eugen_grosbein.net]

Fixed long time ago.