Skip to content

Conversation

@EvanDotPro
Copy link

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

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

@Freeaqingme
Copy link

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?

@naderman
Copy link
Member

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.

@weierophinney
Copy link
Contributor

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

@naderman
Copy link
Member

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

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.

@b-durand
Copy link

👍

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.

@Seldaek
Copy link
Member

Seldaek commented Sep 10, 2012

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

@StormTide
Copy link

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.

@ghost ghost mentioned this pull request Sep 11, 2012
@PHLAK
Copy link

PHLAK commented Sep 11, 2012

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

@padraic
Copy link
Contributor

padraic commented Sep 11, 2012

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

@EvanDotPro
Copy link
Author

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

@fprochazka
Copy link
Contributor

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

@EvanDotPro
Copy link
Author

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

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

@EvanDotPro
Copy link
Author

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

@sandermarechal
Copy link
Contributor

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",
        }
    ]
}

@padraic
Copy link
Contributor

padraic commented Oct 16, 2012

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

@Seldaek
Copy link
Member

Seldaek commented Oct 26, 2012

If anyone likes to do some reading ;) https://crypto.stanford.edu/~dabo/pubs/abstracts/ssl-client-bugs.html

@StormTide
Copy link

@Seldaek http://www.unrest.ca/responsible-disclosure-and-the-academy .... if anyone wants to do some reading ;)

@Seldaek
Copy link
Member

Seldaek commented Oct 26, 2012

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

@Seldaek
Copy link
Member

Seldaek commented Oct 26, 2012

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

@till
Copy link
Contributor

till commented Feb 3, 2013

What happened to this PR?

Can anyone summairze what the outstanding issues are? :)

@padraic
Copy link
Contributor

padraic commented Feb 6, 2013

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

@EvanDotPro
Copy link
Author

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
Copy link
Contributor

till commented Sep 25, 2013

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

@EvanDotPro
Copy link
Author

@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 👍's on Sslurp from known security professionals yet though, which would certainly be helpful.

/me looks at @padraic and @ircmaxell. 😄

@EvanDotPro EvanDotPro closed this Oct 5, 2013
@EvanDotPro EvanDotPro reopened this Oct 5, 2013
@EvanDotPro
Copy link
Author

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

@mhlavac
Copy link

mhlavac commented Oct 6, 2013

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

@EvanDotPro
Copy link
Author

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!

@EvanDotPro EvanDotPro closed this Mar 5, 2014
@rubensayshi
Copy link

@EvanDotPro so to go back on an old issue, but could you reference the stuff you're talking about here?

@bd808
Copy link
Contributor

bd808 commented Nov 28, 2015

@rubensayshi wrote:

could you reference the stuff you're talking about here?

See PR #2745 (which also seems to have stalled out)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.