Skip to content

Commit 2952792

Browse files
committed
Various Hardening Updates
Five hardening changes from a security audit. None is a fresh attacker primitive on its own; the audit recommended shipping them as one PR. - `RemoveIniEntry`: anchor the regex with `\b` and `preg_quote` the extension name, so uninstalling `foo` no longer rewrites the prefix of `extension=foo_other` lines. - `PlaceholderReplacer` and `WindowsInstall`: skip symlinks during the recursive source walk, so the iterator no longer descends into files outside the extracted source dir. - `OverrideDownloadUrlInstallListener`: clear the Packagist-side dist sha after `setDistUrl`, and warn that swapping to a release-asset URL leaves only HTTPS-to-origin as the integrity guarantee. - `ConfigureOption`: validate `php-ext.configure-options[].name` against `/^[a-zA-Z][a-zA-Z0-9_-]*$/`, so configure-flag identifiers can't carry whitespace or shell metacharacters into argv or `installed.json`. - `FallbackVerificationUsingOpenSsl`: promote the fallback notice to `writeError` and document that the OpenSSL path skips Rekor transparency-log verification.
1 parent 2d228f1 commit 2952792

6 files changed

Lines changed: 45 additions & 6 deletions

File tree

src/Building/PlaceholderReplacer.php

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,13 @@ public function replacePlaceholdersWithPlaceholderReplacements(IOInterface $io,
5757
foreach (new RecursiveIteratorIterator(new RecursiveDirectoryIterator($downloadedPackage->extractedSourcePath)) as $file) {
5858
assert($file instanceof SplFileInfo);
5959

60+
// Refuse to follow symlinks into files the package author did not
61+
// legitimately own at build time (e.g., an archive entry that points
62+
// at the invoking user's $HOME).
63+
if ($file->isLink()) {
64+
continue;
65+
}
66+
6067
if (! $file->isFile() || ! in_array($file->getExtension(), self::FILE_EXTENSIONS)) {
6168
continue;
6269
}

src/ComposerIntegration/Listeners/OverrideDownloadUrlInstallListener.php

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
use Throwable;
2121

2222
use function array_walk;
23+
use function parse_url;
2324
use function pathinfo;
2425

2526
use const PATHINFO_EXTENSION;
@@ -129,6 +130,18 @@ function (OperationInterface $operation): void {
129130
$this->composerRequest->pieOutput->write('Found prebuilt archive: ' . $url);
130131
$composerPackage->setDistUrl($url);
131132

133+
// The Packagist-side dist sha1 was computed against the
134+
// original dist URL; once we swap to a release-asset URL
135+
// it no longer corresponds to anything Composer will fetch.
136+
// Clear it so Composer doesn't silently match the wrong
137+
// bytes against a stale checksum, and warn the caller that
138+
// the only integrity guarantee left is HTTPS to the
139+
// release-hosting origin.
140+
$composerPackage->setDistSha1Checksum(null);
141+
$this->composerRequest->pieOutput->write(
142+
'<warning>Note: dist-sha integrity check is not available for prebuilt-binary URLs; relying on HTTPS to ' . parse_url($url, PHP_URL_HOST) . ' only.</warning>',
143+
);
144+
132145
if (pathinfo($url, PATHINFO_EXTENSION) === 'tgz') {
133146
$composerPackage->setDistType('tar');
134147
}

src/ConfigureOption.php

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,14 @@ public static function fromComposerJsonDefinition(array $configureOptionDefiniti
2828
{
2929
Assert::keyExists($configureOptionDefinition, 'name');
3030
Assert::stringNotEmpty($configureOptionDefinition['name']);
31+
// Restrict to identifier characters that match real ./configure flag
32+
// conventions. Whitespace and shell metacharacters in this field flow
33+
// verbatim into argv, log output, and installed.json metadata.
34+
Assert::regex(
35+
$configureOptionDefinition['name'],
36+
'/^[a-zA-Z][a-zA-Z0-9_-]*$/',
37+
'php-ext.configure-options[].name must be a configure-flag identifier (got %s)',
38+
);
3139

3240
$needsValue = false;
3341
if (array_key_exists('needs-value', $configureOptionDefinition)) {

src/Installing/Ini/RemoveIniEntryWithFileGetContents.php

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
use function file_exists;
2020
use function in_array;
2121
use function is_dir;
22+
use function preg_quote;
2223
use function Safe\file_get_contents;
2324
use function Safe\preg_replace;
2425
use function Safe\scandir;
@@ -67,10 +68,14 @@ static function (string $path) use ($additionalIniDirectory): bool {
6768
// Make sure all symlinks are resolved
6869
$allIniFiles = array_filter(array_map('realpath', $allIniFiles));
6970

71+
// Anchor on the right with \b so uninstalling `foo` doesn't also
72+
// rewrite the prefix of `extension=foo_other`. preg_quote on the
73+
// extension name is defence-in-depth in case future ExtensionName
74+
// validation ever loosens past `^[A-Za-z][a-zA-Z0-9_]+$`.
7075
$regex = sprintf(
71-
'/^(%s\s*=\s*%s)/m',
76+
'/^(%s\s*=\s*%s)\b/m',
7277
$package->extensionType() === ExtensionType::PhpModule ? 'extension' : 'zend_extension',
73-
$package->extensionName()->name(),
78+
preg_quote($package->extensionName()->name(), '/'),
7479
);
7580

7681
$updatedIniFiles = [];

src/Installing/WindowsInstall.php

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,10 +58,13 @@ public function __invoke(
5858
assert($file instanceof SplFileInfo);
5959

6060
/**
61-
* Skip directories, the main DLL, PDB
61+
* Skip directories, the main DLL, PDB, and any symlinks the archive
62+
* may have shipped (symlink-followed targets fall outside the source
63+
* dir's containment guarantees).
6264
*/
6365
if (
6466
$file->isDir()
67+
|| $file->isLink()
6568
|| $this->normalisedPathsMatch($file->getPathname(), $sourceDllName)
6669
|| $this->normalisedPathsMatch($file->getPathname(), $sourcePdbName)
6770
) {

src/SelfManage/Verify/FallbackVerificationUsingOpenSsl.php

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,12 @@ public function __construct(
3636

3737
public function verify(ReleaseMetadata $releaseMetadata, BinaryFile $pharFilename, IOInterface $io): void
3838
{
39-
$io->write(
40-
'Falling back to basic verification. To use full verification, install the `gh` CLI tool.',
41-
verbosity: IOInterface::VERBOSE,
39+
// The fallback verifier checks cert chain, cert extension claims, DSSE
40+
// subject digest, and DSSE signature, but does NOT validate Rekor
41+
// transparency-log inclusion. `gh attestation verify` does. Surface the
42+
// reduced guarantees so users on shared / air-gapped hosts know.
43+
$io->writeError(
44+
'<warning>Falling back to OpenSSL verification (no Rekor inclusion check). Install `gh` for full attestation verification.</warning>',
4245
);
4346

4447
try {

0 commit comments

Comments
 (0)