[syslog-ng] [PATCH] Use CAP_SYSLOG instead of CAP_SYS_ADMIN, if available.

[Balazs Scheidler]

Hi,

Thanks for the great stuff, it is really appreciated.

I've some notes, please let me know if you or Serge would like to do
that, or I should.

Nothing extraordinary, the mass of the work has already been done, I
just prefer some things a bit differently.

Thanks.

On Sun, 2011-05-01 at 09:46 +0200, Gergely Nagy wrote:
> If cap_syslog exists, the kernel will complain (once) that we only
> have cap_sys_admin.  Additionally, using cap_syslog instead of
> cap_sys_admin significantly lowers the unneeded privs we are
> using.
> 
> Changelog:
> 	v2: At startup, detect whether libcap knows about CAP_SYSLOG.
> 	    (Thanks to Gergely Nagy for pointing out that case)
> 	v3: Andrew Morgan pointed out a nice way to detect whether
> 	    the kernel has CAP_SYSLOG.  Thanks, Andrew!
> 
> Signed-off-by: Serge Hallyn <serge.hallyn at ubuntu.com>
> Signed-off-by: Gergely Nagy <algernon at balabit.hu>
> ---
>  lib/gprocess.c          |   48 ++++++++++++++++++++++++++++++++++++++++++++++-
>  lib/gprocess.h          |    2 +-
>  modules/affile/affile.c |    4 +++
>  syslog-ng/main.c        |   10 ++------
>  4 files changed, 55 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/gprocess.c b/lib/gprocess.c
> index d9a2dfb..4a4ce52 100644
> --- a/lib/gprocess.c
> +++ b/lib/gprocess.c
> @@ -98,6 +98,7 @@ static gint startup_result_pipe[2] = { -1, -1 };
>  static gint init_result_pipe[2] = { -1, -1 };
>  static GProcessKind process_kind = G_PK_STARTUP;
>  static gboolean stderr_present = TRUE;
> +static int have_capsyslog = FALSE;
>  
>  /* global variables */
>  static struct
> @@ -210,6 +211,14 @@ g_process_cap_modify(int capability, int onoff)
>    if (!process_opts.caps)
>      return TRUE;
>  
> +#ifdef CAP_SYSLOG
> +  /*
> +   * if libcap or kernel doesn't support cap_syslog, then resort to
> +   * cap_sys_admin
> +   */
> +  if (capability == CAP_SYSLOG && !have_capsyslog)
> +    capability = CAP_SYS_ADMIN;
> +#endif

could you check whether CAP_SYSLOG exists in the gprocess.h header file,
and if it doesn't define it to an extremal value, so that call-sites do
not have to be littered with #ifdef CAP_SYSLOG?

Something like:

#ifndef CAP_SYSLOG
#define CAP_SYSLOG -1
#endif

Then in g_process_cap_modify() we could to the translation based on
whether CAP_SYSLOG is -1 or a sane value.

>    caps = cap_get_proc();
>    if (!caps)
>      return FALSE;
> @@ -416,7 +425,7 @@ g_process_set_working_dir(const gchar *cwd)
>   * capability set. The process will change its capabilities to this value
>   * during startup, provided it has enough permissions to do so.
>   **/
> -void 
> +static void
>  g_process_set_caps(const gchar *caps)
>  {
>    if (!process_opts.caps)
> @@ -879,6 +888,43 @@ g_process_change_dir(void)
>  }
>  
>  /**
> + * g_process_setup_caps(void)
> + *
> + * NOTE: polling /proc/kmsg requires cap_sys_admin, otherwise it'll always
> + * indicate readability. Enabling/disabling cap_sys_admin on every poll
> + * invocation seems to be too expensive. So I enable it for now.
> + */
> +void
> +g_process_setup_caps(void)
> +{
> +  int ret;
> +  gchar * capsstr = "cap_net_bind_service,cap_net_broadcast,cap_net_raw,"
> +                   "cap_dac_read_search,cap_dac_override,cap_chown,cap_fowner=p "
> +                   "cap_sys_admin=ep";
> +#ifdef CAP_SYSLOG
> +  cap_t caps;
> +
> +  /* Check whether cap_syslog exists.  If not, get cap_sys_admin, else get
> +   * cap_syslog */
> +  caps = cap_from_text("cap_syslog=p");
> +  if (caps) {
> +    cap_free(caps);
> +    /* libcap knows it, does the kernel? */
> +    ret = prctl(PR_CAPBSET_READ, CAP_SYSLOG);
> +    if (ret != -1) {
> +	    /* kernel knows cap_syslog!  Use it */
> +	    capsstr = "cap_net_bind_service,cap_net_broadcast,cap_net_raw,"
> +		      "cap_dac_read_search,cap_dac_override,cap_chown,cap_fowner=p "
> +		      "cap_syslog=ep";
> +	    have_capsyslog = TRUE;
> +    }
> +  }
> +#endif
> +
> +  g_process_set_caps(capsstr);
> +}

