[busybox:00323] Re: [PATCH 4/8] busybox -- libselinux utilities applets
Bernhard Fischer
rep.dot.nop at gmail.com
Wed Jan 31 14:04:19 UTC 2007
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
>+ if (!is_selinux_enabled())
>+ bb_error_msg_and_die("SELinux is disabled");
>+#else
>+ bb_error_msg_and_die("SELinux support is disabled");
>+#endif
>+}
>+
> /* 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
>+
> #define md5sum_trivial_usage \
> "[OPTION] [FILEs...]" \
> USE_FEATURE_MD5_SHA1_SUM_CHECK("\n or: md5sum [OPTION] -c [FILE]")
>@@ -2714,6 +2731,9 @@
> "$ echo \"foo\" | sed -e 's/f[a-zA-Z]o/bar/g'\n" \
> "bar\n"
>
>+#define selinuxenabled_trivial_usage
>+#define selinuxenabled_full_usage
>+
> #define seq_trivial_usage \
> "[first [increment]] last"
> #define seq_full_usage \
>@@ -2731,6 +2751,10 @@
> "\n\nOptions:\n" \
> " -r Reset output to /dev/console"
>
>+#define setenforce_trivial_usage \
>+ "[ Enforcing | Permissive | 1 | 0 ]"
>+#define setenforce_full_usage
>+
> #define setkeycodes_trivial_usage \
> "SCANCODE KEYCODE ..."
> #define setkeycodes_full_usage \
>Index: sebusybox-libselinux-0131/include/applets.h
>===================================================================
>--- sebusybox-libselinux-0131/include/applets.h (revision 17684)
>+++ sebusybox-libselinux-0131/include/applets.h (working copy)
>@@ -133,7 +133,9 @@
> USE_FTPGET(APPLET_ODDNAME(ftpget, ftpgetput, _BB_DIR_USR_BIN, _BB_SUID_NEVER,ftpget))
> USE_FTPPUT(APPLET_ODDNAME(ftpput, ftpgetput, _BB_DIR_USR_BIN, _BB_SUID_NEVER,ftpput))
> USE_FUSER(APPLET(fuser, _BB_DIR_USR_BIN, _BB_SUID_NEVER))
>+USE_GETENFORCE(APPLET(getenforce, _BB_DIR_USR_SBIN, _BB_SUID_NEVER))
> USE_GETOPT(APPLET(getopt, _BB_DIR_BIN, _BB_SUID_NEVER))
>+USE_GETSEBOOL(APPLET(getsebool, _BB_DIR_USR_SBIN, _BB_SUID_NEVER))
> USE_GETTY(APPLET(getty, _BB_DIR_SBIN, _BB_SUID_NEVER))
> USE_GREP(APPLET(grep, _BB_DIR_BIN, _BB_SUID_NEVER))
> USE_GUNZIP(APPLET(gunzip, _BB_DIR_BIN, _BB_SUID_NEVER))
>@@ -187,6 +189,7 @@
> USE_LSATTR(APPLET(lsattr, _BB_DIR_BIN, _BB_SUID_NEVER))
> USE_LSMOD(APPLET(lsmod, _BB_DIR_SBIN, _BB_SUID_NEVER))
> USE_UNLZMA(APPLET_ODDNAME(lzmacat, unlzma, _BB_DIR_USR_BIN, _BB_SUID_NEVER, lzmacat))
>+USE_MATCHPATHCON(APPLET(matchpathcon, _BB_DIR_USR_SBIN, _BB_SUID_NEVER))
> USE_MAKEDEVS(APPLET(makedevs, _BB_DIR_SBIN, _BB_SUID_NEVER))
> USE_MD5SUM(APPLET_ODDNAME(md5sum, md5_sha1_sum, _BB_DIR_USR_BIN, _BB_SUID_NEVER, md5sum))
> USE_MDEV(APPLET(mdev, _BB_DIR_SBIN, _BB_SUID_NEVER))
>@@ -249,10 +252,12 @@
> USE_RUNSV(APPLET(runsv, _BB_DIR_USR_BIN, _BB_SUID_NEVER))
> USE_RUNSVDIR(APPLET(runsvdir, _BB_DIR_USR_BIN, _BB_SUID_NEVER))
> USE_RX(APPLET(rx, _BB_DIR_USR_BIN, _BB_SUID_NEVER))
>+USE_SELINUXENABLED(APPLET(selinuxenabled, _BB_DIR_USR_SBIN, _BB_SUID_NEVER))
> USE_SED(APPLET(sed, _BB_DIR_BIN, _BB_SUID_NEVER))
> USE_SEQ(APPLET(seq, _BB_DIR_USR_BIN, _BB_SUID_NEVER))
> USE_SETARCH(APPLET(setarch, _BB_DIR_BIN, _BB_SUID_NEVER))
> USE_SETCONSOLE(APPLET(setconsole, _BB_DIR_SBIN, _BB_SUID_NEVER))
>+USE_SETENFORCE(APPLET(setenforce, _BB_DIR_USR_SBIN, _BB_SUID_NEVER))
> USE_SETKEYCODES(APPLET(setkeycodes, _BB_DIR_USR_BIN, _BB_SUID_NEVER))
> USE_SETLOGCONS(APPLET(setlogcons, _BB_DIR_USR_SBIN, _BB_SUID_NEVER))
> USE_SETSID(APPLET(setsid, _BB_DIR_USR_BIN, _BB_SUID_NEVER))
>Index: sebusybox-libselinux-0131/selinux/getenforce.c
>===================================================================
>--- 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 = is_selinux_enabled();
->+ 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();
>+
->+ if (rc == 1) {
>+ rc = security_getenforce();
>+ 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 = 0, active, pending, len = 0;
>+ char **names;
>+ unsigned long opt;
>+
>+ opt = getopt32(argc, argv, "a");
>+
>+ xis_selinux_enabled();
>+
>+ if (opt & GETSEBOOL_OPT_ALL) {
>+ if (argc > 2)
>+ bb_show_usage();
>+
>+ rc = security_get_boolean_names(&names, &len);
>+ 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 = argc - 1;
>+ names = xmalloc(sizeof(char *) * len);
>+ for (i = 0; i < len; i++)
>+ names[i] = xstrdup(argv[i + 1]);
>+ }
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 */
>+
>+ for (i = 0; i < len; i++) {
>+ active = security_get_boolean_active(names[i]);
>+ if (active < 0) {
>+ bb_error_msg("error getting active value for %s", names[i]);
>+ rc = -1;
>+ goto out;
>+ }
>+ pending = security_get_boolean_pending(names[i]);
>+ if (pending < 0) {
>+ bb_error_msg("error getting pending value for %s", names[i]);
>+ rc = -1;
>+ goto out;
>+ }
>+ printf("%s --> %s", names[i], (active ? "on" : "off"));
>+ if (pending != active)
>+ 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.
>+ if (ENABLE_FEATURE_CLEAN_UP) {
>+ for (i = 0; i < len; 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 = matchpathcon(path, 0, &buf);
>+ 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);)
>+ 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 = 1;
>+ int verify = 0;
>+ int notrans = 0;
>+ int error = 0;
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.
>+ char *fcontext, *prefix;
>+
->+ if (argc < 2)
->+ bb_show_usage();
>+
>+ opt_complementary = "?:f--p:p--f";
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 = "-1:f--p:p--f";
>+ opts = getopt32(argc, argv, "nNf:p:V", &fcontext, &prefix);
>+
>+ if (opts & MATCHPATHCON_OPT_NOT_PRINT)
>+ header = 0;
>+ if (opts & MATCHPATHCON_OPT_NOT_TRANS) {
>+ notrans = 1;
>+ 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
>+ }
>+ 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
>+ }
>+ if (opts & MATCHPATHCON_OPT_VERIFY)
>+ verify = 1;
>+
>+ for (i = optind; i < argc; i++) {
>+ security_context_t con;
>+ int rc;
Is caching argv[i] here smaller?
>+
>+ if (!verify) {
>+ error += printmatchpathcon(argv[i], header);
>+ continue;
>+ }
>+
>+ if (selinux_file_context_verify(argv[i], 0)) {
>+ printf("%s verified.\n", argv[i]);
>+ continue;
>+ }
>+
>+ if (notrans)
>+ rc = lgetfilecon_raw(argv[i], &con);
>+ else
>+ rc = lgetfilecon(argv[i], &con);
>+
>+ if (rc >= 0) {
>+ printf("%s has context %s, should be ", argv[i], con);
>+ error += printmatchpathcon(argv[i], 0);
>+ freecon(con);
>+ } else {
>+ printf("actual context unknown: %s, should be ", strerror(errno));
>+ error += printmatchpathcon(argv[i], 0);
>+ }
>+ }
>+ 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 = 0;
>+ if (argc != 2)
>+ bb_show_usage();
>+
>+ xis_selinux_enabled();
>+
>+ if ((argv[1][0] == '0' || argv[1][0] == '1') && argv[1][1] == '\0') {
>+ rc = security_setenforce(atoi(argv[1]));
>+ } else {
>+ if (strcasecmp(argv[1], "enforcing") == 0) {
>+ rc = security_setenforce(1);
>+ } else if (strcasecmp(argv[1], "permissive") == 0) {
rc = security_setenforce(index_in_substr_array);
Isn't case-insensitive, but would suffice IMO and would make that applet
considerably smaller. What do you think?
>+ rc = security_setenforce(0);
>+ } else
>+ bb_show_usage();
>+ }
>+ if (rc < 0)
>+ bb_perror_msg_and_die("setenforce() failed : ");
>+
>+ return 0;
>+}
More information about the busybox
mailing list