[arch-projects] [initscripts][PATCH 2/2] rc.d: Add check to cleanly abort rc.d script if user doesn't have root privileges

[Seblu]

On Thu, Jun 2, 2011 at 2:30 AM, Eric Bélanger <snowmaniscool at gmail.com> wrote:
> On Tue, May 31, 2011 at 7:29 PM, Eric Bélanger <snowmaniscool at gmail.com> wrote:
>> On Mon, May 30, 2011 at 6:23 PM, Seblu <seblu at seblu.net> wrote:
>>> On Sat, May 28, 2011 at 5:52 AM, Eric Bélanger <snowmaniscool at gmail.com> wrote:
>>>> On Fri, May 27, 2011 at 10:19 AM, Seblu <seblu at seblu.net> wrote:
>>>>> On Fri, May 27, 2011 at 11:42 AM, Eric Bélanger <snowmaniscool at gmail.com> wrote:
>>>>>> This implements FS#24095. The check is only made for the start, stop and restart
>>>>>> actions of the daemon scripts. This allows regular user to use the help and list
>>>>>> functionality of rc.d and also to use rc.d for actions that doesn't require root
>>>>>> privileges, like the status action of some daemon scripts.
>>>>>>
>>>>>> Signed-off-by: Eric Bélanger <snowmaniscool at gmail.com>
>>>>>> ---
>>>>>>  rc.d |    4 ++++
>>>>>>  1 files changed, 4 insertions(+), 0 deletions(-)
>>>>>>
>>>>>> diff --git a/rc.d b/rc.d
>>>>>> index 97f266a..2325623 100755
>>>>>> --- a/rc.d
>>>>>> +++ b/rc.d
>>>>>> @@ -43,6 +43,10 @@ case $1 in
>>>>>>                ;;
>>>>>>        *)
>>>>>>                action=$1
>>>>>> +               if [[ "$EUID" != '0' ]] && [[ "$action" == 'start' || "$action" == 'stop' || "$action" == 'restart' ]] ; then
>>>>>> +                   echo 'Error: this script must be run as root to use this functionality.'
>>>>>> +                   exit 1
>>>>>> +               fi
>>>>>>                shift
>>>>>>                # set same environment variables as init
>>>>>>                runlevel=$(/sbin/runlevel)
>>>>>
>>>>> As i said in FS#24095, if we really want do this, we should not do
>>>>> this in rc.d script but in functions which is loaded by real rc
>>>>> scripts.
>>>>>
>>>>> Increasingly, why choose start/stop/restart and not reload by example?
>>>>> By example, in virtualbox_bin we have fixusb, which must be run as root.
>>>>>
>>>>
>>>> I only chose start/stop/restart because they are the only standard
>>>> ones.
>>> There is a page describing arch standard, or at least, a namming convention?
>>
>> The only one I can think of is the daemon prototype file which has a
>> status function to display if the daemon is running or not. There are
>> 2 packages that I maintain (or have maintained) that use the status
>> for something else so it seem to be more standard than I thought. The
>> reload action seem to be quite common and standard. Later tonight,
>> I'll go through all the daemons in the repos and list what action they
>> have (except start/stop/restart) and what these actions do.  It'll
>> give use a better idea if there is a genereal concensus on these
>> various actions.
>>
>>>
>>>> Not only all daemons script have them but they all do the same
>>>> things: start/stop a binary before creating/removing a file
>>>> contatining the PID in a directory that needs root privileges.  I am
>>>> 100% sure that root privilege is required.  The other actions are not
>>>> used by all daemons and what they do depends on the daemon itself. So
>>>> we can't be really sure if root privileges are required or not.
>>>>
>>>> I decided to play it safe and to treat everything else than
>>>> start/stop/restart as edge cases.
>>> It makes sense, but I am forbidden to do
>>>
>>>>> I think we should offer a check_root function which can be called in
>>>>> rc scripts to ensure rootitude. Be we cannot generically know if a rc
>>>>> need to be root or not.
>>>>
>>>> That might be ideal.  But for the reason I mentionned above, the
>>>> daemon scripts will probably need to be modified to indicate what
>>>> privileges are needed for all actions.  Perhaps, demanding root
>>>> privileges by default except when there is a NEED_ROOT=0 defined in
>>>> the case, e.g.
>>> i agree. Idea is better.
>>>
>>>>
>>>>  status)
>>>>     NEED_ROOT=0
>>>>     do stuff that don't need root privs...
>>> Don't easy to implement in bash...
>>>
>>>> would reduce the workload on fixing the packages. That will surely
>>>> take some time before it's implemented unless that can be done without
>>>> modifying the daemon scripts.
>>> As said before, we can use functions scripts which is sourced by all
>>> rc.d scripts to make this check rather than doing it in rc.d
>>>
>>>>
>>>> Until this is done, this proposed patch is a good compromise between
>>>> the ideal situation and the current situation where no check is done
>>>> at all.
>>> Basically, there is maybe no problem to don't check. This is not done
>>> from the beginning.
>>> If you can, you can. If you need right it will fail. KISS.
>>>
>>>> The majority of users will use rc.d to start/stop/restart
>>>> daemons anyway.  Since you can use rc.d on many daemons at once, it's
>>>> better to abort the rc.d script early so you can rerun it with
>>>> sufficient privileges instead of having to wait for all the daemons to
>>>> fail.  It would also be trivial to remove this check in rc.d once (or
>>>> if) we have implemented something better.
>>>>
>>> I do not see the difference. If all daemons will need root privileges,
>>> they will fail.
>>>
>>> I think a lots of users will still use /etc/rc.d/x start/stop/restart,
>>> and it's kind of sugar around if you can or not run a daemon.
>>>
>>> I propose a patch following your recommendation here :
>>> https://github.com/seblu/arch-initscripts/commit/5a59a30
>>>
>>> What do you think?
>>
>> Your patch doesn't do what I was thinking mainly because my idea
>> wasn't a good one.  The problem is that the checking is done in
>> /etc/rc.d/functions which is sourced at the begining of the script so
>> the NEED_ROOT variable needs to be defined globally while I wanted a
>> local approach that would depend on the items in the case statement.
>> I don't know if it's clear but it doesn't matter because I found a
>> better solution that works without needing the NEED_ROOT  variable.
>>
>> We need to add in /etc/rc.d/functions your need_root function followed by:
>>
>> if [[ "$1" == 'start' || "$1" == 'stop' || "$1" == 'restart' ]]; then
>>        need_root
>> fi
>>
>> I only have start/stop/restart in that code sample but we'll probably
>> need to add more actions , like reload, depending on what I find in
>> the daemon investigation I described above.  I did tests with deamon
>> scripts and rc.d and it works without any modifications needed to
>> them.
>>
>> Couple drawbacks:
>>
>> - The list of actions to test for root is hard-coded. If I add a
>> foobar action which requires root privileges to a daemon, it'll need
>> to be added to the list in /etc/rc.d/functions. As this should happen
>> often, it would be minimal maintenance.
>>
>> - It suppose that all daemon using a given action need the same
>> privileges. If it's not the case, we'll need to either fix the
>> packages, enforce root privileges or go with the majority depnding on
>> the case.
>>
>> - When thinking about this, I only had the rc.d and daemon scripts in
>> mind and I monly testes these. If there are other scripts which
>> sources /etc/rc.d/functions, we'll need to check them to see if they
>> still work fine with my change.
>>
>
> Hi,
>
> Here's my findings.  Basically, all daemons actions need root
> privileges except a few of them:
>
> - iflist) and rtlist) from network daemon
> - reload) action for the dbus daemon.  As all other daemon require
> root privileges for reload, it will be best to enforce it for the
> reload action.
> - status).  A majority of daemon don't need root privilege for it but,
> again, there are exceptions:
>    - ufw: looks like a bug in its daemon file.  I'll submit a bug report.
>    - laptop-mode-tools: With user privs, you get some information
> about the system except the ones that need
>      direct access to the hard drive devices. Not really worth
> fixing. Let's ignore it.
>    - apcupsd: I can't test this one as I don't have a UPS.I get the
> same error message whether I have root
>      privileges or not so it might not require them.
>
> Long story short: I suggest enforcing root privileges for all daemon
> actions except: iflist, rtlist and status. So we have:
>
>
> need_root() {
>  (( $EUID != 0 )) && printf 'You need to be root.\n' && exit 1
> }
>
> if [[ "$1" == 'attach' || "$1" == 'careless_start' || "$1" == 'check' || \
>      "$1" == 'clean' || "$1" == 'condrestart' || "$1" == 'down' || \
>      "$1" == 'force-quit' || "$1" == 'force-reload' || "$1" ==
> 'force-restart' || \
>      "$1" == 'ifdown' || "$1" == 'ifup' || "$1" == 'load' || \
>      "$1" == 'logrotate' || "$1" == 'purge' || "$1" == 'reload' || \
>      "$1" == 'restart' || "$1" == 'resume' || "$1" == 'rtdown' ||
> "$1" == 'rtup' || \
>      "$1" == 'save' || "$1" == 'setup' || "$1" == 'start' || "$1" ==
> 'stop' || \
>      "$1" == 'suspend' || "$1" == 'unload' || "$1" == 'up' ]]; then
>       need_root
> fi
>
Hi,

Thanks for crawling initscripts to find problematic options.
You probably miss some case in aur, where rc.d scripts implement
"strange" options which need root priv, for example virtualbox_bin
which have a fixsub.

I really don't like this idea of hardcode which options should or
should not needs root priv in initscripts, because, I think we should
let rc.d scripts depends of initscripts package, and not initscripts
package depends of ufw, laptop-tools or others.

I also think we should be able to resume by a clear and simple policy like :
By default initscripts need to be root (Thomas in FS#24095). If you
want bypass this assumption to offer a by options (or by what you
want) right management, just disable and do what you want.
need_root is just a proposed generic helper to enforce root policy.
But, scripts maintainer can check if user who ask something is member
of a group to allow his script.

Intelligence is in the script, we should not try to predict what it
will do a in special cases (options). Let script maintainers choose.

Cheers,

-- 
Sébastien Luttringer
www.seblu.net