This should fix the image upload bug for good. Uses basename() and upload verificatio...

Also admins can no longer upload active content files.

Tricking the upload by making the directory "evil.ph" and the filename "p" does not work because trailing slashes are appended to directory names.

diff --git a/include/admin/images.inc.php b/include/admin/images.inc.php
index 5903746b13ff1be04579c9b5b7c83def6751e9e7..36523cded01bde09b98edc680efc8fdaf38102b8 100644 (file)
--- a/include/admin/images.inc.php
+++ b/include/admin/images.inc.php
@@ -60,7 +60,7 @@ switch ($serendipity['GET']['adminAction']) {
             return;
         }
 
-        if ($serendipity['serendipityUserlevel'] < USERLEVEL_ADMIN && serendipity_isActiveFile($serendipity['GET']['newname'])) {
+        if (serendipity_isActiveFile(basename($serendipity['GET']['newname']))) {
             printf(ERROR_FILE_FORBIDDEN, $serendipity['GET']['newname']);
             return;
         }
@@ -119,23 +119,24 @@ switch ($serendipity['GET']['adminAction']) {
     if ($serendipity['POST']['imageurl'] != '' && $serendipity['POST']['imageurl'] != 'http://') {
         if (!empty($serendipity['POST']['target_filename'][2])) {
             // Faked hidden form 2 when submitting with JavaScript
-            $tfile   = serendipityNormalizeFilename($serendipity['POST']['target_filename'][2]);
+            $tfile   = $serendipity['POST']['target_filename'][2];
             $tindex  = 2;
         } elseif (!empty($serendipity['POST']['target_filename'][1])) {
             // Fallback key when not using JavaScript
-            $tfile   = serendipityNormalizeFilename($serendipity['POST']['target_filename'][1]);
+            $tfile   = $serendipity['POST']['target_filename'][1];
             $tindex  = 1;
         } else {
-            $tfile   = serendipityNormalizeFilename(basename($serendipity['POST']['imageurl']));
+            $tfile   = $serendipity['POST']['imageurl'];
             $tindex  = 1;
         }
 
-        if ($serendipity['serendipityUserlevel'] < USERLEVEL_ADMIN && serendipity_isActiveFile($tfile)) {
+        $tfile = serendipity_uploadSecure(basename($tfile));
+
+        if (serendipity_isActiveFile($tfile)) {
             printf(ERROR_FILE_FORBIDDEN, $tfile);
             break;
         }
 
-        $tfile = serendipityNormalizeFilename(serendipity_uploadSecure($tfile));
         $serendipity['POST']['target_directory'][$tindex] = serendipity_uploadSecure($serendipity['POST']['target_directory'][$tindex], true, true);
         $target = $serendipity['serendipityPath'] . $serendipity['uploadPath'] . $serendipity['POST']['target_directory'][$tindex] . $tfile;
 
@@ -185,21 +186,22 @@ switch ($serendipity['GET']['adminAction']) {
             $uploadfile = &$_FILES['serendipity']['name']['userfile'][$idx];
             $uploadtmp  = &$_FILES['serendipity']['tmp_name']['userfile'][$idx];
             if (!empty($target_filename)) {
-                $tfile   = serendipityNormalizeFilename($target_filename);
+                $tfile   = $target_filename;
             } elseif (!empty($uploadfile)) {
-                $tfile   = serendipityNormalizeFilename($uploadfile);
+                $tfile   = $uploadfile;
             } else {
                 // skip empty array
                 continue;
             }
             
-           if (preg_match('@^\.@', $tfile) || ($serendipity['serendipityUserlevel'] < USERLEVEL_ADMIN && (preg_match('@\.(php[34]?|[ps]html?)$@i', $tfile)))) {
+            $tfile = serendipity_uploadSecure(basename($tfile));
+
+               if (serendipity_isActiveFile($tfile)) {
                 printf(ERROR_FILE_FORBIDDEN, $tfile);
                 echo '<br />';
                 continue;
             }
     
-            $tfile = serendipityNormalizeFilename(serendipity_uploadSecure($tfile));
             $serendipity['POST']['target_directory'][$idx] = serendipity_uploadSecure($serendipity['POST']['target_directory'][$idx], true, true);
             $target = $serendipity['serendipityPath'] . $serendipity['uploadPath'] . $serendipity['POST']['target_directory'][$idx] . $tfile;
     
@@ -208,7 +210,7 @@ switch ($serendipity['GET']['adminAction']) {
                 echo '<br />';
             } else {
                 // Accept file
-                if (move_uploaded_file($uploadtmp, $target)) {
+                if (is_uploaded_file($uploadtmp) && move_uploaded_file($uploadtmp, $target)) {
                     printf(FILE_UPLOADED . '<br />', $uploadfile, $target);
                     @umask(0000);
                     @chmod($target, 0664);

diff --git a/include/functions_images.inc.php b/include/functions_images.inc.php
index fdb75065bb3f264c6e42d6b561d8868f40c974a2..d8ca99a58eb2840aef3649b22183fedc5b25704f 100644 (file)
--- a/include/functions_images.inc.php
+++ b/include/functions_images.inc.php
@@ -2,14 +2,6 @@
 # Copyright (c) 2003-2005, Jannis Hermanns (on behalf the Serendipity Developer Team)
 # All rights reserved.  See LICENSE file for licensing details
 
-/**
-* Normalize a filename
-**/
-function serendipityNormalizeFilename($in) {
-    $out = preg_replace('![^a-zA-Z0-9\._/-]!', '', $in);
-    return $out;
-}
-
 function serendipity_isActiveFile($file) {
     if (preg_match('@^\.@', $file)) {
         return true;
@@ -1252,7 +1244,7 @@ function serendipity_uploadSecure($var, $strip_paths = true, $append_slash = fal
     $var = preg_replace('@^(/+)@', '', $var);
     
     if ($append_slash) {
-        if (substr($var, -1, 1) != '/') {
+        if (!empty($var) && substr($var, -1, 1) != '/') {
             $var .= '/';
         }
     }