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

[work-in-progress] Fix MITM vulnerability for issue #1074 - Needs review #1092

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Composer/Command/SelfUpdateCommand.php
Expand Up @@ -42,12 +42,12 @@ protected function configure()
protected function execute(InputInterface $input, OutputInterface $output)
{
$rfs = new RemoteFilesystem($this->getIO());
$latest = trim($rfs->getContents('getcomposer.org', 'http://getcomposer.org/version', false));
$latest = trim($rfs->getContents('getcomposer.org', 'https://getcomposer.org/version', false));

if (Composer::VERSION !== $latest) {
$output->writeln(sprintf("Updating to version <info>%s</info>.", $latest));

$remoteFilename = 'http://getcomposer.org/composer.phar';
$remoteFilename = 'https://getcomposer.org/composer.phar';
$localFilename = $_SERVER['argv'][0];
$tempFilename = basename($localFilename, '.phar').'-temp.phar';

Expand Down
4 changes: 2 additions & 2 deletions src/Composer/Config.php
Expand Up @@ -22,13 +22,13 @@ class Config
'vendor-dir' => 'vendor',
'bin-dir' => '{$vendor-dir}/bin',
'notify-on-install' => true,
'github-protocols' => array('git', 'https', 'http'),
'github-protocols' => array('https', 'git'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why that? The Git protocol works way better/faster for many people, so that'd be quite annoying.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately the git protocol has no support for TLS or X.509 client certificate verification, AFAIK. Defaulting to HTTPS, and allowing users to trust Github's cert/CA ensures that users are not vulnerable to MITM. This way, if a MITM occurs, the user will get a prompt that A) the certificate they're being served is not valid for GitHub.com and B) that the CA has changed to an untrusted party. If a user continues at this point, it's their responsibility. Composer has done everything it can to protect them. By defaulting to git://, a MITM could occur silently.

If we could promote and enforce the use of signed tags, this problem would go away entirely.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the github protocols stuff is used only for downloading packages from "source" (as opposed to dist), which means typically it is dev versions of packages, and not tags. Now assuming you get MITM'd, I think the git process would prompt for user acceptance of the new signature, and it'd block there because that is hidden from the user, and then it'd time out after a while.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right if git is using SSL, but there's no such check for the git protocol as far as I understand.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Seldaek The git protocol is simply a tcp protocol, and has no signatures; only when over SSH or SSL are signatures checked.

);

public static $defaultRepositories = array(
'packagist' => array(
'type' => 'composer',
'url' => 'http://packagist.org',
'url' => 'https://packagist.org',
)
);

Expand Down
2 changes: 1 addition & 1 deletion src/Composer/Repository/ComposerRepository.php
Expand Up @@ -39,7 +39,7 @@ public function __construct(array $repoConfig, IOInterface $io, Config $config)
{
if (!preg_match('{^\w+://}', $repoConfig['url'])) {
// assume http as the default protocol
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should update this comment :)

$repoConfig['url'] = 'http://'.$repoConfig['url'];
$repoConfig['url'] = 'https://'.$repoConfig['url'];
}
$repoConfig['url'] = rtrim($repoConfig['url'], '/');
if (function_exists('filter_var') && version_compare(PHP_VERSION, '5.3.3', '>=') && !filter_var($repoConfig['url'], FILTER_VALIDATE_URL)) {
Expand Down
20 changes: 20 additions & 0 deletions src/Composer/Util/BadCryptoException.php
@@ -0,0 +1,20 @@
<?php

/*
* This file is part of Composer.
*
* (c) Nils Adermann <naderman@naderman.de>
* Jordi Boggiano <j.boggiano@seld.be>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Composer\Util;

/**
* @author Evan Coury <me@evancoury.com>
*/
class BadCryptoException extends \Exception
{
}
31 changes: 30 additions & 1 deletion src/Composer/Util/RemoteFilesystem.php
Expand Up @@ -90,9 +90,15 @@ protected function get($originUrl, $fileUrl, $fileName = null, $progress = true)
$this->result = null;
$this->originUrl = $originUrl;
$this->fileUrl = $fileUrl;
$this->fileUrlParts = parse_url($fileUrl);
$this->fileName = $fileName;
$this->progress = $progress;
$this->lastProgress = null;
$this->ssl = ('https' === substr($fileUrl, 0, 5));

if (!extension_loaded('openssl') && $this->ssl) {
throw new \RuntimeException('You must enable the openssl extension in your php.ini to load information from '.$fileUrl);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has to change really, we can print a warning the first time this occurs in the process, but we can't shut out these users. Not everyone can enable openssl.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only shows up if they're trying to access an https:// URL without openssl enabled, in which case they couldn't make the request anyway, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not completely true, because you changed http urls to https above (getcomposer.org & packagist). In those cases a downgrade to http is possible. Maybe it can just always try to downgrade and if it fails with http then fail hard.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also if you keep http in the chain, if https fails it'd then fallback on http, so that kind of defeats the whole point. I think either we force https (but I know for a fact that this won't work for some people.. that's why we have this chain thing in place) or we try https first, then git and http and try to read the https failures to see if it failed because of MITM (in which case abort) or because of an expected failure (then try next in the chain)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Err the above comment was meant for the other thread on top changing the github-protocols

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we really need to support those users, here's how it should work for them: The first time they run composer, it will check for openssl immediately. If it's not found, it'll throw a nice warning explaining that they are strongly encouraged to enable openssl before using composer and explain the implications of not doing so. The prompt will then ask them if they wish to put composer into an "insecure mode", if they say no, composer quits, and on next run the same prompt will show up. If they say yes, we put an 'insecure_mode': true, entry into their config.json file in their composer home dir. If that's enabled, a warning will still be thrown on each composer run, "WARNING: Composer is running in insecure mode!", etc, but it will not stop at the openssl check. We'll also make fallback to http:// and git:// dependent on insecure_mode being on. Additionally, we can have an environment variable, COMPOSER_INSECURE_MODE which would do the same thing if it's set.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok that's fine by me.


$options = $this->getOptionsForUrl($originUrl);
$ctx = StreamContextFactory::getContext($options, array('notification' => array($this, 'callbackGet')));
Expand All @@ -104,11 +110,22 @@ protected function get($originUrl, $fileUrl, $fileName = null, $progress = true)
$errorMessage = null;
set_error_handler(function ($code, $msg) use (&$errorMessage) {
$errorMessage = preg_replace('{^file_get_contents\(.+?\): }', '', $msg);
if ($this->ssl && strpos($errorMessage, 'SSL') !== false || strpos($errorMessage, 'enable crypto') !== false) {
throw new BadCryptoException($errorMessage, $code);
}
if (!ini_get('allow_url_fopen')) {
$errorMessage = 'allow_url_fopen must be enabled in php.ini ('.$errorMessage.')';
}
});
$result = file_get_contents($fileUrl, false, $ctx);
try {
$result = file_get_contents($fileUrl, false, $ctx);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Primary transfer doesnt appear to invoke any CN matching. CN_Match option should be set to string (its not a bool) and that string should be the host name.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you're right about it being a string -- unfortunately it still seems not to take into account the alternative names in the certificate. I tried with 'CN_match' => $this->fileUrlParts['host'], and it still fails for getcomposer.org and packagist.org because the CN is for dl.getcomposer.org and dl.packagist.org -- we need to check the alternative names, and it seems PHP doesn't do this. Perhaps I'm doing something wrong.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but then your catch gets invoked, and you check the SAN... as written, the first transfer doesnt even try to verify the CN, so will accept anything.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a PHP bug or feature omission. You can't rely on the CN_match setting reliably. For getcomposer.org, the CN is actually dl.getcomposer.org (mismatch from PHP's perspective since it ignores the valid SAN entry for getcomposer.org).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So either remove this first download attempt (or better, dont, see if CN_Match will work first)... and fall back to the SAN validation. (though Im not sure this method of SAN validation is ok yet.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just FYI, the dl.* should go away as soon as get my hands on a wildcard cert for the domains. It's probably a good idea to handle this case though, because the cert is valid for both dl.getcomposer.org and getcomposer.org, and works just fine in all browsers for example.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Seldaek The cert you have is perfectly valid. PHP just hasn't been updated to support Subject Alternative Names in line with common practices - something we noticed recently when digging into PHP's SSL/TLS support in depth. The current cert also works fine with Curl.

} catch (BadCryptoException $e) {
// SSL failed -- let's prompt the user about the certificate
$port = isset($this->fileUrlParts['port']) ? $this->fileUrlParts['port'] : 443;
$sslHelper = new SslHelper($this->io);
$sslHelper->verifySslCertificateFromServer($this->fileUrlParts['host'], $port, $fileUrl);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This verify operates on a separate stream context it seems. Verifying a connection unrelated to the actual transfer? This will probably lead to a client-profiling attack. (Eg let the cert check hit the real server, intercept the 2nd request)... this might need more review.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noted. @EvanDotPro, is it not possible to pluck the cert from the connection in use itself? There then wouldn't be a need for the second connection.

$result = file_get_contents($fileUrl, false, $ctx);
}
restore_error_handler();

// fix for 5.4.0 https://bugs.php.net/bug.php?id=61336
Expand Down Expand Up @@ -241,6 +258,18 @@ protected function getOptionsForUrl($originUrl)
$options['http']['header'] .= "Authorization: Basic $authStr\r\n";
}

if (!$this->ssl) return $options;

//if (getenv('COMPOSER_INSECURE_SSL_NOVERIFY') !== false) return $options;

$options['ssl'] = array(
'verify_peer' => true,
'allow_self_signed' => false,
'cafile' => SslHelper::initCaBundleFile(),
// PHP improperly handles CN_match, so it's handled manually in Composer\Util\SslHelper
//'CN_match' => true,
);

return $options;
}
}
147 changes: 147 additions & 0 deletions src/Composer/Util/SslHelper.php
@@ -0,0 +1,147 @@
<?php

/*
* This file is part of Composer.
*
* (c) Nils Adermann <naderman@naderman.de>
* Jordi Boggiano <j.boggiano@seld.be>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Composer\Util;

use Composer\Factory;
use Composer\IO\IOInterface;

/**
* @author Evan Coury <me@evancoury.com>
*/
class SslHelper
{
private $io;

public function __construct(IOInterface $io)
{
$this->io = $io;
$this->config = Factory::createConfig();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should inject the configuration instead of creating a new one.

}

public function verifySslCertificateFromServer($hostname, $port = 443, $url = false)
{
$chain = $this->fetchCertificateChain($hostname, $port);
$cert = $chain[0];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is a safe way to check the CN. I'd have to look into it more, but iirc the cert used to verify the connection is not always the last in the chain. I'm concerned that a self-signed cert after a valid cert for another domain, could be added to the end of the chain, pass the openssl verify, and then get to here and also pass this check. ... I might be wrong though and would have to look into the specifics of how openssl is validating the chain.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure myself at the moment. For those wondering why this is being done here, PHP doesn't check for both the CN and SAN entries, it only checks the CN. As a result, PHP's internal handling can fail a connection even on a valid certificate for the current Host which is a serious reliability issue since SANs have been accepted practice for years now. As a result, we need to increase reliability through some manual CN/SAN checking.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah -- I wasn't 100% on this part. I'd appreciate if someone more familiar with SSL could double check both how I'm assuming the CA / top certificate and also how I get and verify the CN / alternative names. The fact that I'm skip the intermediary certificates is almost certainly wrong; I'd appreciate any pointers or links o docs/specs that would give a clue how to do that part properly.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC openssl uses a scramble algorithm, (ie can it find any path to valid), and the order of the certs is almost irrelevant. See Moxie's TACK proposal for how they add a second chain to a TLS connection for a good practical example of this.... I'd have to source out where that algorithm is in code, but yes, I think this might be exploitable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes me wonder - what is PHP doing internally? Kevin, you ever look at C code? :P

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pretty sure they're just passing it on to openssl verify via the api. Could be wrong though. not looked at the c.

$certInfo = openssl_x509_parse($cert);
$commonName = $certInfo['subject']['CN'];
$caCert = $chain[count($chain)-1];
$caCertInfo = openssl_x509_parse($caCert);

// Check if the certificate has expired
$expired = false;
if ($certInfo['validTo_time_t'] < time()) {
$expired = true;
if (!$this->io->askConfirmation("<comment>WARNING! The SSL certificate for {$hostname} has expired.</comment>\n\nAre you sure you wish to continue with the expired SSL certificate? [y/N] ", false)) {
throw new BadCryptoException('Encountered expired SSL certificate; exiting on user command.');
}
}
$expires = date('Y-m-d', $certInfo['validTo_time_t']) . (($expired) ? ' (WARNING: Certificate has expired!)' : '');

// Proper CN_Match handling -- PHP's is broken
$alternativeNames = explode(', ', str_replace('DNS:', '', $certInfo['extensions']['subjectAltName']));
$validDomain = true;
if ($hostname != $commonName && !in_array($hostname, $alternativeNames)) {
$validDomain = false;
if (!$this->io->askConfirmation("<comment>WARNING! The SSL certificate at {$hostname} is NOT VALID for the requested hostname. " .
"This could be an indication of a man-in-the-middle attack " .
"or a misconfigured server. It is strongly recommended that you DO NOT continue.</comment>\n\n" .
"Are you sure you want to continue with the invalid SSL certificate? [y/N] ", false)) {
throw new BadCryptoException('Encountered invalid SSL certificate; exiting on user command.');
}
}

// Get a readable Certificate Authority name
if (isset($caCertInfo['subject']['O'], $caCertInfo['subject']['CN'])) {
$caName = "{$caCertInfo['subject']['O']} » {$caCertInfo['subject']['CN']}";
} elseif (isset($caCertInfo['issuer']['O'], $caCertInfo['issuer']['CN'])) {
$caName = "{$caCertInfo['issuer']['O']} » {$caCertInfo['issuer']['CN']}";
} else {
$caName = $caCertInfo['name'];
}

// Get the certificate fingerprint
openssl_x509_export($cert, $certString);
$fingerprint = $this->getSha1Fingerprint($certString);

$this->io->write("\n <error>#############################################################</error>");
$this->io->write(' <error># WARNING: Composer is trying to connect to a secure server #</error>');
$this->io->write(' <error># with an untrusted public key. Please review carefully. #</error>');
$this->io->write(" <error>#############################################################</error>\n");

if ($url) $this->io->write("<comment> Requested URL: {$url}</comment>");
$this->io->write("<comment> Hostname: {$hostname}</comment>");
$this->io->write('<comment> Common Name: ' . $commonName .
((count($alternativeNames) > 0) ? ' (' . implode(', ', $alternativeNames) . ')' : '') . '</comment>'); // show alt names
$this->io->write("<comment> Valid Until: {$expires}</comment>");
$this->io->write("<comment> Fingerprint: {$fingerprint}</comment>");
$this->io->write("<comment> Authority: {$caName}</comment>");

$this->io->write("\nTo verify the certificate fingerprint, go to https://{$hostname}/ in your web browser and view the certificate details.");

if ($this->io->askConfirmation("\nAre you sure you trust the Certificate Authority listed above and have verified the certificate fingerprint? [y/N] ", false)) {
openssl_x509_export($caCert, $caCertString);
file_put_contents(static::initCaBundleFile(), $caCertString, FILE_APPEND);
$this->io->write("<info> Added {$caName} as a trustworthy Certificate Authority.</info>");
return true;
}

throw new BadCryptoException('Encountered untrusted SSL certificate; exiting on user command.');
}

protected function getSha1Fingerprint($certificate)
{
$certificate = str_replace('-----BEGIN CERTIFICATE-----', '', $certificate);
$certificate = str_replace('-----END CERTIFICATE-----', '', $certificate);
$certificate = base64_decode($certificate);
$fingerprint = strtoupper(sha1($certificate));
$fingerprint = str_split($fingerprint, 2);
return implode(':', $fingerprint);
}

protected function fetchCertificateChain($hostname, $port)
{
$context = stream_context_create(array('ssl' => array('capture_peer_cert_chain' => true)));
$socket = stream_socket_client("ssl://{$hostname}:{$port}", $errno, $errstr, 30, STREAM_CLIENT_CONNECT, $context);
$params = stream_context_get_params($socket);
return $params['options']['ssl']['peer_certificate_chain'];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using a second context here and going for the chain... use the variable 'capture_peer_cert' and the 'peer_cert' response: see: http://lxr.php.net/xref/PHP_5_4/ext/openssl/xp_ssl.c#533

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty sure we'll need the whole chain rather than just the peer certificate to validate this properly. I should be able to do it without the second context in a way that each request is validated as to not allow for the client profiling attack mentioned above (great find, by the way).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't need it. The peer_cert has already been validated by openssl. See the C code. If you make sure to do the SAN check on the same CTX and make sure that verifypeer and allowselfsigned are set correctly, then you can be sure of the peer validation. Only check left to do is CN and SAN on the peer_cert. You dont need the chain (and really shouldnt look at it)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally Look at RFC 5280 for the untrusted/scramble algorithm. Its pretty complicated to validate a peer cert chain.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I didn't mean we need it to validate the cert. You're right, that's done by openssl. But in the event that it's not trusted yet, won't we need the chain to add the proper one to the trusted CA bundle before trying the request again?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@StormTide Do you have any links/resources about the fingerprint forgery you mentioned on twitter? Trying to think of how to improve the approach used in Sslurp. Seems to be quite a chicken-or-the-egg problem.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@EvanDotPro it really depends on the type of hash being used and the circumstances of the validation. Such attacks are known as preimage attacks and vary depending on the hash etc. Generally speaking they're pretty hard to accomplish in a sha1 hash cert. Since you're sha1'ing the cert manually, this might be ok, but generally speaking the cert fingerprint is not guranteed to be sha1, and could be a preimageable hash. (remember certs can sport many hash and signing algorithms)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's one reference https://www.schneier.com/blog/archives/2005/02/sha1_broken.html for collisions in SHA-1.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting stuff. So I'm trying to figure out how to allow verify_peer on the initial connection to mxr.mozila.org when we don't have any valid CA bundle yet. See my comment here. Thanks for all your help reviewing this, by the way!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's a bit tricky if the initial bundle can't be grabbed from a secure source. Any way getcomposer.org could perhaps host a secure copy and occasionally grab an updated copy and it's hash for distribution? Single point of failure, yes, but Composer can quickly establish HTTPS to getcomposer.org with an initial PEM from the installer so a secure route is now established? It's convoluted, I know!

}

public static function initCaBundleFile()
{
$config = Factory::createConfig();
$caBundleFile = $config->get('home') . '/trusted-ca-bundle.crt';

if (file_exists($caBundleFile)) {
return $caBundleFile;
}
touch($caBundleFile);

// Improve usability for some linux users by automatically
// importing the distro's CA bundle if it's found.
$caBundleLocations = array(
'/etc/pki/tls/certs/ca-bundle.crt',
'/usr/share/ssl/certs/ca-bubdle.crt',
);

foreach ($caBundleLocations as $caBundle) {
if (file_exists($caBundle) && is_readable($caBundle)) {
$trustedBundle = file_get_contents($caBundle);
break;
}
}

if (isset($trustedBundle)) file_put_contents($caBundleFile, $trustedBundle);

return $caBundleFile;
}
}