newsyslog(8): Reject configurations that specify setuid or executable logs
ClosedPublic
Actions

Authored by cem on Aug 21 2018, 4:59 PM.

Details

Reviewers

delphij
jhb
markj
dab
vangyzen

Group Reviewers

manpages

Commits

rS338165: newsyslog(8): Reject configurations that specify setuid or executable logs

Summary

Prevent some classes of foot-shooting that may result in permissions
problems.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

cem created this revision.Aug 21 2018, 4:59 PM

Harbormaster completed remote builds in B19014: Diff 47049.Aug 21 2018, 4:59 PM

I think the direction is good and the changeset looks good to me in principal. Could you please make a change so it would print errline, which would make it easier for administrators to find out where the issue is?

By the way, instead of doing errx, I would suggest issuing a warning with warnx, and do a permission &= DEFFILEMODE (=0666) to filter out the unwanted bits, as this would not stop newsyslog from working for upgrades. (Please note "Relnotes: yes" either way, as this would be a behavior change).

usr.sbin/newsyslog/newsyslog.c
1197 ↗	(On Diff #47049)	errline?
1200 ↗	(On Diff #47049)	errline?

This revision now requires changes to proceed.Aug 21 2018, 6:42 PM

In D16831#358319, @delphij wrote:

I think the direction is good and the changeset looks good to me in principal. Could you please make a change so it would print errline, which would make it easier for administrators to find out where the issue is?

Sure, that seems like a good idea.

By the way, instead of doing errx, I would suggest issuing a warning with warnx, and do a permission &= DEFFILEMODE (=0666) to filter out the unwanted bits, as this would not stop newsyslog from working for upgrades. (Please note "Relnotes: yes" either way, as this would be a behavior change).

Yeah, I wasn't sure which direction was best. I am fine with warnx-and-proceed-restricted. I'll prepare a new patch.

I like it, and I like the suggestions from @delphij.

I would also agree with issuing a warning, masking the offending permission bits, and continuing.

usr.sbin/newsyslog/newsyslog.c
1197 ↗	(On Diff #47049)	Agree, please print the line. (same below as well)

Per CR feedback, print errline, and mask off any non-0666 bits.

Harbormaster completed remote builds in B19021: Diff 47064.Aug 21 2018, 8:13 PM

vangyzen accepted this revision.Aug 21 2018, 8:18 PM

LGTM as-is, but with some minor suggestions inline.

usr.sbin/newsyslog/newsyslog.c
1198 ↗	(On Diff #47064)	Minor nit: Techinically this is setuid or setgid.
1210 ↗	(On Diff #47064)	(No strong opinion here) If I was you I'd probably just do one filtering here, basically: warnx("File mode bits 0%o changed to 0%o in line\n%s", working->permissions, working->permissions & ~DEFFILEMODE, errline); And remove the filtering in line 1200, 1206 as well as the condition (would always be true after removing the filtering) in 1209/1215.

This revision is now accepted and ready to land.Aug 21 2018, 8:26 PM

cem added inline comments.Aug 21 2018, 8:40 PM

usr.sbin/newsyslog/newsyslog.c
1198 ↗	(On Diff #47064)	Yes. I figure anyone with such a line in newsyslog probably doesn't care about the distinction. If you want me to change the print, I will. Do you?
1210 ↗	(On Diff #47064)	Yeah, that would be fine too. I thought there might be some value in justifying the removed bits but it seems disproportionate to the rest of the parser and perhaps it can be left to a comment. I will also document in newsyslog.conf.

dab added inline comments.Aug 21 2018, 9:35 PM

usr.sbin/newsyslog/newsyslog.c
1210 ↗	(On Diff #47064)	Having multiple warnings for essentially the same problem seems excessive. I think I'd go along with @delphij's latest suggestion here. Documenting in newsyslog.conf is a good idea, too.