I would prefer to construct the set of permissions needed for syslog-ng
to be constructed in the main.c file, e.g. the detection code should be
moved into a 

gboolean g_process_is_cap_supported(cap_t cap)

Then in the main.c file (perhaps in a separate function), construct the
required set of caps, and set by g_process_set_caps(). (e.g don't make
it static and remove the g_process_setup_caps() stuff.

Also note that the user can override the capset of syslog-ng using the
--caps option.

> +
> +/**
>   * g_process_send_result:
>   * @ret_num: exit code of the process
>   *
> diff --git a/lib/gprocess.h b/lib/gprocess.h
> index cda35b0..4ad0866 100644
> --- a/lib/gprocess.h
> +++ b/lib/gprocess.h
> @@ -66,7 +66,7 @@ void g_process_set_chroot(const gchar *chroot);
>  void g_process_set_pidfile(const gchar *pidfile);
>  void g_process_set_pidfile_dir(const gchar *pidfile_dir);
>  void g_process_set_working_dir(const gchar *cwd);
> -void g_process_set_caps(const gchar *caps);
> +void g_process_setup_caps(void);
>  void g_process_set_argv_space(gint argc, gchar **argv);
>  void g_process_set_use_fdlimit(gboolean use);
>  void g_process_set_check(gint check_period, gboolean (*check_fn)(void));
> diff --git a/modules/affile/affile.c b/modules/affile/affile.c
> index 67a4632..5c56bc5 100644
> --- a/modules/affile/affile.c
> +++ b/modules/affile/affile.c
> @@ -59,7 +59,11 @@ affile_open_file(gchar *name, gint flags,
>    if (privileged)
>      {
>        g_process_cap_modify(CAP_DAC_READ_SEARCH, TRUE);
> +#ifdef CAP_SYSLOG
> +      g_process_cap_modify(CAP_SYSLOG, TRUE);
> +#else
>        g_process_cap_modify(CAP_SYS_ADMIN, TRUE);
> +#endif
>      }

This would then become g_process_cap_modify(CAP_SYSLOG), the
CAP_SYS_ADMIN is not needed.

>    else
>      {
> diff --git a/syslog-ng/main.c b/syslog-ng/main.c
> index 7e81f2f..5e62c1f 100644
> --- a/syslog-ng/main.c
> +++ b/syslog-ng/main.c
> @@ -167,13 +167,9 @@ main(int argc, char *argv[])
>  
>    g_process_set_argv_space(argc, (gchar **) argv);
>    
> -  /* NOTE: polling /proc/kmsg requires cap_sys_admin, otherwise it'll always
> -   * indicate readability. Enabling/disabling cap_sys_admin on every poll
> -   * invocation seems to be too expensive. So I enable it for now. */
> -  
> -  g_process_set_caps("cap_net_bind_service,cap_net_broadcast,cap_net_raw,"
> -                     "cap_dac_read_search,cap_dac_override,cap_chown,cap_fowner=p "
> -                     "cap_sys_admin=ep");
> +  /* Set up the minimal privilege we'll need */
> +  g_process_setup_caps();
> +
>    ctx = g_option_context_new("syslog-ng");
>    g_process_add_option_group(ctx);
>    msg_add_option_group(ctx);

-- 
Bazsi