Re: [Utopia] Updated logging and security patches for hal 0.2.97

[David Zeuthen <david fubar dk>]

On Tue, 2004-08-31 at 12:39 +0200, Martin Pitt wrote:
> Hi David, hi Utopia readers!
> 
> I updated my patches for the upstream version 0.2.97. Since it is
> tedious to maintain them properly against the rapidly chaning hal
> code, would you consider applying at least some of them? 

Yeah. Sorry for the lag btw. 

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.

> 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?

> 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.

> 02_ioctl_errors.patch: intercept unchecked ioctl calls and log
> failures
> 

Applied; this is useful, 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

> +    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) and removed the trailing '\n' here and many other places.

> +    /* 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.

> 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.

Thanks,
David