Bug 71187

Comment on attachment 88559 [details] [review]
modify the original code to get the selinux class value and perm from seinux policy

Review of attachment 88559 [details] [review]:
-----------------------------------------------------------------

I can't comment on the functionality, whether we support policies not based on the reference policy, or whether the numeric values of class values are considered to be "API" analogous to the numeric values of port numbers (Colin, Lennart, perhaps one of you could?) but here is a review from a coding style point of view.

If I see carelessly-written comments/documentation, I tend to assume that the code is also carelessly-written; please be careful with all aspects of a change.

> Dbus selinux

It's called D-Bus.

(Having said that, it's also unnecessary to specify that a commit to the dbus project affects D-Bus - what else is there? :-)

> From: "osmond.sun" <osmond.sun@gmail.com>

If your name is Osmond Sun (as opposed to "osmond.sun"), please configure git accordingly.

::: bus/selinux.c
@@ +509,5 @@
>      }
> +  get_class_ret = bus_selinux_get_class_perm_value ("dbus",
> +						"acquire_svc",
> +						&class_dbus,
> +						&dbus_av_perm);

Please indent new code with spaces, not tabs. Something seems to have gone wrong with the indentation here: are you assuming that a tab has a size other than 8 spaces?

@@ +522,5 @@
> +    }
> +  else
> +   {
> +     ret = (get_class_ret == 1) ? TRUE : FALSE;
> +   }

The indentation here is weird. I think you're missing 1 space per line on the "else" branch.

