Open shadowcat-mst opened 1 year ago
(comment removed at mst's request)
(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)
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.
@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.
@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?
@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;
}
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(...);
}
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.
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.
Did we get anywhere with this?
Just need some people to agree or disagree about what changes we want to propose to the perl steering council.
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:
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.
@stigtsp I agree that in an ideal world, verify_SSL => 1
would be the default. But because this is a core perl module, and changing the default will break any in-house connection where people don't have a real certificate, and there's no way to evaluate that impact, it should change to a once-per-process warning alerting the user that they didn't configure this, and then maybe in a few years actually change the default. Think of it as a deprecation period, like happens for core perl features.
As mst pointed out, the insistence on changing the default seems to be the main reason a change never got made. A deprecation warning is at least a step in the right direction that maybe enough people can agree to move forward with.
mst suggested the "warn on mismatch" as an alternative idea to my original "warn on not configured to verify". After the discussion above, I think we agreed that warning after the mismatch occurs isn't sufficient, and needs to warn any time the module is used in its default configuration.
My related patch does also provide environment variables like you suggest. I named it HTTP_TINY_VERIFY_SSL. If it isn't set it uses HTTP_TINY_VERIFY_SSL_DEFAULT, and if none of them are set including the constructor then it warns.
@nrdvana verify_SSL => 1
as default isn't something that just belongs in some mythical ideal world. Instead look at what's actually in play today.
I think the folks that are likely to be bit by a default change like this, are the same that most likely don't pay attention to warnings anyway. If someone still runs a system with a broken SSL setup (which this default was a "workaround" for), I think it's safe to assume they don't care about security unless it's pressed down their throats by force (e.g. through legislation or lawsuits). When they eventually (if ever!) try to run their software on a modern system, they won't do it by accident, but rather as part of an effort to modernize and update their code to 2023 standards. At that point, they'll see the new warnings, and get an opportunity to fix it.
But the rest of us, we should be able to have a Perl ecosystem that is secure by default.
@sjn Yes, that's also my position. The warning I chose was
Insecure HTTP::Tiny SSL connection made to '$host'. (default verify_SSL => 0)
Please specify this attribute or set a default in \$ENV{HTTP_TINY_VERIFY_SSL_DEFAULT}
and for "the rest of us", we can set HTTP_TINY_VERIFY_SSL_DEFAULT=1 in the global environment of our containers and that effectively makes the verify_SSL => 1
change.
Insecure HTTP::Tiny SSL connection made to '$host'. (default verify_SSL => 0)
Please specify this attribute or set a default in \$ENV{HTTP_TINY_VERIFY_SSL}
This error message only tells how to fix the symptom, and barely warns about the bad practice.
Insecure HTTP::Tiny SSL (TLS) connection made to '$host'. (default verify_SSL => 0)
DEPRECATION WARNING: The default verify_SSL will change to 1 in HTTP::Tiny releases after October 1st 2023.
Please ensure SSL verification works, and specify this attribute, or set a default in \$ENV{HTTP_TINY_VERIFY_SSL_DEFAULT}
CVE-2023-31486 has been assigned to this issue
So a user came to perlmonks unable to use his CPAN client after an upgrade, and looking for temporary workarounds until they can get their certificate trust store fixed. Unfortunately, we never got this patch applied, so there are not environment variables to bypass certificate verification, and now CPAN has set verify_SSL => 1
and cpan.org is force-redirecting http to https. The only option available to this user is to downgrade to an older CPAN module.
I think it would have been a better result to get this patch applied first, then have the new version of CPAN require the new version of HTTP::Tiny, and then we could helpfully tell this user to set an environment variable to get them up and running again.
Uh, the most recent Perl maven is calling this a "security issue" - doesn't seem like one to me, seems like a discussion about reasonable defaults. https://perlweekly.com/archive/617.html
If your operating system asked you to log in, but did not verify your password by default, it would absolutely be a security issue.
If an HTTP client allows MitM by default, it's a security issue, no matter how many people out there are using self-signed certificates for their critical infrastructure.
Just to provide an inspiration on how to deal with broken users of IO::Socket::SSL (like HTTP::Tiny). Since 2014 there is a documented way to enforce specific arguments no matter how IO::Socket::SSL is used. This is specifically intended for broken code which disables validation, expects old TLS versions or ciphers etc. To be on the safe side simple do
# override broken settings with secure values for all following users of IO::Socket::SSL
IO::Socket::SSL::set_args_filter_hack('use_defaults');
my $http = HTTP::Tiny->new;
Also from my experience with moving IO::Socket::SSL to verification by default in 2012: I started in 11/2012 with a warning for everybody using the default of SSL_verify_mode that the behavior will change and then switched to the new secure default in 7/2013. During and after the transition I've tightly monitored stackoverflow.com to answer any problems not showing up in the bug tracking. I've found it important to provide quick help for users which run into problems, so that they are a) understand that disabling validation is usually a bad idea even if it seems to solve the problem and b) able to use validation in their environment, even with SSL intercepting firewalls.
@oodler577 it's definitely a security issue when it has a CVE number assigned to it. In fact, now it must be fixed for any security audit to pass. In fact many security auditors don't even care if you're affected by the bug, they want to see you upgrade to a version that doesn't have a CVE on it.
@noxxi That's a helpful precedent, and sort of what I aimed for with my patch. Also very helpful to know that it can be forced interpreter-wide without any changes to HTTP::Tiny.
I'm missing the impact(s) if we'd enable the verification by default. Can someone please list all known ones? Thanks!
@abraxxa So far the only real-world case I know of is in my comment above, and untold number of cases like that in the future. stigstp already identified 300+ perl modules on CPAN affected by it, so you can draw your own conclusion about how many users are likely to have broken trust stores.
It's just the difference between a user with a broken trust store getting an emergency situation where they have to fix something (maybe in production) on short notice, vs. a yearlong period where they might notice the warning and get it fixed without an emergency. My patch in #151 was intended to be the "smooth out the speedbump" approach. @stigtsp's patch in #153 is more the "get it done and save them from themselves" approach.
The flip side is that someone vulnerable might be getting spied on long-term as we speak, and the sooner we change it the sooner we cut off the eavesdropper. There could even be secret government campaigns to inject things into the perl modules (via insecure cpan downloads) of known targets going on for years.
I'm actually OK with either patch. I just proposed this one because it seemed more polite to end users and more likely to get applied.
That are the security implications which I‘m aware of. My question was about what we‘d break when we enable it. CPAN downloads should work as the CPAN CDN has a valid certificate. The downstream modules using HTTP::Tiny against a self-signed or private root CA would be. I‘d handle that with a major version bump. And shipping that version with the next major Perl version as those are specially tested by enterprises when upgraded to.
@abraxxa CPAN downloads ought to work, but clearly didn't in the example above. We can only control the server half of that; Perl uses Mozilla::CA to validate servers instead of using the host's CA list, so anyone with a very out-of-date Mozilla::CA who updates to latest CPAN.pm will suddenly be unable to use CPAN to update any more modules, including Mozilla::CA. They'll have to use a browser to download the module and then install it manually. I don't even think Mozilla::CA is a pre-req, so it might not even be installed in the first place.
This same situation may occur for any number of web services that also have perfectly valid certs. If someone has a really old webserver with an old perl app using Paws to orchestrate things in AWS cloud, and have never checked to see whether their trust store is working (because why would they?) it doesn't matter that Amazon has valid certificates - a new version of HTTP::Tiny will suddenly break their access to AWS APIs.
...Perl uses Mozilla::CA to validate servers instead of using the host's CA list, so anyone with a very out-of-date Mozilla::CA who updates to latest CPAN.pm will suddenly be unable to use CPAN to update any more modules, including Mozilla::CA.
"Perl" has no idea of what to use, Perl has not even builtin support for TLS (Net.:SSLeay and IO::Socket::SSL are not CORE modules). What trust store is used is handled individually and differently by the various modules. IO::Socket::SSL has some logic inside to use the same trust store as OpenSSL as long as there are certificates inside, otherwise tries to use Mozilla::CA (which is also not a CORE module). HTTP::Tiny has a different logic (see _find_CA_file
) which thus overrides the one from IO::Socket::SSL. Basically this logic checks environment SSL_CERT_FILE (like OpenSSL), then checks Mozilla::CA and if both fail tries some hard coded path typically found on UNIX systems (taken from golang root_unix.go
).
@xdg Thanks for merging https://github.com/chansen/p5-http-tiny/pull/153!
It changes the default from verify_SSL=>0
to verify_SSL=>1
and adds support for $ENV{PERL_HTTP_TINY_SSL_INSECURE_BY_DEFAULT}
for users who need the previous default.
Hmmm. On Windows, it seems to me that it would be difficult to pass the value of the environment variable PERL_HTTP_TINY_SSL_INSECURE_BY_DEFAULT
on the command prompt by any chance.
Windows 8.1:
C:\home\pianokey>set PERL_HTTP_TINY_SSL_INSECURE_BY_DEFAULT=1
C:\home\pianokey>echo %PERL_HTTP_TINY_SSL_INSECURE_BY_DEFAULT%
1
C:\home\pianokey>perl -e "print $ENV{PERL_HTTP_TINY_INSECURE_BY_DEFAULT};"
C:\home\pianokey>
C:\home\pianokey>perl -v
This is perl 5, version 16, subversion 3 (v5.16.3) built for MSWin32-x64-multi-thread
Copyright 1987-2012, Larry Wall
Perl may be copied only under the terms of either the Artistic License or the
GNU General Public License, which may be found in the Perl 5 source kit.
Complete documentation for Perl, including FAQ lists, should be found on
this system using "man perl" or "perldoc perl". If you have access to the
Internet, point your browser at http://www.perl.org/, the Perl Home Page.
C:\home\pianokey>
@twata1 PERL_HTTP_TINY_SSL_INSECURE_BY_DEFAULT
!= PERL_HTTP_TINY_INSECURE_BY_DEFAULT
@haarg Thank you for pointing out my mistake!
C:\home\pianokey>set PERL_HTTP_TINY_SSL_INSECURE_BY_DEFAULT=1
C:\home\pianokey>perl -e "print $ENV{PERL_HTTP_TINY_SSL_INSECURE_BY_DEFAULT};"
1
C:\home\pianokey>
However...
C:\home\pianokey>set PERL_HTTP_TINY_SSL_INSECURE_BY_DEFAULT=1
C:\home\pianokey>perl -e "print $ENV{PERL_HTTP_TINY_SSL_INSECURE_BY_DEFAULT};"
1
C:\home\pianokey>cpan SZABGAB/Padre-1.02.tar.gz
CPAN: CPAN::SQLite loaded ok (v0.202)
CPAN: HTTP::Tiny loaded ok (v0.084)
CPAN: Net::SSLeay loaded ok (v1.93_02)
CPAN: IO::Socket::SSL loaded ok (v2.083)
Fetching with HTTP::Tiny:
https://cpan.org/authors/01mailrc.txt.gz
HTTP::Tiny failed with an internal error: SSL connection failed for cpan.org: SSL connect attempt failed error:14090086:
SSL routines:SSL3_GET_SERVER_CERTIFICATE:certificate verify failed
Fetching with HTTP::Tiny:
https://cpan.org/modules/02packages.details.txt.gz
HTTP::Tiny failed with an internal error: SSL connection failed for cpan.org: SSL connect attempt failed error:14090086:
SSL routines:SSL3_GET_SERVER_CERTIFICATE:certificate verify failed
Fetching with HTTP::Tiny:
https://cpan.org/modules/03modlist.data.gz
HTTP::Tiny failed with an internal error: SSL connection failed for cpan.org: SSL connect attempt failed error:14090086:
SSL routines:SSL3_GET_SERVER_CERTIFICATE:certificate verify failed
Database was generated on Thu, 15 Jun 2023 20:38:47 GMT
Updating database file ... Done!Fetching with HTTP::Tiny:
https://cpan.org/authors/id/S/SZ/SZABGAB/Padre-1.02.tar.gz
HTTP::Tiny failed with an internal error: SSL connection failed for cpan.org: SSL connect attempt failed error:14090086:
SSL routines:SSL3_GET_SERVER_CERTIFICATE:certificate verify failed
Giving up on 'C:\Strawberry\cpan\sources\authors\id\S\SZ\SZABGAB\Padre-1.02.tar.gz'
C:\home\pianokey>
Am I still doing something wrong? If there is no other mistake on my part, I will create a separate ticket.
The environment variable only affects the default setting for HTTP::Tiny. CPAN.pm was recently updated to set verify_SSL=>1
explicitly in 2.35.
The error message indicates that the client doesnt trust the certificate cpan.org has provided.
Try updating Mozilla::CA, it might be outdated.
@stigtsp
Thank you for your reply.
I have cpan
version is 2.34 and Mozilla::CA
is up to date.
I usually use cpan
and cpanm
.
C:\home\pianokey>set PERL_HTTP_TINY_SSL_INSECURE_BY_DEFAULT=1
C:\home\pianokey>cpanm Mozilla::CA
Mozilla::CA is up to date. (20221114)
C:\home\pianokey>cpan -v
C:\home\pianokey\perl5\bin/cpan script version 1.678, CPAN.pm version 2.34
C:\home\pianokey>cpan SZABGAB/Padre-1.02.tar.gz
CPAN: CPAN::SQLite loaded ok (v0.202)
CPAN: HTTP::Tiny loaded ok (v0.084)
CPAN: Net::SSLeay loaded ok (v1.93_02)
CPAN: IO::Socket::SSL loaded ok (v2.083)
Fetching with HTTP::Tiny:
https://cpan.org/authors/01mailrc.txt.gz
HTTP::Tiny failed with an internal error: SSL connection failed for cpan.org: SSL connect attempt failed error:14090086:
SSL routines:SSL3_GET_SERVER_CERTIFICATE:certificate verify failed
Fetching with HTTP::Tiny:
https://cpan.org/modules/02packages.details.txt.gz
HTTP::Tiny failed with an internal error: SSL connection failed for cpan.org: SSL connect attempt failed error:14090086:
SSL routines:SSL3_GET_SERVER_CERTIFICATE:certificate verify failed
Fetching with HTTP::Tiny:
https://cpan.org/modules/03modlist.data.gz
HTTP::Tiny failed with an internal error: SSL connection failed for cpan.org: SSL connect attempt failed error:14090086:
SSL routines:SSL3_GET_SERVER_CERTIFICATE:certificate verify failed
Database was generated on Thu, 15 Jun 2023 20:38:47 GMT
Updating database file ... Done!Fetching with HTTP::Tiny:
https://cpan.org/authors/id/S/SZ/SZABGAB/Padre-1.02.tar.gz
HTTP::Tiny failed with an internal error: SSL connection failed for cpan.org: SSL connect attempt failed error:14090086:
SSL routines:SSL3_GET_SERVER_CERTIFICATE:certificate verify failed
Giving up on 'C:\Strawberry\cpan\sources\authors\id\S\SZ\SZABGAB\Padre-1.02.tar.gz'
C:\home\pianokey>
Maybe there is another problem, but I made #155.
Just poking my peanut gallery head in on a stalled thread,
(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)
That’s what I might have said myself once upon a time, but in fact, Parse::CPAN::Meta appears to be a data point to the contrary.
BACKGROUND
Before I start: The actual patch in https://github.com/chansen/p5-http-tiny/pull/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:
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 constructor option, and if it's present invoke it something like:
then in the case where SSL_verify isn't provided, the code can set this to C to call
(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