mod_ruid2 ERROR getgroups()

[lx1]

Hi all

In my setup the apache user belogns to several groups more than 8 which is the edfault RUID_MAXGROUPS value. So I get errors "getgroups() failed on child init, ignoring supplementary group IDs" and the process groups are not reset correctly. I could recompile with a higher RUID_MAXGROUPS value, but I prefered to try a simple patch which seems to work. Maybe it (or something like that) could be included?

Thank you

--- mod_ruid2.c.orig    2013-03-19 21:42:00.000000000 +0100
+++ mod_ruid2.c 2014-01-24 17:40:05.181533124 +0100
@@ -107,7 +107,7 @@
 static int coredump, root_handle;
 static const char *old_root;

-static gid_t startup_groups[RUID_MAXGROUPS];
+static gid_t *startup_groups;
 static int startup_groupsnr;


@@ -353,9 +353,15 @@
        cap_value_t capval[4];

        /* detect default supplementary group IDs */
-       if ((startup_groupsnr = getgroups(RUID_MAXGROUPS, startup_groups)) == -1) {
+       if ((startup_groupsnr = getgroups(0, NULL)) == -1) {
                startup_groupsnr = 0;
-               ap_log_error (APLOG_MARK, APLOG_ERR, 0, NULL, "%s ERROR getgroups() failed on child init, ignoring supplementary group IDs", MODULE_NAME);
+               ap_log_error (APLOG_MARK, APLOG_ERR, 0, NULL, "%s ERROR getgroups(0, NULL) failed on child init, ignoring supplementary group IDs", MODULE_NAME);
+       } else {
+               startup_groups = apr_pcalloc(p, startup_groupsnr * sizeof(gid_t));
+               if (getgroups(startup_groupsnr, startup_groups) == -1) {
+                       startup_groupsnr = 0;
+                       ap_log_error (APLOG_MARK, APLOG_ERR, 0, NULL, "%s ERROR getgroups() failed on child init, ignoring supplementary group IDs", MODULE_NAME);
+               }
        }

        /* setup chroot jailbreak */

[mind04]

If you need more supplementary groups increase the RUID_MAXGROUPS value is your best option at this moment. Dynamic allocation of supplementary groups space sounds tempting but, is pretty complicated.

Your proposed patch for example is dangerous if per directory config is used and may lead to buffer overflows if the number of groups in the directory section <> startup groups.

[lx1]

I understand, but when the groups are hundreds, and they grow, it seems to me a waste of time having to remember to check the groups number, then recompile etc. I larned malloc about 25 years ago (this does not mean I remember how to use it :) I waited a while to comment, because I made an updated version of the patch. I tested it on several servers, and it seems to work fine, Maybe it could be reviewed?

--- mod_ruid2-0.9.8/mod_ruid2.c.orig    2013-03-19 21:42:00.000000000 +0100
+++ mod_ruid2-0.9.8/mod_ruid2.c 2014-02-12 22:58:12.248475609 +0100
@@ -53,8 +53,6 @@
 #define RUID_MIN_UID       100
 #define RUID_MIN_GID       100

-#define RUID_MAXGROUPS     8
-
 #define RUID_MODE_CONF     0
 #define RUID_MODE_STAT     1
 #define RUID_MODE_UNDEFINED    2
@@ -81,7 +79,7 @@
    int8_t ruid_mode;
    uid_t ruid_uid;
    gid_t ruid_gid;
-   gid_t groups[RUID_MAXGROUPS];
+   gid_t *groups;
    int groupsnr;
 } ruid_dir_config_t;

@@ -107,7 +105,7 @@
 static int coredump, root_handle;
 static const char *old_root;

-static gid_t startup_groups[RUID_MAXGROUPS];
+static gid_t *startup_groups;
 static int startup_groupsnr;


