Skip to content
This repository

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

Closed
wants to merge 4 commits into from
Evan Coury

Disclaimer: I am not a security expert. This PR is mostly common sense, but should still be reviewed by the community before being merged, especially those well-versed in security.

This PR contains the following security fixes, and attempts to address issue #1074:

  • Self-update and packagist are now forced to use SSL.
  • This introduces a hard dependency on the openssl extension when using self-update or packagist. Users should not be using either without utilizing a proper SSL connection anyway.
  • Keeps a trusted CA bundle file in the composer home directory.
  • Attempts to copy over the operating system CA bundle if available.
  • Upon encountering an untrusted certificate, the user is prompted if they would like to trust the certificate and the CA in the future. If they choose yes, the root CA for that cert is added to the CA bundle in the composer home directory.
  • Expired certificates generate an extra warning and prompt.
  • Invalid common names generate an extra warning and prompt.

(I'll try to add unit tests where possible.)

added some commits September 09, 2012
Evan Coury Fix MITM vulnerability for issue #1074
- Self-update and packagist are now forced to use SSL.
- This introduces a hard dependency on the openssl extension when using
  self-update or packagist. Users should not be using either without
  utilizing a proper SSL connection anyway.
- Keeps a trusted CA bundle file in the composer home directory.
- Attempts to copy over the operating system CA bundle if available.
- Upon encountering an untrusted certificate, the user is prompted if
  they would like to trust the certificate and the CA in the future. If
  they choose yes, the root CA for that cert is added to the CA bundle
  in the composer home directory.
- Expired certificates generate an extra warning and prompt.
- Invalid common names generate an extra warning and prompt.
8cff840
Evan Coury Small cleanup and remove unused variable
- Removed $key var that was used for a certificate cache implementation
  which I've since removed.
- Moved whitespace outside of Symfony console color tags.

(Side note: the color tags aren't working for me in gnome-terminal for
some reason...)
24d587e
Evan Coury Improve console colors for SSL warnings. ed68598
Jordi Boggiano Seldaek commented on the diff September 10, 2012
src/Composer/Config.php
@@ -22,13 +22,13 @@ class Config
22 22
         'vendor-dir' => 'vendor',
23 23
         'bin-dir' => '{$vendor-dir}/bin',
24 24
         'notify-on-install' => true,
25  
-        'github-protocols' => array('git', 'https', 'http'),
  25
+        'github-protocols' => array('https', 'git'),
5
Jordi Boggiano Owner

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

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.

Jordi Boggiano Owner

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.

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.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Jordi Boggiano Seldaek commented on the diff September 10, 2012
src/Composer/Util/RemoteFilesystem.php
((5 lines not shown))
93 94
         $this->fileName = $fileName;
94 95
         $this->progress = $progress;
95 96
         $this->lastProgress = null;
  97
+        $this->ssl = ('https' === substr($fileUrl, 0, 5));
  98
+
  99
+        if (!extension_loaded('openssl') && $this->ssl) {
  100
+            throw new \RuntimeException('You must enable the openssl extension in your php.ini to load information from '.$fileUrl);
  101
+        }
7
Jordi Boggiano Owner

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.

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?

Jordi Boggiano Owner

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.

Jordi Boggiano Owner

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)?

Jordi Boggiano Owner

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

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.

Jordi Boggiano Owner

Ok that's fine by me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Dolf Schimmel

I think this PR is a great step towards making Composer more secure. I cannot help but notice though that a lot of the comments seem defensive, seemingly preferring less secure options. Given that things like these were not built in from the start, and esp. stuff like gpg signing was not included from the start, I have to wonder if it's secure by design, or that the intention is now to patch it up, hoping we cover enough to make it more or less secure?

Nils Adermann
Owner

There was simply no expectation of security at the beginning. To be entirely sure about the security of the code you run you will have to review it yourself. When using 3rd party libraries you always place some trust into those 3rd party libraries. The work being done now, is to make it more difficult for an attacker to get between you and that 3rd party. The responses aren't defensive, they are trying to make sure composer continues to actually work. Because what is the point of making it more secure if you can then no longer use it? Hence we need to come up with secure and working solutions like the proposed option for switching to a mode in which communication is not necessarily secure if that is not an option available on the respective system.

