Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(appmanager): Fix tainted file path when loading appinfos #48604

Merged
merged 1 commit into from
Oct 14, 2024

Conversation

nickvergessen
Copy link
Member

Comments

  • It's arguable whether killing the bool $path parameter is a breaking change and should be done. But I'd say the risk is minimal. The only usage I found is the \OC\Installer code.
  • The entry is actually a "false positive", as the claimed call is with $path = false and therefore
    $file - lib/private/App/AppManager.php:747:4
      		$file = $appId;
    
    is never applicable (it's the if ($path) { block). But to work around that would mean we need to add many psalm comments in all the places calling it, until upstream fixes it (and I'm not sure they can with reasonable effort).
  • So I thought splitting the method in a "tainted" and "path" way is the better solution.

After

  • 1 less baseline entry

Before

TaintedFile - lib/private/App/InfoParser.php:38:50 - Detected tainted file handling (see https://psalm.dev/255)
  oca\updatenotification\controller\changelogcontroller::showchangelog#1 - apps/updatenotification/lib/Controller/ChangelogController.php:41:32
	public function showChangelog(string $app, ?string $version = null): TemplateResponse {

  $app - apps/updatenotification/lib/Controller/ChangelogController.php:41:39
	public function showChangelog(string $app, ?string $version = null): TemplateResponse {

  call to OCP\App\IAppManager::getAppInfo - apps/updatenotification/lib/Controller/ChangelogController.php:43:44
		$appInfo = $this->appManager->getAppInfo($app) ?? [];

  OCP\App\IAppManager::getAppInfo#1 - lib/public/App/IAppManager.php:34:36
	public function getAppInfo(string $appId, bool $path = false, $lang = null);

  OC\App\AppManager::getAppInfo#1 - apps/files_versions/lib/AppInfo/Application.php:118:39
			$appInfo = $appManager->getAppInfo($app);

  $appId - lib/private/App/AppManager.php:745:36
	public function getAppInfo(string $appId, bool $path = false, $lang = null) {

  $file - lib/private/App/AppManager.php:747:4
			$file = $appId;

  call to OC\App\InfoParser::parse - lib/private/App/AppManager.php:761:26
		$data = $parser->parse($file);

  OC\App\InfoParser::parse#1 - lib/private/App/InfoParser.php:25:24
	public function parse($file) {

  $file - lib/private/App/InfoParser.php:25:24
	public function parse($file) {

  call to file_get_contents - lib/private/App/InfoParser.php:38:50
		$xml = simplexml_load_string(file_get_contents($file));

Checklist

Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen nickvergessen force-pushed the bugfix/noid/fix-tainted-file-appinfo branch from e90f9e3 to 0744984 Compare October 14, 2024 12:33
@nickvergessen nickvergessen requested a review from come-nc October 14, 2024 12:33
@nickvergessen nickvergessen merged commit f079931 into master Oct 14, 2024
175 of 177 checks passed
@nickvergessen nickvergessen deleted the bugfix/noid/fix-tainted-file-appinfo branch October 14, 2024 15:03
@skjnldsv skjnldsv mentioned this pull request Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants