Re: Patch: allow /./ and /../ in URL paths
Hi. I fiddled a bit with your patch, and it is now more or less in the new beta. (1.6b7), although the implemtation is a bit different. I took the liberty of slightly changing the semantics because I want to be able to back-port this to 1.5 without breaking existing configurations. To this end, the /./ /../ (and //) smashing must be explicitly enabled in the configuration file via the SanitizePath directive. If you do not set this, mathopd will treat URLs with /./ etc as an error. Also URLs that end with /. and /.. are not allowed. (As they were with the original patch.) It could be argued that one should make some attempt to 'fix' these too, but that would be a bit messy. Perhaps someone has any more thoughts to this? If no problems turn up I will probably release a new 1.5 version sometime next week. Cheers Michiel
Re: Patch: allow /./ and /../ in URL paths
On Thu, Oct 12, 2006 at 12:57:14PM +0300, Peter Pentchev wrote: > Hi, > > First of all, thanks for writing and maintaing a great, lightweight, > simple webserver! > > My attention was recently drawn to Mathopd when a friend decided to use > it as a lightweight replacement of Apache for one of the Bulgarian > mirrors of Debian Linux. It's been doing great so far, with a single > excepion - as y'all might have already guessed, it's /../ components in > the URL paths. It turns out that some configurations for fetching > Debian distribution files do indeed depend on traversing directories > back up - it happens only very rarely, but still, it happens, and our > new mirror webserver does not quite allow it. After another discussion with Alexander Velin, the above-mentioned Debian mirror maintainer, it turned out that it may also be desirable to sometimes allow // in the URL paths, ignoring the second and further slashes. So, here's another version of the patch which changes several things: src/request.c - check_path(): - now returns a bitmask of flags instead of a single boolean value. This makes it possible for it to signal both fatal errors and possible error conditions - see below for deferred error processing. - as in the previous patch, rewrites the request URL path, removing /./ components and processing /../ by removing the last stored "real" path component. - raises a flag in the return code if a dotfile is found in the path. - raises a flag in the return code if a double slash is found in the URL path. src/request.c - process_path(): - only invokes check_path() once now, not twice as in the previous version of the patch. - invokes check_path() before faketoreal(), so faketoreal() works on the real location (consider http://www.example.com/cgi-bin/../doc/). - defers all error processing, including the dotfiles and double-slash checks, after the faketoreal() invocation - check_path() is now called much earlier and it has no idea what the control configuration will be, whether it will allow dotfiles or not, and what error document to send to the client. - checks for allow_dotfiles if check_path() found a dotfile (or dotdir) in the URL path. - new: checks for allow_doubleslash if check_path() found // in the URL path. doc/config.txt: doc/syntax.txt: - document the new AllowDoubleSlash keyword. src/config.c: - process the new AllowDoubleSlash keyword. src/mathopd.h: - add the allow_doubleslash flag to the control block structure. If the patches should not make it to the list, they are also available at http://www.ringlet.net/~roam/patches/mathopd/ G'luck, Peter -- Peter Pentchev [EMAIL PROTECTED][EMAIL PROTECTED][EMAIL PROTECTED] PGP key:http://people.FreeBSD.org/~roam/roam.key.asc Key fingerprint FDBA FD79 C26F 3C51 C95E DF9E ED18 B68D 1619 4553 This inert sentence is my body, but my soul is alive, dancing in the sparks of your brain. Index: doc/config.txt === --- doc/config.txt (revision 333) +++ doc/config.txt (revision 456) @@ -91,6 +91,15 @@ a bit, although constructions like '/./', '/../', etc. are still disallowed. +Keyword: AllowDoubleSlash +Where: Control +Type:Flag +Default: Off +Desc:Normally, URL's with two or more slashes in the path portion, + like http://www.example.com/mathopd//doc/, will be rejected. + If the AllowDoubleSlash flag is set, this restriction is removed, + and Mathopd will silently ignore the second and further slashes. + Keyword: AnyHost Where: Virtual Desc:If a virtual server has this keyword, it will match on any Host: Index: doc/syntax.txt === --- doc/syntax.txt (revision 333) +++ doc/syntax.txt (revision 456) @@ -84,6 +84,7 @@ "ScriptUser" string "RunScriptsAsOwner" string "AllowDotfiles" flag + "AllowDoubleSlash" flag "UserDirectory" flag "PutEnv" string-block "ExtraHeaders" string-block Index: src/mathopd.h === --- src/mathopd.h (revision 333) +++ src/mathopd.h (revision 456) @@ -171,6 +171,7 @@ uid_t script_uid; gid_t script_gid; int allow_dotfiles; + int allow_doubleslash; int user_directory; struct simple_list *putenvs; struct simple_list *extra_headers; Index: src/config.c === --- src/config.c(revision 333) +++ src/config.c(revision 456) @@ -84,6 +84,7 @@ static const char c_alias[] = "Alias"; static const char c_allow[] = "Allow"; static const char c_allow_dotfiles[] = "AllowDotfiles"; +static const char c_allow_doubleslash[] = "AllowDoubleSlash"; static const char c_any_host[] =
Re: Patch: allow /./ and /../ in URL paths
On Thu, Oct 12, 2006 at 12:57:14PM +0300, Peter Pentchev wrote: > Hi, > > First of all, thanks for writing and maintaing a great, lightweight, > simple webserver! [snip] > > If the patches should not come through for any reason, they are also > available at http://www.ringlet.net/~roam/patches/mathopd/ > > So... what do you think? Apologies if anybody has received this twice - I think Michiel might have :) But there seemed to be something wrong with the list when I first sent it a couple of days ago, and now he asked if I could send it again to test the list. Apparently, it's working fine now. G'luck, Peter -- Peter Pentchev [EMAIL PROTECTED][EMAIL PROTECTED][EMAIL PROTECTED] PGP key:http://people.FreeBSD.org/~roam/roam.key.asc Key fingerprint FDBA FD79 C26F 3C51 C95E DF9E ED18 B68D 1619 4553 What would this sentence be like if it weren't self-referential? pgp28wCKbUAAX.pgp Description: PGP signature
Patch: allow /./ and /../ in URL paths
Hi,
First of all, thanks for writing and maintaing a great, lightweight,
simple webserver!
My attention was recently drawn to Mathopd when a friend decided to use
it as a lightweight replacement of Apache for one of the Bulgarian
mirrors of Debian Linux. It's been doing great so far, with a single
excepion - as y'all might have already guessed, it's /../ components in
the URL paths. It turns out that some configurations for fetching
Debian distribution files do indeed depend on traversing directories
back up - it happens only very rarely, but still, it happens, and our
new mirror webserver does not quite allow it.
So... what do you think about the attached patches that let Mathopd
allow /./ path components (ignoring them) and /../ path components
(actually rewriting the URL path as it comes across them) ? The two
patches, against 1.5p5 and 1.6b6, are actually identical except for line
numbers. They introduce a new call sequence - check_path() is actually
invoked twice now, and with a new parameter. The first invocation comes
early on, before faketoreal(), to sanitize the path (remove /./
components and remove /../ components along with the last directory
component), so that faketoreal() operates on a correct path, one not
containing /./'s and /../'s. Then, after faketoreal(), when we have
found the actual location within the server configuration (r->c has been
initialized), check_path() is invoked again, mainly to check whether the
path contains any disallowed dotfile references.
With a bit more work, the support for /../ components could also be made
configurable - the first invocation of check_path() could return a
specific value to indicate that the path was valid, yet it contained
/../'s, and then, after faketoreal(), process_path() could check for a
configuration variable similar to AllowDotfiles. I could do that, too,
if you think it's necessary. Still, IMHO /../'s - *valid* /../'s - are
not all that big a security risk if they are processed properly, and
unless I have messed up the path rewriting code a whole lot, this ought
to be proper processing :)
If the patches should not come through for any reason, they are also
available at http://www.ringlet.net/~roam/patches/mathopd/
So... what do you think?
G'luck,
Peter
--
Peter Pentchev [EMAIL PROTECTED][EMAIL PROTECTED][EMAIL PROTECTED]
PGP key:http://people.FreeBSD.org/~roam/roam.key.asc
Key fingerprint FDBA FD79 C26F 3C51 C95E DF9E ED18 B68D 1619 4553
This sentence claims to be an Epimenides paradox, but it is lying.
Index: src/request.c
===
--- src/request.c (revision 620)
+++ src/request.c (revision 625)
@@ -403,9 +403,9 @@
return -1;
}
-static int check_path(struct request *r)
+static int check_path(struct request *r, int processed)
{
- char *p;
+ char *p, *q, *slash;
char c;
enum {
s_normal,
@@ -414,8 +414,10 @@
s_slashdotdot,
s_forbidden
} s;
+ char newpath[PATHLEN];
p = r->path;
+ q = newpath;
s = s_normal;
do {
c = *p++;
@@ -423,35 +425,65 @@
case s_normal:
if (c == '/')
s = s_slash;
+ *q++ = c;
break;
case s_slash:
- if (c == '/')
+ if (c == '/') {
s = s_forbidden;
- else if (c == '.')
- s = r->c->allow_dotfiles ? s_slashdot :
s_forbidden;
- else
+ } else if (c == '.') {
+ s = s_slashdot;
+ } else {
s = s_normal;
+ *q++ = c;
+ }
break;
case s_slashdot:
- if (c == 0 || c == '/')
+ if (c == 0) {
s = s_forbidden;
- else if (c == '.')
+ } else if (c == '/') {
+ s = s_slash;
+ } else if (c == '.') {
s = s_slashdotdot;
- else
+ } else if (processed && !r->c->allow_dotfiles) {
+ s = s_forbidden;
+ } else {
+ *q++ = '.';
+ *q++ = c;
s = s_normal;
+ }
break;
case s_slashdotdot:
- if (c == 0 || c == '/')
+ if (c == 0) {
s = s_forbidden;
- else
+ } else if (c