Matthew Weier O'Phinney

@naderman I totally understand your points. That said, your quote, "To be entirely sure about the security of the code you run you will have to review it yourself. When using 3rd party libraries you always place some trust into those 3rd party libraries," leaves this open: you need to be able to trust that the 3rd party library whose code you are reviewing is actually from the 3rd party you tried to obtain it from. That's the primary driver of this PR, from my standpoint as primarily a user, and secondarily somebody providing packages for others.

Nils Adermann
Owner

@weierophinney Yes absolutely, that's what I meant by "The work being done now, is to make it more difficult for an attacker to get between you and that 3rd party."

Kevin McArthur StormTide commented on the diff September 10, 2012
src/Composer/Util/SslHelper.php
((19 lines not shown))
  19
+ * @author Evan Coury <me@evancoury.com>
  20
+ */
  21
+class SslHelper
  22
+{
  23
+    private $io;
  24
+
  25
+    public function __construct(IOInterface $io)
  26
+    {
  27
+        $this->io = $io;
  28
+        $this->config = Factory::createConfig();
  29
+    }
  30
+
  31
+    public function verifySslCertificateFromServer($hostname, $port = 443, $url = false)
  32
+    {
  33
+        $chain      = $this->fetchCertificateChain($hostname, $port);
  34
+        $cert       = $chain[0];
6

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.

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.

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.

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.

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Kevin McArthur StormTide commented on the diff September 10, 2012
src/Composer/Util/RemoteFilesystem.php
((7 lines not shown))
107 116
             if (!ini_get('allow_url_fopen')) {
108 117
                 $errorMessage = 'allow_url_fopen must be enabled in php.ini ('.$errorMessage.')';
109 118
             }
110 119
         });
111  
-        $result = file_get_contents($fileUrl, false, $ctx);
  120
+        try {
  121
+            $result = file_get_contents($fileUrl, false, $ctx);
  122
+        } catch (BadCryptoException $e) {
  123
+            // SSL failed -- let's prompt the user about the certificate
  124
+            $port = isset($this->fileUrlParts['port']) ? $this->fileUrlParts['port'] : 443;
  125
+            $sslHelper = new SslHelper($this->io);
  126
+            $sslHelper->verifySslCertificateFromServer($this->fileUrlParts['host'], $port, $fileUrl);
2

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Kevin McArthur StormTide commented on the diff September 10, 2012
src/Composer/Util/RemoteFilesystem.php
((7 lines not shown))
107 116
             if (!ini_get('allow_url_fopen')) {
108 117
                 $errorMessage = 'allow_url_fopen must be enabled in php.ini ('.$errorMessage.')';
109 118
             }
110 119
         });
111  
-        $result = file_get_contents($fileUrl, false, $ctx);
  120
+        try {
  121
+            $result = file_get_contents($fileUrl, false, $ctx);
7

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.

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.

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.

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).

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.)

Jordi Boggiano Owner

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.

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Kevin McArthur StormTide commented on the diff September 10, 2012
src/Composer/Util/SslHelper.php
((101 lines not shown))
  101
+    protected function getSha1Fingerprint($certificate)
  102
+    {
  103
+        $certificate = str_replace('-----BEGIN CERTIFICATE-----', '', $certificate);
  104
+        $certificate = str_replace('-----END CERTIFICATE-----', '', $certificate);
  105
+        $certificate = base64_decode($certificate);
  106
+        $fingerprint = strtoupper(sha1($certificate));
  107
+        $fingerprint = str_split($fingerprint, 2);
  108
+        return implode(':', $fingerprint);
  109
+    }
  110
+
  111
+    protected function fetchCertificateChain($hostname, $port)
  112
+    {
  113
+        $context = stream_context_create(array('ssl' => array('capture_peer_cert_chain' => true)));
  114
+        $socket  = stream_socket_client("ssl://{$hostname}:{$port}", $errno, $errstr, 30, STREAM_CLIENT_CONNECT, $context);
  115
+        $params  = stream_context_get_params($socket);
  116
+        return $params['options']['ssl']['peer_certificate_chain'];
16

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

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).

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)

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

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?

