CVE-2021-44665: Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal') (CWE-22)
value: A Directory Traversal vulnerability exists in the Xerte Project Xerte through 3.10.3 when downloading a project file via download.php.
CWE description: The product uses external input to construct a pathname that is intended to identify a file or directory that is located underneath a restricted parent directory, but the product does not properly neutralize special elements within the pathname that can cause the pathname to resolve to a location that is outside of the restricted directory.
CVSS v3 base score: 6.5
Published: 2022-02-24T21:15Z
Repository: https://github.com/thexerteproject/xerteonlinetoolkits
Language: PHP
Files changed: getfile.php
Commit date: 2021-11-18 16:41:49 +0100
Commit message: Fix vulnarebility

Diff: diff --git a/getfile.php b/getfile.php
index 13fbe55ee..7d02a08d2 100644
--- a/getfile.php
+++ b/getfile.php
@@ -21,40 +21,47 @@
 require_once(dirname(__FILE__) . "/config.php");
 require $xerte_toolkits_site->php_library_path . "user_library.php";
 require $xerte_toolkits_site->php_library_path . "template_library.php";
 require $xerte_toolkits_site->php_library_path . "template_status.php";
 
-// be slightly paranoid over the path the user is requesting to download.
+// be VERY paranoid over the path the user is requesting to download.
+// Even if the file starts with a correct pattern (old implementation) the user could travers path
+// like 542-tom-Notingham/../database.php or like 542-tom-Notingham/../../../../etc/passwd
 $unsafe_file_path = $_GET['file'];
-if(!preg_match('/^([0-9]+)-([a-z0-9]+)-/', $unsafe_file_path, $matches)) {
-    die("path must start with a number, and then a username - e.g. 20-foobar-");
-}
+$full_unsafe_file_path = $xerte_toolkits_site->root_file_path . $xerte_toolkits_site->users_file_area_short . $unsafe_file_path;
+// This gets the canonical file name, so in case of 542-tom-Notingham/../../../../etc/passwd -> /etc/passwd
+$realpath = realpath($full_unsafe_file_path);
+// Check that is start with root_path/USER-FILES
+if ($realpath !== false && $realpath === $full_unsafe_file_path) {
+    if (!preg_match('/^([0-9]+)-([a-z0-9]+)-/', $unsafe_file_path, $matches)) {
+        die("path must start with a number, and then a username - e.g. 20-foobar-");
+    }
 
-$template_id = $matches[1];
-$username = $matches[2];
+    $template_id = $matches[1];
+    $username = $matches[2];
 
-$has_perms = is_user_admin() || has_rights_to_this_template($template_id,$_SESSION['toolkits_logon_id']);
+    $has_perms = is_user_admin() || has_rights_to_this_template($template_id, $_SESSION['toolkits_logon_id']);
 
-if($has_perms) { 
-    if(is_user_an_editor($template_id,$_SESSION['toolkits_logon_id'])){
-        if($username == $_SESSION['toolkits_logon_username']) { 
-            // they're logged in, and hopefully have access to the media contents.
-            $file = dirname(__FILE__) . '/USER-FILES/' . $unsafe_file_path;
-            if(!is_file($file)) { 
-                die("Fail: file not found on disk"); 
-            }
-            $filename = addslashes(basename($file));
+    if ($has_perms) {
+        if (is_user_an_editor($template_id, $_SESSION['toolkits_logon_id'])) {
+            if ($username == $_SESSION['toolkits_logon_username']) {
+                // they're logged in, and hopefully have access to the media contents.
+                $file = dirname(__FILE__) . '/USER-FILES/' . $unsafe_file_path;
+                if (!is_file($file)) {
+                    die("Fail: file not found on disk");
+                }
+                $filename = addslashes(basename($file));
 
-            header("Cache-Control: public");
-            header("Content-Length: " . filesize($file));
-            header("Content-Description: File Transfer");
-            header("Content-Type: application/force-download"); 
-            header("Content-Disposition: attachment; filename=\"$filename\"");
-            header("Content-Transfer-Encoding: binary");
-            flush();
-            readfile($file);
-            exit(0);
+                header("Cache-Control: public");
+                header("Content-Length: " . filesize($file));
+                header("Content-Description: File Transfer");
+                header("Content-Type: application/force-download");
+                header("Content-Disposition: attachment; filename=\"$filename\"");
+                header("Content-Transfer-Encoding: binary");
+                flush();
+                readfile($file);
+                exit(0);
+            }
         }
     }
 }
-
 echo "You do not appear to have permission to view this resource.";

Vulnerable code: // be slightly paranoid over the path the user is requesting to download.
if(!preg_match('/^([0-9]+)-([a-z0-9]+)-/', $unsafe_file_path, $matches)) {
    die("path must start with a number, and then a username - e.g. 20-foobar-");
}
$template_id = $matches[1];
$username = $matches[2];
$has_perms = is_user_admin() || has_rights_to_this_template($template_id,$_SESSION['toolkits_logon_id']);
if($has_perms) { 
    if(is_user_an_editor($template_id,$_SESSION['toolkits_logon_id'])){
        if($username == $_SESSION['toolkits_logon_username']) { 
            // they're logged in, and hopefully have access to the media contents.
            $file = dirname(__FILE__) . '/USER-FILES/' . $unsafe_file_path;
            if(!is_file($file)) { 
                die("Fail: file not found on disk"); 
            }
            $filename = addslashes(basename($file));
            header("Cache-Control: public");
            header("Content-Length: " . filesize($file));
            header("Content-Description: File Transfer");
            header("Content-Type: application/force-download"); 
            header("Content-Disposition: attachment; filename=\"$filename\"");
            header("Content-Transfer-Encoding: binary");
            flush();
            readfile($file);
            exit(0);

Fixed code: // be VERY paranoid over the path the user is requesting to download.
// Even if the file starts with a correct pattern (old implementation) the user could travers path
// like 542-tom-Notingham/../database.php or like 542-tom-Notingham/../../../../etc/passwd
$full_unsafe_file_path = $xerte_toolkits_site->root_file_path . $xerte_toolkits_site->users_file_area_short . $unsafe_file_path;
// This gets the canonical file name, so in case of 542-tom-Notingham/../../../../etc/passwd -> /etc/passwd
$realpath = realpath($full_unsafe_file_path);
// Check that is start with root_path/USER-FILES
if ($realpath !== false && $realpath === $full_unsafe_file_path) {
    if (!preg_match('/^([0-9]+)-([a-z0-9]+)-/', $unsafe_file_path, $matches)) {
        die("path must start with a number, and then a username - e.g. 20-foobar-");
    }
    $template_id = $matches[1];
    $username = $matches[2];
    $has_perms = is_user_admin() || has_rights_to_this_template($template_id, $_SESSION['toolkits_logon_id']);
    if ($has_perms) {
        if (is_user_an_editor($template_id, $_SESSION['toolkits_logon_id'])) {
            if ($username == $_SESSION['toolkits_logon_username']) {
                // they're logged in, and hopefully have access to the media contents.
                $file = dirname(__FILE__) . '/USER-FILES/' . $unsafe_file_path;
                if (!is_file($file)) {
                    die("Fail: file not found on disk");
                }
                $filename = addslashes(basename($file));
                header("Cache-Control: public");
                header("Content-Length: " . filesize($file));
                header("Content-Description: File Transfer");
                header("Content-Type: application/force-download");
                header("Content-Disposition: attachment; filename=\"$filename\"");
                header("Content-Transfer-Encoding: binary");
                flush();
                readfile($file);
                exit(0);
            }