[Secure-testing-team] Bug#703294: davical: fopen mess in caldav.php

[Christoph Anton Mitterer]

Package: davical
Version: 1.1.1-1
Severity: grave
Tags: security upstream


Hi.

Tagging this as security relevant and important... as someone might
even be able to find a tricky way to use this to read arbitrary
files (well at least I can't think of one).
Again... marking this as grave to notify the security team... if you agree
that this is not critical (due to (a) - see below) than please lower the severity
to important - Thanks.)

In caldav.php there is the following code in the very beginning:
if ( isset($_SERVER['PATH_INFO']) && preg_match( '{^(/favicon.ico|davical.css|(images|js|css)/.+)$}', $_SERVER['PATH_INFO'], $matches ) ) {
  $filename = $_SERVER['DOCUMENT_ROOT'] . preg_replace('{(\.\.|\\\\)}', '', $matches[1]);
  $fh = @fopen($matches[1],'r');
  if ( ! $fh ) {
    @header( sprintf("HTTP/1.1 %d %s", 404, 'Not found') );
  }
  else {
    fpassthru($fh);
  }
  @ob_flush(); exit(0);
}

It seems the intention is to pass through some special files even though I can't think
why... this is CalDAV and no one should need icons, css and images.


Nevertheless... looking at the code this sounds quite problematic,... and it never
could have worked anyway.
Let's go through it:

>if ( isset($_SERVER['PATH_INFO']) && preg_match( '{^(/favicon.ico|davical.css|(images|js|css)/.+)$}', $_SERVER['PATH_INFO'], $matches ) ) {
The regexp is already problematic
a) "/" is inside the "(...)" so only a "...caldav.php/favicon.ico" could have ever been matched
   (this is why I say it could have never worked so far).
   PATH_INFO is AFAIK _always_ prefixed by "/" so the other patterns i.e. 
   "davical.css|(images|js|css)/.+" never matched.
b) The pattern contains unescaped "."... so this means "any character" and e.g.
   also a "faviconXico" would be matched.

>$filename = $_SERVER['DOCUMENT_ROOT'] . preg_replace('{(\.\.|\\\\)}', '', $matches[1]);
I strongly doubt this replacement will do away with any evil tricks of escaping
an attacker would do to manipulate the paths.
c) Actually I think it even makes attacks possible.... (if there wouldn't be the above matching bug (a).
   There's only one pass of preg_replace... and not so many until nothing changes.
   Wouldn't (a) be a bug.. then one could probably send an url like
   "/images/.\\./.\\./.\\./etc/shadow)
   The  pattern wouldn't see th \.\. but it would see and remove the \\\\
   thereby creating the filename "whatever/images/../../../etc/shadow".
   A good way to start an attack.

> $fh = @fopen($matches[1],'r');
d) What you probably want is opening $filename.


e) Last but not least.
The whole think will break anyway, when the user has set PHP's
open_basedir...
So if the whole code is really needed (in a correct for)...
then
- the code should handle it when open_basedir is set, and e.g.
log it or what ever.
- the davical documentation need to be updaded, to inform the users
  that DOCUMENT_ROOT must be added to open_basedir.



My suggestion... if you don't really need this pass throughing...
Remove this section alltogether... anything is always likely to be security prone.

These kind of redirects to file content should anyway be done by the webserver.
Especially when "PATH_INFO URLs" are used... it's quite problematic to serve
file content via them... this is not intented... and likely to cause security trouble...
even if it's on the webserver end.