SSL environment controls, and warn about insecure connections #151
Conversation
The verify_SSL option is disabled by default, which is both a vulnerability and an RFC 2818 violation. Being a heavily-used module by many in-house scripts, it is probably not practical to change this default, but users need to be warned so that they can apply whatever security intentions they have rather than blindly receiving an insecure default. This patch changes the verify_SSL attribute to remain 'undef' if the user does not take any steps to assign a value to it. This condition can then detect cases where an SSL connection is being established without verification and where the user did not op-in to this behavior. It generates one warning using 'warn', and then suppresses future warnings. The first new environment variable HTTP_TINY_VERIFY_SSL_DEFAULT is a boolean that can either opt-in to the default of false, or change the default to true. Both settings suppress the warning. The second environment variable HTTP_TINY_VERIFY_SSL is a boolean that can override the attribute chosen by users of the module. This will become necessary if module authors who use HTTP::Tiny begin including the (verify_SSL => 1) setting in the construction of the user agent but end-users of those modules want to override the decision to allow insecure connections. This mirrors the utility of PERL_LWP_SSL_VERIFY_HOSTNAME which end-users can set to quickly permit insecure connections with LWP::UserAgent.
|
As you point out, other modules might use HTTP::Tiny from core without knowing about the insecure default, and exposing downstream users to mitm attacks. This is concerning, and IMHO the default should be changed from NixOS has added a patch that does this: |
|
HTTP::Tiny is very explicit in the list of RFCs that it supports and 2818 is not among that list. The documentation is very clear on the behavior and the rationale for it. You are welcome to create |
|
@xdg What's up with closing this pull request? The addition @nrdvana proposes here is completely reasonable, and in fact the right thing to do. Sure there are bad coding patterns in practice out there, but just shoving these under the carpet to ignore them actively hurts Perl's reputation. Instead, bad practices should be made visible, and warning about them (like in this pull request) is completely sensible. Please don't be part of propagating this bad habit. |
|
This decision was taken a long time and has been discussed extensively since then. Short of the Perl Steering Council directly asking for a change, it's not going to happen. |
|
Would you mind sharing a link to this decision? Links to previous discussions (or tickets) would also be very much appreciated – both for posterity and right now for those of us following this issue, so we get a clearer idea of existing arguments and facts. Secondly, it seems you are deferring decisions about this module to the PSC, does this mean you're in favor of them formally taking over management of this module? Finally – would you mind sharing with us why reconsidering this decision shouldn't be on the table? |
|
@xdg What if it had a CVE number assigned to it and all users were forced to apply the patch in order to pass security compliance for their apps? Would that change your mind? |
|
We've searched CPAN to get an overview of how verify_SSL is used:
|
|
@nrdvana @sjn @stigtsp As per the discussion on #134 since this module is shipped as part of core in order to be used to bootstrap a full toolchain, this is something that needs to be discussed by the core and toolchain teams. I look forward to participating in the perl5-porters thread and irc.perl.org #toolchain discussions about this, after which we can work out whether the documentation should be changed, a warning added if verify_SSL isn't specified (so users have to pick explicitly what affordance they want) or some combination thereof. Given that reality, this PR is putting the cart before the horse, and actually putting together a patch should wait until the situation has been discussed so it's in line with the resulting consensus - it would be nice if some of the work done for this could be re-used, but under the circumstances there's obviously going to be no guarantee of that. In the mean time, if anybody wants to improve the current situation, given HTTP::Tiny is designed explicitly for situations where one is forced into minimal dependencies (and hence quite possibly has no CA cert information available at all) arguably the root issue with most of the cpan dists there is that there was never a good reason for them to be using HTTP::Tiny in the first place and patches should be sent replacing it with the full user agent they should have already been using. |
|
@shadowcat-mst I started with the reddit post before I realized that it was a core module, and I made this patch next just to be really clear about what I was proposing. (i.e. it does not "break" anything, and doesn't spam people with warnings, just a single targeted blip on stderr for the cases where it matters, and that can be prevented with an environment variable) I do understand now that perl doesn't ship with CA information, and the module was meant as a bare-minimal http client to make tooling possible. But, 381 CPAN modules have proven that people just really want an in-core https client and that they either don't read the documentation, or don't care about security; it can't help that the module documentation itself seems kind of dismissive about verification like it is nothing more than a money grab by CA companies. Meanwhile, aside from there being an impractical number of CPAN modules to submit bug reports to (and how do you monitor for future ones?) the big problem is that if these authors do update their code with |
|
@nrdvana I think arguably those distributions prove that people will lazily use in-core modules for stupid reasons, which has sadly been proven repeatedly (I'm going to try not to remember any of the previous examples right now for the sake of my remaining SAN score but ... yeah ...). Anyway, it's clear this conversation has already gone horribly wrong and that given the issue has come up repeatedly already I think we're better off going discussion-first rather than patch-first, so I've opened #152 to attempt to make a start at that. |
|
@shadowcat-mst I'm getting offtopic, but I think those distributions prove that people want a batteries-included perl, and that the module name and documentation failed to communicate that it was a partial implementation rather than a minimalist implementation. Seeing "::Tiny" in a name is generally a good thing and for other modules means that the author has reduced the API to the pure essence of the problem. |
Trivial patch from alpine Bug: https://bugs.gentoo.org/905296 See-also: chansen/p5-http-tiny#151 See-also: chansen/p5-http-tiny#152 Signed-off-by: Andreas K. Hüttel <[email protected]>
This module does not perform host checking on SSL connections by default. This is a major security flaw. I would expect most people to agree with that, but given the rationale in the documentation, it sounds like I need to thoroughly make the case for this change.
Violates RFC
This module claims to be RFC compliant, but RFC 2818 says:
Since the RFC says "MUST", all users of a "standards compliant HTTP client" should expect that host name checking happens unless they specifically disable it.
There Is No SSL Without Verifying The Certificate
The module documentation describes SSL encryption as being a separate feature from SSL verification, but the encryption offered by SSL is nearly useless without the verification. In almost any case where an attacker has network access to sniff packets of a communication, they also have the ability to conduct man-in-the-middle attacks, and a man-in-the-middle attack negates any value of the encryption. For example, in modern switched Ethernet a host will not see packets unless the packet arrives at the NIC destined for that host's MAC address. If the packet is destined for the host, then the host can just as easily play man-in-the-middle rather than re-broadcasting the packet to the gateway. If the network provider is the attacker, they control the routing of packets anyway. If the government of the internet backbone is the attacker, they also have control over routing, and so on. There is simply no value in SSL if certificate verification is not enabled. In a context where security is not relevant, people should just use plain HTTP since it is faster.
Nobody Reads the Documentation!
Documenting this security flaw is not sufficient, as proven by the CPAN modules using it. Here's a breakdown of what I found in a dozen CPAN modules I picked at semi-random based on how important they sound:
Proposed Change
I think that with 100+ public modules using HTTP::Tiny and a vast majority of them unaware of their security flaw, the greater number of internal applications that will likewise be affected, and even more untold users of those modules and applications who don't realize they have a security flaw, the only responsible thing to do is start emitting a warning from any script affected. I wrote up a patch that does this, emitting one warning the first time an SSL connection is created without any configuration of the verify_SSL attribute.
I have also added two environment variables, one to change the default (so that end users can quickly enable SSL verification, or decide to silence the warning) and a second to override the attribute globally so that when modules start specifying
verify_SSL => 1the end-users who actually need to disable SSL verification still have a way to do so.My patch does not include unit tests, but if the change is acceptable I will write those next.