Simplify security check

Logic in ComponentPartial was rolled back and moved to the Controller. Since there are issues with throwing exceptions inside the component partial lookup logic (exceptions are conditionally suppressed), it seems like it would be better to bubble up the security logic to the controller level as a simple base dir security check, which is no longer concerned about any suppression logic. This looks to have logic parity with the previous solution

Refs #4652
This commit is contained in:
Samuel Georges 2019-12-14 12:37:44 +11:00
parent 80f870c313
commit 7902cfa58a
2 changed files with 16 additions and 35 deletions

View File

@ -107,10 +107,6 @@ class ComponentPartial extends Extendable implements CmsObjectContract
$partial = Partial::loadCached($theme, $component->alias . '/' . $fileName); $partial = Partial::loadCached($theme, $component->alias . '/' . $fileName);
} }
if ($partial !== null) {
static::ensureValidPartialPath($partial->getFileName(), $partial->getFilePath(), null);
}
return $partial; return $partial;
} }
@ -164,36 +160,19 @@ class ComponentPartial extends Extendable implements CmsObjectContract
*/ */
protected function validateFileName($fileName) protected function validateFileName($fileName)
{ {
if (!FileHelper::validatePath($fileName, $this->maxNesting)) {
throw new ApplicationException(Lang::get('cms::lang.cms_object.invalid_file', [
'name' => $fileName
]));
}
if (!strlen(File::extension($fileName))) { if (!strlen(File::extension($fileName))) {
$fileName .= '.'.$this->defaultExtension; $fileName .= '.'.$this->defaultExtension;
} }
static::ensureValidPartialPath($fileName, $this->getFilePath($fileName), $this->maxNesting);
return $fileName; return $fileName;
} }
/**
* Ensures that a partial path is valid and local to the application.
*
* @param string $fileName
* @param $realpath
* @param int $maxNesting
*
* @return bool
* @throws ApplicationException
*/
protected static function ensureValidPartialPath($fileName, $realpath, $maxNesting = 2)
{
if (FileHelper::validatePath($fileName, $maxNesting) && FileHelper::validateIsLocalFile($realpath)) {
return true;
}
throw new ApplicationException(Lang::get('cms::lang.cms_object.invalid_file', [
'name' => $fileName,
]));
}
/** /**
* Returns the file content. * Returns the file content.
* @return string * @return string

View File

@ -1003,14 +1003,7 @@ class Controller
* Check the component partial * Check the component partial
*/ */
if ($partial === null) { if ($partial === null) {
try { $partial = ComponentPartial::loadCached($componentObj, $partialName);
$partial = ComponentPartial::loadCached($componentObj, $partialName);
} catch (ApplicationException $e) {
if ($throwException) {
throw $e;
}
return false;
}
} }
if ($partial === null) { if ($partial === null) {
@ -1037,6 +1030,15 @@ class Controller
return false; return false;
} }
/*
* Security check
*/
if (!\Cms\Helpers\File::validateIsLocalFile($partial->getFilePath())) {
throw new CmsException(Lang::get('cms::lang.cms_object.invalid_file', [
'name' => $partial->getFileName()
]));
}
/* /*
* Run functions for CMS partials only (Cms\Classes\Partial) * Run functions for CMS partials only (Cms\Classes\Partial)
*/ */