Bug#499191: Possible security issues
Alexander Prinsier wrote:
> I have a patch prepared. Attached is what I got so far, and seems to be
> working fine. (It's the modified .dpatch file, not a patch to a dpatch).
And this is the file...
#! /bin/sh /usr/share/dpatch/dpatch-run
## 202_suexec-custom.dpatch by Stefan Fritsch <sf@debian.org>
##
## All lines beginning with `## DP:' are a description of the patch.
## DP: the actual patch to make suexec-custom read a config file
@DPATCH@
diff -urNad apache2-2.2.9~/config.nice apache2-2.2.9/config.nice
--- apache2-2.2.9~/config.nice 1970-01-01 01:00:00.000000000 +0100
+++ apache2-2.2.9/config.nice 2009-01-27 18:09:30.000000000 +0100
@@ -0,0 +1,6 @@
+#! /bin/sh
+#
+# Created by configure
+
+"./configure" \
+"$@"
diff -urNad apache2-2.2.9~/support/suexec-custom.c apache2-2.2.9/support/suexec-custom.c
--- apache2-2.2.9~/support/suexec-custom.c 2009-01-27 18:09:30.000000000 +0100
+++ apache2-2.2.9/support/suexec-custom.c 2009-01-27 18:09:50.000000000 +0100
@@ -29,6 +29,7 @@
*
*
*/
+#define SUEXEC_CONFIG_DIR "/etc/apache2/suexec/"
#include "apr.h"
#include "ap_config.h"
@@ -39,6 +40,7 @@
#include <sys/types.h>
#include <string.h>
#include <time.h>
+#include <ctype.h>
#if APR_HAVE_UNISTD_H
#include <unistd.h>
#endif
@@ -203,6 +205,26 @@
return;
}
+static int read_line(char *buf, FILE *file) {
+ char *p;
+ p = fgets(buf, AP_MAXPATH+1, file);
+ if (!p) return 0;
+ if (*p == '\0') return 1;
+
+ p = buf;
+ while (*p)
+ p++;
+ p--;
+
+ /* remove trailing space and slash */
+ while ( isspace(*p) && p >= buf )
+ *p-- = '\0';
+ while ( *p == '/' && p >= buf )
+ *p-- = '\0';
+
+ return 1;
+}
+
static void clean_env(void)
{
char pathbuf[512];
@@ -261,11 +283,18 @@
char *cmd; /* command to be executed */
char cwd[AP_MAXPATH]; /* current working directory */
char dwd[AP_MAXPATH]; /* docroot working directory */
+ char dwd2[AP_MAXPATH]; /* alternate docroot */
struct passwd *pw; /* password entry holder */
struct group *gr; /* group entry holder */
struct stat dir_info; /* directory info holder */
struct stat prg_info; /* program info holder */
int cwdh; /* handle to cwd */
+ int inside_cgi_docroot;
+ char *suexec_docroot = NULL;
+ char *suexec_userdir_suffix = NULL;
+ char *suexec_cgiroot = NULL;
+ char *filename = NULL;
+ FILE *configfile;
/*
* Start with a "clean" environment
@@ -296,15 +325,13 @@
|| (! strcmp(AP_HTTPD_USER, pw->pw_name)))
#endif /* _OSD_POSIX */
) {
-#ifdef AP_DOC_ROOT
- fprintf(stderr, " -D AP_DOC_ROOT=\"%s\"\n", AP_DOC_ROOT);
-#endif
+ fprintf(stderr, "This suexec is PATCHED.\n"
+ " *Support for variable docroot setting\n"
+ " *Support for a cgi docroot\n");
+ fprintf(stderr, " -D SUEXEC_CONFIG_DIR=%s\n", SUEXEC_CONFIG_DIR);
#ifdef AP_GID_MIN
fprintf(stderr, " -D AP_GID_MIN=%d\n", AP_GID_MIN);
#endif
-#ifdef AP_HTTPD_USER
- fprintf(stderr, " -D AP_HTTPD_USER=\"%s\"\n", AP_HTTPD_USER);
-#endif
#ifdef AP_LOG_EXEC
fprintf(stderr, " -D AP_LOG_EXEC=\"%s\"\n", AP_LOG_EXEC);
#endif
@@ -317,9 +344,6 @@
#ifdef AP_UID_MIN
fprintf(stderr, " -D AP_UID_MIN=%d\n", AP_UID_MIN);
#endif
-#ifdef AP_USERDIR_SUFFIX
- fprintf(stderr, " -D AP_USERDIR_SUFFIX=\"%s\"\n", AP_USERDIR_SUFFIX);
-#endif
exit(0);
}
/*
@@ -334,23 +358,6 @@
target_gname = argv[2];
cmd = argv[3];
- /*
- * Check to see if the user running this program
- * is the user allowed to do so as defined in
- * suexec.h. If not the allowed user, error out.
- */
-#ifdef _OSD_POSIX
- /* User name comparisons are case insensitive on BS2000/OSD */
- if (strcasecmp(AP_HTTPD_USER, pw->pw_name)) {
- log_err("user mismatch (%s instead of %s)\n", pw->pw_name, AP_HTTPD_USER);
- exit(103);
- }
-#else /*_OSD_POSIX*/
- if (strcmp(AP_HTTPD_USER, pw->pw_name)) {
- log_err("user mismatch (%s instead of %s)\n", pw->pw_name, AP_HTTPD_USER);
- exit(103);
- }
-#endif /*_OSD_POSIX*/
/*
* Check for a leading '/' (absolute path) in the command to be executed,
@@ -375,6 +382,69 @@
}
/*
+ * Check to see if the user running this program
+ * is the user allowed to do so as defined in
+ * SUEXEC_CONFIG_DIR/username
+ * If not, error out.
+ */
+ filename = malloc(AP_MAXPATH+1);
+ suexec_docroot = malloc(AP_MAXPATH+1);
+ suexec_userdir_suffix = malloc(AP_MAXPATH+1);
+ suexec_cgiroot = malloc(AP_MAXPATH+1);
+ if (!filename || !suexec_docroot || !suexec_userdir_suffix || !suexec_cgiroot) {
+ log_err("malloc failed\n");
+ exit(120);
+ }
+
+ strncpy(filename, SUEXEC_CONFIG_DIR, AP_MAXPATH);
+ strncat(filename, pw->pw_name, AP_MAXPATH);
+ filename[AP_MAXPATH] = '\0';
+
+ configfile = fopen(filename, "r");
+ if (!configfile) {
+ log_err("User %s not allowed: Could not open config file %s\n", pw->pw_name, filename);
+ exit(123);
+ }
+
+ if (!read_line(suexec_docroot, configfile)) {
+ log_err("Could not read docroot from %s\n", filename);
+ exit(124);
+ }
+
+ if (!read_line(suexec_userdir_suffix, configfile)) {
+ log_err("Could not read userdir suffix from %s\n", filename);
+ exit(125);
+ }
+
+ if (!read_line(suexec_cgiroot, configfile)) {
+ log_err("Could not read cgiroot from %s\n", filename);
+ exit(125);
+ }
+
+ fclose(configfile);
+
+ if (userdir) {
+ if ( !isalnum(*suexec_userdir_suffix) && suexec_userdir_suffix[0] != '.') {
+ log_err("userdir suffix disabled in %s\n", filename);
+ exit(126);
+ }
+ }
+ else {
+ if (suexec_docroot[0] != '/') {
+ log_err("docroot disabled in %s\n", filename);
+ exit(127);
+ }
+
+ if (suexec_docroot[1] == '/' ||
+ suexec_docroot[1] == '.' ||
+ suexec_docroot[1] == '\0' )
+ {
+ log_err("invalid docroot %s in %s\n", suexec_docroot, filename);
+ exit(128);
+ }
+ }
+
+ /*
* Error out if the target username is invalid.
*/
if (strspn(target_uname, "1234567890") != strlen(target_uname)) {
@@ -508,7 +578,7 @@
if (userdir) {
if (((chdir(target_homedir)) != 0) ||
- ((chdir(AP_USERDIR_SUFFIX)) != 0) ||
+ ((chdir(suexec_userdir_suffix)) != 0) ||
((getcwd(dwd, AP_MAXPATH)) == NULL) ||
((fchdir(cwdh)) != 0)) {
log_err("cannot get docroot information (%s)\n", target_homedir);
@@ -516,25 +586,57 @@
}
}
else {
- if (((chdir(AP_DOC_ROOT)) != 0) ||
+ /* Resolve symlinked directories: put the _real_ suexec_docroot
+ directory name in dwd2. */
+ if (((chdir(suexec_docroot)) != 0) ||
((getcwd(dwd, AP_MAXPATH)) == NULL) ||
((fchdir(cwdh)) != 0)) {
- log_err("cannot get docroot information (%s)\n", AP_DOC_ROOT);
+ log_err("cannot get docroot information (%s)\n", suexec_docroot);
+ exit(113);
+ }
+ /* Resolve symlinked directories: put the _real_ suexec_cgiroot
+ directory name in dwd2. */
+ if (((chdir(suexec_cgiroot)) != 0) ||
+ ((getcwd(dwd2, AP_MAXPATH)) == NULL) ||
+ ((fchdir(cwdh)) != 0)) {
+ log_err("cannot get docroot information (%s)\n", suexec_cgiroot);
exit(113);
}
}
close(cwdh);
+ /* TODO: document what this exactly does, and why */
if (strlen(cwd) > strlen(dwd)) {
strncat(dwd, "/", AP_MAXPATH);
dwd[AP_MAXPATH-1] = '\0';
}
+
+ inside_cgi_docroot = 0;
+
+ /* The first part of cwd should match dwd */
if ((strncmp(cwd, dwd, strlen(dwd))) != 0) {
- log_err("command not in docroot (%s/%s)\n", cwd, cmd);
+ /* So we're not in dwd. Maybe in dwd2 then? */
+ if((strncmp(cwd, dwd2, strlen(dwd2))) != 0) {
+ /* We're in none of the 2. */
+ log_err("command not in docroot and not in cgi_docroot (%s/%s/%s)\n", cmd, dwd, dwd2);
+ exit(114);
+ } else {
+ /* We're not in dwd but We are in cgi_docroot. That's OK. */
+ inside_cgi_docroot = 1;
+ }
+ } else {
+ if((strncmp(cwd, dwd2, strlen(dwd2))) == 0) {
+ /*
+ * We are in dwd AND we are somewhere in dwd2 too.
+ * That means the one is a subdirectory of the other.
+ * That's not good.
+ */
+ log_err("Docroot or cgi_docroot appears to be a subdirectory of the other. Bad! (%s/%s)\n", dwd, dwd2);
exit(114);
+ }
}
-
+
/*
* Stat the cwd and verify it is a directory, or error out.
*/
@@ -576,24 +678,49 @@
}
/*
+ * If we're inside the cgi_docroot, the owner of the target file
+ * must be root.
+ */
+ if(inside_cgi_docroot == 1)
+ {
+ if((0 != dir_info.st_uid) ||
+ (0 != dir_info.st_gid) ||
+ (0 != prg_info.st_uid) ||
+ (0 != prg_info.st_gid)) {
+ log_err("Target inside cgi_docroot but directory "
+ "(%ld/%ld) or program (%ld/%ld) is not owner by root\n",
+ dir_info.st_uid, dir_info.st_gid,
+ prg_info.st_uid, prg_info.st_gid);
+ exit(120);
+ }
+ }
+
+ /*
* Error out if the target name/group is different from
* the name/group of the cwd or the program.
*/
- if ((uid != dir_info.st_uid) ||
- (gid != dir_info.st_gid) ||
- (uid != prg_info.st_uid) ||
- (gid != prg_info.st_gid)) {
- log_err("target uid/gid (%ld/%ld) mismatch "
- "with directory (%ld/%ld) or program (%ld/%ld)\n",
- uid, gid,
- dir_info.st_uid, dir_info.st_gid,
- prg_info.st_uid, prg_info.st_gid);
- exit(120);
+ if(inside_cgi_docroot == 0)
+ {
+ if ((uid != dir_info.st_uid) ||
+ (gid != dir_info.st_gid) ||
+ (uid != prg_info.st_uid) ||
+ (gid != prg_info.st_gid)) {
+ log_err("target uid/gid (%ld/%ld) mismatch "
+ "with directory (%ld/%ld) or program (%ld/%ld)\n",
+ uid, gid,
+ dir_info.st_uid, dir_info.st_gid,
+ prg_info.st_uid, prg_info.st_gid);
+ exit(120);
+ }
}
+
/*
* Error out if the program is not executable for the user.
* Otherwise, she won't find any error in the logs except for
* "[error] Premature end of script headers: ..."
+ *
+ * TODO: this check should also be done in case
+ * inside_cgi_docroot == 1, but not crucial.
*/
if (!(prg_info.st_mode & S_IXUSR)) {
log_err("file has no execute permission: (%s/%s)\n", cwd, cmd);
Reply to: