[D7 backport] Update status does not verify the identity or authenticity of the release history URL

Just to summarize progress on this issue. I think the patch looks good, we have tests and are passing. But there are few open question from #4, #5 and #6.

1. D9 has Guzzle, D7 only has drupal_http_request() which is using stream_socket_client(). Therefore we cannot simply catch TransferException as it is in D9. I am not sure if it is a good idea to run the second drupal_http_request() each time there is *any* error returned (if update_fetch_with_http_fallback is TRUE). More logical would be to select only specific error codes/messages returned by drupal_http_request() and only on these try to run the insecure request - if possible, of course.

The current status of the patch is, that it is doing the HTTP fallback if there is any error, see:

+    if (isset($xml->error)) {
+      // @todo: Check only for specific error codes or messages?
+      watchdog('update', 'Error %errorcode (%message) occurred when trying to fetch available update data for the project %project.', array('%errorcode' => $xml->code, '%message' => $xml->error, '%project' => $project_name), WATCHDOG_ERROR);
+      if ($with_http_fallback && strpos($url, "http://") === FALSE) {
+        $url = str_replace('https://', 'http://', $url);
+        $xml = drupal_http_request($url);
+      }

We can either check for the one specific message, as proposed in #5 (but there probably can be multiple messages regarding SSL errors), or let it be as it is. I do not think we can 100% replicate the Guzzles' behavior from D10.

2. Are we sure we want to force this HTTP->HTTPS change for all D7 sites by default? See David comment: #1538118-104: Update status does not verify the identity or authenticity of the release history URL. There could be sites on archaic hostings, with an outdated CA list, or similar. They will end up without updates (by default) or without this security improvement (after manual action - change in settings.php).

See:

-define('UPDATE_DEFAULT_URL', 'http://updates.drupal.org/release-history');
+define('UPDATE_DEFAULT_URL', 'https://updates.drupal.org/release-history');

From the security point of view this is a good change though.

This is done.

4. OpenSSL extension is only recommended in D7 - do we need to set that extension as required? See: https://www.drupal.org/docs/7/system-requirements/php-requirements#openssl →

5. Maybe we would need to create separate handbook page for D7 and replace this link: https://www.drupal.org/node/3170647 → , or at least incorporate info about D7 into that page.

Still open questions.

https://badssl.com/ could be useful for manual testing - I don't think we should send requests there from automated tests.

It has several URLs with different SSL/TLS problems.

A few examples:

$ ddev drush php
Psy Shell v0.9.9 (PHP 7.4.33 — cli) by Justin Hileman

>>> var_dump(drupal_http_request('https://expired.badssl.com/'));
PHP Warning:  stream_socket_client(): SSL operation failed with code 1. OpenSSL Error messages:
error:1416F086:SSL routines:tls_process_server_certificate:certificate verify failed in /var/www/html/includes/common.inc on line 908
PHP Warning:  stream_socket_client(): Failed to enable crypto in /var/www/html/includes/common.inc on line 908
PHP Warning:  stream_socket_client(): unable to connect to ssl://expired.badssl.com:443 (Unknown error) in /var/www/html/includes/common.inc on line 908
object(stdClass)#5754 (2) {
  ["code"]=>
  int(0)
  ["error"]=>
  string(49) "Error opening socket ssl://expired.badssl.com:443"
}

>>> var_dump(drupal_http_request('https://wrong.host.badssl.com/'));
PHP Warning:  stream_socket_client(): Peer certificate CN=`*.badssl.com' did not match expected CN=`wrong.host.badssl.com' in /var/www/html/includes/common.inc on line 908
PHP Warning:  stream_socket_client(): Failed to enable crypto in /var/www/html/includes/common.inc on line 908
PHP Warning:  stream_socket_client(): unable to connect to ssl://wrong.host.badssl.com:443 (Unknown error) in /var/www/html/includes/common.inc on line 908
object(stdClass)#5758 (2) {
  ["code"]=>
  int(0)
  ["error"]=>
  string(52) "Error opening socket ssl://wrong.host.badssl.com:443"
}

>>> var_dump(drupal_http_request('https://untrusted-root.badssl.com/'));
PHP Warning:  stream_socket_client(): SSL operation failed with code 1. OpenSSL Error messages:
error:1416F086:SSL routines:tls_process_server_certificate:certificate verify failed in /var/www/html/includes/common.inc on line 908
PHP Warning:  stream_socket_client(): Failed to enable crypto in /var/www/html/includes/common.inc on line 908
PHP Warning:  stream_socket_client(): unable to connect to ssl://untrusted-root.badssl.com:443 (Unknown error) in /var/www/html/includes/common.inc on line 908
object(stdClass)#5757 (2) {
  ["code"]=>
  int(0)
  ["error"]=>
  string(56) "Error opening socket ssl://untrusted-root.badssl.com:443"
}

...etc.

So that makes me think we probably could check for quite a specific error being returned by drupal_http_request().

I feel like that's probably better than always attempting a http fallback whenever there's any problem with the initial request.

I'll do some more manual testing with different PHP versions etc.. (but keeping in mind recent discussions around no longer supporting really old PHP 5.x)

Did a quick test along the lines of my previous comment using ddev to switch PHP version:

$ cat bad_ssl_test.php 
<?php

print 'phpversion: ' . phpversion() . PHP_EOL . PHP_EOL;

print_r(drupal_http_request('https://expired.badssl.com/'));

print_r(drupal_http_request('https://wrong.host.badssl.com/'));

print_r(drupal_http_request('https://untrusted-root.badssl.com/'));

The results didn't vary between PHP 5.6 and 8.2:

$ ddev drush scr bad_ssl_test.php 
phpversion: 5.6.40-65+0~20230214.72+debian11~1.gbp473a1b

stdClass Object
(
    [code] => 0
    [error] => Error opening socket ssl://expired.badssl.com:443
)
stdClass Object
(
    [code] => 0
    [error] => Error opening socket ssl://wrong.host.badssl.com:443
)
stdClass Object
(
    [code] => 0
    [error] => Error opening socket ssl://untrusted-root.badssl.com:443
)

$ ddev drush scr bad_ssl_test.php 
phpversion: 7.0.33-65+0~20230214.72+debian11~1.gbp9842f4

stdClass Object
(
    [code] => 0
    [error] => Error opening socket ssl://expired.badssl.com:443
)
stdClass Object
(
    [code] => 0
    [error] => Error opening socket ssl://wrong.host.badssl.com:443
)
stdClass Object
(
    [code] => 0
    [error] => Error opening socket ssl://untrusted-root.badssl.com:443
)

phpversion: 8.2.3

stdClass Object
(
    [code] => 0
    [error] => Error opening socket ssl://expired.badssl.com:443
)
stdClass Object
(
    [code] => 0
    [error] => Error opening socket ssl://wrong.host.badssl.com:443
)
stdClass Object
(
    [code] => 0
    [error] => Error opening socket ssl://untrusted-root.badssl.com:443
)

The results of testing with PHP 7.4 were the same (and already in the previous comment).

I think we can conclude that this is pretty consistent when drupal_http_request hits common problems with SSL(/TLS) connections.

I'd be happy to use a test for this result to decide whether the fallback to http should be attempted.

I'll work on updating the patch.

The interdiff is a bit messy as the default.settings.php entry needed to be rerolled.

We'd expect the tests to pass as we're mocking the failing responses from drupal_http_request(), but I think the manual testing shows that this is fairly realistic.

As to the other points in the helpful summary in #12...

I think hopefully archaic hosting configurations that don't support outbound SSL/TLS would be pretty unusual these days.

We'd certainly want to call this change out in a CR and in the Release Notes to ensure that anyone managing a site that won't be able to use https for these checks knows to enable the fallback. I don't think we should turn it on by default though.

In order to avoid breaking any existing sites, I don't think I'd change the openssl requirement now. Any sites that don't have it will have to enable the fallback, but hopefully that won't be many.

Assuming the tests pass, anything else needed on this one?

Tests looking okay (although as noted, that's somewhat self-fulfilling).

Updated IS.

Tagging for Framework Manager Review; would still be good to get RTBC on the latest patch from other reviewers (/ maintainers) though.

One thing my manual testing didn't cover was the complete lack of openssl support in PHP.

It doesn't look like it's trivial to just turn it off e.g. in php.ini if it was compiled in (which it almost always is, perhaps with the exception of on Windows?).

It's be good to see what happens in openssl is absent, but I haven't had time to set anything up to test that.

Rudimentary way of getting PHP up and running without openssl, with a copy of the drush phar sitting in the docroot for convenience:

$ docker run -v /var/www/drupal-7.x:/tmp/drupal -it --rm alpine:latest sh

/tmp/drupal # apk add php82 php82-phar php82-tokenizer php82-pdo_sqlite php82-sqlite3 php82-session php82-mbstring php82-dom php82-gd php82-simplexml php82-xml sqlite

/tmp/drupal # php82 /tmp/drupal/drush-8.4.11.phar si

...snip - a few warnings etc, but successful installation...

/tmp/drupal # php82 /tmp/drupal/drush-8.4.11.phar scr bad_ssl_test.php 
phpversion: 8.2.6

stdClass Object
(
    [code] => -32581
    [error] => Unable to find the socket transport "ssl" - did you forget to enable it when you configured PHP?
)
stdClass Object
(
    [code] => -32581
    [error] => Unable to find the socket transport "ssl" - did you forget to enable it when you configured PHP?
)
stdClass Object
(
    [code] => -32581
    [error] => Unable to find the socket transport "ssl" - did you forget to enable it when you configured PHP?
)

So there's something else we could check for; perhaps we should have a helper that we pass the response object to which checks if the fallback should be attempted?

Added checking for "did you forget to SSL your PHP?" errors as well.

This doesn't use the error codes, just checks for specific strings in the error messages.

We could conceivably add a "always try to fallback to http" override variable too which would try http regardless of the error, but I think that'd be overkill.

This is not very scientific, but a little searching around seems to suggest that the error message when there's no SSL support seems to be about as old as Drupal 7 - e.g.:

https://stackoverflow.com/questions/21962849/unable-to-find-the-socket-t...

One thought though... are these error messages localised?

I have tested this on Windows / PHP 8.1, with openssl disabled and the error message in all three scenarios (expired, wrong host, untrusted) was:

stdClass Object ( [code] => -10064 [error] => Unable to find the socket transport "ssl" - did you forget to enable it when you configured PHP? )

So I suppose if there are sites without openssl support (windows/linux), these will need to set $conf['update_fetch_with_http_fallback'] = TRUE; in settings.php to have the update manager working after this change. Otherwise these sites will end up with the error mentioned above.

Just to note, the message displayed to the user in this case was the following (see the attached image). We should consider if the wording is correct, if we don't want to require openssl. Probably we would need to swap the openssl mention with the configuration change needed in settings.php.

And one small issue I have found - in default.settings.php there is a $settings['update_fetch_with_http_fallback'] and it should be $conf['update_fetch_with_http_fallback'] (seems like it's left over from my first patch attempt).

@poker10 what do you think about the localisation of the strings the latest patch is looking for to identify SSL errors?

I've seen a couple of things that suggest that socket errors are always in English, but I'm not convinced that these strings will always be correct where other system locales have been set; in which case we'd need to revert to the "try the fallback for any error" approach or work out a better way of identifying specific SSL/TLS errors.

Thinking about it... one simple solution would be to make the updates URL itself configurable.

Sites that can't use the https URL can set it to http instead, and it would also open the door for any future changes to where sites get the updates info from.

In fact there's already a variable used in _update_get_fetch_url_base().

..and the variable is already used to set different URLs in tests like UpdateTestUploadCase::testUpdateManagerCoreSecurityUpdateMessages.

So that makes this patch very simple.

No interdiff as it's a completely different approach.

Is this all we need here?

This simple approach looks good! If we change this for sites with openssl, I think it should not cause much troubles.

But there is some risk, that some sites without openssl support will run into the issue, when they fill up the fetch_failures cache row in the cache_update table and then even if they will set the $conf['update_fetch_url'] to the HTTP URL, the updates will not work. I think this is a bug and try to search for an existing issue (I think the failures limit is 2 and these are not cleaned up properly):

function _update_process_fetch_task($project) {
  ...
  $max_fetch_attempts = variable_get('update_max_fetch_attempts', UPDATE_MAX_FETCH_ATTEMPTS);

  $success = FALSE;
  $available = array();
  $site_key = drupal_hmac_base64($base_url, drupal_get_private_key());
  $url = _update_build_fetch_url($project, $site_key);
  $fetch_url_base = _update_get_fetch_url_base($project);
  $project_name = $project['name'];

  if (empty($fail[$fetch_url_base]) || $fail[$fetch_url_base] < $max_fetch_attempts) {
    $xml = drupal_http_request($url);
    ...
  }

So in case we will choose this approach, we would need to write release notes / change record carefully to help sites which will run into this "loop". Also not sure if there would be any helpful message for the sites without openssl in case of failure, as that message from previous patches was removed (I will check this so we are sure what these sites will see).

Question is, why D9 does not selecte this simpler approach and went with the more complicated one?

I think the closest issue to the mentioned problem is probably this: #512218: Checking for available modules fails for most of the modules →

It can be simulated with next steps:

1. Install Drupal 7.97 and apply the patch #24
2. Install at least two other modules, for example ctools and devel
3. Turn off openssl (or block requests to https://updates.drupal.org/release-history)
4. Go to admin/modules/update and click "Check manually"
5. First two projects will attempt to do $xml = drupal_http_request($url); and each one increments the $fail[$fetch_url_base] after an error is returned (note that $fetch_url_base is the same for all projects, as it is https://updates.drupal.org/release-history)
6. Each failure will also update fetch_failures cid in the cache_update table - expire timestamp is +5 minutes
7. Third project will not check updates at all, because it will not pass this condition
   

   if (empty($fail[$fetch_url_base]) || $fail[$fetch_url_base] < $max_fetch_attempts) {
   

   But it will increment the counter in the cache_update table, so the +5min expiration of the cache entry is updated as well

8. User will get a message Failed to get available update data for 3 projects. with no other information in watchdog or anywhere.
9. If the user try to run updates again (before the 5min expiration timer runs out), the code will fetch the fetch_failures from cache_update and no update check will run at all, because the cache entry has > 2 failures for this URL set and it is still valid. But the cache entry will be updated on each "unsuccessfull" (e.g. not attempted) update check and the expiration timer of that cache entry is extended to another 5 minutes... Therefore this can be a neverending loop.

Just to note, this is an old problem with the update manager and it is not caused by the patch. Previous patches in this issue does not help it, because if you set $conf['update_fetch_with_http_fallback'] = TRUE; after the first errors, it was possible that the failures treshold was already reached and as the fallback URL was not set to the $fetch_url_base, the condition was still evaluated as FALSE.

Another problem is, that the cache_update table is not cleaned after clear cache operation. So it makes that even harder for user to fix.

User has only three options:

• truncate the cache_update table
• increase the update_max_fetch_attempts variable / UPDATE_MAX_FETCH_ATTEMPTS constant
• wait 5 minutes and do not check updates until the cache entry expires

-----------------------------------

However, there is a positive effect of the patch #24 and that is, if the user experience the problem mentioned above and (somehow) is able to figure it out, that the problem is in SSL, then after setting this in settings.php, the problem is gone:

$conf['update_fetch_url'] = 'http://updates.drupal.org/release-history';

It is because the new URL is not same as the old URL (http vs https) and therefore it will not use the failures counter already set in the cache_update table. So this is great.

What is not that good is, that the patch #24 does not add any additional information to notify the user about the possibility to set the URL in settings.php. This was added only in previous patches, see these parts:

+ form_set_error('project_downloads', theme('update_fetch_error_message'));

+    if ($fetch_failed) {
+      drupal_set_message(theme('update_fetch_error_message'), 'error');
+    }

So the patch #24 will force all users to secure URL, but will not add any new message for users experiencing the failure after this patch. They will only see:

Failed to get available update data for 3 projects.

And no other information (not even in the watchdog).

If we are OK with this (and we suppose that everyone will read the release notes/change record), then I think the patch is OK. Probably this was the reason why D9 took the more complicated solution.

I think that if we would like to use the approach from patch #24, we should either add a watchdog call to the fetching process:

   if (empty($fail[$fetch_url_base]) || $fail[$fetch_url_base] < $max_fetch_attempts) {
     $xml = drupal_http_request($url);
+    if (isset($xml->error)) {
+      watchdog('update', 'Error %errorcode (%message) occurred when trying to fetch available update data for the project %project.', array('%errorcode' => $xml->code, '%message' => $xml->error, '%project' => $project_name), WATCHDOG_ERROR);
+    }

Or keep the new error messages from the previous patches. Or both options.

But I think that at least the watchdog call would be a minimum to help users.

Ok, this adds the watchdog logging of the error back in.

I am not entirely happy displaying an error message that mentions PHP openssl problems unconditionally if there's been any error with the fetching of the update info... but we've already been into the problems identifying specific SSL related errors.

I suppose we could do something like display an error message with a link to the Change Record for this change to https?

The watchdog call is a good idea either way. We could test for this using some of the test code from earlier patches.

The interdiff looks a little odd as the default.settings.php entry needed a reroll.

Simple addition to existing tests to check for the new log message when an update failed.

Added a Draft CR (which came out a bit florid for some reason).

The D7 maintainers discussed this in chat and Fabian approved it, so removing FM tag.

The last patch changed UPDATE_DEFAULT_URL from http to https and added a simple watchdog message in case the drupal_http_request returned an error. Users can opt-out changing the variable in settings.php (when they see a specific error message regarding SSL in watchdog). I think this is sufficient.

In the future probably a follow-up issue could be created to extend the default message Failed to get available update data for 3 projects. to something more meaningfull. But it is tricky with the conditions when to display the updated message, so we decided to not add more changes in this patch.

We have discussed this on Slack and @Fabianx reviewed and approved this as well:

Approved as well, RTBM - makes sense!

Therefore moving to RTBC.

poker10 → credited Fabianx → .

Committed and pushed this. Thanks!

Adding credits for Fabianx for the review.

Automatically closed - issue fixed for 2 weeks with no activity.