Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Secure by default #293

Open
Exadra37 opened this issue Apr 20, 2019 · 8 comments · May be fixed by #626
Open

Secure by default #293

Exadra37 opened this issue Apr 20, 2019 · 8 comments · May be fixed by #626

Comments

@Exadra37
Copy link

Exadra37 commented Apr 20, 2019

The Motivation

Since my earlier days as developer I always have been astonished with the mindset of our industry of being insecure by default, where normally security is opt-in, instead of opt-out, and this mindset is one of the main culprits for so many data-breaches that occur every week/month/year.

So the current mindset is to release software with all the doors and windows of your home open, while your are away, and then hope that the developers using it will learn how to properly close all that doors and windows, and then also hope that when they learn how to properly do it, that they don't forget one open.

Just to put things in perspective, try to use shodan.io to see how software ends up in production without being properly secured, due to the current mindset of our industry of preferring convenience over security.

This all started in this tweet and I was asked in this other tweet to open this issue.

The Context

So this have been brought up before by @lau in the issue #235 and by @rawkode in issue #138.

By @lau

The default adapter is httpc, which does not check certificates when using HTTPS. This seems like an unsafe default. Many people will probably use the default and use it to communicate with APIs. Perhaps with sensitive data.

I cannot agree more with this, but the issue on the time was solved with only a note in the README.

Alerts, notes in the README or any other docs will be missed, ignored or forgotten, by beginners, juniors and senior developers, because human beings tend to prefer convenience above anything else., but convenience should not be put in front of Security.

The Proposal

@teamon says on this comment:

The point of Tesla is not to get rid of hackney, nor any other http adapter. The point is to let the end use choose the adapter that suits best or write a custom one if needed.

So in the README we have:

Configure default adapter in config/config.exs (optional).

But instead we should have:

Configure default adapter in config/config.exs (REQUIRED).

And we could then have a list of possible secure by default adapters.

This change would leave Tesla agnostic of an adapter as per the goal of @teamon, but would not leave Tesla insecure by default... The developer would be required to explicitly opt-in for the adapter to use.

So this means that if the Tesla was to be used without configuring the adapter, an exception should be raised with a very clear message.

Once this is a breaking change a major version would be needed.

PS: I appeal to all developers to always embrace Secure By Default aproach when building software.

@chulkilee
Copy link
Contributor

👍

BTW when creating a client dynamically, app config (config/config.exs) may not be used - e.g. using Tesla.client/2.

@valpackett
Copy link
Contributor

btw, to find the default CA bundle, you can do something like

  def default_cert_bundle() do
    cond do
      File.exists?("/etc/ssl/cert.pem") -> "/etc/ssl/cert.pem"
      File.exists?("/etc/pki/tls/cert.pem") -> "/etc/pki/tls/cert.pem"
      File.exists?("/usr/lib/ssl/cert.pem") -> "/usr/lib/ssl/cert.pem"
      File.exists?("/etc/ssl/certs/ca-certificates.crt") -> "/etc/ssl/certs/ca-certificates.crt"
      Code.ensure_loaded(:certifi) == {:module, :certifi} -> apply(:certifi, :cacertfile, [])
      true -> nil
    end
  end

and the ssl_verify_fun library should be used for proper hostname verification, e.g.:

ssl_options: [
  verify: :verify_peer,
  verify_fun: &:ssl_verify_hostname.verify_fun/3,
  depth: 69,
  cacertfile: default_cert_bundle() ]

@teamon teamon added this to To do in v2.0 Mar 3, 2020
@tanguilp
Copy link
Contributor

tanguilp commented Jul 30, 2020

Alerts, notes in the README or any other docs will be missed, ignored or forgotten, by beginners, juniors and senior developers, because human beings tend to prefer convenience above anything else., but convenience should not be put in front of Security.

@Exadra37 has got a point here. I recently shot myself in the foot. I have, too, a background in IT security before I switched to programming, and I'm well aware of how TLS works, and what are the Elixir HTTP libraries which are secure by default and which who are not.

It didn't prevent me from becoming convinced around 2 years ago that the default adapter of Tesla was Hackney, which is secure by default. What a surprise when I read last week the following comment I let 3 years ago in my code:

# default httpc Tesla's adapter is unsafe (does not check TLS certificates)

So the question is not whether a programmer should carefully read the README (by the way the formatting of the warning makes it look less important than the rest of the text!). It seems this security vulnerability will be fixed with v2.0. Meanwhile, I suggest writing a flashy warning at the very beginning of the README. Another option would be to raise on use of https URI with httpc when SSL options were not set.

@Exadra37
Copy link
Author

Well I am astonished that after more then a year have passed by Tesla still continue to use unsafe defaults. Why is security not taken more seriously?

@Exadra37
Copy link
Author

I keep being downvoted and libraries depending on Tesla continue to be vulnerable:

iex> LinkPreview.create "https://badssl.com" 
{:ok,
 %LinkPreview.Page{
   description: "🎛Dashboard",
   images: [%{url: "front-page-icons/chrome.svg"}],
   original_url: "https://badssl.com",
   title: "badssl.com",
   website_url: "badssl.com"
 }}

That request should fail, but once it uses Tesla with the insecure default then it succeeds.

appunite/link_preview#17

@teamon teamon removed this from To do in v2.0 May 8, 2022
@yordis
Copy link
Member

yordis commented Sep 10, 2023

👋🏻 sorry it took me this long to get to this issue. I am trying to do some maintenance.

Before I continue, let me clarify that I **NEVER** use the httpc client ever, and I **AGREE** that the HTTP client should be secure by default. Period.

httpc is the built-in client from Erlang itself. I will continue being the default adapter because anything else will require the installation of extra dependencies.

I am going to try to address the situation and try my best to add sensitive defaults around httpc, being said, let us keep in mind that httpc itself is at fault here to some extent, OTP team should be fixing such situation, to begin with.

@Exadra37 I would love your support since it seems you like the Security topic.

@axelson
Copy link

axelson commented Sep 10, 2023

@voltone (https://elixirforum.com/u/voltone/summary on the ElixirForum) might be another good person to get some ideas from. He's posted quite a bit about security and the beam and is part of the security EEF working group (e.g. here's an old post about httpc security). And here's the related page for httpc https://github.com/erlef/security-wg/blob/92345ab62864ebb4efab11479cc40298f314c47a/docs/secure_coding_and_deployment_hardening/inets.md

You are quite possibly already aware/talking to him but I figured I'd mention him in case it's helpful!

@teamon
Copy link
Member

teamon commented Sep 12, 2023

How about we switch to mint as the default adapter, making :httpc an explicit choice (config :tesla, adapter: Tesla.Adatper.Httpc)? This would require a good error message in case mint is not installed, but I think it's worth it.

yordis added a commit that referenced this issue Oct 10, 2023
fixes #293

Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
@yordis yordis linked a pull request Oct 10, 2023 that will close this issue
yordis added a commit that referenced this issue Oct 10, 2023
fixes #293

Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
yordis added a commit that referenced this issue Oct 10, 2023
fixes #293

Signed-off-by: Yordis Prieto <yordis.prieto@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Idea
Development

Successfully merging a pull request may close this issue.

7 participants