You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by co...@apache.org on 2001/10/26 20:05:26 UTC
cvs commit: apache-1.3/src/support checkgid.c Makefile.tmpl
coar 01/10/26 11:05:26
Modified: src CHANGES
src/support Makefile.tmpl
Added: src/support checkgid.c
Log:
Some platforms varf on a setgid(-1) and hence httpd will fall
over immediately after being started. However, since
'Group #-1' is syntactically correct, apachectl won't catch
this and will assume the server started successfully. This
checkgid app will return -1 if any of the Apache-understandable
group values (i.e., name or "#n") are invalid. apachestl still
needs to be enhanced to use this.
Revision Changes Path
1.1738 +4 -0 apache-1.3/src/CHANGES
Index: CHANGES
===================================================================
RCS file: /home/cvs/apache-1.3/src/CHANGES,v
retrieving revision 1.1737
retrieving revision 1.1738
diff -u -u -r1.1737 -r1.1738
--- CHANGES 2001/10/23 15:45:17 1.1737
+++ CHANGES 2001/10/26 18:05:26 1.1738
@@ -1,5 +1,9 @@
Changes with Apache 1.3.23
+ *) Add checkgid app to do run-time validation of Group directive
+ values which might cause the server to fall over, but which
+ are syntactically correct. [Ken Coar]
+
*) NetWare: Added mod_unique_id to the project file.
[Brad Nicholes bnicholes@novell.com]
1.33 +5 -2 apache-1.3/src/support/Makefile.tmpl
Index: Makefile.tmpl
===================================================================
RCS file: /home/cvs/apache-1.3/src/support/Makefile.tmpl,v
retrieving revision 1.32
retrieving revision 1.33
diff -u -u -r1.32 -r1.33
--- Makefile.tmpl 2001/04/04 19:15:21 1.32
+++ Makefile.tmpl 2001/10/26 18:05:26 1.33
@@ -12,9 +12,9 @@
INCLUDES=$(INCLUDES1) $(INCLUDES0) $(EXTRA_INCLUDES)
LDFLAGS=$(LDFLAGS1) $(EXTRA_LDFLAGS) -L$(OSDIR) -L$(SRCDIR)/ap
-TARGETS=htpasswd htdigest rotatelogs logresolve ab apxs
+TARGETS=htpasswd htdigest rotatelogs logresolve ab apxs checkgid
-OBJS=htpasswd.o htdigest.o rotatelogs.o logresolve.o ab.o
+OBJS=htpasswd.o htdigest.o rotatelogs.o logresolve.o ab.o checkgid.o
.c.o:
$(CC) -c $(INCLUDES) $(CFLAGS) $<
@@ -35,6 +35,9 @@
ab: ab.o
$(CC) $(CFLAGS) -o ab $(LDFLAGS) ab.o $(LIBS)
+
+checkgid: checkgid.o
+ $(CC) $(CFLAGS) -o checkgid $(LDFLAGS) checkgid.o $(LIBS)
apxs: apxs.pl
sed <apxs.pl >apxs \
1.1 apache-1.3/src/support/checkgid.c
Index: checkgid.c
===================================================================
/* ====================================================================
* The Apache Software License, Version 1.1
*
* Copyright (c) 2000 The Apache Software Foundation. All rights
* reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
* are met:
*
* 1. Redistributions of source code must retain the above copyright
* notice, this list of conditions and the following disclaimer.
*
* 2. Redistributions in binary form must reproduce the above copyright
* notice, this list of conditions and the following disclaimer in
* the documentation and/or other materials provided with the
* distribution.
*
* 3. The end-user documentation included with the redistribution,
* if any, must include the following acknowledgment:
* "This product includes software developed by the
* Apache Software Foundation (http://www.apache.org/)."
* Alternately, this acknowledgment may appear in the software itself,
* if and wherever such third-party acknowledgments normally appear.
*
* 4. The names "Apache" and "Apache Software Foundation" must
* not be used to endorse or promote products derived from this
* software without prior written permission. For written
* permission, please contact apache@apache.org.
*
* 5. Products derived from this software may not be called "Apache",
* nor may "Apache" appear in their name, without prior written
* permission of the Apache Software Foundation.
*
* THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESSED OR IMPLIED
* WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
* OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
* DISCLAIMED. IN NO EVENT SHALL THE APACHE SOFTWARE FOUNDATION OR
* ITS CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
* SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
* LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
* USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
* ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
* OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
* OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
* SUCH DAMAGE.
* ====================================================================
* ====================================================================
*
* This software consists of voluntary contributions made by many
* individuals on behalf of the Apache Software Foundation. For more
* information on the Apache Software Foundation, please see
* <http://www.apache.org/>.
*/
/*
* Given one or more group identifers on the command line (e.g.,
* "httpd" or "#-1"), figure out whether they'll be valid for
* the server to use at run-time.
*
* If a groupname isn't found, or we can't setgid() to it, return
* -1. If all groups are valid, return 0.
*
* This may need to be run as the superuser for the setgid() to
* succeed; running it as any other user may result in a false
* negative.
*/
#include <stdio.h>
#include "httpd.h"
#include "http_conf_globals.h"
int main(int argc, char *argv[])
{
int i;
int result;
gid_t gid;
struct group *grent;
struct group fake_grent;
/*
* Assume success. :-)
*/
result = 0;
for (i = 1; i < argc; ++i) {
char *arg;
arg = argv[i];
/*
* If it's from a 'Group #-1' statement, get the numeric value
* and skip the group lookup stuff.
*/
if (*arg == '#') {
gid = atoi(&arg[1]);
fake_grent.gr_gid = gid;
grent = &fake_grent;
}
else {
grent = getgrnam(arg);
}
/*
* A NULL return means no such group was found, so we're done
* with this one.
*/
if (grent == NULL) {
fprintf(stderr, "%s: group '%s' not found\n", argv[0], arg);
result = -1;
}
else {
int check;
/*
* See if we can switch to the numeric GID we have. If so,
* all well and good; if not, well..
*/
gid = grent->gr_gid;
check = setgid(gid);
if (check != 0) {
fprintf(stderr, "%s: invalid group '%s'\n", argv[0], arg);
perror(argv[0]);
result = -1;
}
}
}
/*
* Worst-case return value.
*/
return result;
}
/*
* Local Variables:
* mode: C
* c-file-style: "bsd"
* End:
*/
Re: cvs commit: apache-1.3/src/support checkgid.c Makefile.tmpl
Posted by Martin Kraemer <Ma...@Fujitsu-Siemens.com>.
On Mon, Oct 29, 2001 at 02:41:43PM -0500, Rodent of Unusual Size wrote:
> Marc Slemko wrote:
> >
> > It is the start of a very ugly trend.
I fear I have to agree with Marc here.
> Okey, I'm convinced. It would be better to have this
> done by httpd itself in some way in which apachectl
> can detect the failure -- like doing the setuid() and
> setgid() before detach(). (Having apachectl check the
> log for the start message is just as ugly as my original
> idea because of the locating and parsing the logfile
> issue.)
IMHO this is an issue for "make install" and neither for
apachectl nor for httpd's startup phase.
"Make install" inserts "Group #-1" into the config, and could
just as well do a quick check over "#-1", "nobody" and "nogroup".
Anything the admin will configure after that is up to her, and she
better know where the error_log file is.
With that in mind, I agree on the source of checkgid being in the
source tree, but I disagree a little (-0.5) with it being installed
as a support tool proper. On a different target machine (in the case
of binbuild.sh), groups may be configured differently, but that
should IMO be handled by the administrator there.
Just my $.02
Martin
--
<Ma...@Fujitsu-Siemens.com> | Fujitsu Siemens
Fon: +49-89-636-46021, FAX: +49-89-636-47655 | 81730 Munich, Germany
Re: cvs commit: apache-1.3/src/support checkgid.c Makefile.tmpl
Posted by Rodent of Unusual Size <Ke...@Golux.Com>.
Marc Slemko wrote:
>
> It is the start of a very ugly trend.
Okey, I'm convinced. It would be better to have this
done by httpd itself in some way in which apachectl
can detect the failure -- like doing the setuid() and
setgid() before detach(). (Having apachectl check the
log for the start message is just as ugly as my original
idea because of the locating and parsing the logfile
issue.)
ISTR some tool that would analyse a source file and
display a tree of who calls what; that would help immensely
in reordering things to move these check pre-detach().
Anyone remember it or anything like it? (Usable on Unix,
preferably.. ;-)
--
#ken P-)}
Ken Coar, Sanagendamgagwedweinini http://Golux.Com/coar/
Author, developer, opinionist http://Apache-Server.Com/
"All right everyone! Step away from the glowing hamburger!"
Re: cvs commit: apache-1.3/src/support checkgid.c Makefile.tmpl
Posted by Marc Slemko <ma...@znep.com>.
On Mon, 29 Oct 2001, Rodent of Unusual Size wrote:
> Marc Slemko wrote:
> >
> > It is completely bogus to start adding support
> > programs to check every possible error condition
>
> Hardly what is happening here.
It is the start of a very ugly trend. How long will it be before there
is the next bug report "we need a checkxxx because error condition x
isn't properly reported".
You can do a hell of a lot of damage to the design of a piece of software
by throwing random hacks at it saying "oh, this is only a little bit of
ugly stuff, and it is ok because I don't want to do it the right
way". Please don't do that to apache any more than already happens.
>
> > It results in all the error checking code being
> > duplicated in two places
>
> Since there is NO 'error checking code' for this
> case in the server proper, there is no duplication.
Of course there is! Apache tries, if it fails it logs an error.
What exactly do you think:
if (setgid(ap_group_id) == -1) {
ap_log_error(APLOG_MARK, APLOG_ALERT, server_conf,
"setgid: unable to set group id to Group %u",
(unsigned)ap_group_id);
clean_child_exit(APEXIT_CHILDFATAL);
does?
> > The proper solution is to either have Apache do
> > more checking itself before it daemonizes or change
> > apachectl to handle errors that happen after it
> > daemonizes.
>
> Since AFAICS this condition can only reliably be
> checked by actually calling setgid(), the first would
> seem to potentially require some major work. And
> the latter introduces timing issues that are worse
> than the proposed solution.
???
If it is so impossible to have Apache do all config checking before
daemonizing, then what is so horrible about having apachectl start
Apache, then wait to make sure it started? apachectl could even watch
the logfile for a server-has-started indication that we make sure to
only log after all initialization has been done.
>
> I disagree with your remarks in any event, considering
> all the noise on the list in the past about 'the way
> for non-httpd apps to do things with/to the config is
> through preprocessing' -- this is just preprocessing
> that doesn't store the config in LDAP, or XML, or
> whatever, but does a band-aid check the server doesn't.
Umh... this is a COMPLETELY different ballpark. Just how do you
expect apachectl to parse Apache configuration files anyway? I guess
the next commit will be code to reparse all apache config files.
>
> > It is not to duplicate all the error checking code
> > in external programs.
>
> Again, that ain't what's happening here, so a little less
> hyperbole, eh?
>
> I'll look into what's involved in doing the setgid()
> before daemonising, but if it's a hassle on 1.3 I'm
> going to stick with this band-aid solution because it
> *does* address an existing problem. 2.0 can be done better.
Well, sorry to interrupt you in doing what you Will Do no matter what.
This is wrong because:
1. it is non-trivial to parse apache config files from apachectl How do
you deal with includes? How do you deal with the fact that there can
be Group directives in vhosts which may be before or after the main
Group directive?
2. there are a huge array of other problems that people complain about with
similar causes that are not fixed by this and that, if we continue down
this path, will require duplicating a hell of a lot of code for no reason.
Why not just do it right once and fix it for _EVERYTHING_?
3. a lot of other similar problems can not be detected so trivially (eg.
problems in third party module init functions), but
actually require the full server and all it's code to check for.
4. people are perfectly capable of figuring out "hey, the server didn't
work, what's in the error log?", and if they can't then they will have a
lot of other problems using Apache. This particular problem is one
that generally _ONLY_ happens the very first time someone is trying to
start Apache, so the "I added some configuration then tried to restart
Apache, and it said it worked so I went and got drunk, but it actually
failed to restart" complaint is in a completely different ballpark.
This is a very bad route to be going down just to change the behaviour of
some default installs the first time someone tries to run the server so
that apachectl spits out an error instead of them finding it in the
error log.
Nearly every post I see complaining about "can't setgid to xxxx"
includes the error message from the logs, the user just has no clue
what to do with it. If this is a problem, then we need to fix the problem,
not go off on some wild journey...
Re: cvs commit: apache-1.3/src/support checkgid.c Makefile.tmpl
Posted by Rodent of Unusual Size <Ke...@Golux.Com>.
Marc Slemko wrote:
>
> It is completely bogus to start adding support
> programs to check every possible error condition
Hardly what is happening here.
> It results in all the error checking code being
> duplicated in two places
Since there is NO 'error checking code' for this
case in the server proper, there is no duplication.
> The proper solution is to either have Apache do
> more checking itself before it daemonizes or change
> apachectl to handle errors that happen after it
> daemonizes.
Since AFAICS this condition can only reliably be
checked by actually calling setgid(), the first would
seem to potentially require some major work. And
the latter introduces timing issues that are worse
than the proposed solution.
I disagree with your remarks in any event, considering
all the noise on the list in the past about 'the way
for non-httpd apps to do things with/to the config is
through preprocessing' -- this is just preprocessing
that doesn't store the config in LDAP, or XML, or
whatever, but does a band-aid check the server doesn't.
> It is not to duplicate all the error checking code
> in external programs.
Again, that ain't what's happening here, so a little less
hyperbole, eh?
I'll look into what's involved in doing the setgid()
before daemonising, but if it's a hassle on 1.3 I'm
going to stick with this band-aid solution because it
*does* address an existing problem. 2.0 can be done better.
--
#ken P-)}
Ken Coar, Sanagendamgagwedweinini http://Golux.Com/coar/
Author, developer, opinionist http://Apache-Server.Com/
"All right everyone! Step away from the glowing hamburger!"
Re: cvs commit: apache-1.3/src/support checkgid.c Makefile.tmpl
Posted by Marc Slemko <ma...@znep.com>.
On 26 Oct 2001 coar@apache.org wrote:
> coar 01/10/26 11:05:26
>
> Modified: src CHANGES
> src/support Makefile.tmpl
> Added: src/support checkgid.c
> Log:
> Some platforms varf on a setgid(-1) and hence httpd will fall
> over immediately after being started. However, since
> 'Group #-1' is syntactically correct, apachectl won't catch
> this and will assume the server started successfully. This
> checkgid app will return -1 if any of the Apache-understandable
> group values (i.e., name or "#n") are invalid. apachestl still
> needs to be enhanced to use this.
Woah!
It is completely bogus to start adding support programs to check every
possible error condition, and then expect to add them all to apachectl to
second-guess Apache. You will end up with subtle differences in what one
accepts and the other doesn't, I have no idea how you expect to find out
what Group directive is set in the config file from apachectl, etc.
It results in all the error checking code being duplicated in two places,
and requires twice the work to maintain. The proper solution is to either
have Apache do more checking itself before it daemonizes or change
apachectl to handle errors that happen after it daemonizes. It is not to
duplicate all the error checking code in external programs.