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 == '/') {
+ /* Find the last slash and go from there */
+ for (slash = q - 2; slash >= newpath; slash--)
+ if (*slash == '/')
+ break;
+ if (slash < newpath || *slash != '/') {
+ s = s_forbidden;
+ } else {
+ q = slash + 1;
+ s = s_slash;
+ }
+ } else if (processed && !r->c->allow_dotfiles) {
+ s = s_forbidden;
+ } else {
+ *q++ = '.';
+ *q++ = '.';
+ *q++ = c;
s = s_normal;
+ }
break;
case s_forbidden:
c = 0;
break;
}
} while (c);
- return s == s_forbidden ? -1 : 0;
+ if (s == s_forbidden)
+ return -1;
+ strcpy(r->path, newpath);
+ return 0;
}
static int makedir(struct request *r)
@@ -794,21 +826,35 @@
static void process_path(struct request *r)
{
+ int sanitized;
+
if (find_vs(r) == -1) {
if (debug)
log_d("find_vs failed (host=%s)", r->host ? r->host :
"[not set]");
r->status = 400;
return;
}
+ sanitized = check_path(r, 0);
+ if (sanitized == -1) {
+ if (debug)
+ log_d("check_path(sanitize) failed for %s", r->path);
+ r->path[0] = '\0';
+ }
if ((r->c = faketoreal(r->path, r->path_translated, r, 1, sizeof
r->path_translated)) == 0) {
if (debug)
log_d("faketoreal failed");
r->status = 500;
return;
}
- if (check_path(r) == -1) {
+ if (sanitized == -1) {
+ /* Finish the error processing now that we have r->c */
+ r->error_file = r->c->error_404_file;
+ r->status = 404;
+ return;
+ }
+ if (check_path(r, 1) == -1) {
if (debug)
- log_d("check_path failed for %s", r->path);
+ log_d("check_path(dotfiles) failed for %s", r->path);
r->error_file = r->c->error_404_file;
r->status = 404;
return;
-----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.5 (FreeBSD) iD8DBQBFLg4Q7Ri2jRYZRVMRAjSKAJ0dRp9WBIwT5TNt5jQC5OAWR3eAHgCfRd1l SKcFa3pZBxWwV19HlzsT82E= =crte -----END PGP SIGNATURE-----
Index: src/request.c
===================================================================
--- src/request.c (revision 631)
+++ src/request.c (revision 633)
@@ -394,9 +394,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,
@@ -405,8 +405,10 @@
s_slashdotdot,
s_forbidden
} s;
+ char newpath[PATHLEN];
p = r->path;
+ q = newpath;
s = s_normal;
do {
c = *p++;
@@ -414,35 +416,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 == '/') {
+ /* Find the last slash and go from there */
+ for (slash = q - 2; slash >= newpath; slash--)
+ if (*slash == '/')
+ break;
+ if (slash < newpath || *slash != '/') {
+ s = s_forbidden;
+ } else {
+ q = slash + 1;
+ s = s_slash;
+ }
+ } else if (processed && !r->c->allow_dotfiles) {
+ s = s_forbidden;
+ } else {
+ *q++ = '.';
+ *q++ = '.';
+ *q++ = c;
s = s_normal;
+ }
break;
case s_forbidden:
c = 0;
break;
}
} while (c);
- return s == s_forbidden ? -1 : 0;
+ if (s == s_forbidden)
+ return -1;
+ strcpy(r->path, newpath);
+ return 0;
}
static int makedir(struct request *r)
@@ -783,21 +815,35 @@
static void process_path(struct request *r)
{
+ int sanitized;
+
if (find_vs(r) == -1) {
if (debug)
log_d("find_vs failed (host=%s)", r->host ? r->host :
"[not set]");
r->status = 400;
return;
}
+ sanitized = check_path(r, 0);
+ if (sanitized == -1) {
+ if (debug)
+ log_d("check_path(sanitize) failed for %s", r->path);
+ r->path[0] = '\0';
+ }
if ((r->c = faketoreal(r->path, r->path_translated, r, 1, sizeof
r->path_translated)) == 0) {
if (debug)
log_d("faketoreal failed");
r->status = 500;
return;
}
- if (check_path(r) == -1) {
+ if (sanitized == -1) {
+ /* Finish the error processing now that we have r->c */
+ r->error_file = r->c->error_404_file;
+ r->status = 404;
+ return;
+ }
+ if (check_path(r, 1) == -1) {
if (debug)
- log_d("check_path failed for %s", r->path);
+ log_d("check_path(dotfiles) failed for %s", r->path);
r->error_file = r->c->error_404_file;
r->status = 404;
return;
-----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.5 (FreeBSD) iD8DBQBFLg387Ri2jRYZRVMRAqlVAJ9mKreHlKQdyYqvs/bhD8S8/Fs//QCdER8E LQQh6O37WMJ3dZmPBFCEiIo= =9FZa -----END PGP SIGNATURE-----
pgpmxngeZWsLa.pgp
Description: PGP signature
