[TikiWiki-commits] [Git][tikiwiki/tiki][master] [FIX] Enhance attachment filename handling to prevent path traversal vulnerabilities

Espoir Baraka pushed to branch master at Tiki Wiki CMS Groupware / Tiki


Commits:
a50752aa by Espoir Baraka at 2026-08-18T21:28:54+02:00
[FIX] Enhance attachment filename handling to prevent path traversal vulnerabilities
---
* [FIX] Enhance attachment filename handling to prevent path traversal vulnerabilities

- Implemented `sanitizeAttachmentFileName` method to validate and normalize attachment filenames from MediaWiki dumps, rejecting any that contain directory components.
- Added `resolveAttachmentDestPath` method to ensure the destination path for attachments remains within the designated directory.
- Updated `downloadAttachments` method to utilize the new filename sanitization and path resolution, logging errors for invalid filenames and paths.
- Introduced tests to verify the filename sanitization and ensure path traversal attempts are correctly rejected.

This change improves security by preventing potential directory traversal attacks during file imports.

(cherry picked from commit 4abee1fb6eb62b30a15a280096c12123f6ff6e6b)

* [FIX] Wiki: Improve permission checks and feedback for 301 redirects

(cherry picked from commit a31756efdce6a2c6da87588a3e271a294bda0639)

See merge request tikiwiki/tiki!10960

- - - - -


4 changed files:

- lib/Importer/WikiMediawiki.php
- lib/core/Services/Wiki/Controller.php
- lib/test/Importer/WikiMediawikiTest.php
- + lib/test/Importer/fixtures/mediawiki_path_traversal_upload.xml


Changes:

=====================================
lib/Importer/WikiMediawiki.php
=====================================
@@ -294,6 +294,60 @@ class WikiMediawiki extends Wiki
         return $parsedData;
     }
 
