From 5f6c2beb3b5c9fd4416422f7d336438b8fb7c47f Mon Sep 17 00:00:00 2001 From: El RIDO Date: Mon, 10 Nov 2025 12:00:29 +0100 Subject: [PATCH 01/14] Unit test on escaping the template directory --- tst/TemplateSwitcherTest.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tst/TemplateSwitcherTest.php b/tst/TemplateSwitcherTest.php index 16aec6b0..7f4ef8d8 100644 --- a/tst/TemplateSwitcherTest.php +++ b/tst/TemplateSwitcherTest.php @@ -41,6 +41,7 @@ class TemplateSwitcherTest extends TestCase $defaultTemplateFallback = 'bootstrap5'; $customTemplate = 'bootstrap-dark'; $customWrongTemplate = 'bootstrap-wrong'; + $escapeTemplateDirectory = '../index'; TemplateSwitcher::setTemplateFallback($defaultTemplateFallback); @@ -49,6 +50,9 @@ class TemplateSwitcherTest extends TestCase $_COOKIE['template'] = $customTemplate; $this->assertEquals($customTemplate, TemplateSwitcher::getTemplate(), 'Custom template'); + + $_COOKIE['template'] = $escapeTemplateDirectory; + $this->assertEquals($defaultTemplateFallback, TemplateSwitcher::getTemplate(), 'Fallback on escaping template directory'); } public function testGetAvailableTemplates() From 13949349af48cf6044e66b9aaf3ae1adfe87bf5b Mon Sep 17 00:00:00 2001 From: El RIDO Date: Mon, 10 Nov 2025 12:22:29 +0100 Subject: [PATCH 02/14] improve readability of logic --- lib/TemplateSwitcher.php | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/lib/TemplateSwitcher.php b/lib/TemplateSwitcher.php index 22c65b08..ad509759 100644 --- a/lib/TemplateSwitcher.php +++ b/lib/TemplateSwitcher.php @@ -101,14 +101,13 @@ class TemplateSwitcher */ public static function isTemplateAvailable(string $template): bool { - $available = in_array($template, self::getAvailableTemplates()); - - if (!$available && !View::isBootstrapTemplate($template)) { - $path = View::getTemplateFilePath($template); - $available = file_exists($path); + if (in_array($template, self::getAvailableTemplates())) { + return true; } - - return $available; + if (View::isBootstrapTemplate($template)) { + return false; + } + return file_exists(View::getTemplateFilePath($template)); } /** @@ -120,13 +119,10 @@ class TemplateSwitcher */ private static function getSelectedByUserTemplate(): ?string { - $selectedTemplate = null; $templateCookieValue = $_COOKIE['template'] ?? ''; - if (self::isTemplateAvailable($templateCookieValue)) { - $selectedTemplate = $templateCookieValue; + return $templateCookieValue; } - - return $selectedTemplate; + return null; } } From 17ff44037aba4669f310395afb9375e220ca3f01 Mon Sep 17 00:00:00 2001 From: El RIDO Date: Mon, 10 Nov 2025 12:23:50 +0100 Subject: [PATCH 03/14] prevent use of paths in template names, only file names inside tpl directory are allowed --- lib/View.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/View.php b/lib/View.php index dd15e321..65830323 100644 --- a/lib/View.php +++ b/lib/View.php @@ -66,7 +66,7 @@ class View */ public static function getTemplateFilePath(string $template): string { - $file = self::isBootstrapTemplate($template) ? 'bootstrap' : $template; + $file = self::isBootstrapTemplate($template) ? 'bootstrap' : basename($template); return PATH . 'tpl' . DIRECTORY_SEPARATOR . $file . '.php'; } From a479d7540533d1e8bac78447548b9c0c65753023 Mon Sep 17 00:00:00 2001 From: El RIDO Date: Mon, 10 Nov 2025 12:25:19 +0100 Subject: [PATCH 04/14] belt and braces: reset the template cookie, if function is not enabled --- lib/Controller.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/Controller.php b/lib/Controller.php index 3d316687..188524d5 100644 --- a/lib/Controller.php +++ b/lib/Controller.php @@ -434,11 +434,15 @@ class Controller setcookie('lang', $languageselection, array('SameSite' => 'Lax', 'Secure' => true)); } - // set template cookie if that functionality was enabled + // set template cookie if that functionality was enabled, otherwise delete any existing cookie $templateselection = ''; if ($this->_conf->getKey('templateselection')) { $templateselection = TemplateSwitcher::getTemplate(); setcookie('template', $templateselection, array('SameSite' => 'Lax', 'Secure' => true)); + } elseif (array_key_exists('template', $_COOKIE)) { + unset($_COOKIE['template']); // ensure value is not re-used in template switcher + $expiredInAllTimezones = time() - 86400; + setcookie('template', '', array('expires' => $expiredInAllTimezones, 'SameSite' => 'Lax', 'Secure' => true)); } // strip policies that are unsupported in meta tag From dae5f7fd616ffeee3c88ed7d1441337dda5ff6a8 Mon Sep 17 00:00:00 2001 From: El RIDO Date: Mon, 10 Nov 2025 17:27:11 +0100 Subject: [PATCH 05/14] partially revert #1559 Instead of automatically adding custom templates, we log an error if that template is missing in the available templates. Still mitigates arbitrary file inclusion, as the string is now checked against a fixed allow list. --- lib/TemplateSwitcher.php | 20 +++++++------------- lib/View.php | 28 ++-------------------------- tst/Bootstrap.php | 3 +++ tst/ViewTest.php | 16 ---------------- 4 files changed, 12 insertions(+), 55 deletions(-) diff --git a/lib/TemplateSwitcher.php b/lib/TemplateSwitcher.php index ad509759..142a9235 100644 --- a/lib/TemplateSwitcher.php +++ b/lib/TemplateSwitcher.php @@ -59,11 +59,8 @@ class TemplateSwitcher { if (self::isTemplateAvailable($template)) { self::$_templateFallback = $template; - - if (!in_array($template, self::getAvailableTemplates())) { - // Add custom template to the available templates list - self::$_availableTemplates[] = $template; - } + } else { + error_log('failed to set "' . $template . '" as a fallback, it needs to be added to the list of `availabletemplates` in the configuration file'); } } @@ -101,13 +98,11 @@ class TemplateSwitcher */ public static function isTemplateAvailable(string $template): bool { - if (in_array($template, self::getAvailableTemplates())) { + if (in_array($template, self::getAvailableTemplates(), true)) { return true; } - if (View::isBootstrapTemplate($template)) { - return false; - } - return file_exists(View::getTemplateFilePath($template)); + error_log('template "' . $template . '" is not in the list of `availabletemplates` in the configuration file'); + return false; } /** @@ -119,9 +114,8 @@ class TemplateSwitcher */ private static function getSelectedByUserTemplate(): ?string { - $templateCookieValue = $_COOKIE['template'] ?? ''; - if (self::isTemplateAvailable($templateCookieValue)) { - return $templateCookieValue; + if (array_key_exists('template', $_COOKIE) && self::isTemplateAvailable($_COOKIE['template'])) { + return $_COOKIE['template']; } return null; } diff --git a/lib/View.php b/lib/View.php index 65830323..666a03f7 100644 --- a/lib/View.php +++ b/lib/View.php @@ -49,7 +49,8 @@ class View */ public function draw($template) { - $path = self::getTemplateFilePath($template); + $file = substr($template, 0, 10) === 'bootstrap-' ? 'bootstrap' : $template; + $path = PATH . 'tpl' . DIRECTORY_SEPARATOR . $file . '.php'; if (!file_exists($path)) { throw new Exception('Template ' . $template . ' not found!', 80); } @@ -57,31 +58,6 @@ class View include $path; } - /** - * Get template file path - * - * @access public - * @param string $template - * @return string - */ - public static function getTemplateFilePath(string $template): string - { - $file = self::isBootstrapTemplate($template) ? 'bootstrap' : basename($template); - return PATH . 'tpl' . DIRECTORY_SEPARATOR . $file . '.php'; - } - - /** - * Is the template a variation of the bootstrap template - * - * @access public - * @param string $template - * @return bool - */ - public static function isBootstrapTemplate(string $template): bool - { - return substr($template, 0, 10) === 'bootstrap-'; - } - /** * echo script tag incl. SRI hash for given script file * diff --git a/tst/Bootstrap.php b/tst/Bootstrap.php index 5c5ceb6f..e905312c 100644 --- a/tst/Bootstrap.php +++ b/tst/Bootstrap.php @@ -6,7 +6,9 @@ use Google\Cloud\Storage\Bucket; use Google\Cloud\Storage\Connection\ConnectionInterface; use Google\Cloud\Storage\StorageClient; use Google\Cloud\Storage\StorageObject; +use PrivateBin\Configuration; use PrivateBin\Persistence\ServerSalt; +use PrivateBin\TemplateSwitcher; error_reporting(E_ALL | E_STRICT); @@ -26,6 +28,7 @@ if (!defined('CONF_SAMPLE')) { require PATH . 'vendor/autoload.php'; Helper::updateSubresourceIntegrity(); +TemplateSwitcher::setAvailableTemplates(Configuration::getDefaults()['main']['availabletemplates']); /** * Class Helper provides unit tests pastes and comments of various formats diff --git a/tst/ViewTest.php b/tst/ViewTest.php index fcb0bdac..fb261f6c 100644 --- a/tst/ViewTest.php +++ b/tst/ViewTest.php @@ -141,20 +141,4 @@ class ViewTest extends TestCase $this->expectExceptionCode(80); $test->draw('123456789 does not exist!'); } - - public function testTemplateFilePath() - { - $template = 'bootstrap'; - $templatePath = PATH . 'tpl' . DIRECTORY_SEPARATOR . $template . '.php'; - $path = View::getTemplateFilePath($template); - $this->assertEquals($templatePath, $path, 'Template file path'); - } - - public function testIsBootstrapTemplate() - { - $bootstrapTemplate = 'bootstrap-dark'; - $nonBootstrapTemplate = 'bootstrap5'; - $this->assertTrue(View::isBootstrapTemplate($bootstrapTemplate), 'Is bootstrap template'); - $this->assertFalse(View::isBootstrapTemplate($nonBootstrapTemplate), 'Is not bootstrap template'); - } } From f2164353c31cf9491d917bbf2a2ea5712fcb6cc9 Mon Sep 17 00:00:00 2001 From: El RIDO Date: Tue, 11 Nov 2025 09:34:54 +0100 Subject: [PATCH 06/14] use realpath and validate tpl directory contents to ensure only php files inside the tpl dir can get used as templates --- lib/View.php | 17 +++++++++++++---- tst/ViewTest.php | 9 +++++++++ 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/lib/View.php b/lib/View.php index 666a03f7..9a83e6e4 100644 --- a/lib/View.php +++ b/lib/View.php @@ -12,6 +12,7 @@ namespace PrivateBin; use Exception; +use GlobIterator; /** * View @@ -49,13 +50,21 @@ class View */ public function draw($template) { + $dir = PATH . 'tpl' . DIRECTORY_SEPARATOR; $file = substr($template, 0, 10) === 'bootstrap-' ? 'bootstrap' : $template; - $path = PATH . 'tpl' . DIRECTORY_SEPARATOR . $file . '.php'; - if (!file_exists($path)) { + $path = realpath($dir . $file . '.php'); + if ($path === false) { throw new Exception('Template ' . $template . ' not found!', 80); } - extract($this->_variables); - include $path; + foreach (new GlobIterator($dir . '*.php') as $tplFile) { + if ($tplFile->getRealPath() === $path) { + $templatesInPath = new GlobIterator(PATH . 'tpl' . DIRECTORY_SEPARATOR . '*.php'); + extract($this->_variables); + include $path; + return; + } + } + throw new Exception('Template ' . $file . '.php not found in ' . $dir . '!', 81); } /** diff --git a/tst/ViewTest.php b/tst/ViewTest.php index fb261f6c..b3d4aa37 100644 --- a/tst/ViewTest.php +++ b/tst/ViewTest.php @@ -141,4 +141,13 @@ class ViewTest extends TestCase $this->expectExceptionCode(80); $test->draw('123456789 does not exist!'); } + + public function testInvalidTemplate() + { + $test = new View; + $this->expectException(Exception::class); + $this->expectExceptionCode(81); + $test->draw('../index'); + } + } From be6a3702fca101456788521d71c7b7d10394c449 Mon Sep 17 00:00:00 2001 From: El RIDO Date: Tue, 11 Nov 2025 09:43:41 +0100 Subject: [PATCH 07/14] simplify logic and improve readability function was only used in one place and only indirectly tested, so it could be inlined, which also makes the test for null and the extra variable allocation unnecessary --- lib/TemplateSwitcher.php | 23 +++++------------------ 1 file changed, 5 insertions(+), 18 deletions(-) diff --git a/lib/TemplateSwitcher.php b/lib/TemplateSwitcher.php index 142a9235..abfdffc0 100644 --- a/lib/TemplateSwitcher.php +++ b/lib/TemplateSwitcher.php @@ -65,7 +65,7 @@ class TemplateSwitcher } /** - * get currently loaded template + * get user selected template or fallback * * @access public * @static @@ -73,8 +73,10 @@ class TemplateSwitcher */ public static function getTemplate(): string { - $selectedTemplate = self::getSelectedByUserTemplate(); - return $selectedTemplate ?? self::$_templateFallback; + if (array_key_exists('template', $_COOKIE) && self::isTemplateAvailable($_COOKIE['template'])) { + return $_COOKIE['template']; + } + return self::$_templateFallback; } /** @@ -104,19 +106,4 @@ class TemplateSwitcher error_log('template "' . $template . '" is not in the list of `availabletemplates` in the configuration file'); return false; } - - /** - * get the template selected by user - * - * @access private - * @static - * @return string|null - */ - private static function getSelectedByUserTemplate(): ?string - { - if (array_key_exists('template', $_COOKIE) && self::isTemplateAvailable($_COOKIE['template'])) { - return $_COOKIE['template']; - } - return null; - } } From ea73300e155780aaf7a2ce3b710f1aa5b763c8bf Mon Sep 17 00:00:00 2001 From: El RIDO Date: Tue, 11 Nov 2025 09:45:51 +0100 Subject: [PATCH 08/14] don't always set the cookie, having to unset it later but still unset it, if it currently should not be in use (templateselection = false) --- lib/Controller.php | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/lib/Controller.php b/lib/Controller.php index 188524d5..08192d4d 100644 --- a/lib/Controller.php +++ b/lib/Controller.php @@ -219,10 +219,11 @@ class Controller TemplateSwitcher::setAvailableTemplates($templates); TemplateSwitcher::setTemplateFallback($template); - // force default template, if template selection is disabled and a default is set - if (!$this->_conf->getKey('templateselection') && !empty($template)) { - $_COOKIE['template'] = $template; - setcookie('template', $template, array('SameSite' => 'Lax', 'Secure' => true)); + // force default template, if template selection is disabled + if (!$this->_conf->getKey('templateselection') && array_key_exists('template', $_COOKIE)) { + unset($_COOKIE['template']); // ensure value is not re-used in template switcher + $expiredInAllTimezones = time() - 86400; + setcookie('template', '', array('expires' => $expiredInAllTimezones, 'SameSite' => 'Lax', 'Secure' => true)); } } @@ -434,15 +435,11 @@ class Controller setcookie('lang', $languageselection, array('SameSite' => 'Lax', 'Secure' => true)); } - // set template cookie if that functionality was enabled, otherwise delete any existing cookie + // set template cookie if that functionality was enabled $templateselection = ''; if ($this->_conf->getKey('templateselection')) { $templateselection = TemplateSwitcher::getTemplate(); setcookie('template', $templateselection, array('SameSite' => 'Lax', 'Secure' => true)); - } elseif (array_key_exists('template', $_COOKIE)) { - unset($_COOKIE['template']); // ensure value is not re-used in template switcher - $expiredInAllTimezones = time() - 86400; - setcookie('template', '', array('expires' => $expiredInAllTimezones, 'SameSite' => 'Lax', 'Secure' => true)); } // strip policies that are unsupported in meta tag From 94a854faca5b997783b935ae6194fa13dbea6ce0 Mon Sep 17 00:00:00 2001 From: El RIDO Date: Tue, 11 Nov 2025 10:59:55 +0100 Subject: [PATCH 09/14] do add the configured template to the available ones, if missing --- lib/Controller.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/Controller.php b/lib/Controller.php index 08192d4d..1d726da5 100644 --- a/lib/Controller.php +++ b/lib/Controller.php @@ -216,6 +216,9 @@ class Controller { $templates = $this->_conf->getKey('availabletemplates'); $template = $this->_conf->getKey('template'); + if (!in_array($template, $templates, true)) { + $templates[] = $template; + } TemplateSwitcher::setAvailableTemplates($templates); TemplateSwitcher::setTemplateFallback($template); From 51bb637411bf03e5f907f827b13f9fdc28c4bc42 Mon Sep 17 00:00:00 2001 From: El RIDO Date: Tue, 11 Nov 2025 11:00:19 +0100 Subject: [PATCH 10/14] document the change --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c368725..2bd8f43d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,7 @@ # PrivateBin version history ## 2.0.3 (not yet released) +* FIXED: Prevent arbitrary PHP file inclusion when enabling template switching ## 2.0.2 (2025-10-28) * CHANGED: Upgrading libraries to: DOMpurify 3.3.0 @@ -44,7 +45,7 @@ * FIXED: Page template scripts loading order (#1579) ## 1.7.7 (2025-06-28) -* ADDED: Switching templates using the web ui (#1501) +* ADDED: Switching templates using the web UI (#1501) * ADDED: Show file name and size on download page (#603) * CHANGED: Passing large data structures by reference to reduce memory consumption (#858) * CHANGED: Removed use of ctype functions and polyfill library for ctype From a371f5cab51c8a03eea898a7ed7f8773564fd066 Mon Sep 17 00:00:00 2001 From: El RIDO Date: Tue, 11 Nov 2025 12:49:37 +0100 Subject: [PATCH 11/14] remove dead code --- lib/View.php | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/View.php b/lib/View.php index 9a83e6e4..f1c192cc 100644 --- a/lib/View.php +++ b/lib/View.php @@ -58,7 +58,6 @@ class View } foreach (new GlobIterator($dir . '*.php') as $tplFile) { if ($tplFile->getRealPath() === $path) { - $templatesInPath = new GlobIterator(PATH . 'tpl' . DIRECTORY_SEPARATOR . '*.php'); extract($this->_variables); include $path; return; From f456fb576eef76031659d077d90c3a5600b0409b Mon Sep 17 00:00:00 2001 From: El RIDO Date: Tue, 11 Nov 2025 17:52:48 +0100 Subject: [PATCH 12/14] ensure template cookie cannot be a path --- lib/TemplateSwitcher.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/TemplateSwitcher.php b/lib/TemplateSwitcher.php index abfdffc0..fa9fdb18 100644 --- a/lib/TemplateSwitcher.php +++ b/lib/TemplateSwitcher.php @@ -73,8 +73,11 @@ class TemplateSwitcher */ public static function getTemplate(): string { - if (array_key_exists('template', $_COOKIE) && self::isTemplateAvailable($_COOKIE['template'])) { - return $_COOKIE['template']; + if (array_key_exists('template', $_COOKIE)) { + $template = basename($_COOKIE['template']); + if (self::isTemplateAvailable($template)) { + return $template; + } } return self::$_templateFallback; } From c35fc4f7904fd5f386c1f2d9e0b806217b6922a1 Mon Sep 17 00:00:00 2001 From: El RIDO Date: Tue, 11 Nov 2025 17:53:50 +0100 Subject: [PATCH 13/14] use more straight forward in_array check kudos @Ribas160 for the suggestion --- lib/View.php | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/lib/View.php b/lib/View.php index f1c192cc..4aebfe43 100644 --- a/lib/View.php +++ b/lib/View.php @@ -52,18 +52,15 @@ class View { $dir = PATH . 'tpl' . DIRECTORY_SEPARATOR; $file = substr($template, 0, 10) === 'bootstrap-' ? 'bootstrap' : $template; - $path = realpath($dir . $file . '.php'); - if ($path === false) { - throw new Exception('Template ' . $template . ' not found!', 80); + $path = $dir . $file . '.php'; + if (!is_file($path)) { + throw new Exception('Template ' . $template . ' not found in file ' . $path . '!', 80); } - foreach (new GlobIterator($dir . '*.php') as $tplFile) { - if ($tplFile->getRealPath() === $path) { - extract($this->_variables); - include $path; - return; - } + if (!in_array($path, glob($dir . '*.php', GLOB_NOSORT | GLOB_ERR), true)) { + throw new Exception('Template ' . $file . '.php not found in ' . $dir . '!', 81); } - throw new Exception('Template ' . $file . '.php not found in ' . $dir . '!', 81); + extract($this->_variables); + include $path; } /** From 2e11b13464cf568cd5405179a58c5ca3635aaa80 Mon Sep 17 00:00:00 2001 From: El RIDO Date: Tue, 11 Nov 2025 17:56:49 +0100 Subject: [PATCH 14/14] remove dead code --- lib/View.php | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/View.php b/lib/View.php index 4aebfe43..514ac2b1 100644 --- a/lib/View.php +++ b/lib/View.php @@ -12,7 +12,6 @@ namespace PrivateBin; use Exception; -use GlobIterator; /** * View