Details

Reviewers

kevans
yuripv
0mp
pauamma_gundo.com

Group Reviewers

manpages
rc

Summary

syslogd has an altlog_proglist stanza, which can place the log socket
into a program's chroot directory, so it can still log.

This patch adds an equivalent to that for jails.
If used with jails, we lookup the each jail's "path" with jls appending
it to the sockfile and, now also privsockfile.

Caveats:

syslogd must be restarted
-H must be added to syslogd_flags before this is actually useful

Test Plan

install script in /etc/rc.d/syslogd
restart syslog, it still works, phew

create a jail with syslogd_enable="NO" and add altlog_jaillist="jail-name" to the host's rc.conf
verify that the jail logs to the host.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

freebsd_igalic.co created this revision.Nov 29 2020, 12:00 PM

Herald added subscribers: Contributor Reviews (src), imp. · View Herald TranscriptNov 29 2020, 12:00 PM

freebsd_igalic.co requested review of this revision.Nov 29 2020, 12:00 PM

I think this generally LGTM; you'll need to also add a default and description for altlog_jaillist in ^/libexec/rc.conf (preferably near the other altlog_*)

libexec/rc/rc.d/syslogd
56	This line seems to be indented with spaces, and subsequent lines with a mix of tabs -- these should be entirely tabs

address @kevans's review; and document! fix almost all igor & mandoc concerns, except for this one:

mandoc: usr.sbin/syslogd/syslogd.8:281:14: STYLE: no blank before trailing delimiter: Li kernel:

Herald added a reviewer: manpages. · View Herald TranscriptNov 29 2020, 9:35 PM

yuripv added a subscriber: yuripv.Nov 29 2020, 10:21 PM

yuripv added inline comments.

usr.sbin/syslogd/syslogd.8
416	Should not these be documented in rc.conf(5) instead? As it is, rc variables are not related to syslogd daemon.

dch added a subscriber: dch.Nov 29 2020, 10:51 PM

great idea!

libexec/rc/rc.d/syslogd
54	s/priviledged/privileged/

address @dch and @yuripv review, moving rc.conf settings to rc.conf(5) and fixing most style issues

i've now finally gotten around to testing this change ^_^;; and i'm quite happy with the results…