Oh you're trying to override a peer validation failure too? Rather than have the users accept an insecure bundle, why not have them configure a secure bundle? (eg prompt the user to apt-get install ca-certificates ) I think manually adding CA roots might be a bad idea, especially given that they may need to be revoked.

That's actually a good point... I have no clue how that would work on Windows though.

On *nix, we can just check the common paths like /etc/ssl/certs/ca-certificates.crt, /etc/ssl/ca-bundle.pem, /etc/pki/tls/certs/ca-bundle.crt and /usr/share/ssl/certs/ca-bundle.crt and we have most everyone covered.

My original thought was that we show the user the sha1 fingerprint and strongly encourage them to verify that against what they get in their browser. (For a simpler process, we could even provide an https://getcomposer.org/ssl-fingerprint and https://packagist.org/ssl-fingerprint) but you're right that this could be bad if the certificates need to be revoked; ideally the root certs are installed via some mechanism that auto-updates.

The less than ideal solution if the host has no trust might be if its the composer url itself that is failing to validate (ie have no ca bundle at all), then bundle the current known composer public key in the phar. (chances are folks are going to experience this error shortly after downloading the latest version anyway) Then I'd add a new command, "composer build-ca", which will grab a known-good cacert.pem from https using the pre-known key. Install that for composer-use only, and go from there.

I'd also update the install instructions on and nuke the file_get_contents version as that will happily grab an insecure url. http://getcomposer.org/download/ (eg try php -r "echo('?>'.file_get_contents('https://www.hstscookie.ca'));") <-- that site has a self-signed ca.

Jordi Boggiano Owner

Again, this is just there for usability reasons, without it presently if you don't have unix utils installed on windows, you can't download composer via CLI. This one might be solvable by shipping a windows MSI installer though, at which point I would happily remove the eval hack.

As for what @EvanDotPro suggested (adding CA roots) AFAIK it's only added to ~/.composer/trusted-ca-bundle.crt, which is composer specific and will not affect anything else. Obviously it is still a problem if it contains revoked CAs, so I guess the build-ca or similar command would be a good solution and could be done as part of the installer script as well.

I've started building a small library for dealing with SSL in PHP that we can use for this... Currently the primary feature is building an up-to-date CA bundle from the Mozilla source tree.

If you wouldn't mind, take a minute to try out and review Sslurp. I'll add it to Packagist so we can include it in composer.

@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.

@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)

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

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!

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Jordi Boggiano
Owner

@Freeaqingme if you look at the comments here, you may notice that we've for many people deeply interested in security struggling to figure out how exactly to do validate an ssl connection in php. That is part of the reason the network security hasn't been addressed before: it's a huge pain and php is clearly not helping. As for the defensive comments, I was just trying to find a balance between security requirements and usability. Some people just can not enable openssl for various reasons (often bad ones IMO, but we can only nudge in the right direction), and it should not mean they can't use composer at all. That said, I'm glad we now have so many people interested in fixing this for good.

Kevin McArthur

No one should be using composer over http the way it is today without package signing. IMO It shouldnt even be an option. It's like voluntarily installing a backdoor on your machine.

The difficulty here comes from trying to use the built-in openssl stuff which, yes, requires some understanding of SSL to use... cURL could handle this easily, safely and securely with default options on the other hand.

Chris Kankiewicz

@Seldaek It's understandable that many people can't enable openssl, I know how policy and bureaucracy can get in the way of things. To address this, however, I believe composer should be secure by default and only be insecure if the user specifically configures it to be that way and throws lots of warnings in the users face when they do.

Pádraic Brady

@PHLAK That's what the PR is aiming for. A lot of what is now being discussed (several of us being security knowledgeable) is working through PHP's obstacle course. It's native SSL/TLS handling sucks compared to Curl which is defaulted to be secure and usually has a CA bundle PEM pre-configured on package repos.

Evan Coury

@PHLAK, @padraic, The nice thing is, with sslurp, Composer will be able to build and keep up-to-date the exact same CA bundle PEM that cURL uses -- it builds from the same source and in the same way as cURL's mk-ca-bundle.pl, except in a much more secure way (funny that cURL's build script fetches the CA bundle over HTTP).