@@ +541,5 @@
> +/**
> + * Get dbus's class value and it's access vecotr bit from policy instead of
> + * hard coded.
> + * If can't get find the class from the policy ,use deny_unknown bit to
> + * determine the queries

"its access vector bit"

"policy, use" (space after comma, not before)

"If we can't find the class" or something

@@ +543,5 @@
> + * hard coded.
> + * If can't get find the class from the policy ,use deny_unknown bit to
> + * determine the queries
> + *
> + * @param class_str the class to get,here will always be "dbus"

If it will always be "dbus", why is it a parameter at all?

@@ +546,5 @@
> + *
> + * @param class_str the class to get,here will always be "dbus"
> + * @param perm_str the access vector name
> + * @param class_dbus the class value corresponding to the string name class_str
> + * @param dbus_perm the access vector bit corresponding the string perm_str

If these two are "out" parameters, "@param class_dbus used to return the class value ..." would be better.

@@ +549,5 @@
> + * @param class_dbus the class value corresponding to the string name class_str
> + * @param dbus_perm the access vector bit corresponding the string perm_str
> + * @returns 	0 get the class value and access vector success
> + * 		-1 deny all the request
> + * 		1 allow all the request

If the logic is correct, then I think this should return an enum something like this:

typedef enum {
    CONTINUE_WITH_CHECK,
    DENY_ALL,
    ALLOW_ALL
} GetClassPermResult;

@@ +552,5 @@
> + * 		-1 deny all the request
> + * 		1 allow all the request
> + */
> +int
> +bus_selinux_get_class_perm_value(char * class_str,

General coding style: "foo (bar)" (space before parenthesis).

@@ +560,5 @@
> +{
> +  int deny_unknown;
> +  if ((deny_unknown = security_deny_unknown()) == -1)
> +    {
> +      _dbus_warn("Error getting deny_unknown status.");

How could this happen?

If it's possible that this happens due to sysadmin misconfiguration, please use _dbus_system_log() to put a message in the syslog/Journal, and use a more informative message.

Also, if security_deny_unknown() sets errno or some similar error indicator, please use it.

Putting those together, maybe something like this?

    _dbus_system_log (DBUS_SYSTEM_LOG_SECURITY,
                      "Could not determine SELinux policy, assuming unknown actions should be denied (security_deny_unknown(): %s)",
                      strerror (errno));

@@ +561,5 @@
> +  int deny_unknown;
> +  if ((deny_unknown = security_deny_unknown()) == -1)
> +    {
> +      _dbus_warn("Error getting deny_unknown status.");
> +		//if security_deny_unknown error ,we treat the queries as being denied;

We use /* C89 comments */ not // C++ comments.

@@ +566,5 @@
> +		deny_unknown = 1;
> +    }
> +  if ((*class_dbus = string_to_security_class(class_str)) == 0)
> +    {
> +      _dbus_warn("Error getting security class \"%s\" value.",class_str);

The same comments.

I prefer to avoid assignments inside conditionals, and I also prefer to avoid "out" parameters that can't be NULL, so maybe something like this:

    cls = string_to_security_class ("dbus");

    if (cls == 0)
      {
        _dbus_system_log (...);
        goto error;
      }
    else
      {
        if (class_dbus != NULL)
          *class_dbus = cls;
      }

The same comments apply to getting dbus_perm.

I sent a mail to the SELinux mailing list about review of this bug.

Created attachment 88664 [details]
get the selinux class value and perm from seinux policy

style fixups for first patch

Thanks, Simon.
I have rewritten the patch.
(In reply to comment #1)
> Comment on attachment 88559 [details] [review] [review]
> modify the original code to get the selinux class value and perm from seinux
> policy
> 
> Review of attachment 88559 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> I can't comment on the functionality, whether we support policies not based
> on the reference policy, or whether the numeric values of class values are
> considered to be "API" analogous to the numeric values of port numbers
> (Colin, Lennart, perhaps one of you could?) but here is a review from a
> coding style point of view.
> 
> If I see carelessly-written comments/documentation, I tend to assume that
> the code is also carelessly-written; please be careful with all aspects of a
> change.
> 
> > Dbus selinux
> 
> It's called D-Bus.
> 
> (Having said that, it's also unnecessary to specify that a commit to the
> dbus project affects D-Bus - what else is there? :-)
> 
> > From: "osmond.sun" <osmond.sun@gmail.com>
> 
> If your name is Osmond Sun (as opposed to "osmond.sun"), please configure
> git accordingly.
> 
> ::: bus/selinux.c
> @@ +509,5 @@
> >      }
> > +  get_class_ret = bus_selinux_get_class_perm_value ("dbus",
> > +						"acquire_svc",
> > +						&class_dbus,
> > +						&dbus_av_perm);
> 
> Please indent new code with spaces, not tabs. Something seems to have gone
> wrong with the indentation here: are you assuming that a tab has a size
> other than 8 spaces?
> 
> @@ +522,5 @@
> > +    }
> > +  else
> > +   {
> > +     ret = (get_class_ret == 1) ? TRUE : FALSE;
> > +   }
> 
> The indentation here is weird. I think you're missing 1 space per line on
> the "else" branch.
> 
> @@ +541,5 @@
> > +/**
> > + * Get dbus's class value and it's access vecotr bit from policy instead of
> > + * hard coded.
> > + * If can't get find the class from the policy ,use deny_unknown bit to
> > + * determine the queries
> 
> "its access vector bit"
> 
> "policy, use" (space after comma, not before)
> 
> "If we can't find the class" or something
> 
> @@ +543,5 @@
> > + * hard coded.
> > + * If can't get find the class from the policy ,use deny_unknown bit to
> > + * determine the queries
> > + *
> > + * @param class_str the class to get,here will always be "dbus"
> 
> If it will always be "dbus", why is it a parameter at all?
> 
> @@ +546,5 @@
> > + *
> > + * @param class_str the class to get,here will always be "dbus"
> > + * @param perm_str the access vector name
> > + * @param class_dbus the class value corresponding to the string name class_str
> > + * @param dbus_perm the access vector bit corresponding the string perm_str
> 
> If these two are "out" parameters, "@param class_dbus used to return the
> class value ..." would be better.
> 
> @@ +549,5 @@
> > + * @param class_dbus the class value corresponding to the string name class_str
> > + * @param dbus_perm the access vector bit corresponding the string perm_str
> > + * @returns 	0 get the class value and access vector success
> > + * 		-1 deny all the request
> > + * 		1 allow all the request
> 
> If the logic is correct, then I think this should return an enum something
> like this:
> 
> typedef enum {
>     CONTINUE_WITH_CHECK,
>     DENY_ALL,
>     ALLOW_ALL
> } GetClassPermResult;
> 
> @@ +552,5 @@
> > + * 		-1 deny all the request
> > + * 		1 allow all the request
> > + */
> > +int
> > +bus_selinux_get_class_perm_value(char * class_str,
> 
> General coding style: "foo (bar)" (space before parenthesis).
> 
> @@ +560,5 @@
> > +{
> > +  int deny_unknown;
> > +  if ((deny_unknown = security_deny_unknown()) == -1)
> > +    {
> > +      _dbus_warn("Error getting deny_unknown status.");
> 
> How could this happen?
> 
> If it's possible that this happens due to sysadmin misconfiguration, please
> use _dbus_system_log() to put a message in the syslog/Journal, and use a
> more informative message.
> 
> Also, if security_deny_unknown() sets errno or some similar error indicator,
> please use it.
> 
> Putting those together, maybe something like this?
> 
>     _dbus_system_log (DBUS_SYSTEM_LOG_SECURITY,
>                       "Could not determine SELinux policy, assuming unknown
> actions should be denied (security_deny_unknown(): %s)",
>                       strerror (errno));
> 
> @@ +561,5 @@
> > +  int deny_unknown;
> > +  if ((deny_unknown = security_deny_unknown()) == -1)
> > +    {
> > +      _dbus_warn("Error getting deny_unknown status.");
> > +		//if security_deny_unknown error ,we treat the queries as being denied;
> 
> We use /* C89 comments */ not // C++ comments.
> 
> @@ +566,5 @@
> > +		deny_unknown = 1;
> > +    }
> > +  if ((*class_dbus = string_to_security_class(class_str)) == 0)
> > +    {
> > +      _dbus_warn("Error getting security class \"%s\" value.",class_str);
> 
> The same comments.
> 
> I prefer to avoid assignments inside conditionals, and I also prefer to
> avoid "out" parameters that can't be NULL, so maybe something like this:
> 
>     cls = string_to_security_class ("dbus");
> 
>     if (cls == 0)
>       {
>         _dbus_system_log (...);
>         goto error;
>       }
>     else
>       {
>         if (class_dbus != NULL)
>           *class_dbus = cls;
>       }
> 
> The same comments apply to getting dbus_perm.

See Stephen's comment here: http://marc.info/?l=selinux&m=138358514311952&w=2

Created attachment 88719 [details]
use selinux_set_mapping to create a mapping from class perm indices and the policy values

rewrite a patch according to the XSELinux.

(In reply to comment #6)
> Created attachment 88719 [details]
> use selinux_set_mapping to create a mapping from class perm indices and the
> policy values
> 
> rewrite a patch according to the XSELinux.

Ok, I read Stephen's reply and studied selinux_set_mapping(), after doing so this patch makes sense to me.  And it is a *lot* simpler.

I tested it by rebuilding the Fedora 19 dbus with it, and it appears to work well.

Reviewed-By: Colin Walters <walters@verbum.org>
Tested-By: Colin Walters <walters@verbum.org>

Ok, I'm going to push this with just a few changes:

* Update commit message to match DBus guidelines (topic prefix, Bug: link)
* Reword commit message for clarity
* Moved the #defines into selinux.c since they're only used there.
* Added a warning that the #defines must exactly match the structure array.

http://cgit.freedesktop.org/dbus/dbus/commit/?id=ba088208bc0c35ca418a097a8482c4a7705f4a43

(In reply to comment #8)
> Ok, I'm going to push this with just a few changes:
> 
> * Update commit message to match DBus guidelines (topic prefix, Bug: link)
> * Reword commit message for clarity
> * Moved the #defines into selinux.c since they're only used there.
> * Added a warning that the #defines must exactly match the structure array.

Colin, thanks for you changes. :)