Verify peer on HTTPS if cURL available (but be careful of built-in cert bundles in the codebase)

I was asking in relation to OpenID discovery and HTTPS redirects during that discovery. I dived into the JanRain OpenID and there is nothing like this. But it makes sense to me to verify HTTPS connections against trusted roots and give possibility to users/site admins to extend these trusted roots - as every common internet browser does these days.

I wasn't right with the JanRain, it uses curl to verify hosts - so it uses actually the same trusted roots as Heine included in his patch. But unfortunately JanRain library uses curl HTTPS verification in a bad way - see https://github.com/openid/php-openid/issues#issue/38.

And also this needs to be backported to Drupal 6 IMO.

I think we should make this a feature of drupal_http_request() rather than something used specifically for OpenID.

I doubt many users will change this setting, so I think it is sufficient to point to one .crt file that is distributed with Drupal. The file name can be wrapped in a variable_get('ca_bundle', DOCUMENT_ROOT . 'misc/ca_bundle.crt') call so that people wishing to change the file can copy the original file into a different location and edit it, or possibly point to a central file on their system. In the latter case it would be relevant to support both cafile and capath.

Why is the .crt file called curl-ca-bundle.crt? It isn't specifically connected to curl, is it?

+1 for more general solution as c960657 pointed out. I will rather place it to drupal_http_request too.

@Heine: Will the general - core library level - solution cause some significant drawback?

@c960657: Heine took the bundle from the CURL library

as to the API and UNICES:

I think we have two ways:

1) simple: allow user to override default CRT file with his own like c960657 mentioned in #4 #1081192-4: Verify peer on HTTPS if cURL available (but be careful of built-in cert bundles in the codebase)

2) more robust: provide API similar to drupal_add_css() which will allow modules to add their own certificates. For better performance, there could be cached collection of certificates in DB or in a file. But I guess it will be dangerous to store cached certificates in tmp folder with 777 permissions. (Or am I too paranoid?)... This will create place for more advanced work with certificates. I could imagine some contrib modules like e.g. Root Certificates UI for certificates administration...

Anyway both alternatives leads to implement this as default context inside drupal_http_request if no $context is specified and HTTPS scheme is detected IMO.

Trying to move it to the drupal_http_request. I basically took Heine's patch, roll back his changes in openid and I moved ca-bundle.crt to the /misc directory.

Tested sucessfully:

HTTP:

http://seznam.cz => 
   'request' => 'GET / HTTP/1.0
   'data' => 'Some data',
   'protocol' => 'HTTP/1.1',
   'status_message' => 'OK',
   ...
   'code' => '200',
   'redirect_code' => '302',
   'redirect_url' => 'http://www.seznam.cz/',
))

HTTPS - untrusted root certificate:

https://domain-with-untrusted-cert.cz => stdClass::__set_state(array(
   'code' => 0,
   'error' => 'Error opening socket ssl://domain-with-untrusted-cert.cz:443',
))

HTTPS - trusted root certificate:

https://ssl.aukro.cz/code/ => 
   'data' => 'Some data',
   'protocol' => 'HTTP/1.1',
   'status_message' => 'OK',
   ...
   'code' => '200',
))

When I append untrusted certificate to the ca-bundle:

https://domain-with-untrusted-cert.cz => 
   'request' => 'GET / HTTP/1.0
  ...
   'data' => 'Some data',
   'protocol' => 'HTTP/1.1',
   'status_message' => 'OK',
   ...
   'code' => '200',
))

Using variable_get('ca_bundle') as suggested c960657 in #4 #1081192-4: Verify peer on HTTPS if cURL available (but be careful of built-in cert bundles in the codebase).

Following text added to the default.settings.php:

/**
 * Root certificates override:
 *
 * Drupal is using a bundle of X.509 certificates of public Certificate
 * Authorities (CA) borrowed from the CURL library, which were extracted from
 * Mozilla's root certificates file (certdata.txt). It contains the certificates
 * in PEM format and Drupal is using them by default when PHP is compiled with
 * OpenSSL support for SSL client authentication (e.g. when connecting to HTTPS
 * using drupal_http_request()). You can override the default CA certificates
 * bundle with your own bundle - in most cases with appended trusted
 * authorities which aren't included.
 *
 * Remove the leading hash sign to enable the setting and set the relative path
 * to the file from the Drupal root directory. It is recommended to set the file
 * to be read-only for the webserver after appending your custom certificates.
 *
 * Sample Linux command to set your custom CA bundle to be read-only:
 *       chmod go-w path/to/ca-bundle.crt
 */
# $conf['ca_bundle'] = 'path/to/ca-bundle.crt';

@Heine: Ok, so "my approach" for 8.x dev and "your approach" for 7.x dev? But even in your case - openid only - we need some settings to allow override the default ca bundle IMO.

I don't think it is too big change, especially if it will be limited to openid module only - remember e.g. the semaphore introduced in 6.16.

Changing drupal_http_request() to enforce strict SSL checking is probably too late for D7. But how about adding a new option to the option array, e.g. $option['secure_ssl'], as a short-hand for this blurb:

     $context = stream_context_create(array(
      'ssl' => array(
        'verify_peer' => TRUE,
        'allow_self_signed' => FALSE,
        'cafile' => drupal_get_path('module','openid') .'/ca/curl-ca-bundle.crt'),
       )
    );

The new option should default to FALSE in D7 but possibly to TRUE in D8.

* Sample Linux command to set your custom CA bundle to be read-only:
*       chmod go-w path/to/ca-bundle.crt

I think this is taking the explanation a bit too far. The settings.php file itself should be kept read-only, but we don't go into detail about how to do that.

Would it make sense to allow $conf['ca_bundle'] to be an absolute path? This would allow the usage of file in a central location, e.g. provided by the OS distribution.

@c960657 interesting thoughts

BTW interesting topic to this - detecting compromited certificates:
https://blog.torproject.org/blog/detecting-certificate-authority-comprom...

Patch for D8, all three suggestions from #11 #1081192-11: Verify peer on HTTPS if cURL available (but be careful of built-in cert bundles in the codebase) included.
- ca-bundle path is now absolute
- introduced new option $option['secure_ssl'] which defaults to TRUE for D8, but it should defaults to FALSE for D7.

Gosh. I'd _really_ love to get rid of drupal_http_request() and rely on something provided by PHP. Should we make CURL a requirement?

@Dries I basically agree, but CURL isn't always available on shared hostings. Questions is do we really want to Drupal be hosted on such hostings without CURL? And will exist in two-three years horizont any reasonable PHP hosting without CURL support?

I don't think that removing dhr() would be a wise direction/decision. Not sure why you think we should get rid of it, @Dries.

cURL-over-PHP has its very own flaws, requiring deep understanding of a couple of things. dhr() itself contains some bits, HTTP request handling methods in DrupalWebTestCase contain some others. dhr() allows the average Drupal coder to easily do stuff without necessarily understanding all of the major things under the hood.

Tagging issues not yet using summary template.

#13: d8_certificates_api_1081192.patch queued for re-testing.

Reroll from #13 with just little cosmetic change. Returning to needs review status based on comment #16 by sun.

Adding root certificates without an easy and quick way to revoke is a particularly unwise idea. cURL can handle with OS updates. We can't. Example:

DigiNotar Root CA

So while cURL might be more complex I dont see another easy way out.

@chx Might be we can provide one built-in bundle which will be included automatically + give option to site admin add own CA file.

So if another "DigiNotar Root CA" cause happen we can release Drupal security update, user defined certificates will be unaffected. And also user defined certificates will be user's responsibility... the same responsibility as not-updated drupal site.

Anyway even if the one certificate from the bundle will be compromised, the provided solution will be still better than the current state, when Drupal totally don't care about https security.

Or would be better to import the certificates to the database table? I prefer the config file, but I'm just curious if someone finds some kind of admin UI useful to manage the certificates.

To clarify: the proposal is to add peer verify only if you have cURL. We still work w/o cURL just less secure.

So if another "DigiNotar Root CA" cause happen we can release Drupal security update

In general this idea feels tough for us to do. It increases the number of things we have to pay attention to and do an SA about. It's also the kind of thing where we would get the news *after* something bad happened so it would encourage us to do a release very quickly which is not great.

I think chx' proposal in #23 is a solid idea ;)

Here's a crack at chx's suggestion in #23, untested. PHP reference: http://php.net/manual/en/context.curl.php.

Moving back to D7 as D8 uses Guzzle now.

As far as I can tell, Guzzle does this by default. However, it does so using a cacert.pem file that is shipped with the Guzzle codebase. So basically, this is the exact scenario @chx and @greggles were concerned about above?

Moving back to D8 for discussion of whether Drupal actually needs to override that...

I believe the file is from mozilla so we could track it.

https://github.com/guzzle/guzzle/issues/280
https://forums.aws.amazon.com/message.jspa?messageID=400240
http://guzzlephp.org/tour/http.html#configuration-options

Using the system cert instead of the bundled one might be a way around this issue 'ssl.certificate_authority' => 'system',

Great find, mikeytown2. I'd say setting it to system makes more sense than shipping our own.

Updated issue summary as part of drupalofficehours.org, with advice from xjm.

25: verify-ssl-peer-1081192-25.patch queued for re-testing.

This does not need a reroll , it needs a re think because it has been replaced by guzzle.

I don't think we can ship with the bundled guzzle cacert.pem, so this makes the change suggested by @mikeytown2 #28 and as per http://guzzle.readthedocs.org/en/latest/http-client/client.html#configur...

Bumping to critical as security follow-ups/issues are always critical.

If someone wants to use their own, they can implement ServiceModifierInterface and alter the arguments to nominate an absolute path as per the documentation.

Yes, we should not use the Guzzle shipped cacert.pem. I guess there is no meaningful way we can automatically test this?

Is there an upstream issue/ticket to use the system certificate file per default?

Awesome patch!

Not pushing back to "needs work" but could we add a comment above that line with some of the considerations in this issue. I.e. if someone swaps out the http_client they might not be aware of the implications. For a security feature this is pretty hidden away.

klausi opened a new pull request for this issue.

Like this? Suggestions for better wording welcome.

Yay! That's exactly the reason why I didn't propose an example description: I couldn't have described it this precisely.

The comment looks good to me. It explains why this setting is made.

I opened https://github.com/guzzle/guzzle/issues/623 against guzzle.

Committed/pushed to 8.x, thanks! Moving to 7.x for backport.

Just testing the D7 Backport. Looks like the patch doesn't apply because it was created with /core/ in the path.
If you drop /core/ it applies just fine. Haven't yet given it a bad cert to test.

@selfuntitled: What patch did you use for D7? I don't see one included here, and the one from #38 does not apply (there is no core.services.yml file).

Is this all that's needed? It turns on curl_verify_ssl_peer and ssl['verify_peer'] in the context when using https connections. FYI PHP 5.6 enables verify_peer and verify_peer_name by default.

The $options['context'] has to be an actual resource object created by stream_context_create(), not just an array.

I have no idea why my patch in #46 would case FileNameMungingTest->testMunging() to fail?

Ok, it worked the second time. Go figure.

The latest patch no longer sets verify_peer to true in any cases. Is that your intent?

Whoops, sorry, copy/paste mixup.

@DamienMcKenna - I was working from the D7 patch by nedjo which failed testing in comment #25.

This was almost a year ago that I was playing with this, and my memory is that, for whatever reason, the patch didn't solve the problem and my client didn't want to pay for me to troubleshoot, so I just implemented what I needed directly with curl and didn't bother with drupal_http_request.