Filip Procházka

@EvanDotPro your code needs a refactoring. Get rid of that autoload_* mess in root and refactor strings with values to contants and apply some conding standards.

Sorry for the OT.

Evan Coury

@HosipLan, I'll eventually get around to cleaning the code -- right now we are focusing on actual security and integrity of the data. If it's a problem for you right now, I'd gladly accept a pull request.

Christophe Coevoet stof commented on the diff September 11, 2012
src/Composer/Repository/ComposerRepository.php
@@ -39,7 +39,7 @@ public function __construct(array $repoConfig, IOInterface $io, Config $config)
39 39
     {
40 40
         if (!preg_match('{^\w+://}', $repoConfig['url'])) {
41 41
             // assume http as the default protocol
1
Christophe Coevoet
stof added a note September 11, 2012

you should update this comment :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Christophe Coevoet stof commented on the diff September 11, 2012
src/Composer/Util/SslHelper.php
((13 lines not shown))
  13
+namespace Composer\Util;
  14
+
  15
+use Composer\Factory;
  16
+use Composer\IO\IOInterface;
  17
+
  18
+/**
  19
+ * @author Evan Coury <me@evancoury.com>
  20
+ */
  21
+class SslHelper
  22
+{
  23
+    private $io;
  24
+
  25
+    public function __construct(IOInterface $io)
  26
+    {
  27
+        $this->io = $io;
  28
+        $this->config = Factory::createConfig();
1
Christophe Coevoet
stof added a note September 11, 2012

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Evan Coury

Status update: I'm just finishing up some stuff in sslurp, then I'll update this PR.

Sander Marechal

If #1177 is merged, perhaps this can be simplified? For example, if openssl support is enabled, default Composer\Config#defaultRepositories to something like:

public static $defaultRepositories = array(
    'packagist' => array(
        'type' => 'composer',
        'url' => 'https://packagist.org',
        'options' => array(
            'ssl' => array(
                'verify_peer' => true,
                'allow_self_signed' => false,
                'cafile' => SslHelper::initCaBundleFile(),
            ),
        ),
    ),
);

Then, if people have openssl enabled but for some reason want to force a plain HTTP connection, they can simply do this in their composer.json:

{
    "repositorries": [
        {
            "packagist": false,
        },
        {
            "type": "composer",
            "url": "http://packagist.org",
        }
    ]
}
Jordi Boggiano Seldaek referenced this pull request in composer/packagist October 05, 2012
Closed

Support SSL/TLS on packagist.org #160

Pádraic Brady

@sandermarechal You would also need to include the CN_match option in the SSL Context (to ensure the valid certificate is for the Host being requested over HTTPS). Secondly, you also need to replace all instances of file_get_contents() across Composer. Recommended security practice is to disable the allow_url_fopen setting in php.ini that Composer currently requires to be enabled. This means switching from file_get_contents() to a non-file op using sockets.

Jordi Boggiano
Owner

@StormTide eh well, not saying it was smart, but now that it's out there, we might as well take good note of it.

Jordi Boggiano
Owner

@StormTide ah, read in more details.. sorry to hear. Anyway if sslurp helps with that I'm glad.

Till!

What happened to this PR?

Can anyone summairze what the outstanding issues are? :)

Pádraic Brady

@till @EvanDotPro Anything needed to poke this along to completion? It's been five months so where do we stand at this point?

Evan Coury

As mentioned on Twitter, I'm looking to finally finish this up this month now that I have the time. I need some help reviewing Sslurp, as I want to make sure the method it uses for establishing a trusted certificate bundle is well-reviewed and agreed to be secure before I integrate it into Composer. I also want to look at fixing the installer script as well, because if they get a compromised copy of Composer in the first place, all this work is for nothing.

Till!

@EvanDotPro did you get anywhere? No idea if @padraic helped you review Sslurp.

Evan Coury

@till unfortunately, I shortly after SunshinePHP, I began to get pretty busy again. It's looking like there's beginning to be a renewed interest in this again though, and I have a new client interested in implementing composer into their workflow, so maybe with enough poking and prodding from the community, I can try to find some time to wrap this up once and for all. I haven't really gotten any official :+1:'s on Sslurp from known security professionals yet though, which would certainly be helpful.

/me looks at @padraic and @ircmaxell. :smile:

Evan Coury EvanDotPro closed this October 04, 2013
Evan Coury EvanDotPro reopened this October 04, 2013
Evan Coury

(sorry, didn't mean to close and reopen...)

Martin Hlaváč

@EvanDotPro would it make sense to crowdfund this for you? Because this is really important issues which has to be solved. I believe a lot of developers would help you to get your time paid so you can find some time to do it.

Also there are many groups in PHP world which would benefit from this pull request, like Zend, Sensio and everyone who uses composer to install their dependencies which are then used on production servers (this is major issue, because lot of people uses CI to automatically install dependencies and they then deploy these to production directly).

Evan Coury

Closing this per @padraic's awesome contributions lately which have addressed most of these concerns!

Thank you so much @padraic for taking the time to do this!

Evan Coury EvanDotPro closed this March 05, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 4 unique commits by 1 author.

Sep 09, 2012
Evan Coury Fix MITM vulnerability for issue #1074
- Self-update and packagist are now forced to use SSL.
- This introduces a hard dependency on the openssl extension when using
  self-update or packagist. Users should not be using either without
  utilizing a proper SSL connection anyway.
- Keeps a trusted CA bundle file in the composer home directory.
- Attempts to copy over the operating system CA bundle if available.
- Upon encountering an untrusted certificate, the user is prompted if
  they would like to trust the certificate and the CA in the future. If
  they choose yes, the root CA for that cert is added to the CA bundle
  in the composer home directory.
- Expired certificates generate an extra warning and prompt.
- Invalid common names generate an extra warning and prompt.
8cff840
Sep 10, 2012
Evan Coury Small cleanup and remove unused variable
- Removed $key var that was used for a certificate cache implementation
  which I've since removed.
- Moved whitespace outside of Symfony console color tags.

(Side note: the color tags aren't working for me in gnome-terminal for
some reason...)
24d587e
Evan Coury Improve console colors for SSL warnings. ed68598
Evan Coury Change default composer repository protocol to https afd84db
This page is out of date. Refresh to see the latest.
4  src/Composer/Command/SelfUpdateCommand.php
@@ -42,12 +42,12 @@ protected function configure()
42 42
     protected function execute(InputInterface $input, OutputInterface $output)
43 43
     {
44 44
         $rfs = new RemoteFilesystem($this->getIO());
45  
-        $latest = trim($rfs->getContents('getcomposer.org', 'http://getcomposer.org/version', false));
  45
+        $latest = trim($rfs->getContents('getcomposer.org', 'https://getcomposer.org/version', false));
46 46
 
47 47
         if (Composer::VERSION !== $latest) {
48 48
             $output->writeln(sprintf("Updating to version <info>%s</info>.", $latest));
49 49
 
50  
-            $remoteFilename = 'http://getcomposer.org/composer.phar';
  50
+            $remoteFilename = 'https://getcomposer.org/composer.phar';
51 51
             $localFilename = $_SERVER['argv'][0];
52 52
             $tempFilename = basename($localFilename, '.phar').'-temp.phar';
53 53
 
4  src/Composer/Config.php
@@ -22,13 +22,13 @@ class Config
22 22
         'vendor-dir' => 'vendor',
23 23
         'bin-dir' => '{$vendor-dir}/bin',
24 24
         'notify-on-install' => true,
25  
-        'github-protocols' => array('git', 'https', 'http'),
  25
+        'github-protocols' => array('https', 'git'),
26 26
     );
27 27
 
28 28
     public static $defaultRepositories = array(
29 29
         'packagist' => array(
30 30
             'type' => 'composer',
31  
-            'url' => 'http://packagist.org',
  31
+            'url' => 'https://packagist.org',
32 32
         )
33 33
     );
34 34
 
2  src/Composer/Repository/ComposerRepository.php
@@ -39,7 +39,7 @@ public function __construct(array $repoConfig, IOInterface $io, Config $config)
39 39
     {
40 40
         if (!preg_match('{^\w+://}', $repoConfig['url'])) {
41 41
             // assume http as the default protocol
42  
-            $repoConfig['url'] = 'http://'.$repoConfig['url'];
  42
+            $repoConfig['url'] = 'https://'.$repoConfig['url'];
43 43
         }
44 44
         $repoConfig['url'] = rtrim($repoConfig['url'], '/');
45 45
         if (function_exists('filter_var') && version_compare(PHP_VERSION, '5.3.3', '>=') && !filter_var($repoConfig['url'], FILTER_VALIDATE_URL)) {
20  src/Composer/Util/BadCryptoException.php
... ...
@@ -0,0 +1,20 @@
  1
+<?php
  2
+
  3
+/*
  4
+ * This file is part of Composer.
  5
+ *
  6
+ * (c) Nils Adermann <naderman@naderman.de>
  7
+ *     Jordi Boggiano <j.boggiano@seld.be>
  8
+ *
  9
+ * For the full copyright and license information, please view the LICENSE
  10
+ * file that was distributed with this source code.
  11
+ */
  12
+
  13
+namespace Composer\Util;
  14
+
  15
+/**
  16
+ * @author Evan Coury <me@evancoury.com>
  17
+ */
  18
+class BadCryptoException extends \Exception
  19
+{
  20
+}
31  src/Composer/Util/RemoteFilesystem.php
@@ -90,9 +90,15 @@ protected function get($originUrl, $fileUrl, $fileName = null, $progress = true)
90 90
         $this->result = null;
91 91
         $this->originUrl = $originUrl;
92 92
         $this->fileUrl = $fileUrl;
  93
+        $this->fileUrlParts = parse_url($fileUrl);
93 94
         $this->fileName = $fileName;
94 95
         $this->progress = $progress;
95 96
         $this->lastProgress = null;
  97
+        $this->ssl = ('https' === substr($fileUrl, 0, 5));
  98
+
  99
+        if (!extension_loaded('openssl') && $this->ssl) {
  100
+            throw new \RuntimeException('You must enable the openssl extension in your php.ini to load information from '.$fileUrl);
  101
+        }
96 102
 
97 103
         $options = $this->getOptionsForUrl($originUrl);
98 104
         $ctx = StreamContextFactory::getContext($options, array('notification' => array($this, 'callbackGet')));
@@ -104,11 +110,22 @@ protected function get($originUrl, $fileUrl, $fileName = null, $progress = true)
104 110
         $errorMessage = null;
105 111
         set_error_handler(function ($code, $msg) use (&$errorMessage) {
106 112
             $errorMessage = preg_replace('{^file_get_contents\(.+?\): }', '', $msg);
  113
+            if ($this->ssl && strpos($errorMessage, 'SSL') !== false || strpos($errorMessage, 'enable crypto') !== false) {
  114
+                throw new BadCryptoException($errorMessage, $code);
  115
+            }
107 116
             if (!ini_get('allow_url_fopen')) {
108 117
                 $errorMessage = 'allow_url_fopen must be enabled in php.ini ('.$errorMessage.')';
109 118
             }
110 119
         });
111  
-        $result = file_get_contents($fileUrl, false, $ctx);
  120
+        try {
  121
+            $result = file_get_contents($fileUrl, false, $ctx);
  122
+        } catch (BadCryptoException $e) {
  123
+            // SSL failed -- let's prompt the user about the certificate
  124
+            $port = isset($this->fileUrlParts['port']) ? $this->fileUrlParts['port'] : 443;
  125
+            $sslHelper = new SslHelper($this->io);
  126
+            $sslHelper->verifySslCertificateFromServer($this->fileUrlParts['host'], $port, $fileUrl);
  127
+            $result = file_get_contents($fileUrl, false, $ctx);
  128
+        }
112 129
         restore_error_handler();
113 130
 
114 131
         // fix for 5.4.0 https://bugs.php.net/bug.php?id=61336
@@ -241,6 +258,18 @@ protected function getOptionsForUrl($originUrl)
241 258
             $options['http']['header'] .= "Authorization: Basic $authStr\r\n";
242 259
         }
243 260
 
  261
+        if (!$this->ssl) return $options;
  262
+
  263
+        //if (getenv('COMPOSER_INSECURE_SSL_NOVERIFY') !== false) return $options;
  264
+
  265
+        $options['ssl'] = array(
  266
+            'verify_peer'       => true,
  267
+            'allow_self_signed' => false,
  268
+            'cafile'            => SslHelper::initCaBundleFile(),
  269
+            // PHP improperly handles CN_match, so it's handled manually in Composer\Util\SslHelper
  270
+            //'CN_match'          => true,
  271
+        );
  272
+
244 273
         return $options;
245 274
     }
246 275
 }
147  src/Composer/Util/SslHelper.php
... ...
@@ -0,0 +1,147 @@
  1
+<?php
  2
+
  3
+/*
  4
+ * This file is part of Composer.
  5
+ *
  6
+ * (c) Nils Adermann <naderman@naderman.de>
  7
+ *     Jordi Boggiano <j.boggiano@seld.be>
  8
+ *
  9
+ * For the full copyright and license information, please view the LICENSE
  10
+ * file that was distributed with this source code.
  11
+ */
  12
+
  13
+namespace Composer\Util;
  14
+
  15
+use Composer\Factory;
  16
+use Composer\IO\IOInterface;
  17
+
  18
+/**
  19
+ * @author Evan Coury <me@evancoury.com>
  20
+ */
  21
+class SslHelper
  22
+{
  23
+    private $io;
  24
+
  25
+    public function __construct(IOInterface $io)
  26
+    {
  27
+        $this->io = $io;
  28
+        $this->config = Factory::createConfig();
  29
+    }
  30
+
  31
+    public function verifySslCertificateFromServer($hostname, $port = 443, $url = false)
  32
+    {
  33
+        $chain      = $this->fetchCertificateChain($hostname, $port);
  34
+        $cert       = $chain[0];
  35
+        $certInfo   = openssl_x509_parse($cert);
  36
+        $commonName = $certInfo['subject']['CN'];
  37
+        $caCert     = $chain[count($chain)-1];
  38
+        $caCertInfo = openssl_x509_parse($caCert);
  39
+
  40
+        // Check if the certificate has expired
  41
+        $expired = false;
  42
+        if ($certInfo['validTo_time_t'] < time()) {
  43
+            $expired = true;
  44
+            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)) {
  45
+                throw new BadCryptoException('Encountered expired SSL certificate; exiting on user command.');
  46
+            }
  47
+        }
  48
+        $expires = date('Y-m-d', $certInfo['validTo_time_t']) . (($expired) ? ' (WARNING: Certificate has expired!)' : '');
  49
+
  50
+        // Proper CN_Match handling -- PHP's is broken
  51
+        $alternativeNames = explode(', ', str_replace('DNS:', '', $certInfo['extensions']['subjectAltName']));
  52
+        $validDomain = true;
  53
+        if ($hostname != $commonName && !in_array($hostname, $alternativeNames)) {
  54
+            $validDomain = false;
  55
+            if (!$this->io->askConfirmation("<comment>WARNING! The SSL certificate at {$hostname} is NOT VALID for the requested hostname. " .
  56
+                "This could be an indication of a man-in-the-middle attack " .
  57
+                "or a misconfigured server. It is strongly recommended that you DO NOT continue.</comment>\n\n" .
  58
+                "Are you sure you want to continue with the invalid SSL certificate? [y/N] ", false)) {
  59
+                throw new BadCryptoException('Encountered invalid SSL certificate; exiting on user command.');
  60
+            }
  61
+        }
  62
+
  63
+        // Get a readable Certificate Authority name
  64
+        if (isset($caCertInfo['subject']['O'], $caCertInfo['subject']['CN'])) {
  65
+            $caName = "{$caCertInfo['subject']['O']} » {$caCertInfo['subject']['CN']}";
  66
+        } elseif (isset($caCertInfo['issuer']['O'], $caCertInfo['issuer']['CN'])) {
  67
+            $caName = "{$caCertInfo['issuer']['O']} » {$caCertInfo['issuer']['CN']}";
  68
+        } else {
  69
+            $caName = $caCertInfo['name'];
  70
+        }
  71
+
  72
+        // Get the certificate fingerprint
  73
+        openssl_x509_export($cert, $certString);
  74
+        $fingerprint = $this->getSha1Fingerprint($certString);
  75
+
  76
+        $this->io->write("\n <error>#############################################################</error>");
  77
+        $this->io->write(' <error># WARNING: Composer is trying to connect to a secure server #</error>');
  78
+        $this->io->write(' <error>#   with an untrusted public key. Please review carefully.  #</error>');
  79
+        $this->io->write(" <error>#############################################################</error>\n");
  80
+
  81
+        if ($url) $this->io->write("<comment> Requested URL: {$url}</comment>");
  82
+        $this->io->write("<comment>      Hostname: {$hostname}</comment>");
  83
+        $this->io->write('<comment>   Common Name: ' . $commonName .
  84
+            ((count($alternativeNames) > 0) ? ' (' . implode(', ', $alternativeNames) . ')' : '') . '</comment>'); // show alt names
  85
+        $this->io->write("<comment>   Valid Until: {$expires}</comment>");
  86
+        $this->io->write("<comment>   Fingerprint: {$fingerprint}</comment>");
  87
+        $this->io->write("<comment>     Authority: {$caName}</comment>");
  88
+
  89
+        $this->io->write("\nTo verify the certificate fingerprint, go to https://{$hostname}/ in your web browser and view the certificate details.");
  90
+
  91
+        if ($this->io->askConfirmation("\nAre you sure you trust the Certificate Authority listed above and have verified the certificate fingerprint? [y/N] ", false)) {
  92
+            openssl_x509_export($caCert, $caCertString);
  93
+            file_put_contents(static::initCaBundleFile(), $caCertString, FILE_APPEND);
  94
+            $this->io->write("<info> Added {$caName} as a trustworthy Certificate Authority.</info>");
  95
+            return true;
  96
+        }
  97
+
  98
+        throw new BadCryptoException('Encountered untrusted SSL certificate; exiting on user command.');
  99
+    }
  100
+
  101
+    protected function getSha1Fingerprint($certificate)
  102
+    {
  103
+        $certificate = str_replace('-----BEGIN CERTIFICATE-----', '', $certificate);
  104
+        $certificate = str_replace('-----END CERTIFICATE-----', '', $certificate);
  105
+        $certificate = base64_decode($certificate);
  106
+        $fingerprint = strtoupper(sha1($certificate));
  107
+        $fingerprint = str_split($fingerprint, 2);
  108
+        return implode(':', $fingerprint);
  109
+    }
  110
+
  111
+    protected function fetchCertificateChain($hostname, $port)
  112
+    {
  113
+        $context = stream_context_create(array('ssl' => array('capture_peer_cert_chain' => true)));
  114
+        $socket  = stream_socket_client("ssl://{$hostname}:{$port}", $errno, $errstr, 30, STREAM_CLIENT_CONNECT, $context);
  115
+        $params  = stream_context_get_params($socket);
  116
+        return $params['options']['ssl']['peer_certificate_chain'];
  117
+    }
  118
+
  119
+    public static function initCaBundleFile()
  120
+    {
  121
+        $config = Factory::createConfig();
  122
+        $caBundleFile = $config->get('home') . '/trusted-ca-bundle.crt';
  123
+
  124
+        if (file_exists($caBundleFile)) {
  125
+            return $caBundleFile;
  126
+        }
  127
+        touch($caBundleFile);
  128
+
  129
+        // Improve usability for some linux users by automatically
  130
+        // importing the distro's CA bundle if it's found.
  131
+        $caBundleLocations = array(
  132
+            '/etc/pki/tls/certs/ca-bundle.crt',
  133
+            '/usr/share/ssl/certs/ca-bubdle.crt',
  134
+        );
  135
+
  136
+        foreach ($caBundleLocations as $caBundle) {
  137
+            if (file_exists($caBundle) && is_readable($caBundle)) {
  138
+                $trustedBundle = file_get_contents($caBundle);
  139
+                break;
  140
+            }
  141
+        }
  142
+
  143
+        if (isset($trustedBundle)) file_put_contents($caBundleFile, $trustedBundle);
  144
+
  145
+        return $caBundleFile;
  146
+    }
  147
+}
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.