+    /**
+     * Validate and normalize an attachment filename from a MediaWiki dump.
+     *
+     * Filenames are attacker-controlled via the uploaded XML. Reject any value
+     * that contains a directory component or resolves to something other than a
+     * plain basename, so path traversal cannot escape $attachmentsDestDir.
+     *
+     * @param string $fileName Raw <filename> value from the dump
+     * @return string|null Safe basename, or null if unsafe / empty
+     */
+    public function sanitizeAttachmentFileName($fileName)
+    {
+        $fileName = trim((string) $fileName);
+
+        if ($fileName === '' || str_contains($fileName, "\0")) {
+            return null;
+        }
+
+        // Reject absolute paths and any directory separator (Unix or Windows).
+        if (preg_match('#[/\\\\]#', $fileName)) {
+            return null;
+        }
+
+        $baseName = basename($fileName);
+        if ($baseName === '' || $baseName === '.' || $baseName === '..' || $baseName !== $fileName) {
+            return null;
+        }
+
+        return $baseName;
+    }
+
+    /**
+     * Build the absolute destination path for an attachment and ensure it stays
+     * inside $attachmentsDestDir (defense in depth after filename sanitization).
+     *
+     * @param string $safeFileName Already-sanitized basename
+     * @return string|null Absolute path under the destination directory, or null
+     */
+    public function resolveAttachmentDestPath($safeFileName)
+    {
+        $destDir = realpath($this->attachmentsDestDir);
+        if ($destDir === false || ! is_dir($destDir)) {
+            return null;
+        }
+
+        $destPath = $destDir . DIRECTORY_SEPARATOR . $safeFileName;
+        $parent = realpath(dirname($destPath));
+        if ($parent === false || $parent !== $destDir) {
+            return null;
+        }
+
+        return $destPath;
+    }
+
     /**
      * Searches for the last version of each attachments in the XML file
      * and try to download it to the img/wiki_up/ directory
@@ -329,15 +383,28 @@ class WikiMediawiki extends Wiki
                 $i = $attachments->length - 1;
                 $lastVersion = $attachments->item($i);
 
-                $fileName = basename($lastVersion->getElementsByTagName('filename')->item(0)->nodeValue);
+                $rawFileName = $lastVersion->getElementsByTagName('filename')->item(0)->nodeValue;
                 $fileUrl = $lastVersion->getElementsByTagName('src')->item(0)->nodeValue;
 
-                if ($fileName === '' || $fileName === '.' || $fileName === '..') {
-                    $this->saveAndDisplayLog(tr('File not imported: invalid attachment filename.') . "\n", true);
+                $fileName = $this->sanitizeAttachmentFileName($rawFileName);
+                if ($fileName === null) {
+                    $this->saveAndDisplayLog(
+                        tr('File %0 is not being imported because the filename must not contain directory separators or `..`.', $rawFileName) . "\n",
+                        true
+                    );
+                    continue;
+                }
+
+                $destPath = $this->resolveAttachmentDestPath($fileName);
+                if ($destPath === null) {
+                    $this->saveAndDisplayLog(
+                        tr('File %0 is not being imported because the destination path is invalid.', $fileName) . "\n",
+                        true
+                    );
                     continue;
                 }
 
-                if (file_exists($this->attachmentsDestDir . $fileName)) {
+                if (file_exists($destPath)) {
                     $this->saveAndDisplayLog(
                         tr(
                             'File %0 is not being imported because there is already a file with the same name in the destination directory (%1)',
@@ -364,8 +431,13 @@ class WikiMediawiki extends Wiki
 
                 $attachmentContent = $this->fetchAttachmentContents($fileUrl);
                 if ($attachmentContent !== false) {
-                    $newFile = fopen($this->attachmentsDestDir . $fileName, 'w');
+                    $newFile = fopen($destPath, 'w');
+                    if ($newFile === false) {
+                        $this->saveAndDisplayLog(tr('Unable to write file %0.', $fileName) . "\n", true);
+                        continue;
+                    }
                     fwrite($newFile, $attachmentContent);
+                    fclose($newFile);
                     $this->saveAndDisplayLog(tr('File %0 successfully imported!', $fileName) . "\n");
                 } else {
                     $this->saveAndDisplayLog(tr('Unable to download file %0. File not found.', $fileName) . "\n", true);


=====================================
lib/core/Services/Wiki/Controller.php
=====================================
@@ -485,8 +485,23 @@ class Services_Wiki_Controller
                         ];
                         Feedback::warning($feedback);
                     } else {
-                        $appendString = "";
-                        foreach ($util->items as $page) {
+                        $tikilib = TikiLib::lib('tiki');
+                        $destinationExists = $tikilib->page_exists($destinationPage);
+                        // Creating or updating the redirect destination requires edit permission
+                        if ($destinationExists) {
+                            $canEditDestination = Perms::get('wiki page', $destinationPage)->edit;
+                        } else {
+                            $canEditDestination = Perms::get()->edit;
+                        }
+                        if (! $canEditDestination) {
+                            $msg = tr('You do not have permission to edit the redirect destination page. 301 redirect not created.');
+                            $feedback = [
+                                'tpl' => 'action',
+                                'mes' => $msg,
+                            ];
+                            Feedback::error($feedback);
+                        } else {
+                            $appendString = "";
                             // Append on the destination page's content the following string,
                             // where $page is the name of the deleted page:
                             // "\r\n~tc~(alias($page))~/tc~"
@@ -494,42 +509,44 @@ class Services_Wiki_Controller
                             if (count($util->items) > 1) {
                                 $comment = tr('Semantic aliases (301 Redirects) to this page were created when other pages were deleted');
                             } else {
-                                $comment = tr('A semantic alias (301 Redirect) to this page was created when page %0 was deleted', $page);
+                                $comment = tr('A semantic alias (301 Redirect) to this page was created when page %0 was deleted', $util->items[0]);
                             }
-                            $appendString .= "\r\n~tc~ (alias($page)) ~/tc~";
-                        }
-                        if (TikiLib::lib('tiki')->page_exists($destinationPage)) {
-                            // Get wiki page content
-                            $infoDestinationPage = TikiLib::lib('tiki')->get_page_info($destinationPage);
-                            $page_data = $infoDestinationPage['data'];
-                            $page_data .= $appendString;
-                            TikiLib::lib('tiki')->update_page($destinationPage, $page_data, $comment, $user, TikiLib::lib('tiki')->get_ip_address());
-                            if (count($util->items) > 1) {
-                                $msg = tr('301 Redirects to the following page were created:');
-                            } else {
-                                $msg = tr('A 301 Redirect to the following page was created:');
+                            foreach ($util->items as $page) {
+                                $appendString .= "\r\n~tc~ (alias($page)) ~/tc~";
                             }
-                        } else {
-                            if (count($util->items) > 1) {
-                                $page_data = tr("THIS PAGE WAS CREATED AUTOMATICALLY when other pages were removed. Please edit and write the definitive contents.");
-                            } else {
-                                $page_data = tr("THIS PAGE WAS CREATED AUTOMATICALLY when another page was removed. Please edit and write the definitive contents.");
-                            }
-                            $page_data .= $appendString;
-                            // Create a new page
-                            TikiLib::lib('tiki')->create_page($destinationPage, 0, $page_data, TikiLib::lib('tiki')->now, $comment, $user, TikiLib::lib('tiki')->get_ip_address());
-                            if (count($util->items) > 1) {
-                                $msg = tr('The following page and 301 Redirects to it were created:');
+                            if ($destinationExists) {
+                                // Get wiki page content
+                                $infoDestinationPage = $tikilib->get_page_info($destinationPage);
+                                $page_data = $infoDestinationPage['data'];
+                                $page_data .= $appendString;
+                                $tikilib->update_page($destinationPage, $page_data, $comment, $user, $tikilib->get_ip_address());
+                                if (count($util->items) > 1) {
+                                    $msg = tr('301 Redirects to the following page were created:');
+                                } else {
+                                    $msg = tr('A 301 Redirect to the following page was created:');
+                                }
                             } else {
-                                $msg = tr('The following page and a 301 Redirect to it were created:');
+                                if (count($util->items) > 1) {
+                                    $page_data = tr("THIS PAGE WAS CREATED AUTOMATICALLY when other pages were removed. Please edit and write the definitive contents.");
+                                } else {
+                                    $page_data = tr("THIS PAGE WAS CREATED AUTOMATICALLY when another page was removed. Please edit and write the definitive contents.");
+                                }
+                                $page_data .= $appendString;
+                                // Create a new page
+                                $tikilib->create_page($destinationPage, 0, $page_data, $tikilib->now, $comment, $user, $tikilib->get_ip_address());
+                                if (count($util->items) > 1) {
+                                    $msg = tr('The following page and 301 Redirects to it were created:');
+                                } else {
+                                    $msg = tr('The following page and a 301 Redirect to it were created:');
+                                }
                             }
+                            $feedback = [
+                                'tpl' => 'link',
+                                'mes' => $msg,
+                                'items' => is_array($destinationPage) ? $destinationPage : [$destinationPage],
+                            ];
+                            Feedback::note($feedback);
                         }
-                        $feedback = [
-                            'tpl' => 'link',
-                            'mes' => $msg,
-                            'items' => is_array($destinationPage) ? $destinationPage : [$destinationPage],
-                        ];
-                        Feedback::note($feedback);
                     }
                 }
             }


=====================================
lib/test/Importer/WikiMediawikiTest.php
=====================================
@@ -314,6 +314,50 @@ XML;
         );
     }
 
+    public function testSanitizeAttachmentFileNameRejectsPathTraversal(): void
+    {
+        $this->assertSame('photo.jpg', $this->obj->sanitizeAttachmentFileName('photo.jpg'));
+        $this->assertSame('photo.jpg', $this->obj->sanitizeAttachmentFileName('  photo.jpg  '));
+
+        $this->assertNull($this->obj->sanitizeAttachmentFileName('../../outside.jpg'));
+        $this->assertNull($this->obj->sanitizeAttachmentFileName('..\\..\\outside.jpg'));
+        $this->assertNull($this->obj->sanitizeAttachmentFileName('/etc/passwd'));
+        $this->assertNull($this->obj->sanitizeAttachmentFileName('subdir/photo.jpg'));
+        $this->assertNull($this->obj->sanitizeAttachmentFileName('..'));
+        $this->assertNull($this->obj->sanitizeAttachmentFileName('.'));
+        $this->assertNull($this->obj->sanitizeAttachmentFileName(''));
+        $this->assertNull($this->obj->sanitizeAttachmentFileName("evil\0.jpg"));
+    }
+
+    public function testDownloadAttachmentsShouldRejectPathTraversalFilenames(): void
+    {
+        ob_start();
+
+        $this->obj->attachmentsDestDir = __DIR__ . '/fixtures/';
+        $outsidePath = __DIR__ . '/outside.jpg';
+        $outsidePath2 = __DIR__ . '/outside2.jpg';
+
+        if (file_exists($outsidePath)) {
+            unlink($outsidePath);
+        }
+        if (file_exists($outsidePath2)) {
+            unlink($outsidePath2);
+        }
+
+        $this->obj->dom = new DOMDocument();
+        $this->obj->dom->load(__DIR__ . '/fixtures/mediawiki_path_traversal_upload.xml');
+        $this->obj->downloadAttachments();
+
+        $this->assertFileDoesNotExist($outsidePath);
+        $this->assertFileDoesNotExist($outsidePath2);
+        $this->assertFileDoesNotExist($this->obj->attachmentsDestDir . 'outside.jpg');
+        $this->assertFileDoesNotExist($this->obj->attachmentsDestDir . 'outside2.jpg');
+
+        $output = ob_get_clean();
+        $this->assertStringContainsString('filename must not contain directory separators or `..`', $output);
+        $this->assertStringNotContainsString('successfully imported', $output);
+    }
+
     public function testExtractInfo(): void
     {
         ob_start();


=====================================
lib/test/Importer/fixtures/mediawiki_path_traversal_upload.xml
=====================================
@@ -0,0 +1,28 @@
+<mediawiki>
+  <page>
+    <upload>
+      <timestamp>2009-07-22T21:43:45Z</timestamp>
+      <contributor>
+        <username>Admin</username>
+        <id>1</id>
+      </contributor>
+      <comment>invalid path</comment>
+      <filename>../outside.jpg</filename>
+      <src>fixtures/sourceTest.jpg</src>
+      <size>100</size>
+    </upload>
+  </page>
+  <page>
+    <upload>
+      <timestamp>2009-07-22T21:50:42Z</timestamp>
+      <contributor>
+        <username>Admin</username>
+        <id>1</id>
+      </contributor>
+      <comment />
+      <filename>..\outside2.jpg</filename>
+      <src>fixtures/sourceTest.jpg</src>
+      <size>100</size>
+    </upload>
+  </page>
+</mediawiki>



View it on GitLab: https://gitlab.com/tikiwiki/tiki/-/commit/a50752aa36d14f02e8fae95178ed4942708ef34b

-- 
View it on GitLab: https://gitlab.com/tikiwiki/tiki/-/commit/a50752aa36d14f02e8fae95178ed4942708ef34b
You're receiving this email because of your account on gitlab.com. Manage all notifications: https://gitlab.com/-/profile/notifications | Help: https://gitlab.com/help

_______________________________________________
TikiWiki-cvs mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/tikiwiki-cvs