Some privilege reduction patches

[David Zeuthen]

Hi,

Sorry for the delay,

On Tue, 2006-02-14 at 12:58 +0100, Martin Pitt wrote: 
> Hi again,
> 
> Since the probes and callouts are now started from hald-runner (thanks
> to Sjoerd again!), the main hald doesn't need to call initgroups() any
> more. See 09_privileges-hald.patch.

Committed.

> Also, it is now reasonable to run some of the helpers with reduced
> privileges. 

Right. This makes a ton of sense.

> E. g. the ACPI helper doesn't need root privileges since
> it can happily read from acpid. This even helps to prevent some race
> conditions between acpid and hal which apparently crash acpid in
> Debian in some cases.  

As discussed some distributions don't ship acpid and I don't want hal to
depend on it. I also don't want hald to be restarted on package
upgrades, but, eh, that's an old discussion...

Btw, with my GNOME hat on, I remark that some day g-p-m will be able to
completely replace acpid, we just need the infrastructure for running
policy pieces when no user is logged in. I rambled about that here

http://blog.fubar.dk/?p=63

Until then I guess that one may configure hald to always connect to
acpid.

> Likewise, I added privilege dropping to addon-storage.c; for polling
> CD-ROMs, USB devices, and the like, the respective device groups
> should be sufficient, i. e. the same groups that hald itself ran in in
> earlier versions. 

Right. It does require that all device files can be accessed by the one
of the groups hal is in (assume "disk" for now). This requires two
things:

 1) Linux distros needs configure udev to make sure group 'disk' owns
    the device file and it's at least readable. Questions..

    - does this happen in the default upstream udev configuration? Kay?
      It works on Fedora

       $ ls -l /dev/hda
       brw-r----- 1 root disk 3, 0 Feb 16 14:33 /dev/hda

      but I wonder if it's a show stopper that group 'disk' cannot
      write? Probably not, I don't think we do ioctl's on hard disks.

    - what if someone changes ownership? I know on Fedora

       $ ls -l /dev/hdc
       brw------- 1 davidz disk 22, 0 Feb 16 14:33 /dev/hdc

      Ugh, so this won't work.. Group 'disk' cannot even read, and I
      think we want to write as well to do ioctl's on optical drives.
      Right?

 2) Need to make sure that the haldaemon user (or 'hal' on Debian) is
    member of the appropriate groups.. Not the case right now on Fedora,
    definitely fixable in the SPEC file, but something we need to make
    very clear to distributors.

I'm not against doing this per se, don't get me wrong, I just want to
understand the full ramifications.

> The latter two modifications are contained in
> 08_privileges-addons.patch. However, I don't consider them ready for
> upstream inclusion so far. Currently it's just blunt code duplication
> from the original drop_privileges() function in hald.c, I would rather
> like to move that code into a library. If you are interested in the
> general approach, I would like to generalize that function and put it
> into hald/linux2/probing/shared.h, and also modify the other addons as
> appropriate.

Yes, moving things to shared.h sounds good - for the time being it is
our 'library' though we may move it to a real library at some point :-)

> Last, a question: do you have any prefered strategy how to implement
> sanity checking in the hal-system-storage-* scripts? We don't
> currently ship them since they do not do any checking on their own
> (they just use the hal properties, which are unreliable in the current
> trust model). So, if the privilege separation code should have any
> sense, all callouts have to do input sanity checking on their own. I
> would like to work on this if you want, I just want to make sure that
> nobody else does ATM.

Yes, let me guess.. Your prime concern is that in a compromised hald,
for example the property volume.fstype contains "`/some/evil/binary`"
and a shell script callout expands $HAL_PROP_VOLUME_FSTYPE and then the
attacker successfully runs /some/evil/binary?

One idea; would it be sufficient to do e.g.

 FSTYPE=${HAL_PROP_VOLUME_FSTYPE//[^a-zA-Z0-9_=]/_}

and just use e.g. $FSTYPE instead of $HAL_PROP_VOLUME_FSTYPE in our
scripts? I think so.

Apart from that, also some basic sanity checking like ensuring that the
device file is prepended with "/dev" and other stuff

Something like that?

Cheers,
David