…except when the jail is -red, it doesn't show up in jls -a output… it did with my libioc jails…
(aaaand, it's too late to figure out why)

So, another test round.
This works fine with syslogd_flags="-s" and syslogd_flags="-ss"; however, only when we add -H to the flags, do the syslog entries become more useful, because we get the host.hostname of the jail in our logs.
so I'll add that to that to the rc.conf man page.

add explanation that "-H" is needed for altlog_jaillist

freebsd_igalic.co marked 3 inline comments as done.Dec 4 2020, 5:12 PM

yuripv added inline comments.Dec 4 2020, 5:14 PM

share/man/man5/rc.conf.5
1380 ↗	(On Diff #80308)	i think you want `Ns` before `.` (might need to escape it as `\&.` if mandoc lint whines).
1506 ↗	(On Diff #80308)	i think you want `Ns` before `.` (might need to escape it as `\&.` if mandoc lint whines).
3917 ↗	(On Diff #80308)	same `Ns` note, so you don't break the path in parts.
4614 ↗	(On Diff #80308)	SEE ALSO is sorted by section first, move it below.
usr.sbin/syslogd/syslogd.8
476	Same sort note.

address @yuripv's review: by removing additional lints from rc.conf.5

jls -a returns an error on system startup; should we silence it with 2>/dev/null?

freebsd_igalic.co added inline comments.Dec 5 2020, 10:40 PM

libexec/rc/rc.d/syslogd
56	`jls -aj foo path` doesn't return anything useful if the jail isn't running, so we should probably silence the error here.

freebsd_igalic.co marked 4 inline comments as done.Dec 5 2020, 10:42 PM

freebsd_igalic.co updated this revision to Diff 80374.Dec 5 2020, 10:45 PM

freebsd_igalic.co marked an inline comment as done.

otis added a subscriber: otis.Dec 13 2020, 9:11 PM

freebsd_igalic.co added projects: rc, manpages.Dec 13 2020, 9:14 PM

freebsd_igalic.co added a reviewer: rc.Dec 13 2020, 9:16 PM

0mp added a subscriber: 0mp.Jan 8 2021, 1:47 PM

0mp added inline comments.

libexec/rc/rc.conf
289–290	Missing period.
290	Missing period.
libexec/rc/rc.d/syslogd
54	Comments seems to be indented in a slightly different way in this file. Could you new comments in a similar fashion to maintain a uniform style?
share/man/man5/rc.conf.5
2158 ↗	(On Diff #80374)	I think this line is unnecessary in this patch. We may think about adding section separators like this in a separate commit for all the other sections we have in this file.

freebsd_igalic.co updated this revision to Diff 81917.Jan 8 2021, 8:57 PM

address @0mp's comments.

rtyler_brokenco.de added a subscriber: rtyler_brokenco.de.Feb 25 2021, 8:01 PM

debdrup added a subscriber: debdrup.Feb 25 2021, 8:01 PM

freebsd_igalic.co mentioned this in D34563: devfs.rules: Do not expose "log" in the default devfs rules..May 23 2022, 7:19 AM

Add previous reviewers to ... Reviewers: as this ... needs review :)

pauamma_gundo.com requested changes to this revision.May 26 2022, 2:36 AM

pauamma_gundo.com added a subscriber: pauamma_gundo.com.

pauamma_gundo.com added inline comments.

libexec/rc/rc.conf
290
share/man/man5/rc.conf.5
2189–2192 ↗	(On Diff #81917)	If you don't go for this, you still need to s/recommend/recommended/. And maybe "add ..." instead of "adding .... is recommended", but that's arguably taste.
2193 ↗	(On Diff #81917)

This revision now requires changes to proceed.May 26 2022, 2:36 AM

address pauamma's comments

address @pauamma_gundo.com's comments

Manual pages English LGTM.

This revision is now accepted and ready to land.May 27 2022, 1:20 AM

it's been a while. update this to bug people

This revision now requires review to proceed.Feb 23 2023, 11:17 PM

I'm afraid this change is a potential security problem.

Ultimately content of the jail is considered fundamentally untrusted, meaning /var/run in the jail can point literally anywhere and in particular can be a symlink outside of the jail. syslog must *not* follow it.

But let's say none of the above was a problem. As this allows a jailed program to talk to host's syslog, it introduces another potential security problem -- what if syslog is exploitable? Furthermore, are there any provisions made to prevent jailed process from faking messages from the main host?

This is not a simple problem and the current change is definitely a no-go.

I also feel inclined to add that any remaining code which mingles with a running jail should be fixed.

Contrary to popular belief the jail subsystem was not thought out very well, I refer you to sample bugs in the area:
https://www.freebsd.org/security/advisories/FreeBSD-SA-21:10.jail_mount.asc
https://www.freebsd.org/security/advisories/FreeBSD-SA-21:05.jail_chdir.asc
https://www.freebsd.org/security/advisories/FreeBSD-SA-21:04.jail_remove.asc

closing this based on @mjg's feedback.
I'm going to have to rethink this completely.

add altlog_jaillist to syslogd's rc script
AbandonedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 80130

libexec/rc/rc.conf

libexec/rc/rc.d/syslogd

usr.sbin/syslogd/syslogd.8

add altlog_jaillist to syslogd's rc scriptAbandonedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 80130

libexec/rc/rc.conf

libexec/rc/rc.d/syslogd

usr.sbin/syslogd/syslogd.8

add altlog_jaillist to syslogd's rc script
AbandonedPublic
Actions

Revision Contents
Changeset List