[Merge] lp:~cjwatson/ubuntu/trusty/rsyslog/repair-groups into lp:ubuntu/rsyslog

Wed Dec 4 13:08:43 UTC 2013

Colin Watson has proposed merging lp:~cjwatson/ubuntu/trusty/rsyslog/repair-groups into lp:ubuntu/rsyslog.

Requested reviews:
  Ubuntu branches (ubuntu-branches)
Related bugs:
  Bug #484336 in rsyslog (Ubuntu): "/etc/rsyslog.conf permissions incorrect/missing for creation of dynamic files"
  https://bugs.launchpad.net/ubuntu/+source/rsyslog/+bug/484336
  Bug #1256695 in rsyslog (Ubuntu Trusty): "cannot create log files in /var/log"
  https://bugs.launchpad.net/ubuntu/trusty/+source/rsyslog/+bug/1256695

For more details, see:
https://code.launchpad.net/~cjwatson/ubuntu/trusty/rsyslog/repair-groups/+merge/197705

This fixes log file creation in trusty.  IRC conversation for the record:

<pitti> xnox_, slangasek: I adjusted bug 1257633; apparently rsyslog is now completely unable to create  any of its logs?
<ubottu> bug 1257633 in rsyslog (Ubuntu Trusty) "/var/log/syslog and others don't exist on fresh installation of Trusty." [Critical,Confirmed] https://launchpad.net/bugs/1257633
<cjwatson> pitti: I'm investigating that at the moment, bug 1256695
<ubottu> bug 1256695 in rsyslog (Ubuntu) "Trusty desktop Installation logs are not copied to /var/log/installer/" [Undecided,Confirmed] https://launchpad.net/bugs/1256695
<cjwatson> pitti: it seems to be to do with a rearrangement of how privilege-dropping works
<pitti> cjwatson: ah, thank you
<cjwatson> +- bugfix: do not open files with full privileges, if privs will be dropped
<cjwatson> +  This make the privilege drop code more bulletproof, but breaks Ubuntu's
<cjwatson> +  work-around for log files created by external programs with the wrong
<cjwatson> +  user and/or group. Note that it was long said that this "functionality"
<cjwatson> +  would break once we go for serious privilege drop code, so hopefully
<cjwatson> +  nobody still depends on it (and, if so, they lost...).
<cjwatson> e.g.
<cjwatson> so, uh, oh dear
<pitti> cjwatson: ah, that's because /var/log is root:root 755?
<cjwatson> Yeah, it might be as simple as changing that
<cjwatson> I see that rsyslog-controlled files there were already syslog:adm
<pitti> rsyslog runs as syslog:syslog, so we could just make it root:syslog 775?
<cjwatson> that's certainly sufficient to make it work
<cjwatson> I can't immediately think of a reason not to do that
<cjwatson> what do other distros do?
<cjwatson> it's not clear to me that Fedora drops privileges, looking only at its rsyslog packaging
<pitti> cjwatson: do they even use rsyslog still? it's not all journald now?
<pitti> I understand you can still install it, but it's not by default
<cjwatson> Nor Gentoo
<cjwatson> pitti: could be
<cjwatson> pitti: however, there's a further difficulty
<cjwatson> pitti: if I just change the /var/log perms, then it appears to work, but /var/log/syslog is created syslog:syslog not syslog:adm, so the ability for users in group adm to read logs has been broken
<cjwatson> (and mode 640)
<cjwatson> pitti: this is related to bug 484336
<ubottu> bug 484336 in rsyslog (Ubuntu) "/etc/rsyslog.conf permissions incorrect/missing for creation of dynamic files" [Undecided,Confirmed] https://launchpad.net/bugs/484336
<pitti> cjwatson: ah, so we'd need to put user "syslog" into "adm", or perhaps even make that its primary group?
<pitti> (that's ugly to do on upgrades, though)
<cjwatson> or run rsyslogd as group adm
<cjwatson> I don't know which is worse
<cjwatson> we have to do one of those if rsyslogd is no longer willing to use raised privileges to open files, though ...
<cjwatson> it wouldn't need to be syslog's primary group
<cjwatson> syslog is a dynamic user owned by the rsyslog package, so it's not necessarily horrible to change its supplementary groups
<cjwatson> hmm, that doesn't fix the ownership though
<pitti> right, with a non-primary groupd it'd need a source patch
<cjwatson> oh, because rsyslogd doesn't setgroups when it drops privileges?
<cjwatson> or rather it does but only to deliberately drop supplementary groups
<pitti> cjwatson: no, I meant if it creates a new file, it would need to explicitly set their group to "adm", unless that's its primary group?
<cjwatson> it's already configured with FileGroup to do that
<pitti> ah, of course; sorry
<cjwatson> but the problem is that it can only do the chown() if it did the appropriate setgroups at privdrop
<cjwatson> http://kb.monitorware.com/feature-request-privdroptogroup-setgroups-initgroups-t11491.html
<pitti> $PrivDropToGroup syslog
<pitti> so that's what's not keeping the aux groups
<cjwatson> right.  now the questions would be (a) would changing that to adm expose rsyslog to any malice by users in group adm? (b) might there be systems right now that are relying on rsyslog being able to open files in group syslog?
<pitti> I don't see an immediate case for (a) as "adm" is pretty much only a "get read privs" group, but (b) certainly might happen
<cjwatson> the link above has an example of situations where you want rsyslogd to be able to set different log files to be owned by different groups
<pitti> cjwatson: TBH it seems safer to me to change the code to not drop the aux groups
<cjwatson> dropping the aux groups is required to drop the root group, but it could call initgroups afterwards
<cjwatson> pitti: Considering something like http://paste.ubuntu.com/6519272/
<pitti> cjwatson: I'm a bit rusty on the order of the various set* calls; does initgroups() come before or after setgid()?
<pitti> cjwatson: AFAIR, you have to setgid(), then initgroups(), then setuid(), right?
<cjwatson> pitti: rsyslog calls doDropPrivGid then doDropPrivUid
<pitti> the patch looks like initgroups() would come after set[ug]id?
<pitti> ah, so these two hunks are not from one functino
<cjwatson> pitti: doDropPrivGid already calls setgroups before setgid to discard the previously-held supplementary groups
<cjwatson> pitti: they are
<pitti> cjwatson: ack; LGTM then, thank you!
<cjwatson> pitti: the important bit is that you have to drop the old groups before you lose the ability to do so
<pitti> cjwatson: but this is still missing a chgrp syslog /var/log, isn't it?
<cjwatson> pitti: I think initgroups is fine after setuid
<cjwatson> pitti: oh yes good point
<pitti> cjwatson: after setuid> that's surprising; that sounds like increasing privileges after you dropped them and shouldn't be able to get them any more?
<cjwatson> I think you can gain your own supplementary groups ... but wait, something just occurred to me
<pitti> cjwatson: (and chmod 775)
<cjwatson> /var/log must not be owned by group adm
<cjwatson> because if we do that then users in group adm will be able to add files to it
<cjwatson> which is not expected
<pitti> no, adm group members must nto be able to write there
<pitti> that's why I thought root:syslog would be better
<cjwatson> oh, right, yeah, too many pieces
<pitti> so that rsyslog can write into it, but no real users
 * pitti adds to this TODO list to create an autopkgtest for rsyslog to cover new logs, log rotation, etc.
<cjwatson> http://paste.ubuntu.com/6519302/ then
<pitti> cjwatson: chmod g+w /var/log ?
<cjwatson> added
<cjwatson> pitti: hm, you were right earlier, setgroups (called by initgroups) requires the CAP_SETGID capability
<cjwatson> so I'll move that before the setuid
<pitti> ok, that's relieving (would be weird otherwise)
<cjwatson> pitti: OK, so after some fixups this basically works, but there's an amusing complication
<cjwatson> pitti: On upgrade, we restart rsyslog in postinst rather than stop in prerm / start in postinst, for generally obvious reasons
<cjwatson> pitti: But this means that when it restarts it tries to log one final message to say it's shutting down (and others might racily be logged, of course)
<cjwatson> pitti: And on a system without /var/log/syslog, that last message now gets successfully written (since we just adjusted permissions) but /var/log/syslog ends up as syslog:syslog (since we hadn't yet completed the restart with supplementary groups)
<cjwatson> pitti: So, I don't know; I suppose we could choose to not care because it's only been a problem for five days or so and it would go away at the next logrotate
<cjwatson> pitti: wdyt?
<pitti> cjwatson: re (sorry, lunch)
<pitti> cjwatson: TBH I'd go with the "don't care" approach, as it didn't affect any stables
<pitti> cjwatson: and in trusty, logs are already broken anyway
<pitti> cjwatson: i. e. the fully "correct" solution would be to create the file in preinst with the correct permissions on upgrade?
<cjwatson> pitti: well, except that the race case could involve log messages going to any log file
<cjwatson> I'm cool with "don't care" if you are :-)
<pitti> I am
<pitti> the main conclusion I draw from this is "this needs an autopkgtest" :)
-- 
https://code.launchpad.net/~cjwatson/ubuntu/trusty/rsyslog/repair-groups/+merge/197705
Your team Ubuntu branches is requested to review the proposed merge of lp:~cjwatson/ubuntu/trusty/rsyslog/repair-groups into lp:ubuntu/rsyslog.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: review-diff.txt
Type: text/x-diff
Size: 54203 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/ubuntu-reviews/attachments/20131204/180cc2ff/attachment-0001.diff>