RFC: Making SSL_verify safer

[shadowcat-mst]

BACKGROUND

Before I start: The actual patch in #151 is not honestly a bad starting point here, it appears this fact has been obscured in the pre-existing discussion due to the description being right at the end of the PR text - and the start of said text seeming sufficiently absolutist that people pattern matched to previous less well thought out proposals.

However, before anybody tries to put together another PR I think it's probably best if we actually discuss what's desired as a matter of policy first (also I owe xdg a debt of thanks still for providing me with Nagios Enterprises as a chew toy a few years back, so I'm inclined to at least -try- and be nice).

I'll note that if we'd had a time machine, having SSL_verify default to 1 out of the box could have turned out to be better overall, but I don't have a TARDIS and Let's Encrypt didn't exist when that decision was made, and, welp, so it goes.

CURRENT SITUATION

SSL_verify defaults to 0, which is not the worst idea for maximally enabling bootstrapping of cpan, but since lots of people have decided to use this module all over CPAN rather than depending on a proper useragent it's fairly clear the playing field is not what it was.

(and believe me, if we'd only cored it as CPAN::HTTP::Client then cpan authors would've used that anyway, humans are gonna human)

Debian keeps having "let's just force it to 1" flare ups which they understandably keep rejecting due to the compat and user confusion issues.

NixOS has apparently applied that patch anyway, but it basically specialises in "everything is different and confusing" so I somehow doubt anybody who's got as far as actually being able to install it will be surprised or troubled by that (and everybody should try doing it at least once, because nixos' qemu/kvm based configuration integration testing system is in fact really cool).

DEFINITELY A GOOD IDEA

In order to not break anything existing but make users aware of potential problems, I think @nrdvana's proposal to have a once-per-process warning plus environment variables is actually excellent - and having an env var for setting default behaviour and one for overriding provides a nice level of flexibility.

