Hi David!
On 2004-09-03 20:41 +0200, David Zeuthen wrote:
> I do think this is important to work on, however, as it is right now,
> the callouts need to run as root and thus, effectively, hald needs to
> run as root. I've discussed earlier having a helper process to do the
> callouts; there might be other ways though.
Our Company's distribution does use another way; we do not mangle
fstab, but use a suid root wrapper around mount that allows users to
mount removeable devices if a certain policy is fulfilled. This way,
the amount of code that runs as root is kept to a minimum, fstab is
not touched any more and hald can run as normal user.
> > Most of the
> > stuff fixes bugs and is completely independent from privilege dropping
> > anyway and none of the patches are Debian specific.
>
> How does this work; is the Debian package using --drop-privileges as
> default?
By now these changes are not contained in Debian, they are just
proposed as a patch (I'm not the maintainer of hal in Debian).
However, our Company's distribution uses this modified package for
quite a while now.
The patch proposed to Debian asks a Debconf question (default no)
whether hald should run as root. Our company then just needs to change
the default to yes and don't show the question.
> > 01_log_to_syslog.patch: actually log to syslog, not to the void
> >
>
> This I don't think is a good idea; hald should never log to the system
> log, not even if an ioctl fails. If there is a bug in hald ask the user
> to run with --daemon=no --verbose=yes.
If that works, it is fine for me. Syslog is just mentioned in the code
(or at least, was at the time I did the initial patches) and I needed
this to debug hal (see below).
> > 02_ioctl_errors.patch: intercept unchecked ioctl calls and log
> > failures
> >
>
> Applied; this is useful, thanks.
Thanks.
>
> > 03_drop_privileges.patch: add option --drop-privileges which causes
> > hald not to run as root, but as @HAL_USER@ in @HAL_GROUP@ and all
> > additional groups set in /etc/group, and keeping the necessary
> > capabilities to do its job. This does _not_ change the default
> > behaviour, if the option is not specified, hald runs as root, as
> > before.
>
> I've applied this with some changes:
>
> > +
> > +/**
> > + * Drop all but necessary privileges from hald when it runs as root.
> > Set the
> > + * running user id to 'hal' and groups to 'disk', 'floppy', and
> > 'cdrom'. Drops
> > + * all unnecessary capabilities. On error, a message is sent to
> > syslog and
> > + * the program terminates with exit code -1.
> > + */
> > +void
> > +drop_privileges()
> >
>
> The function can be static and the Doxygen comment is wrong
Correct, the function itself does not set any specific groups, but
just calls initgroups() (if I remember correctly). If you modified
this, it is fine for me. Thanks.
> > + pw = getpwnam( HAL_USER );
> > + if( !pw ) {
> > + HAL_ERROR(( "drop_privileges: user " HAL_USER " does not exist
> > \n" ));
> > + exit( -1 );
> > + }
>
> I've fixed up wrong use of whitespace (in relation to the rest of the
> code)
Sorry; I tend to fall back to this style, it is my normal style of
programming.
> and removed the trailing '\n' here and many other places.
Fine for me.
> > + /* only keep necessary capabilities */
> > + cap = cap_from_text( "cap_net_admin=ep" );
>
> Are you sure we don't need other capabilities? I can't think of any
> though.
That's why I enabled logging and the additional ioctl() error logging:
I kept adding groups and capabilities until the syslog of hald running
as root and as user were identical. Of course I don't have every
possible device, but so far hald runs fine like this.
> > 04_fix_hal_conf_in.patch: replace hardcoded policy user "root" by
> > "@HAL_USER@".
> >
>
> This is wrong; now root can't own the service. I've fixed this so
> both HAL_USER and root can own the service.
So you doubled the policy stanzas, one for each user? But the build
system should enter the correct user depending on the presence or
absence of --with-hal-user, doesn't it?
Our Debian package just uses the .in file and dynamically fills in the
@HAL_USER@ templates according to the Debconf answer, so I did not
notice that.
> Thanks, David
Thanks for applying these changes! It is really a great relief for
future versions.
Have a nice weekend!
Martin
--
Martin Pitt Debian GNU/Linux Developer
martin piware de mpitt debian org
http://www.piware.de http://www.debian.org
Attachment:
signature.asc
Description: Digital signature