[busybox:00323] Re: [PATCH 4/8] busybox -- libselinux utilities applets
KaiGai Kohei
kaigai at ak.jp.nec.com
Wed Jan 31 21:44:22 PST 2007
Bernhard, Thanks for your comments.
Bernhard Fischer wrote:
> On Wed, Jan 31, 2007 at 09:13:47PM +0900, KaiGai Kohei wrote:
>> Bernhard, Thanks for your comments.
>>
>> The attached patch fixes following items:
>> - avcstat and togglesebool applet were removed
>> - xis_selinux_enabled() was added at libbb/xfuncs.c
>> - unneccesary headers were removed.
>> - bb_error_msg_and_die() + strerror() were replaced
>> with bb_perror_msg_and_die()
>> - "Selinux Utilities" at menuconfig got dependency with CONFIG_SELINUX
>> - some cleanups.
>>
> [snip]
>> Index: sebusybox-libselinux-0131/libbb/xfuncs.c
>> =================================>> --- sebusybox-libselinux-0131/libbb/xfuncs.c (revision 17684)
>> +++ sebusybox-libselinux-0131/libbb/xfuncs.c (working copy)
>> @@ -574,6 +574,17 @@
>> bb_perror_msg_and_die("can't stat '%s'", name);
>> }
>>
>> +// xis_selinux_enabled() - die if SELinux is disabled.
>> +void xis_selinux_enabled(void)
>> +{
>> +#ifdef CONFIG_SELINUX
>
> Please rather use #if ENABLE_SELINUX
OK, Fixed.
>> + if (!is_selinux_enabled())
>> + bb_error_msg_and_die("SELinux is disabled");
>> +#else
>> + bb_error_msg_and_die("SELinux support is disabled");
>> +#endif
>> +}
It have a possibility to print incorrect message when is_selinux_enabled()
return -1. It may happen when libselinux could not open "/proc/filesystem"
due to resource limitation and so on.
>> /* It is perfectly ok to pass in a NULL for either width or for
>> * height, in which case that value will not be set. */
>> int get_terminal_width_height(const int fd, int *width, int *height)
>> Index: sebusybox-libselinux-0131/Makefile
>> =================================>> --- sebusybox-libselinux-0131/Makefile (revision 17684)
>> +++ sebusybox-libselinux-0131/Makefile (working copy)
>> @@ -442,6 +442,7 @@
>> networking/udhcp/ \
>> procps/ \
>> runit/ \
>> + selinux/ \
>> shell/ \
>> sysklogd/ \
>> util-linux/ \
>> Index: sebusybox-libselinux-0131/include/libbb.h
>> =================================>> --- sebusybox-libselinux-0131/include/libbb.h (revision 17684)
>> +++ sebusybox-libselinux-0131/include/libbb.h (working copy)
>> @@ -571,6 +571,7 @@
>> extern void renew_current_security_context(void);
>> extern void set_current_security_context(security_context_t sid);
>> #endif
>> +extern void xis_selinux_enabled(void);
>> extern int restricted_shell(const char *shell);
>> extern void setup_environment(const char *shell, int loginshell, int changeenv, const struct passwd *pw);
>> extern int correct_password(const struct passwd *pw);
>> Index: sebusybox-libselinux-0131/include/usage.h
>> =================================>> --- sebusybox-libselinux-0131/include/usage.h (revision 17684)
>> +++ sebusybox-libselinux-0131/include/usage.h (working copy)
>> @@ -1013,6 +1013,9 @@
>> " -6 When using port/proto only search IPv6 space\n" \
>> " -SIGNAL When used with -k, this signal will be used to kill"
>>
>> +#define getenforce_trivial_usage
>> +#define getenforce_full_usage
>> +
>> #define getopt_trivial_usage \
>> "[OPTIONS]..."
>> #define getopt_full_usage \
>> @@ -1047,6 +1050,11 @@
>> " esac\n" \
>> "done\n"
>>
>> +#define getsebool_trivial_usage \
>> + "-a or getsebool boolean..."
>> +#define getsebool_full_usage \
>> + "-a Show all SELinux booleans."
>> +
>> #define getty_trivial_usage \
>> "[OPTIONS]... baud_rate,... line [termtype]"
>> #define getty_full_usage \
>> @@ -1896,6 +1904,15 @@
>> "/dev/hda[0-15]\n"
>> #endif
>>
>> +#define matchpathcon_trivial_usage \
>> + "[-n] [-N] [-f file_contexts_file] [-p prefix] [-V]"
>> +#define matchpathcon_full_usage \
>> + "\t-n Do not display path.\n" \
>> + "\t-N Do not use translations.\n" \
>> + "\t-f file_context_file Use alternate file_context file\n" \
>> + "\t-p prefix Use prefix to speed translations\n" \
>> + "\t-V Verify file context on disk matches defaults"
>
> This isn't easily readable. Perhaps put some \t\t before the explanation
Did the comment means that the following style is more preferable?
If so, it was fixed.
"\t-n\tDo not display path.\n" \
"\t-N\tDo not use translations.\n" \
"\t-f\tfile_context_file Use alternate file_context file\n" \
"\t-p\tprefix Use prefix to speed translations\n" \
"\t-V\tVerify file context on disk matches defaults"
- snip -
>> =================================>> --- sebusybox-libselinux-0131/selinux/getenforce.c (revision 0)
>> +++ sebusybox-libselinux-0131/selinux/getenforce.c (revision 0)
>> @@ -0,0 +1,33 @@
>> +/*
>> + * getenforce
>> + *
>> + * Based on libselinux 1.33.1
>> + * Port to BusyBox Hiroshi Shinji <shiroshi at my.email.ne.jp>
>> + *
>> + */
>> +
>> +#include "busybox.h"
>> +
>> +int getenforce_main(int argc, char **argv)
>> +{
>> + int rc;
>> +
> ->+ rc > ->+ if (rc < 0)
> ->+ bb_error_msg_and_die("is_selinux_enabled() failed");
>
> That's exactly the place where you should use xis_selinux_enabled(). It
> obviously has to return an int due to this :)
>
> xis_selinux_enabled();
It prints different message.
getenforce have to print "Disabled", when SELinux is disabled.
But xis_selinux_enabled() print "SELinux is disabled".
If shell script refers the output, it easily become a incompatibility
between up-streamed getenforce and busybox's one.
> ->+ if (rc = 1) {
>> + rc >> + if (rc < 0)
>> + bb_error_msg_and_die("getenforce() failed");
>> +
>> + if (rc)
>> + puts("Enforcing");
>> + else
>> + puts("Permissive");
> ->+ } else {
> ->+ puts("Disabled");
> ->+ }
>> +
>> + return 0;
>> +}
>> Index: sebusybox-libselinux-0131/selinux/selinuxenabled.c
>> =================================>> --- sebusybox-libselinux-0131/selinux/selinuxenabled.c (revision 0)
>> +++ sebusybox-libselinux-0131/selinux/selinuxenabled.c (revision 0)
>> @@ -0,0 +1,13 @@
>> +/*
>> + * selinuxenabled
>> + *
>> + * Based on libselinux 1.33.1
>> + * Port to BusyBox Hiroshi Shinji <shiroshi at my.email.ne.jp>
>> + *
>> + */
>> +#include "busybox.h"
>> +
>> +int selinuxenabled_main(int argc, char **argv)
>> +{
>> + return !is_selinux_enabled();
>> +}
>> Index: sebusybox-libselinux-0131/selinux/getsebool.c
>> =================================>> --- sebusybox-libselinux-0131/selinux/getsebool.c (revision 0)
>> +++ sebusybox-libselinux-0131/selinux/getsebool.c (revision 0)
>> @@ -0,0 +1,73 @@
>> +/*
>> + * getsebool
>> + *
>> + * Based on libselinux 1.33.1
>> + * Port to BusyBox Hiroshi Shinji <shiroshi at my.email.ne.jp>
>> + *
>> + */
>> +
>> +#include "busybox.h"
>> +
>> +#define GETSEBOOL_OPT_ALL 1
>> +
>> +int getsebool_main(int argc, char **argv)
>> +{
>> + int i, rc >> + char **names;
>> + unsigned long opt;
>> +
>> + opt >> +
>> + xis_selinux_enabled();
>> +
>> + if (opt & GETSEBOOL_OPT_ALL) {
>> + if (argc > 2)
>> + bb_show_usage();
>> +
>> + rc >> + if (rc)
>> + bb_perror_msg_and_die("cannot get boolean names: ");
>> +
>> + if (!len) {
>> + puts("No booleans");
>> + return 0;
>> + }
>> + }
>> +
>> + if (!len) {
>> + if (argc < 2)
>> + bb_show_usage();
>> + len >> + names >> + for (i >> + names[i] >> + }
>
> I still think that the whole applet above can be written smaller.
>
> Perhaps you can reuse i instead of "opt" without a size penalty?
> Also, var & val is bloat there, i guess. Smaller if you just
> if (i) { /* GETSEBOOL_OPT_ALL set */
I think what 'i' holds an option bitmask is not obviously to understand.
It should not be used as an alternative of 'opt'.
I modified only a point where '&' operation was removed from
the 'if (...)' block, to reduce the space of constant.
>> +
>> + for (i >> + active >> + if (active < 0) {
>> + bb_error_msg("error getting active value for %s", names[i]);
>> + rc >> + goto out;
>> + }
>> + pending >> + if (pending < 0) {
>> + bb_error_msg("error getting pending value for %s", names[i]);
>> + rc >> + goto out;
>> + }
>> + printf("%s --> %s", names[i], (active ? "on" : "off"));
>> + if (pending !>> + printf(" pending: %s", (pending ? "on" : "off"));
>> + putchar('\n');
>> + }
>> +
>> + out:
>
> Please put this at the start of the line as it's easier to find labels
> there.
OK, Fixed.
>> + if (ENABLE_FEATURE_CLEAN_UP) {
>> + for (i >> + free(names[i]);
>> + free(names);
>> + }
>> +
>> + return rc;
>> +}
> [snip Kbuild and make stuff which is ok]
>> Index: sebusybox-libselinux-0131/selinux/matchpathcon.c
>> =================================>> --- sebusybox-libselinux-0131/selinux/matchpathcon.c (revision 0)
>> +++ sebusybox-libselinux-0131/selinux/matchpathcon.c (revision 0)
>> @@ -0,0 +1,98 @@
>> +/* matchpathcon - get the default security context for the specified
>> + * path from the file contexts configuration.
>> + * based on libselinux-1.32
>> + * Port to busybox: KaiGai Kohei <kaigai at kaigai.gr.jp>
>> + *
>> + */
>> +#include "busybox.h"
>> +
>> +static int printmatchpathcon(char *path, int header)
>> +{
>> + char *buf;
>> + int rc >> + if (rc < 0) {
>> + fprintf(stderr, "matchpathcon(%s) failed: %s\n",
>> + path, strerror(errno));
>> + return 1;
>> + }
>
> Nah.. Please fix. (bb_perror_msg_and_die("%s", path);)
Dying here is not a compatible behavior.
I replaced fprintf() + strerror() by bb_perror_mag() without '_and_die()'.
>> + if (header)
>> + printf("%s\t%s\n", path, buf);
>> + else
>> + printf("%s\n", buf);
>> +
>> + freecon(buf);
>> + return 0;
>> +}
>> +
>> +#define MATCHPATHCON_OPT_NOT_PRINT (1<<0) /* -n */
>> +#define MATCHPATHCON_OPT_NOT_TRANS (1<<1) /* -N */
>> +#define MATCHPATHCON_OPT_FCONTEXT (1<<2) /* -f */
>> +#define MATCHPATHCON_OPT_PREFIX (1<<3) /* -p */
>> +#define MATCHPATHCON_OPT_VERIFY (1<<4) /* -V */
>> +
>> +int matchpathcon_main(int argc, char **argv)
>> +{
>> + int i;
>> + int header >> + int verify >> + int notrans >> + int error >
> It's very likely smaller to use one (smallu)int and mask those bits -- or use
> opts or remove opts altogether and use option_mask32. You need to see
> which one creates smaller code.
>
>> + unsigned long opts;
>
> See above.
Are you saying to re-define the above variables as 'char' ?
Its effect is a bit unclear for me, but I replaced it with 'char'.
>> + char *fcontext, *prefix;
>> +
> ->+ if (argc < 2)
> ->+ bb_show_usage();
>> +
>> + opt_complementary >
> from libbb/getopt32.c:
> "-N" A dash as the first char in a opt_complementary group followed
> by a single digit (0-9) means that at least N non-option
> arguments must be present on the command line
>
> so we want (don't need '?', i think):
> opt_complementary
Thanks for this information.
I removed 'if (argc < 2) ...' and adopted the above opt_complementary.
>> + opts >> +
>> + if (opts & MATCHPATHCON_OPT_NOT_PRINT)
>> + header >> + if (opts & MATCHPATHCON_OPT_NOT_TRANS) {
>> + notrans >> + set_matchpathcon_flags(MATCHPATHCON_NOTRANS);
>> + }
>> + if (opts & MATCHPATHCON_OPT_FCONTEXT) {
>> + if (matchpathcon_init(fcontext))
>> + bb_error_msg_and_die("error while processing %s: %s",
>> + fcontext, errno ? strerror(errno) : "invalid");
> bb_perror_msg_and_die
OK, Fixed.
>> + }
>> + if (opts & MATCHPATHCON_OPT_PREFIX) {
>> + if (matchpathcon_init_prefix(NULL, prefix))
>> + bb_error_msg_and_die("error while processing %s: %s",
>> + prefix, errno ? strerror(errno) : "invalid");
> bb_perror_msg_and_die
OK, Fixed
>> + }
>> + if (opts & MATCHPATHCON_OPT_VERIFY)
>> + verify >> +
>> + for (i >> + security_context_t con;
>> + int rc;
>
> Is caching argv[i] here smaller?
add 'char *path a iteration of indirect reference.
>> +
>> + if (!verify) {
>> + error +>> + continue;
>> + }
>> +
>> + if (selinux_file_context_verify(argv[i], 0)) {
>> + printf("%s verified.\n", argv[i]);
>> + continue;
>> + }
>> +
>> + if (notrans)
>> + rc >> + else
>> + rc >> +
>> + if (rc >>> + printf("%s has context %s, should be ", argv[i], con);
>> + error +>> + freecon(con);
>> + } else {
>> + printf("actual context unknown: %s, should be ", strerror(errno));
>> + error +>> + }
>> + }
>> + matchpathcon_fini();
>> + return error;
>> +}
>> Index: sebusybox-libselinux-0131/selinux/setenforce.c
>> =================================>> --- sebusybox-libselinux-0131/selinux/setenforce.c (revision 0)
>> +++ sebusybox-libselinux-0131/selinux/setenforce.c (revision 0)
>> @@ -0,0 +1,33 @@
>> +/*
>> + * setenforce
>> + *
>> + * Based on libselinux 1.33.1
>> + * Port to BusyBox Hiroshi Shinji <shiroshi at my.email.ne.jp>
>> + *
>> + */
>> +
>> +#include "busybox.h"
>> +
>> +int setenforce_main(int argc, char **argv)
>> +{
>> + int rc >> + if (argc !>> + bb_show_usage();
>> +
>> + xis_selinux_enabled();
>> +
>> + if ((argv[1][0] = '0' || argv[1][0] = '1') && argv[1][1] = '\0') {
>> + rc >> + } else {
>> + if (strcasecmp(argv[1], "enforcing") = 0) {
>> + rc >> + } else if (strcasecmp(argv[1], "permissive") = 0) {
>
> rc > Isn't case-insensitive, but would suffice IMO and would make that applet
> considerably smaller. What do you think?
In the attached one, I add 'setenforce_options' array as you say.
Do you think it is a preferable way?
>> + rc >> + } else
>> + bb_show_usage();
>> + }
>> + if (rc < 0)
>> + bb_perror_msg_and_die("setenforce() failed : ");
>> +
>> + return 0;
>> +}
Thanks,
--
Open Source Software Promotion Center, NEC
KaiGai Kohei <kaigai at ak.jp.nec.com>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: busybox-libselinux.v4.patch
Type: text/x-patch
Size: 15535 bytes
Desc: not available
Url : http://busybox.net/lists/busybox/attachments/20070201/69420f0b/attachment-0001.bin
More information about the busybox
mailing list