You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Thom May <th...@planetarytramp.net> on 2002/11/21 02:58:44 UTC
[Patch] Be more selective on includes
This is in response to a debian bug request; basically it just tightens up
the list of allowed characters, so we don't include .dotfiles and backups
etc.
Thoughts?
-Thom
Index: server/config.c
===================================================================
RCS file: /home/cvspublic/httpd-2.0/server/config.c,v
retrieving revision 1.156
diff -u -r1.156 config.c
--- server/config.c 12 Sep 2002 20:04:07 -0000 1.156
+++ server/config.c 21 Nov 2002 01:58:06 -0000
@@ -76,6 +76,7 @@
#include "apr_portable.h"
#include "apr_file_io.h"
#include "apr_fnmatch.h"
+#include "apr_lib.h"
#define APR_WANT_STDIO
#define APR_WANT_STRFUNC
@@ -1434,6 +1435,20 @@
return strcmp(f1->fname,f2->fname);
}
+static int fname_valid(const char *fname)
+{
+ const char *c = fname;
+ if (!apr_isalnum(*c))
+ return 0;
+ ++c;
+ while (*c) {
+ if(!apr_isalnum(*c) && *c!='_' && *c!='-' && *c!='.')
+ return 0;
+ ++c;
+ }
+ return 1;
+}
+
AP_DECLARE(void) ap_process_resource_config(server_rec *s, const char *fname,
ap_directive_t **conftree,
apr_pool_t *p,
@@ -1510,7 +1525,8 @@
&& strcmp(dirent.name, "..")
&& (!ispatt ||
apr_fnmatch(pattern, dirent.name,
- FNM_PERIOD) == APR_SUCCESS)) {
+ FNM_PERIOD) == APR_SUCCESS)
+ && fname_valid(dirent.name)) {
fnew = (fnames *) apr_array_push(candidates);
fnew->fname = ap_make_full_path(p, path, dirent.name);
}
Re: [Patch] Be more selective on includes
Posted by André Malo <nd...@perlig.de>.
* Thom May wrote:
> * André Malo (nd@perlig.de) wrote :
>> * Thom May wrote:
>>
>>> This is in response to a debian bug request; basically it just tightens up
>>> the list of allowed characters, so we don't include .dotfiles and backups
>>> etc.
>>> Thoughts?
>>
>> hmm. I don't like it. The most can easily be done with normal wildcard
>> matching. If your patch is applied and I have filenames (already), that
>> don't match the hardcoded (!) rules, I'm lost.
>>
> OK, so Justin and I were just discussing this in the bar at apachecon, and
> I explained the use-case I was interested in, and why I wanted this patch:
> We have a directory tree containing vhosts:
> /etc/apache2/sites-enabled
> this contains file in this form:
> samizdat.positive-internet.com
> www.planetarytramp.net
> www.example.org.uk
> ...
ah ok, understand.
A workaround would be:
/etc/apache2/vhosts/ contains the config files and
/etc/apache2/sites-enabled/ contains (sym-)links to the enabled ones.
to be very safe, one can name the symlinks *.conf or similar and use
wildcards to include them.
> So, I guess the second proposal would be to keep the behaviour my patch
> proposes, but add a SafeInclude on/off directive (defaulting to Off) to
> define whether you want the behaviour or not.
A Feature that can't be disabled is a bug ;-)
Seriously, I would like that - if configurable - but would then be +1 for
defaulting to *On*, so the User can loosen the rules, if he's able to find
the directive in the docs...
Another alternative would be a directive similar to the mod_autoindex
IndexIgnore (e.g. IncludeIgnore). I think, that would be the most flexible
way to handle the problem. (Proposed priority handling: IncludeIgnore
overrides Include wildcards and directory includes, but *not* wildcard-free
filenames)
nd
--
"Die Untergeschosse der Sempergalerie bleiben währenddessen aus
statistischen Gründen geflutet." -- Spiegel Online
Re: [Patch] Be more selective on includes
Posted by Joshua Slive <sl...@finance.commerce.ubc.ca>.
On Thu, 21 Nov 2002, Thom May wrote:
> OK, so Justin and I were just discussing this in the bar at apachecon, and
> I explained the use-case I was interested in, and why I wanted this patch:
> We have a directory tree containing vhosts:
> /etc/apache2/sites-enabled
> this contains file in this form:
> samizdat.positive-internet.com
> www.planetarytramp.net
> www.example.org.uk
> ...
>
> ie, the file name matches the ServerName entry. In this case, wildcard
> matching is either too generic (Include sites-enabled/*), or not workable at
> all(Include sites-enabled/*.com\nInclude sites-enabled/*.org etc etc).
Awww, come on... Just rename the damn things
samizdat.postitive-internet.com.conf
or something to that effect.
I am opposed to any SafeInclude type directives because:
1. Almost all reasonable use cases can be solved with fnmatch wildcards.
2. As soon as we add those exceptions you want, 100 people will come
crawling out of the woodwork asking for exceptions for their special
files.
3. It is a bad idea to have file-types hardcoded in Apache.
4. It is unnecessary complexity, which makes things harder for everybody.
Joshua.
Re: [Patch] Be more selective on includes
Posted by Thom May <th...@planetarytramp.net>.
* André Malo (nd@perlig.de) wrote :
> * Thom May wrote:
>
> > This is in response to a debian bug request; basically it just tightens up
> > the list of allowed characters, so we don't include .dotfiles and backups
> > etc.
> > Thoughts?
>
> hmm. I don't like it. The most can easily be done with normal wildcard
> matching. If your patch is applied and I have filenames (already), that
> don't match the hardcoded (!) rules, I'm lost.
>
OK, so Justin and I were just discussing this in the bar at apachecon, and
I explained the use-case I was interested in, and why I wanted this patch:
We have a directory tree containing vhosts:
/etc/apache2/sites-enabled
this contains file in this form:
samizdat.positive-internet.com
www.planetarytramp.net
www.example.org.uk
...
ie, the file name matches the ServerName entry. In this case, wildcard
matching is either too generic (Include sites-enabled/*), or not workable at
all(Include sites-enabled/*.com\nInclude sites-enabled/*.org etc etc).
Now, say we edit a file in this directory, and while we are doing so,
apache gets restarted. most (unix) editors will leave a .name-of-file.swp
or name-of-file~ in the
directory as a backup while you are editing the file, so at this point you
know have two semi-identical lumps of config loaded, one of which may be
broken, or missing config or whatever. This seems like an uber-bad thing.
So, I guess the second proposal would be to keep the behaviour my patch
proposes, but add a SafeInclude on/off directive (defaulting to Off) to
define whether you want the behaviour or not.
Cheers,
-Thom
Re: [Patch] Be more selective on includes
Posted by Justin Erenkrantz <je...@apache.org>.
--On Wednesday, November 20, 2002 9:19 PM -0500 Joshua Slive
<jo...@slive.ca> wrote:
> Right. People should be using
> Include conf/*.conf
>
> The docs now explicitly discourage the use of directory includes to
> avoid exactly this problem.
++1. -- justin
Re: [Patch] Be more selective on includes
Posted by Joshua Slive <jo...@slive.ca>.
On Thu, 21 Nov 2002, André Malo wrote:
> * Thom May wrote:
>
> > This is in response to a debian bug request; basically it just tightens up
> > the list of allowed characters, so we don't include .dotfiles and backups
> > etc.
> > Thoughts?
>
> hmm. I don't like it. The most can easily be done with normal wildcard
> matching. If your patch is applied and I have filenames (already), that
> don't match the hardcoded (!) rules, I'm lost.
Right. People should be using
Include conf/*.conf
The docs now explicitly discourage the use of directory includes to avoid
exactly this problem.
Joshua.
Re: [Patch] Be more selective on includes
Posted by André Malo <nd...@perlig.de>.
* Thom May wrote:
> This is in response to a debian bug request; basically it just tightens up
> the list of allowed characters, so we don't include .dotfiles and backups
> etc.
> Thoughts?
hmm. I don't like it. The most can easily be done with normal wildcard
matching. If your patch is applied and I have filenames (already), that
don't match the hardcoded (!) rules, I'm lost.
nd
--
sub the($){+shift} sub answer (){ord q
[* It is always 42! *] }
print the answer
# André Malo # http://www.perlig.de/ #
Re: [Patch] Be more selective on includes
Posted by Thom May <th...@planetarytramp.net>.
* William A. Rowe, Jr. (wrowe@apache.org) wrote :
> At 07:58 PM 11/20/2002, Thom May wrote:
> >This is in response to a debian bug request; basically it just tightens up
> >the list of allowed characters, so we don't include .dotfiles and backups
> >etc.
> >Thoughts?
>
> Yea... I'd ask for exactly what I've been asking for over a year...
>
> ...does anyone have a decent reference to the exclusion applied
> for other packages that provide the config directory globbing that
> we added/emulated with this feature? Can we be consistent for
> the benefit of the administrator community here?
>
Well, my suggested change is (on debian at least) providing exactly the same
logic as run-parts(8), which is used to process /etc/cron.*/ and others. So
it's consistent for our platform, at least.
run-parts is present on Red Hat 8, also.
Cheers,
-Thom
> Bill
Re: [Patch] Be more selective on includes
Posted by "William A. Rowe, Jr." <wr...@apache.org>.
At 07:58 PM 11/20/2002, Thom May wrote:
>This is in response to a debian bug request; basically it just tightens up
>the list of allowed characters, so we don't include .dotfiles and backups
>etc.
>Thoughts?
Yea... I'd ask for exactly what I've been asking for over a year...
...does anyone have a decent reference to the exclusion applied
for other packages that provide the config directory globbing that
we added/emulated with this feature? Can we be consistent for
the benefit of the administrator community here?
Bill