Make SSL options configurable in drupal_http_request()

Created on 2 December 2018, about 6 years ago
Updated 15 May 2023, over 1 year ago

Problem/Motivation

Drupal does not currently verify that SSL certificates use a trusted root cert. This affects any requests initiated with drupal_http_request. It also affects OpenID discovery and HTTPS redirects during that discovery.

Proposed resolution

This issue includes solutions for Drupal 8 and the Drupal 7 backport.

For Drupal 8

see πŸ› Verify peer on HTTPS if cURL available (but be careful of built-in cert bundles in the codebase) Fixed

For Drupal 7

1) Verify SSL certificates using curl_verify_ssl_peer. This only works when PHP is compiled with cURL support.

One drawback to this approach is that cURL is not always available in shared host environments. Commenters indicated that cURL is a relatively standard feature, and this should not be as much of a factor when D8 is released.

2) Include in Drupal a bundle of X.509 certificates of public Certificate Authorities (CA) borrowed from the cURL library.

greggles mentioned in comment 24 that this might require rapid security advisories in the case of a compromised cert. Also, including certs in Drupal core would logically add a configuration requirement where admins could add new certs.

Remaining tasks

Get Drupal 8 patch in.
Backport to Drupal 7 - Comment #25 includes a patch using curl_verify_ssl_peer, but it requires testing.

Possibly backport to D6.

API changes

This could affect developers who use drupal_http_request to issue HTTPS requests against servers that do not have proper SSL certs. For example, HTTPS connections would fail when issued to an internal dev server that used a self-signed cert.

Original report by Heine

Wojtha just asked on IRC if certificates are verified. They are not.

Demo patch in a few mins.

FABIANX - #1081192-81: Verify peer on HTTPS if cURL available (but be careful of built-in cert bundles in the codebase) β†’

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!

πŸ› Bug report
Status

Fixed

Version

7.0 ⚰️

Component
BaseΒ  β†’

Last updated about 3 hours ago

Created by

πŸ‡¨πŸ‡¦Canada joseph.olstad

Live updates comments and jobs are added and updated live.
  • Security improvements

    It makes Drupal less vulnerable to abuse or misuse. Note, this is the preferred tag, though the Security tag has a large body of issues tagged to it. Do NOT publicly disclose security vulnerabilities; contact the security team instead. Anyone (whether security team or not) can apply this tag to security improvements that do not directly present a vulnerability e.g. hardening an API to add filtering to reduce a common mistake in contributed modules.

  • Needs issue summary update

    Issue summaries save everyone time if they are kept up-to-date. See Update issue summary task instructions.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

    This seems a lot less relevant now than it did several years ago when PHP (5.6) had only just started verifying SSL/TLS by default, and when self-signed certificates were a lot more common.

    @Fabianx and I both +1'ed this approach in πŸ› Verify peer on HTTPS if cURL available (but be careful of built-in cert bundles in the codebase) Fixed but that was several years ago. I remember being very in favour of it back then.

    2017 me mentioned a contrib. implementation in the original issue: #2774269: provide conditional configuration β†’ . I outlined a more flexible approach where the context could be customised per port and/or URL as well as just by host. It looks like I didn't implement that though, I don't recall why exactly but it does seem like it might be overly complicated.

    If we were to commit this, nothing would change by default.. but sites would be able to set up new configurations.

    I think that sounds okay, but I am really wondering whether this is "still a thing". Would any sites actually use this?

    I'll RTBC it on the basis that I think it'd be safe to commit it, but I'd like to discuss whether we should a bit further with the other maintainers.

    Oh, and I expect it might need a reroll as default.settings.php keeps getting new additions.

  • last update over 1 year ago
    2,151 pass
  • πŸ‡¨πŸ‡¦Canada joseph.olstad

    Patch 5 still applies (with fuzz).

  • πŸ‡ΈπŸ‡°Slovakia poker10

    but I am really wondering whether this is "still a thing". Would any sites actually use this?

    I think that the benefit of this change is that sites on PHP 5.6+ can disable SSL/TLS verification for specific hosts when using drupal_http_request(). By default it does verify SSL and it is not possible to change it (without replacing the function entirely, see below). So probably the issue has a bit different (opposite) purpose now, as it has years before and probably doesn't have much in common with the parent/original issue.

    In overall, I think it is a good change, as it will add a flexibility and more users will be able to use this API function without need of the replacement.

    On the other hand, two points come to my mind:

    1. We have possibility to replace the drupal_http_request() with a different function, see:

    $override_function = variable_get('drupal_http_request_function', FALSE);
    

    So it is questionable, if there are still sites which will benefit from this - probably most of them are using this override or do not use drupal_http_request() at all.

    2. The settings.php is getting bigger and bigger (but it is not necessary a bad thing, we just need to carefully consider adding new config options).

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

    Anyway, if we want to commit this, I think we need to add a test for the new settings. We can probably test a subdomain with expired/invalid SSL (for example something like the https://badssl.com/ offers) and check if drupal_http_request() works with the validation enabled / disabled.

    I will not change the status before the final decision.

  • last update over 1 year ago
    2,151 pass
  • πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

    We have possibility to replace the drupal_http_request() with a different function ...

    Yup, I think the self-signed SSL cert use case was the main reason I created https://www.drupal.org/project/alt_http_client β†’ which does exactly that.

    I've updated the project a few times over the years to bring in updates made to core's drupal_http_request(), but I'm not certain when I last did so.

    On the basis that it's perhaps not that common to need to deviate from PHP's defaults these days, there's an argument to leave this to be dealt with in contrib.

  • πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

    Oh, πŸ“Œ re-sync with D7 core's drupal_http_request() Fixed is about updating alt_http_client with changes from core's drupal_http_request().

    I think when I filed that I was waiting for a patch to be committed. Looks like it's all ready to go now.

    If I updated that project, shall we close this issue for core with the recommendation that sites use alt_http_client if they need to tweak SSL options?

  • πŸ‡ΈπŸ‡°Slovakia poker10

    If I updated that project, shall we close this issue for core with the recommendation that sites use alt_http_client if they need to tweak SSL options?

    +1 for this. There is/will be a fully functional module with the same functionality as is proposed here. Considering a number of sites which will possible use this and also other options they have, I think this will be the best solution.

  • Status changed to Fixed over 1 year ago
  • πŸ‡¬πŸ‡§United Kingdom mcdruid πŸ‡¬πŸ‡§πŸ‡ͺπŸ‡Ί

    There's now a 7.x-1.1 release of alt_http_client with the updates from core plus tests.

    The module includes pretty much the same code as the patch in this issue:

    https://git.drupalcode.org/project/alt_http_client/-/blob/7.x-1.1/alt_ht...

    We should try to remember to update the module whenever core's drupal_http_request() changes.

    Thanks everyone who worked on this, especially @kmcculloch for the original patch.

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

Production build 0.71.5 2024