Created attachment 88559 [details] [review] modify the original code to get the selinux class value and perm from seinux policy When I use Fedora 19 to write some selinux policy based on MDP which is provided by kernel.There is something wrong with dbus, the error message in dmesg is "SELinux: Invalid class 52", and I found the definition of dbus in falsk.h is "#define SECCLASS_DBUS 52". Because current dbus's code use the harded code selinux class value, the definition of "dbus" class must be 52nd in every selinux policy. SELinux can dynamically discover class and permission values upon policy load, the order for the selilnux class is not important. I think we should get the class value and perm access vector bit according to the selinux policy, instead of the hard coded value from flask.h and av_permission.h. What do you think of it?The attachment is the code that I have modified to achieve this function.
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. :)
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.