svn commit: r349256 - head/libexec/rc/rc.d

[Enji Cooper]

> On Jun 21, 2019, at 12:32, Xin LI <delphij at gmail.com> wrote:
> 
> 
>> On Thu, Jun 20, 2019 at 7:38 PM Conrad Meyer <cem at freebsd.org> wrote:
>> Author: cem
>> Date: Fri Jun 21 02:37:54 2019
>> New Revision: 349256
>> URL: https://svnweb.freebsd.org/changeset/base/349256
>> 
>> Log:
>>   rc.d/motd: Update motd more robustly
>> 
>>   Use appropriate fsyncs to persist the rewritten /etc/motd file, when a
>>   rewrite is performed.
> 
> Why is /etc/motd so important to deserve this kind of construct?  The worst that could happen to /etc/motd with previous code is that you might end up with an empty or non-existent /etc/motd, and the change have introduced more problems than it intended to solve.
>  
>> 
>>   Reported by:  Jonathan Walton <jonathan AT isilon.com>
>>   Reviewed by:  allanjude, vangyzen
>>   Sponsored by: Dell EMC Isilon
>>   Differential Revision:        https://reviews.freebsd.org/D20701
>> 
>> Modified:
>>   head/libexec/rc/rc.d/motd
>> 
>> Modified: head/libexec/rc/rc.d/motd
>> ==============================================================================
>> --- head/libexec/rc/rc.d/motd   Fri Jun 21 00:52:30 2019        (r349255)
>> +++ head/libexec/rc/rc.d/motd   Fri Jun 21 02:37:54 2019        (r349256)
>> @@ -37,11 +37,15 @@ motd_start()
>>         uname -v | sed -e 's,^\([^#]*\) #\(.* [1-2][0-9][0-9][0-9]\).*/\([^\]*\) $,\1 (\3) #\2,' > ${T}
>>         awk '{if (NR == 1) {if ($1 == "FreeBSD") {next} else {print "\n"$0}} else {print}}' < /etc/motd >> ${T}
>> 
>> -       cmp -s $T /etc/motd || {
>> -               cp $T /etc/motd
>> +       if ! cmp -s $T /etc/motd; then
>> +               mv -f $T /etc/.motd.tmp
> 
> This have partially defeated the benefit of doing mktemp in the code above.  The old code tries to avoid excessive writes to /, which is preserved, but it is not safe to assume /etc/.motd.tmp is a non-existent plain file: mv would happily proceed when .motd.tmp is a preexisting directory, for instance (and subsequent mv -f would fail).  A malicious system administrator can now plant a timing bomb which will fill / eventually.
> 
> You can do a TMPDIR=/etc/ mktemp .motd.tmp.XXXXXXXX and use that to mitigate the attack above, but then if the system crash in the middle you would still end up with a dangling .motd.tmp.XXXXXXXX file in /etc.
> 
> Also note that $T is on /tmp which is likely to be a different filesystem, internally this is mostly equivalent to cp + rm.  You can simplify the code by reverting to previous cp and keep the rm -f below as an unconditional remove like before.
> 
>> +               fsync /etc/.motd.tmp
>> +               mv -f /etc/.motd.tmp /etc/motd
>>                 chmod ${PERMS} /etc/motd
>> -       }
>> -       rm -f $T
>> +               fsync /etc
> 
> There is absolutely no reason to fsync /etc here.  mv'ing on the same file system is performed with rename(2) which is required to be atomic by POSIX.  /etc/motd will be pointing to either the old one or the new one in the event of a crash, and both are acceptable.
>  
> So, in my opinion the new code would have made the situation worse than the old code.
> 
> But ultimately, I think the real design question here that needs to be solved would probably be "Why are piling up multiple layers of workarounds around motd? Does it even need to be located in /etc?"  The contents is meant to be updated every time when there is a kernel change, and to that extent it seems to be more appropriate for /var/run and generated at boot from a template located somewhere in /etc.  The benefit of this approach is that you would have one less file to merge for each etcupdate/mergemaster (or at least only need to do it when some customization is made), and there is no need to worry about write durability.

    I don’t know the exact reasoning today, but once upon a time, there used to be a number of things hooked to /etc/motd generation that could influence information provided to downstream callers/systems.
Thank you,
-Enji