Change verify_SSL default to 1, add ENV var to enable insecure default [CVE-2023-31486]

[stigtsp]

To address #152

• Changes the verify_SSL default parameter from 0 to 1

  Based on patch by Dominic Hargreaves:
  https://salsa.debian.org/perl-team/interpreter/perl/-/commit/1490431e40e22052f75a0b3449f1f53cbd27ba92

  Fixes CVE-2023-31486

• Add check for $ENV{PERL_HTTP_TINY_SSL_INSECURE_BY_DEFAULT} that
  enables the previous insecure default behaviour if set to 1.

  This provides a workaround for users who encounter problems with the
  new verify_SSL default.

  Example to disable certificate checks:

    $ PERL_HTTP_TINY_SSL_INSECURE_BY_DEFAULT=1 ./script.pl
  

• Updates to documentation:

  • Describe changing the verify_SSL value
  • Describe the escape-hatch environment variable
  • Remove rationale for not enabling verify_SSL
  • Add missing certificate search paths
  • Replace "SSL" with "TLS/SSL" where appropriate
  • Use "machine-in-the-middle" instead of "man-in-the-middle"

• Update 210_live_ssl.t

  • Use github.com, cpan.org and badssl.com hosts for checking
    certificates.
  • Add self signed snake-oil certificate for checking failures rather
    than bypassing the SSL_verify_callback
  • Test verify_SSL parameter in addition to low level SSL_options
  • Test that PERL_HTTP_TINY_SSL_INSECURE_BY_DEFAULT=1 behaves as
    expected against badssl.com

• Added 180_verify_SSL.t

  • Test that verify_SSL default is 1
  • Test that PERL_HTTP_TINY_SSL_INSECURE_BY_DEFAULT behaves as expected
  • Test that using different values for verify_SSL and legacy verify_ssl
    doesn't disable cert checks

[haarg]

I don't love the name of the environment variable. If it's going to override the choice of whatever code is using HTTP::Tiny, rather than just changing the default, it should be more explicit about that. PERL_HTTP_TINY_FORCE_INSECURE or something. Or possibly it should only change the default, rather than overriding.

[nrdvana]

@haarg See my comment from the related issue. I feel it's fairly important for an environment variable to exist so that we provide an escape hatch for users surprised by these changes who suddenly discover they don't have a working trust store.

For comparison, LWP::UserAgent uses an environment variable PERL_LWP_SSL_VERIFY_HOSTNAME and Mojo::UserAgent uses a broader environment variable MOJO_INSECURE. I suggested HTTP_TINY_VERIFY_SSL. PERL_HTTP_TINY_INSECURE isn't bad though I prefer the more specific description of in what way it's insecure. i.e. it's not insecure because of file permissions, or executing javascript, or skipping CORS restrictions, etc.

[stigtsp]

@haarg @nrdvana I agree that the environment variable name should be more descriptive, both to indicate that it forces behavior and that it applies to TLS/SSL.

