On Wed, Dec 11, 2002 at 09:25:19AM -0500, Jim Jagielski wrote:
>
> Comments??
What a coincidence. I was just about to post this mail:
---snip--
Hi,
This patch takes Jim's ap_note_cleanups_for_*_ex() functions and
plugs the open fd's being passed to children of, say, PHP.
With this patch, I tested an apache_1.3.28-dev with php3 on FreeBSD
which was configured
* to listen on 2 sockets (*:8080 and localhost:8081)
Reason: some lock files are only created when more than one
listening socket is used
* to use piped logs (cronolog) for its access_log
Reason: though I want a log _file_ to be closed on exec, the
specific log fd for the piped logger process must survive the
exec() for the logger's invocation.
in the following scenarios (using Apache-1.3.28-dev from today):
1) Unchanged source (patch NOT aplied):
* exec cgi program: /cgi-bin/openfds.cgi
-> p--------- 0 root wheel 0 /dev/fd/0
-> p--------- 0 root wheel 0 /dev/fd/1
-> -rw-r----- 1 martin kraemer 7971 /dev/fd/2 == /tmp/apa13/logs/error_log
* <!--#exec cmd="openfds" --> from an .shtml page
-> crw-rw-rw- 1 root wheel 2, 2 /dev/fd/0
-> p--------- 0 root wheel 0 /dev/fd/1
-> -rw-r----- 1 martin kraemer 7971 /dev/fd/2 == /tmp/apa13/logs/error_log
* <?php system("openfds"); ?>
-> crw-rw-rw- 1 root wheel 2, 2 /dev/fd/0
-> p--------- 0 root wheel 0 /dev/fd/1
-> -rw-r----- 1 martin kraemer 7971 /dev/fd/2 == /tmp/apa13/logs/error_log
-> srw-rw-rw- 0 martin kraemer 0 /dev/fd/5
lcl:[deejai.mch.fsc.net:8080] rmt:[deejai2.mch.fsc.net:3902]
-> -rw-r----- 1 martin kraemer 7971 /dev/fd/15 ==
/tmp/apa13/logs/error_log
-> srw-rw-rw- 0 martin kraemer 0 /dev/fd/16 lcl:[localhost:8081]
(Socket is not connected)
-> srw-rw-rw- 0 martin kraemer 0 /dev/fd/17 lcl:[0.0.0.0:8080] (Socket
is not connected)
-> -rw------- 1 martin kraemer 0 /dev/fd/18 ==
/tmp/apa13/logs/httpd.lock.28043
-> -rw------- 1 martin kraemer 0 /dev/fd/19 ==
/tmp/apa13/logs/httpd.lock.28043
-> -rw-r----- 1 martin kraemer 209 /dev/fd/20 ==
/tmp/apa13/htdocs/openfds.php
When CustomLog refers to a file, not a pipe, then this becomes:
-> crw-rw-rw- 1 root wheel 2, 2 /dev/fd/0
-> p--------- 0 root wheel 0 /dev/fd/1
-> -rw-r----- 1 martin kraemer 8308 /dev/fd/2 == /tmp/apa13/logs/error_log
-> srw-rw-rw- 0 martin kraemer 0 /dev/fd/3
lcl:[deejai.mch.fsc.net:8080] rmt:[deejai2.mch.fsc.net:3914]
-> -rw-r----- 1 martin kraemer 8308 /dev/fd/15 ==
/tmp/apa13/logs/error_log
-> srw-rw-rw- 0 martin kraemer 0 /dev/fd/16 lcl:[localhost:8081]
(Socket is not connected)
-> srw-rw-rw- 0 martin kraemer 0 /dev/fd/17 lcl:[0.0.0.0:8080] (Socket
is not connected)
-> -rw-r----- 1 martin kraemer 5130 /dev/fd/18 ==
/tmp/apa13/logs/access_log-2002-12-11
-> -rw------- 1 martin kraemer 0 /dev/fd/19 ==
/tmp/apa13/logs/httpd.lock.28043
-> -rw------- 1 martin kraemer 0 /dev/fd/20 ==
/tmp/apa13/logs/httpd.lock.28043
-> -rw-r----- 1 martin kraemer 209 /dev/fd/21 ==
/tmp/apa13/htdocs/openfds.php
(i.e., the access_log is leaked as well).
Note especially that PHP allows full access for child programs
not only to the requesting socket, but also to all listener sockets.
(here: 8080 and 8081)
* piped logs work.
2) With appended patch applied:
* exec cgi program: /cgi-bin/openfds.cgi
-> p--------- 0 root wheel 0 /dev/fd/0
-> p--------- 0 root wheel 0 /dev/fd/1
-> -rw-r----- 1 martin kraemer 7719 /dev/fd/2 == /tmp/apa13/logs/error_log
* <!--#exec cmd="openfds" --> from an .shtml page
-> crw-rw-rw- 1 root wheel 2, 2 /dev/fd/0
-> p--------- 0 root wheel 0 /dev/fd/1
-> -rw-r----- 1 martin kraemer 7719 /dev/fd/2 == /tmp/apa13/logs/error_log
* <?php system("openfds"); ?>
-> crw-rw-rw- 1 root wheel 2, 2 /dev/fd/0
-> p--------- 0 root wheel 0 /dev/fd/1
-> -rw-r----- 1 martin kraemer 7719 /dev/fd/2 == /tmp/apa13/logs/error_log
-> -rw-r----- 1 martin kraemer 7719 /dev/fd/15 ==
/tmp/apa13/logs/error_log
* piped logs work too.
==================================================================================
To recap,
* Apache by itself does not leak fd's, because its ap_cleanup_for_exec() API
has made sure (for ages already) that nothing is passed to exec'ed processes.
* "certain 3rd party modules" do not use the API correctly (either because
they don't know better, or because they wanted top use existing unsafe
functions like system() and popen()) and do not invoke ap_cleanup_for_exec(),
thus leaking fd's.
The leaked fd's include the server's listener sockets, the accept()ed
request sockets, the access_log file, several lock files.
* By using FD_CLOEXEC with the new ap_note_cleanups_for_*_ex() functions,
we can proactively plug these leakages, even though it would be better if
the 3rd party module authors would "do it right" in _their_ code.
* By showing that is actually works, I want to encourage the maintainers
of the other OS platforms (Win32, Netware, TPF, OS/2, Cygwin) to test
their platform for a feature analogous to POSIX's FD_CLOEXEC, and add it
to the function main/alloc.c::ap_close_fd_on_exec().
CAVEAT
The appended patch changes the semantics of the two functions ap_popenf()
and ap_psocket(), by defaulting to creating fd's which have the FD_CLOEXEC
feature set (where the OS supports it). I checked the occurrences of these
two functions, and inside the Apache source distro they are only used
in a context where this behavior is desired.
However, there are many 3rd party modules out there, and if they use
these functions they may break.
So: how to proceed?
- create a new set of ap_popenf_autoclosing() and ap_psocket_autoclosing()?
- change the API and update the new_features_1_3 doc file? ;-)
- other proposals?
Have fun,
Martin
PS: I append the code for the openfds cgi program as well. It is based on
a very ancient hack, so bear with me if the code is not very readable.
What it does: it loops over all open fd's and tries to describe their
properties in a format similar to 'ls -l'. For regular files, it also tries
to determine the path name of the file by invoking mount(1) and find(1).
That makes it very easy to understand to what open file the fd refers.
---snip---
Now your patch answers my main question, since you created a new
ap_popenf_ex() (== ap_popenf_autoclosing()).
But in addition to ap_popenf_ex(), my patch, which I'll have to
rework of course, also plugs the socket fd leakage.
So, do you suggest we should do the same with ap_psocket_ex()?
Martin
--
<[EMAIL PROTECTED]> | Fujitsu Siemens
Fon: +49-89-636-46021, FAX: +49-89-636-47655 | 81730 Munich, Germany
Index: CHANGES
===================================================================
RCS file: /home/cvs/apache-1.3/src/CHANGES,v
retrieving revision 1.1866
diff -u -r1.1866 CHANGES
--- CHANGES 9 Dec 2002 20:21:00 -0000 1.1866
+++ CHANGES 11 Dec 2002 13:34:11 -0000
@@ -1,5 +1,14 @@
Changes with Apache 1.3.28
+ *) Certain 3rd party modules would bypass the Apache API and not
+ invoke ap_cleanup_for_exec() before creating sub-processes.
+ To such a child process, Apache's file descriptors (lock
+ fd's, log files, sockets) were accessible, allowing them
+ direct access to Apache log file etc. Where the OS allows,
+ we now add precautious close functions to prevent these file
+ descriptors to leak to the child processes.
+ [Martin Kraemer], based on Jim's ap_note_cleanups_for_*_ex() funcs.
+
*) Prevent obscenely large values of precision in ap_vformatter
from clobbering a buffer. [Sander Striker, Jim Jagielski]
Index: main/alloc.c
===================================================================
RCS file: /home/cvs/apache-1.3/src/main/alloc.c,v
retrieving revision 1.134
diff -u -r1.134 alloc.c
--- main/alloc.c 11 Dec 2002 12:24:27 -0000 1.134
+++ main/alloc.c 11 Dec 2002 13:34:11 -0000
@@ -1837,7 +1837,7 @@
save_errno = errno;
if (fd >= 0) {
fd = ap_slack(fd, AP_SLACK_HIGH);
- ap_note_cleanups_for_fd(a, fd);
+ ap_note_cleanups_for_fd_ex(a, fd, 1); /* close on exec */
}
ap_unblock_alarms();
errno = save_errno;
@@ -2059,7 +2059,7 @@
errno = save_errno;
return -1;
}
- ap_note_cleanups_for_socket(p, fd);
+ ap_note_cleanups_for_socket_ex(p, fd, 1); /* close socket on exec */
ap_unblock_alarms();
return fd;
}
Index: main/http_main.c
===================================================================
RCS file: /home/cvs/apache-1.3/src/main/http_main.c,v
retrieving revision 1.596
diff -u -r1.596 http_main.c
--- main/http_main.c 25 Oct 2002 21:12:23 -0000 1.596
+++ main/http_main.c 11 Dec 2002 13:34:13 -0000
@@ -3655,7 +3655,7 @@
s = ap_slack(s, AP_SLACK_HIGH);
#endif
- ap_note_cleanups_for_socket(p, s); /* arrange to close on exec or restart */
+ ap_note_cleanups_for_socket_ex(p, s, 1); /* arrange to close on exec or restart
+*/
#ifdef TPF
os_note_additional_cleanups(p, s);
#endif /* TPF */
@@ -3796,7 +3796,7 @@
#ifdef WORKAROUND_SOLARIS_BUG
s = ap_slack(s, AP_SLACK_HIGH);
- ap_note_cleanups_for_socket(p, s); /* arrange to close on exec or restart */
+ ap_note_cleanups_for_socket_ex(p, s, 1); /* arrange to close on exec or restart
+*/
#endif
ap_unblock_alarms();
@@ -3903,7 +3903,7 @@
fd = make_sock(p, &lr->local_addr);
}
else {
- ap_note_cleanups_for_socket(p, fd);
+ ap_note_cleanups_for_socket_ex(p, fd, 1);
}
/* if we get here, (fd >= 0) && (fd < FD_SETSIZE) */
FD_SET(fd, &listenfds);
@@ -4517,7 +4517,7 @@
*/
signal(SIGUSR1, SIG_IGN);
- ap_note_cleanups_for_socket(ptrans, csd);
+ ap_note_cleanups_for_socket_ex(ptrans, csd, 1);
/* protect various fd_sets */
#ifdef CHECK_FD_SETSIZE
@@ -4565,7 +4565,7 @@
"dup: couldn't duplicate csd");
dupped_csd = csd; /* Oh well... */
}
- ap_note_cleanups_for_socket(ptrans, dupped_csd);
+ ap_note_cleanups_for_socket_ex(ptrans, dupped_csd, 1);
/* protect various fd_sets */
#ifdef CHECK_FD_SETSIZE
@@ -5092,7 +5092,7 @@
#ifdef SCOREBOARD_FILE
else {
ap_scoreboard_fname = ap_server_root_relative(pconf, ap_scoreboard_fname);
- ap_note_cleanups_for_fd(pconf, scoreboard_fd);
+ ap_note_cleanups_for_fd_ex(pconf, scoreboard_fd, 1); /* close on exec */
}
#endif
@@ -5892,7 +5892,7 @@
requests_this_child++;
- ap_note_cleanups_for_socket(ptrans, csd);
+ ap_note_cleanups_for_socket_ex(ptrans, csd, 1);
/*
* We now have a connection, so set it up with the appropriate
@@ -5924,7 +5924,7 @@
"dup: couldn't duplicate csd");
dupped_csd = csd; /* Oh well... */
}
- ap_note_cleanups_for_socket(ptrans, dupped_csd);
+ ap_note_cleanups_for_socket_ex(ptrans, dupped_csd, 1);
#endif
ap_bpushfd(conn_io, csd, dupped_csd);
@@ -6140,7 +6140,7 @@
if (fd > listenmaxfd)
listenmaxfd = fd;
}
- ap_note_cleanups_for_socket(p, fd);
+ ap_note_cleanups_for_socket_ex(p, fd, 1);
lr->fd = fd;
if (lr->next == NULL) {
/* turn the list into a ring */
static char rcsid[] = { "$Id: openfds.c,v 1.3 2002/12/11 15:07:23 martin Exp $" };
/*
* $Log: openfds.c,v $
* Revision 1.3 2002/12/11 15:07:23 martin
* Delete debugging stuff
*
* Revision 1.2 2002/12/11 15:05:56 martin
* Delete unneeded stuff
*
* Revision 1.1 2002/12/11 14:52:42 martin
* Derived from machine.c: open fd lister
*/
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <varargs.h>
#include <ctype.h>
#include <limits.h>
#include <locale.h>
#include <dirent.h>
#include <string.h>
#include <pwd.h>
#include <grp.h>
#include <sys/stat.h>
#if defined(__FreeBSD__) || defined(__linux__)
#include <errno.h>
#include <fcntl.h> /* for locking */
#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/in.h>
#if defined(__FreeBSD__)
#include <libutil.h>
#endif
#define sintosa(sin) ((struct sockaddr *)(sin))
#include <netdb.h>
#endif
#define DIM(a) (sizeof(a) / sizeof((a)[0]))
static void _nfile(void);
int
main(int argc, char *argv[])
{
const char *p = strrchr(argv[0], '.');
if (p != NULL && strcmp(p, ".cgi") == 0)
printf("Content-Type: text/plain\r\n\r\n");
_nfile();
exit(0);
return 0;
}
static void
warn(va_alist)
va_dcl
{
static int warned = 0;
char *str;
va_list args;
va_start(args);
str = va_arg(args, char *);
if (warned == 0) {
warned = 1;
printf("\nList of open file
descriptors:\n==============================\n");
}
printf(" -> ");
vprintf(str, args);
printf("\n");
va_end(args);
}
static void *
StrChr(str, chr)
char *str;
int chr;
{
for (; *str != '\0'; ++str)
if (*str == chr)
return (void *)str;
return NULL;
}
#if defined(__FreeBSD__) || defined(__linux__)
const char *
filename(dev_t dev, mode_t mode, int inum)
{
static struct
{
dev_t dev;
mode_t mode;
const char *name;
const char *mountpt;
} cache[128];
int i;
FILE *pipe_f;
static char buf[1024];
struct stat stbuf;
char *devname, *mounton;
dev_t d = 0;
mode &= S_IFMT;
i=0;
if (cache[i].dev == 0 && cache[i].mode == 0)
{
pipe_f = popen("mount", "r");
while (fgets(buf, sizeof buf, pipe_f) != NULL)
{
devname = strtok(buf, " \t\r\n");
if (strchr(devname, ':') != NULL)
continue;
if (0 != stat(devname, &stbuf))
continue;
d = (S_ISCHR(stbuf.st_mode)||S_ISBLK(stbuf.st_mode)) ? stbuf.st_rdev :
stbuf.st_dev;
if (strcmp("on", mounton = strtok(NULL, " \t\r\n")) == 0)
mounton = strtok(NULL, " \t\r\n");
devname = strdup(devname);
if (i < DIM(cache))
{
cache[i].dev = d;
cache[i].mode = stbuf.st_mode & S_IFMT;
cache[i].name = devname;
cache[i].mountpt = strdup(mounton);
++i;
}
}
pclose(pipe_f);
}
for (i=0; i<DIM(cache); ++i)
{
if (cache[i].dev == 0 && cache[i].mode == 0)
return NULL;
if ((d = cache[i].dev) == dev)
break;
}
strcpy(buf,"");
if (dev == d && inum != 0)
{
sprintf(buf, "find %s -xdev -type %c -inum %ld -print 2>/dev/null",
cache[i].mountpt,
S_ISCHR(mode) ? 'c' :
S_ISBLK(mode) ? 'b' :
S_ISREG(mode) ? 'f' :
S_ISLNK(mode) ? 'l' :
S_ISSOCK(mode) ? 's' : 'd',
inum);
pipe_f = popen(buf, "r");
strcpy(buf, "");
if (pipe_f == NULL || fgets(buf, sizeof buf, pipe_f) == 0)
strcpy(buf,"");
if (pipe_f) pclose(pipe_f);
}
if ((i = strlen(buf)) == 0)
sprintf(buf, (minor(dev) > 65535) ? "ino=%d,dev=(%d,0x%08x)" :
"ino=%d,dev=(%d,%d)", inum, major(dev), minor(dev));
else if (buf[i-1] == '\n')
buf[i-1] = '\0'; /* strip trailing '\n' */
return buf;
}
/* #define devname(a,b) NULL */
#endif
static const char *
identify(int fd)
{
struct stat stbuf;
char *type = "?";
static char buf[8192];
char size_or_device[16];
static const char *rwx[8] = {
"---", "--x", "-w-","-wx",
"r--", "r-x", "rw-","rwx",
};
struct passwd *pwd;
struct group *grp;
char user[16], group[16];
if (fstat(fd, &stbuf) < 0)
return "fstat failed";
pwd = getpwuid(stbuf.st_uid);
if (pwd == 0) sprintf(user, "%d", stbuf.st_uid);
else strcpy (user, pwd->pw_name);
grp = getgrgid(stbuf.st_gid);
if (grp == 0) sprintf(group, "%d", stbuf.st_gid);
else strcpy (group, grp->gr_name);
if (S_ISDIR(stbuf.st_mode)) type = "d";
else if (S_ISCHR(stbuf.st_mode)) type = "c";
else if (S_ISBLK(stbuf.st_mode)) type = "b";
else if (S_ISREG(stbuf.st_mode)) type = "-";
else if (S_ISLNK(stbuf.st_mode)) type = "l";
else if (S_ISSOCK(stbuf.st_mode)) type = "s";
#ifdef S_ISWHT
else if (S_ISWHT(stbuf.st_mode)) type = "w";
#endif
else if (S_ISFIFO(stbuf.st_mode)) type = "p";
else type = "?";
if (S_ISCHR(stbuf.st_mode) || S_ISBLK(stbuf.st_mode)) {
sprintf(size_or_device, "%4d, ", major(stbuf.st_rdev));
sprintf(&size_or_device[strlen(size_or_device)],
minor(stbuf.st_rdev) & ~0xFFFF ? "0x%08x" : "%5d",
minor(stbuf.st_rdev));
}
else
sprintf(size_or_device, "%11d", stbuf.st_size);
sprintf(buf, "%s%s%s%s %d %-8.8s %-8.8s %s /dev/fd/%d", type,
rwx[0x07 & (stbuf.st_mode>>6)],
rwx[0x07 & (stbuf.st_mode>>3)],
rwx[0x07 & (stbuf.st_mode>>0)],
stbuf.st_nlink, user, group,
size_or_device, fd);
#if defined(__FreeBSD__) || defined(__linux__)
if (S_ISREG(stbuf.st_mode))
{
const char *dev = filename(stbuf.st_dev, stbuf.st_mode, stbuf.st_ino);
if (dev == NULL)
sprintf(&buf[strlen(buf)], " ino=%d,dev=(%d,%d)", stbuf.st_ino,
major(stbuf.st_dev), minor(stbuf.st_dev));
else if (dev[0] == '/')
{
sprintf(&buf[strlen(buf)], " == %s", dev);
}
else
{
sprintf(&buf[strlen(buf)], " ino=%d,%s", stbuf.st_ino, dev);
}
}
if (S_ISSOCK(stbuf.st_mode))
{
struct sockaddr_in sa;
int size = sizeof sa;
char host[NI_MAXHOST];
char port[NI_MAXSERV];
memset((void*)&sa,'\xFF', sizeof sa);
size = sizeof sa;
if (0 != getsockname(fd, sintosa(&sa), &size))
sprintf(&buf[strlen(buf)], " (getsockname failed: %s)", strerror(errno));
else if (0 != getnameinfo(sintosa(&sa), size, host, sizeof host, port, sizeof
port, NI_NUMERICSERV))
strcat(buf, " (getnameinfo failed)");
else
sprintf(&buf[strlen(buf)], " lcl:[%s:%s]", host, port);
memset((void*)&sa,'\xFF', sizeof sa);
size = sizeof sa;
if (0 != getpeername(fd, sintosa(&sa), &size))
sprintf(&buf[strlen(buf)], " (%s)", strerror(errno));
else if (0 != getnameinfo(sintosa(&sa), size, host, sizeof host, port, sizeof
port, NI_NUMERICSERV))
strcat(buf, " (getnameinfo failed)");
else
sprintf(&buf[strlen(buf)], " rmt:[%s:%s]", host, port);
}
#endif
return buf;
}
static void
_nfile(void)
{
static int fd = -1;
FILE *file;
int anz;
++fd;
if ((file = fopen("/dev/null", "r")) == NULL) {
#ifdef _NFILE
if (fd != _NFILE) {
warn("%d file descriptors are available but _NFILE == %d", fd,
_NFILE);
}
#endif
return;
} else {
if (fileno(file) != fd) {
while (fd < fileno(file)) {
warn("%s", identify(fd));
++fd;
}
fd = fileno(file);
}
#ifdef _NFILE
if (fd < _NFILE)
#endif
_nfile();
fclose(file);
--fd;
return;
}
}