If I get some further time, I'd love to poke at the D7 backport of this, but haven't thought to look at it since.

Edit - just reviewing your other comments and patch, I'm happy to test with my use case from last year.

How about making the SSL context options configurable via a variable?

This would allow existing sites to opt-out of this change, and those moving to PHP 5.6 can be set up in a way that's backwards compatible with the way they used to work - for example being able to accept self signed certificates.

Attached is a patch which does the same thing as that from #52, but uses a variable to make the options configurable.

This makes it possible to do something like this in settings.php

$conf['drupal_ssl_context_options'] = array('verify_peer_name' => FALSE);

Yeah, I don't think it should be overridable globally - just by the caller of drupal_http_request(). In the patch in #52 it is, although the method for doing so is awkward; however it is not a common need so it's probably OK if we just document it.

Questions:

1. Should we be setting 'verify_peer_name' to TRUE and 'allow_self_signed' to FALSE as well?
2. The earlier patches (for example #45) set CURL options also (e.g. 'curl_verify_ssl_peer' => TRUE) but that was removed in #52. Is it not necessary?
3. Are we OK with making verification the default behavior? For better backwards-compatibility this would be opt-in (as was also proposed earlier in the thread); however for better security it would be opt-out. I guess the fact that PHP 5.6 changes the default to opt-out is an argument in favor of Drupal doing it too, even in a stable release.

This is not only an issue of incorrect certificates developers may use. Nearly all installations are affected except a few systems with updated ca list.

The root cause is cURL that was distributed too long with bad default ca list. My dev machine has xampp with php 5.6 and fails to request recaptcha webservice. As guzzle uses curl in most cases this means ssl peer verification causes fails on nearly every system. I have not disabled peer verification and documented this at Known issue with cURL and outdated root certificates.

I hope and support adding the latest ca to core to get this issues resolved by default.

In PHP < 5.6 verify_peer default was FALSE, verify_peer_name didn't exist.
in PHP 5.6 verify_peer has been changed to default to TRUE, verify_peer_name has been introduced and the default set to TRUE.

So to enable certificate verification on an old system with PHP <5.6 we must set verify_peer to TRUE. For systems >=5.6 the defaults should be fine.

We could add a check to hook_requirements to inform people about their status.

We should probably add a CN_match for PHP <5.6 which is the ancestor of peer_name in PHP >=5.6
See http://php.net/manual/en/context.ssl.php

I'm not sure the summary reflects the discussion or patch for Drupal 7?

It's been a year, maybe we can get this finished off?

Per the new policies lets open up a follow-up for this issue for D7 with title:

"[D7] Verify peer on HTTPS if cURL available (but be careful of built-in cert bundles in the codebase)"

The option to set these with a variable in #54 works well. In our case we need to actually set the "peer_name" in an AWS ELB (DEV with no CNAME) with Advagg doing some checks on /reports/status and this solves that use case as well.

I don't like the idea of turning "verify_peer" on by default for <5.6. I think with the release of this patch we should recommend adding that to settings.php and add an example there as well.

Here is an approach that allows for global defaults as well as per-host overrides. It requires a slightly more complicated configuration array in settings.php:

$conf['drupal_ssl_context_options'] = array(
  'default' => array(
    'ssl' => array(
      'verify_peer' => TRUE,
      'verify_peer_name' => TRUE,
      'allow_self_signed' => FALSE,
    ),
  ),
  'my-self-signed-solr-test-server@example.com' => array(
    'ssl' => array(
      'verify_peer' => FALSE,
      'verify_peer_name' => FALSE,
      'allow_self_signed' => TRUE,
    ),
  ),
);

This patch does not apply any new defaults for PHP<5.6 unless they're declared in settings.php. I think that's the wiser course, backwards-compatibility-wise, but I could be convinced otherwise.

I like the idea of #68 because usually you'll want to relax the security restrictions for a specific host rather than system-wide.

Forgot to supply default empty array for variable_get

I have just tested #71 patch on my https lets encrypt site and got my favourite "The NodeSquirrel server returned the following error:". I would say the patch does not work on php 7.1.

+1 for the approach in #71 allowing overrides per host.

I outlined something similar in #2774269: provide conditional configuration.

Confirming patch in #71 solves my issue #2864550: How to set stream_context in SearchApiSolrConnection. Thanks, @kmcculloch.

A quick workaround :

function MODULE_http_request($url, &$options) {
  // By default, verify SSL certificates in OpenSSL connections. This is
  // enabled by default in PHP 5.6, older versions do not enable it
  // automatically. Allow other SSL context options to be configured.
  if (!isset($options['context'])) {
    // Parse the URL and make sure we can handle the schema.
    $uri = @parse_url($url);
    $default_conf = array(
      'ssl' => array('verify_peer' => TRUE),
    );
    $opts = variable_get('drupal_ssl_context_options', $default_conf);
    if (in_array($uri['host'], array_keys($opts))) {
      $options['context'] = stream_context_create($opts[$uri['host']]);
    }
    elseif (array_key_exists('default', $opts)) {
      $options['context'] = stream_context_create($opts['default']);
    }
  }
}

function MODULE_update_N() {
  variable_set('drupal_http_request_function', 'MODULE_http_request');
}

// You can override drupal_ssl_context_options through settings.php like comment#68

$conf['drupal_ssl_context_options'] = array(
  'default' => array(
    'ssl' => array(
      'verify_peer' => TRUE,
      'verify_peer_name' => TRUE,
      'allow_self_signed' => FALSE,
    ),
  ),
  'my-self-signed-solr-test-server@example.com' => array(
    'ssl' => array(
      'verify_peer' => FALSE,
      'verify_peer_name' => FALSE,
      'allow_self_signed' => TRUE,
    ),
  ),
);

+1 for the patch in #71
Setup: PHP7.1, Apache 2.4, MariaDB 10
Detailed:
mariadb101u.x86_64 1:10.1.29-1.ius.centos7 @ius
mod_php71u.x86_64 7.1.11-1.ius.centos7 @ius
httpd.x86_64 2.4.6-67.el7.centos.6 @updates

So yet another RTBC

Could we document a little better the 'my-self-signed-solr-test-server@example.com' part of the patch?
If the check from the patch is in_array($uri['host'], array_keys($opts)) would we want to say that 'base_url' or the value inside of base_url is an acceptable option, particularly for self requests?

Looks like this needs a reroll.

Here's a re-roll.

I added a usage example to default.settings.php and changed the example host entry to 'my-self-signed-solr-test-server.example.com' to address @coolestdude1's concerns. (Not sure why there was an "@" sign in the example host name, but I agree that's pretty confusing.)

I am still very much +1 to the patch and would RTBC it, but please create a new issue for it, because the 8.x issue code change is different than the intention here.

#2761345: PHP 5.6, 7 - drupal_http_request() behavior changes for SSL using self-signed certs

might be a good candidate and we might also want to set a default 'default' entry of securing even older PHP versions and deliberately break compatibility to make upgrade to newer versions of PHP easier (break early with docs, rather than it breaking when upgrading PHP).

Thanks!

Bumping to 7.61. This didn't make it into 7.60.

see the new issue as per FabianX's request in comment #81

I think this issue can be closed, continue on in .
#3017522: Make SSL options configurable in drupal_http_request()

Actually this has to get into D8 first, as per comment #25

latest D8 patch probably needs a reroll too.

It looks like it was already committed to D8, see comments #42, and #60-64. Thanks.

As per @izmeez, this issue was committed to D8 in #42, so changing the status to Fixed, so that the D8 patch contributors can get credits. In the meantime, D7 "backport" is discussed here: #3017522: Make SSL options configurable in drupal_http_request() , though it is a bit different approach now.