What about PERL_HTTP_TINY_FORCE_INSECURE_SSL=1? (I'd like to use "..._TLS" instead, but maybe it would just be confusing).

[haarg]

For comparison, LWP::UserAgent uses an environment variable PERL_LWP_SSL_VERIFY_HOSTNAME and Mojo::UserAgent uses a broader environment variable MOJO_INSECURE.

Both of these environment variables set defaults. They do not override an explicit setting by the user of the library.

[rjbs]

Having the environment variable affect the actual setting, overriding explicit requests from the user, seems odd.

I would have thought, instead, we'd say something like:

$default_verify
  = length $ENV{PERL_HTTP_TINY_VERIFY_CERTIFICATES} 
  ? !! $ENV{PERL_HTTP_TINY_VERIFY_CERTIFICATES}
  : 1;

…and then fall back to that when the user has provided no explicit value in the constructor.

[haarg]

I would definitely prefer that the environment variable only impact the default.

The only catch with that is that because HTTP::Tiny has been acting insecurely until now, many users are already overriding the default to be true. Many of these would probably be fine having an environment variable that could tell them to act insecurely.

Possibly this indicates that there should be two environment variables. I'm not entirely sure if that is necessary though. But if there is a variable that overrides the default, it definitely needs to be named appropriately to indicate that it is an override.

[nrdvana]

Another way of looking at it is that most frequently, a module author is making the decision of whether to verify SSL, but it is the end user who actually knows if they have a working trust store and the risks involved. It seems right to me that the user's choice should override the module author's choice.

We're also mostly considering authors who add verify_SSL => 1, but an author could also write verify_SSL => 0 and what would that even mean? That the author knows better than the user that some site should never be checked? If I'm the user connecting to a self-signed certificate site, I'd rather add the cert to my trust store and go back to verify_SSL => 1 via some environment variable.

Environment that only changes the default is a good pattern for most of the perl https clients, but only because most of them use a default of true and no module authors feel the need to override the default in normal usage.

[SoumyaWind]

Hi
I am looking for (CVE-2023-31486) fix. Is this the right fix? Could this be merged anytime soon?

[stigtsp]

@rjbs I don't have a strong opinion about the environment variable, other that one should be included in the upstream patch to provide an escape hatch. Would you like me to change the PR to that behavior?

I do like PERL_HTTP_TINY_FORCE_INSECURE_SSL=1 though, as it explicitly describes that insecure behavior is forced.

[stigtsp]

Hi I am looking for (CVE-2023-31486) fix. Is this the right fix? Could this be merged anytime soon?

While we wait for some agreement on the environment variable, and maybe other details, you can check out patches already included in some distributions:

• NixOS: perl: verify_SSL=>1 by default in HTTP::Tiny NixOS/nixpkgs#187480
• Alpine: https://gitlab.alpinelinux.org/alpine/aports/-/issues/14951
• Gentoo: https://bugs.gentoo.org/show_bug.cgi?id=905296

This PR and those patches are based on [PATCH] Enable SSL by default in HTTP::Tiny which was proposed for Debian

[rjbs]

@stigtsp I think changing the default is the correct behavior, and with not much weighing in elsewise, let's do that. I think that PERL_HTTP_TINY_FORCE_INSECURE_SSL is not the right name if the default is changed, because things aren't forced to insecure behavior. I would say PERL_HTTP_TINY_SSL_INSECURE_BY_DEFAULT gets the point across.

[stigtsp]

@stigtsp I think changing the default is the correct behavior, and with not much weighing in elsewise, let's do that. I think that PERL_HTTP_TINY_FORCE_INSECURE_SSL is not the right name if the default is changed, because things aren't forced to insecure behavior. I would say PERL_HTTP_TINY_SSL_INSECURE_BY_DEFAULT gets the point across.

I've updated the PR with a single commit containing the changes and support for PERL_HTTP_TINY_SSL_INSECURE_BY_DEFAULT

[rjbs]

Thanks very much @stigtsp, I am now moving on this.

[xdg]

Looks good. I have some doc/build comments and a readability nit.

[stigtsp]

@xdg Thanks for the feedback! I've updated the commit:

• // has been avoided
• Behavior when using verify_ssl and verify_SSL attributes as pointed out by @mauke has been fixed, and tests for that has been added.
• Docs are updated, and I've added a line at the top of the TLS section to describe the ENV var. Let me know if you need any changes to the text. I'm assuming version 0.083 is next btw.
• Your suggestions for 210_live_ssl.t has been added

A diff between the previous commit and this one should be here:
https://github.com/chansen/p5-http-tiny/compare/247ce9ba9e7307700922e811b0657ad441b289f4..6cd99f80ab769e4d86abd3eb321708e2cd44b4d5

[akhuettel]

Question: by introducing the variable, don't you just introduce a fresh (allthough less critical) bypass vulnerability?

(We're already building Perl with -DNO_PERL_RAND_SEED in Gentoo, and I see us patching out support for this bypass here in future HTTP::Tiny as well - restoring the current state where the default is unconditionally patched to 1.)

[Grinnz]

verify_ssl can be turned off easily in code regardless (and this capability must be present to support local requests with self-signed certs). the only reason for the variable is as an option to keep code running without needing code changes. the insecurity here is the default, which is being changed

[akhuettel]

verify_ssl can be turned off easily in code regardless (and this capability must be present to support local requests with self-signed certs). the only reason for the variable is as an option to keep code running without needing code changes. the insecurity here is the default, which is being changed

You might as well overlay a local HTTP::Tiny for such fragile code. Similar amount of ENV messing and noone else affected.

[nrdvana]

@akhuettel Gentoo didn't patch out -k in curl, or --no-check-certificate in wget, or PERL_LWP_SSL_VERIFY_HOSTNAME in libwww-perl. Why would this one be any different?

[xdg]

HTTP-Tiny-0.083-TRIAL.tar.gz released to CPAN. Thanks everyone for contributing to this change!

[xdg]

HTTP-Tiny-0.084.tar.gz released to CPAN.