I would, in fact, endorse a completed version of the existing PR as being a substantial improvement on the status quo (and the single-warning model means it's unlikely to cause carnage).

It may also be preferable to allow a user to add something like:

use HTTP::Tiny::Defaults { SSL_verify => 1 };

at the top of a script in order to deal with a recalcitrant cpan module used multiple layers down.

POSSIBLY AN EVEN BETTER IDEA

Rather than preserving the undef value internally and modifying the accessor, I think we can go one better.

(please consider all names both below to have been pulled out of whichever orifice is least traumatic for you to imagine)

If we add an C<SSL_verify_cb> constructor option, and if it's present invoke it something like:

if (my $cb = $self->SSL_verify_cb) {
  weaken(my $weak = $self);
  Net::SSLeay::set_verify ($ssl, Net::SSLeay::VERIFY_PEER, sub { $weak->$cb(@_) });
}

then in the case where SSL_verify isn't provided, the code can set this to C<SSL_verify_warn_on_mismatch> to call

sub SSL_verify_warn_on_mismatch {
  my ($self, $ok, $ctx) = @_;
  return $ok if $ok;
  my $error = Net::SSLeay::X509_STORE_CTX_get_error($ctx);
  warn "Certificate mismatch: ${error}";
  return 1;
}

(note, I lifted the callback arguments and the how to get the error part from an SSLeay test, so any part of it that works should be credited to that and any part that doesn't is presumably me being daft)

which should mean that users running in what's effectively "dunno if they wanted verification and forgot to ask" mode will still get to see problems without AFAICT breaking anything except possibly people's Test::NoWarnings (or equivalent) using t/ files.

This would also allow users who're unsure what their current situation is to set this option themselves so they can -see- if there are verification failures in the code they're running without breaking anything - which seems like an excellent way of making people less scared of turning the security feature properly at some point.

SUMMARY

I think the number of previous attempts at "just turn it on, obviously that couldn't possibly cause any problems" has made this issue trickier to discuss than would've been optimal, but I think if we actually -have- a discussion about it we can get somewhere.

I'll also drop this into #toolchain for comments, and if the consensus is "we like this idea but let's get the PSC to ack it" (for whichever value of "this idea" eventually gets agreed upon) I'm happy to go chase them down.

-- mst

[xdg]

(comment removed at mst's request)

[shadowcat-mst]

(response to deleted comment elided)

@nrdvana @stigtsp @sjn I'm happy thrashing this out and taking it to PSC once we've got a decent idea what we're doing (this is an edit in light of the reopen comment)

[xdg]

At @shadowcat-mst's request, I'm reopening this issue as a forum for discussion about what to take to PSC. To be clear, I'm unsubscribed from the issue and no decision will be taken by me on this topic. If PSC makes a decision, have them contact me with a request for a change.

[nrdvana]

@shadowcat-mst Hooking into SSL verification callback for the warning isn't a bad idea. One catch with your implementation above is that the user doesn't get a warning if the certificate is valid, and then only gets a warning after an attacker has successfully MITM'd them. So, in cases where certificates are set correctly, it still leaves them vulnerable to an attack without advanced warning.

Small nitpick, it's verify_SSL not SSL_verify.

[shadowcat-mst]

@nrdvana sorry for the typo - also, I don't understand, given I proposed something to do as -well- as the warning you'd implemented I don't see why your warning wouldnt ... warn the user?

Is there something wrong with your code for #151 that I've missed?

[nrdvana]

@shadowcat-mst You said "Rather than preserving the undef value internally" which I took to mean that ->{verify_SSL} will now be defined to the default (of 0) and so my code would not know whether to warn. The code you proposed could take over that role though, if it were:

sub SSL_verify_warn_on_mismatch {
  my ($self, $ok, $ctx) = @_;
  warn "Certificate trust is not being enforced, see HTTP::Tiny verify_SSL option\n"
    unless $first_warning++;
  return $ok if $ok;
  my $error = Net::SSLeay::X509_STORE_CTX_get_error($ctx);
  warn "Certificate mismatch: ${error}";
  return 1;
}

[nrdvana]

Actually on second thought, there was more reason for preserving the undef in the attribute. Consider the following sequence:

my $ua= HTTP::Tiny->new();
$ua->verify_SSL(0) if $ENV{SKIP_VERIFY};
$ua->get(...);

By preserving the undef from the constructor, we can tell the difference whether someone has intentionally set the attribute some time after construction. If the callback is our "flag" for tracking this, then the accessor for verify_SSL needs additional logic to say "is the verify callback our default callback" and change it.

On that same note, preserving the undef allows us to see if the ENV vars changed between the time of construction and use, for things like:

my $ua= HTTP::Tiny->new();
{
  local $ENV{HTTP_TINY_VERIFY_SSL}= 0;
  $ua->get(...);
}

[shadowcat-mst]

That sounds reasonable but in that case why not let the accessor return undef so users can perform that check as well?

The bit I didn't like was the part where the accessor wasn't reflecting the contents of the object, for something this low level that doesn't seem wise.

[nrdvana]

I don't really feel strongly about that design detail, but the rationale that led up to that decision was if people have fatal warnings enabled, changing a thing that used to be defined to a default of undefined could break things.

I'm fine if it just returns undef.

And actually, another interesting alternative is if reading the verify_SSL attribute returns the default value according to the current environment variables, if the internal value wasn't defined.

[sjn]

Did we get anywhere with this?

[nrdvana]

Just need some people to agree or disagree about what changes we want to propose to the perl steering council.

[stigtsp]

I'm worried that a warning from SSL_verify_warn_on_mismatch could be easy to overlook, and wouldn't prevent a mitm attack.

Imho, the default should be changed to verify_SSL=>1, to protect downstream users, and so module authors using the module doesn't have to update their code. So a test like this passes:

HTTP::Tiny->new->get("https://wrong.host.badssl.com/")->{success} && die "insecure";

Authors using HTTP::Tiny might be unaware of the insecure verify_SSL default, or forget to enable it. This could result in problems like:

• Add verify_SSL=>1 to HTTP::Tiny in CPAN::HTTP::Client to verify https server identity andk/cpanpm#175
• Add verify_SSL=>1 to HTTP::Tiny to verify https server identity bluefeet/GitLab-API-v4#57

Turning verification on by default will break things for end users that are (maybe unknowingly) using HTTPS insecurely without explicitly disabling verification. But that's a good thing.

Suggestions for corner cases where server host verification is not needed:

• An ENV variable like PERL_HTTP_TINY_INSECURE=1 that disables certificate checks, similar to curl --insecure. This could be a useful break-glass option for sysadmins and users who needs to disable verification in their setups without patching the code.

• For systems without certs, unencrypted HTTP can be used. I'm not sure if unverified HTTPS provides any meaningful security (other than some confidentiality) in that case.

There are also maybe some issues with how ca certs are looked up on the system. (I've just had a quick look at these, so it would be great if someone with a deeper understanding of these things could have a look):

• The cert path defaults to Mozilla::CA, as documented. This can be a problem if Mozilla::CA is outdated or if user expects their default system cert path to be used.

• The SSL_CERT_FILE environment variable is supported, but SSL_CERT_DIR is not.