search

chansen / p5-http-tiny

Tiny HTTP Client
https://metacpan.org/dist/HTTP-Tiny
53 stars 52 forks source link

SSL environment controls, and warn about insecure connections #151

Closed nrdvana closed 1 year ago

nrdvana commented 1 year ago

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:

3.1.  Server Identity

   In general, HTTP/TLS requests are generated by dereferencing a URI.
   As a consequence, the hostname for the server is known to the client.
   If the hostname is available, the client MUST check it against the
   server's identity as presented in the server's Certificate message,
   in order to prevent man-in-the-middle attacks.

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 => 1 the 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.

karenetheridge commented 1 year ago

https://www.reddit.com/r/perl/comments/111tadi/psa_httptiny_disabled_ssl_verification_by_default/

stigtsp commented 1 year ago

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 0 to 1 so the modules depending on HTTP::Tiny wouldn't need to be patched.

NixOS has added a patch that does this:

xdg commented 1 year ago

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 HTTPS::Tiny if you desire.

sjn commented 1 year ago

@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.

xdg commented 1 year ago

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.

sjn commented 1 year ago

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?

nrdvana commented 1 year ago

@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?

stigtsp commented 1 year ago

We've searched CPAN to get an overview of how verify_SSL is used:

https://hackeriet.github.io/cpan-http-tiny-overview/

shadowcat-mst commented 1 year ago

@nrdvana @sjn @stigtsp As per the discussion on https://github.com/chansen/p5-http-tiny/issues/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.

stigtsp commented 1 year ago

Patches submitted to some distributions:

nrdvana commented 1 year ago

@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 verify_SSL => 1, there is no way for the end-user to opt-out of the change other than possibly awkward changes to code that they might not be familiar with. There needs to be some quick environment variable like LWP::UserAgent has that lets you connect to in-house stuff that you haven't properly configured the server or the local CA trust store.

shadowcat-mst commented 1 year ago

@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.

nrdvana commented 1 year ago

@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.