@@ -150,10 +148,12 @@
        conf->ruid_uid = (child->ruid_uid == UNSET) ? parent->ruid_uid : child->ruid_uid;
        conf->ruid_gid = (child->ruid_gid == UNSET) ? parent->ruid_gid : child->ruid_gid;
        if (child->groupsnr > 0) {
-           memcpy(conf->groups, child->groups, sizeof(child->groups));
+           conf->groups = apr_pcalloc(p, child->groupsnr * sizeof(gid_t));
+           memcpy(conf->groups, child->groups, child->groupsnr * sizeof(gid_t));
            conf->groupsnr = child->groupsnr;
        } else if (parent->groupsnr > 0) {
-           memcpy(conf->groups, parent->groups, sizeof(parent->groups));
+           conf->groups = apr_pcalloc(p, parent->groupsnr * sizeof(gid_t));
+           memcpy(conf->groups, parent->groups, parent->groupsnr * sizeof(gid_t));
            conf->groupsnr = parent->groupsnr;
        } else {
            conf->groupsnr = (child->groupsnr == UNSET) ? parent->groupsnr : child->groupsnr;
@@ -225,17 +225,32 @@
        return err;
    }

-   if (strcasecmp(arg,"@none") == 0) {
-       dconf->groupsnr=NONE;
+   char * pch;
+   int npch = 0;
+   char * last = NULL;
+   char * a = apr_pstrdup(cmd->temp_pool, arg);
+   apr_array_header_t * arr = apr_array_make(cmd->temp_pool, 1, sizeof(gid_t));
+   pch = apr_strtok (a, " \t\n", &last);
+   while (pch != NULL) {
+       if (strcasecmp(arg,"@none") == 0) {
+           dconf->groupsnr=NONE;
+           return NULL;
+       }
+
+       npch++;
+       *(gid_t *) apr_array_push(arr) = ap_gname2id (pch);
+       pch = apr_strtok (NULL, " \t\n", &last);
    }

-   if (dconf->groupsnr == UNSET) {
-       dconf->groupsnr = 0;
-   }
-   if ((dconf->groupsnr < RUID_MAXGROUPS) && (dconf->groupsnr >= 0)) {
-       dconf->groups[dconf->groupsnr++] = ap_gname2id (arg);
+   if (npch == 0) {
+       dconf->groupsnr=NONE;
+       return NULL;
    }

+   dconf->groupsnr = npch;
+   dconf->groups = apr_pcalloc(cmd->pool, npch * sizeof(gid_t));
+   memcpy(dconf->groups, (gid_t *)arr->elts, npch * sizeof(gid_t));
+
    return NULL;
 }

@@ -294,7 +309,7 @@

    AP_INIT_TAKE1 ("RMode", set_mode, NULL, RSRC_CONF | ACCESS_CONF, "Set mode to config or stat (default: config)"),
    AP_INIT_TAKE2 ("RUidGid", set_uidgid, NULL, RSRC_CONF | ACCESS_CONF, "Minimal uid or gid file/dir, else set[ug]id to default (User,Group)"),
-   AP_INIT_ITERATE ("RGroups", set_groups, NULL, RSRC_CONF | ACCESS_CONF, "Set additional groups"),
+   AP_INIT_RAW_ARGS ("RGroups", set_groups, NULL, RSRC_CONF | ACCESS_CONF, "Set additional groups"),
    AP_INIT_TAKE2 ("RDefaultUidGid", set_defuidgid, NULL, RSRC_CONF, "If uid or gid is < than RMinUidGid set[ug]id to this uid gid"),
    AP_INIT_TAKE2 ("RMinUidGid", set_minuidgid, NULL, RSRC_CONF, "Minimal uid or gid file/dir, else set[ug]id to default (RDefaultUidGid)"),
    AP_INIT_TAKE2 ("RDocumentChRoot", set_documentchroot, NULL, RSRC_CONF, "Set chroot directory and the document root inside"),
@@ -353,9 +368,15 @@
    cap_value_t capval[4];

    /* detect default supplementary group IDs */
-   if ((startup_groupsnr = getgroups(RUID_MAXGROUPS, startup_groups)) == -1) {
+   if ((startup_groupsnr = getgroups(0, NULL)) == -1) {
        startup_groupsnr = 0;
-       ap_log_error (APLOG_MARK, APLOG_ERR, 0, NULL, "%s ERROR getgroups() failed on child init, ignoring supplementary group IDs", MODULE_NAME);
+       ap_log_error (APLOG_MARK, APLOG_ERR, 0, NULL, "%s ERROR getgroups(0, NULL) failed on child init, ignoring supplementary group IDs", MODULE_NAME);
+   } else {
+       startup_groups = apr_pcalloc(p, startup_groupsnr * sizeof(gid_t));
+       if (getgroups(startup_groupsnr, startup_groups) == -1) {
+           startup_groupsnr = 0;
+           ap_log_error (APLOG_MARK, APLOG_ERR, 0, NULL, "%s ERROR getgroups() failed on child init, ignoring supplementary group IDs", MODULE_NAME);
+       }
    }

    /* setup chroot jailbreak */
@@ -466,7 +487,7 @@
    int retval = DECLINED;
    gid_t gid;
    uid_t uid;
-   gid_t groups[RUID_MAXGROUPS];
+   gid_t *groups;
    int groupsnr;

    cap_t cap;
@@ -502,9 +523,11 @@

    /* set supplementary groups */
    if ((dconf->groupsnr == UNSET) && (startup_groupsnr > 0)) {
-       memcpy(groups, startup_groups, sizeof(groups));
+       groups = apr_pcalloc(r->pool, startup_groupsnr * sizeof(gid_t));
+       memcpy(groups, startup_groups, startup_groupsnr * sizeof(gid_t));
        groupsnr = startup_groupsnr;
    } else if (dconf->groupsnr > 0) {
+       groups = apr_pcalloc(r->pool, dconf->groupsnr * sizeof(gid_t));
        for (groupsnr = 0; groupsnr < dconf->groupsnr; groupsnr++) {
            if (dconf->groups[groupsnr] >= conf->min_gid) {
                groups[groupsnr] = dconf->groups[groupsnr];
@@ -514,6 +537,7 @@
        }
    } else {
        groupsnr = 0;
+       groups = NULL;
    }
    setgroups(groupsnr, groups);

I propose another patch related with the one above, because IMHO if the admin asks for STAT, then he wants to be sure that the process runs with uid/gid of the file, not supplementary process groups (see comment in the patch:)

diff -uarN mod_ruid2-0.9.8.orig/mod_ruid2.c mod_ruid2-0.9.8/mod_ruid2.c
--- mod_ruid2-0.9.8.orig/mod_ruid2.c    2014-02-08 14:58:25.780468528 +0100
+++ mod_ruid2-0.9.8/mod_ruid2.c 2014-02-08 15:12:11.528416353 +0100
@@ -507,7 +507,11 @@
    }

    /* set supplementary groups */
-   if ((dconf->groupsnr == UNSET) && (startup_groupsnr > 0)) {
+   /* IMHO, only in CONF mode the supplementary groups should be set to the original process supplementary groups (when the admin does not explicitly ask for some set of supplementary goups).
+           In STAT mode, the admin expects the process uid/gid to be the same as the file (or parent dir), not inheriting implicit additional groups. This would, IMHO, open a security risk
+           because the admin thinks the process runs as the file gid, when it really has more groups (the original supplementary groups: but if the admin asked for STAT, he wants to get rid
+           of that groups) */
+   if ((dconf->ruid_mode == RUID_MODE_CONF) && (dconf->groupsnr == UNSET) && (startup_groupsnr > 0)) {
        groups = apr_pcalloc(r->pool, startup_groupsnr * sizeof(gid_t));
        memcpy(groups, startup_groups, startup_groupsnr * sizeof(gid_t));
        groupsnr = startup_groupsnr;

Thank you for the attention. Anyway, should someone need these patches, they are in my RH/CentOS RPM packages in repo.iotti.biz.