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

Wed Jun 1 20:30:17 EDT 2011

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

Eric

>>
>>
>> --
>> Sébastien Luttringer
>> www.seblu.net
>>
>