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

[Gergely Nagy]

Balazs Scheidler <bazsi at balabit.hu> writes:

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

I'll be sending a patch in a few moments, but I've done a few things a
bit differently than you suggested, so I'd like to explain that in
advance.

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

This is something I've done differently: I introduced
g_process_check_cap_syslog(), which checks for CAP_SYSLOG presence, and
sets have_capsyslog to TRUE if it is found. This was done so that
g_process_cap_modify() wouldn't have to do it itself.

But come to think of it, I could've done something like this (in
lib/gprocess.c):

static gint cap_syslog_override = -1;

And in g_process_cap_modify():

if (capability == CAP_SYSLOG)
  {
    if (cap_syslog_override != -1)
      capability = cap_syslog_override;
    else
      {
        if (g_process_is_cap_supported(CAP_SYSLOG))
          cap_syslog_override = CAP_SYSLOG;
        else
          cap_syslog_override = CAP_SYS_ADMIN;
      }
  }

Or something similar...

This would have the advantage of having a generic
g_process_is_cap_supported(), with the downside of the code being
longer. And for reasons I can't properly explain, I like the
_check_cap_syslog() solution better.

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

With my solution, that doesn't need special handling, I believe. But I
haven't tested all scenarios.

-- 
|8]