Support CLI tools that use phars without a .phar extension

[alexpott]

Some CLI tools are shipped as phar files and the install instructions tell users to rename the cli_command.phar to cli_command.

In this instance the interceptor can produce false positives because it is handling a phar file without a phar extension. But the very first caller is a phar file so I think it is safe to assume this is intentional.

[alexpott]

For example of such a CLI tool - see https://getcomposer.org/doc/00-intro.md#globally

[alexpott]

Here's the code for cli_phar as it is a binary diff and therefore is not so easy to work with.

<?php
// Use the current working directory so we don't have to include all the code in
// the phar file.
require_once getcwd() . '/../../../vendor/autoload.php';
stream_wrapper_unregister('phar');
stream_wrapper_register('phar', \TYPO3\PharStreamWrapper\PharStreamWrapper::class);

\TYPO3\PharStreamWrapper\Manager::initialize(
    (new \TYPO3\PharStreamWrapper\Behavior())
        ->withAssertion(new \TYPO3\PharStreamWrapper\Interceptor\PharExtensionInterceptor())
);

// This should not throw an exception.
if (file_exists(__DIR__ . '/index.php')) {
  echo "Can access phar files without .phar extension if they are the CLI command.\n";
}

// Try an insecure phar without an extension.
if (file_exists('phar://bundle.phar')) {
  echo "Can access phar files with .phar extension.\n";
}

try {
  file_exists('phar://bundle.phar.png');
}
catch (\TYPO3\PharStreamWrapper\Exception $e) {
  echo "Cannot access other phar files without .phar extension.\n";
}

[ohader]

Thanks for your pull-request. I'm going to look into that in more details during the next days...

[ohader]

In addition I've created an experimental Phar meta-data analyzer a couple of weeks ago in branch feature/phar-reader, see https://github.com/TYPO3/phar-stream-wrapper/blob/feature/phar-reader/src/Interceptor/PharMetaDataInterceptor.php

        Manager::initialize(
            (new \TYPO3\PharStreamWrapper\Behavior())
                ->withAssertion(new PharMetaDataInterceptor())
        );

This way if could be detected, whether the file itself is malicious in terms of insecure deserialization - instead of just checking the file extension. Any feedback or tests are welcome of course...

[mortenson]

Note: This PR will need updated - per https://www.drupal.org/project/drupal/issues/3026386, not all backtraces have a "file" key, and some "file" entries include the phar:// extension.

[alexpott]

Drupal discovered another issue now we've put this out in a security release - see https://www.drupal.org/project/drupal/issues/3026443

Basically Phar aliases break \TYPO3\PharStreamWrapper\Helper::determineBaseFile() because there's no way that can resolve the Phar alias that only exists inside the phar file.

[ohader]

Thanks for your feedback. I tried to read through the comments in the issue you mentioned, but failed to understand the according context. Are those failure related to CLI invocation only? Or does it happen when being called in a web context (using different PHP_SAPI)?

If you could point me to source code or 3rd party packages and how they are using Phar aliases, I could create according test cases.

[ohader]

@alexpott I just read your last comment https://www.drupal.org/project/drupal/issues/3026443#comment-12930646

$phar = new \Phar(__DIR__ . '/alias_phar.phar', NULL, 'alias_phar.phar');
var_dump($phar->getAlias());

I assume that should be used in order to determine the alias (I still have to dig into the problem in real source code)... However, having new \Phar($baseName) will already execute meta-data deserialization - which might be insecure and compromised.

[alexpott]

@ohader re which might be insecure and compromised. yeah let's why we only do this after checking the extension. So basically we're at the point where we would allow access. I think the this library what we need to do is store a phars alias once we've determined it to be safe.

[alexpott]

@ohader the drupal issue has all the test code, necessary test phars and also the code to build them :)

[LionsAd]

$path should be $basePath here unless I miss something.

[alexpott]

So there are some issues with this PR.

• The architecture of the stream wrapper allows for per command interception - the new cache / alias approach breaks that. We don't have test coverage of this. I wonder why the per command stuff was added?
• We know that not all phar commands have aliases set the same way - for example https://github.com/maxmind/GeoIP2-php/releases/download/v2.9.0/geoip2.phar - as a result of being built with box

[ohader]

• Defining behavior based on commands would have allowed different strategies for invoking interceptors - I currently don't see how this might break, given that the $alias cache is located in the base PharStreamWrapper.
• Concerning alias resolving (especially for those that are not part of Phar manifests - but using Phar::mapPhar('alias')) I've extended the experimental branch to include this behavior https://github.com/TYPO3/phar-stream-wrapper/blob/feature/phar-reader/tests/Functional/Phar/ReaderTest.php - reading stub content feels ugly, but I did not find any other reference on how PHP is exposing internal Phar alias mapping...

[LionsAd]

@ohader I agree, I also searched the PHP code base for how mapPhar and setAlias work and the best I could determine was that if you use new Phar($filename, ..., $alias) -- it is only a temporary alias and hence not written to disk.

The better way for box and co would be to call ->setAlias(), which does write to disk and is permanent to make things easier for us.

In theory we could create a patcher tool to allow people to patch their phar files without aliases to have the same as internally used via mapPhar().

---

Edit: I tested it, a tool to set an alias afterwards is as simple as with phar.readonly=0:

$phar = new Phar("geoip2.phar");
$phar->setAlias("geoip2.phar");

Important fact, if alias is not set, then it seems to fallback to the whole file name.

[ohader]

Alright, I'd like to proceed as following

• integrate Phar Reader feature into master (PR [FEATURE ] Add possibility to retrieve low-level Phar internals #10)
• separate this PR into CLI and alias related topics (PR [BUGFIX] Consider Phar aliases when resolving base file names #12)
• define & create test cases

[ohader]

Could not resolve CLI flaws anymore after having alias handling and realpath resolving of PR #12 applied. Find according test here 1ada68c

[ohader]

Seems to be resolved with c267f38 and 42730ed

[ohader]

Addressed in PR #12