Repository: rrrene/credo
Pull request #570 (2018-07-30 22:14:39)

Add check for unsafe conversions to atoms
Allowing input from unknown sources to be converted into atoms, if exploited, may cause the virtual machine to halt when the atom table overflows. Except in rare circumstances, one should avoid creating atoms at runtime for this reason, instead relying on conversion of input to _existing_ atoms. 


Patch:
diff --git a/lib/credo/check/warning/unsafe_to_atom.ex b/lib/credo/check/warning/unsafe_to_atom.ex
new file mode 100644
index 00000000..537d6fab
--- /dev/null
+++ b/lib/credo/check/warning/unsafe_to_atom.ex
@@ -0,0 +1,86 @@
+defmodule Credo.Check.Warning.UnsafeToAtom do
+  @moduledoc """
+  Creating atoms from unknown or external sources dynamically is a potentially
+  unsafe operation because atoms are not garbage-collected by the runtime.
+
+  Creating an atom from a string or charlist should be done by using
+
+      String.to_existing_atom(string)
+
+  or
+
+      List.to_existing_atom(charlist)
+
+  Module aliases should be constructed using
+
+      Module.safe_concat(prefix, suffix)
+
+  or
+
+      Module.safe_concat([prefix, infix, suffix])
+
+  """
+
+  @explanation [check: @moduledoc]
+
+  use Credo.Check, base_priority: :high, category: :warning
+
+  @doc false
+  def run(source_file, params \\ []) do
+    issue_meta = IssueMeta.for(source_file, params)
+
+    Credo.Code.prewalk(source_file, &traverse(&1, &2, issue_meta))
+  end
+
+  defp traverse({{:., _loc, call}, meta, args} = ast, issues, issue_meta) do
+    case get_forbidden_call(call, args) do
+      {bad, suggestion} ->
+        {ast, issues_for_call(bad, suggestion, meta, issue_meta, issues)}
+
+      nil ->
+        {ast, issues}
+    end
+  end
+
+  defp traverse(ast, issues, _issue_meta) do
+    {ast, issues}
+  end
+
+  defp get_forbidden_call([:erlang, :list_to_atom], [_]) do
+    {":erlang.list_to_atom/1", ":erlang.list_to_existing_atom/1"}
+  end
+
+  defp get_forbidden_call([:erlang, :binary_to_atom], [_, _]) do
+    {":erlang.binary_to_atom/2", ":erlang.binary_to_existing_atom/2"}
+  end
+
+  defp get_forbidden_call([{:__aliases__, _, [:String]}, :to_atom], [_]) do
+    {"String.to_atom/1", "String.to_existing_atom/1"}
+  end
+
+  defp get_forbidden_call([{:__aliases__, _, [:List]}, :to_atom], [_]) do
+    {"List.to_atom/1", "List.to_existing_atom/1"}
+  end
+
+  defp get_forbidden_call([{:__aliases__, _, [:Module]}, :concat], [_]) do
+    {"Module.concat/1", "Module.safe_concat/1"}
+  end
+
+  defp get_forbidden_call([{:__aliases__, _, [:Module]}, :concat], [_, _]) do
+    {"Module.concat/2", "Module.safe_concat/2"}
+  end
+
+  defp get_forbidden_call(_, _) do
+    nil
+  end
+
+  defp issues_for_call(call, suggestion, meta, issue_meta, issues) do
+    options = [
+      message: "Prefer #{suggestion} over #{call} to avoid creating atoms at runtime",
+      trigger: call,
+      line_no: meta[:line]
+    ]
+
+    [format_issue(issue_meta, options) | issues]
+  end
+end


Test patch:
diff --git a/test/credo/check/warning/unsafe_to_atom_test.exs b/test/credo/check/warning/unsafe_to_atom_test.exs
new file mode 100644
index 00000000..9d42f1de
--- /dev/null
+++ b/test/credo/check/warning/unsafe_to_atom_test.exs
@@ -0,0 +1,117 @@
+defmodule Credo.Check.Warning.UnsafeToAtomTest do
+  use Credo.TestHelper
+
+  @described_check Credo.Check.Warning.UnsafeToAtom
+
+  #
+  # cases NOT raising issues
+  #
+
+  test "it should NOT report expected code" do
+    """
+    defmodule CredoSampleModule do
+      def convert_module(parameter) do
+        Module.safe_concat(__MODULE__, parameter)
+      end
+
+      def convert_module_2(parameter1, parameter2) do
+        Module.safe_concat([__MODULE__, parameter1, parameter2])
+      end
+
+      def convert_atom(parameter) do
+        String.to_existing_atom(parameter)
+      end
+
+      def convert_atom_2(parameter) do
+        List.to_existing_atom(parameter)
+      end
+
+      def convert_erlang_list(parameter) do
+        :erlang.list_to_existing_atom(parameter)
+      end
+
+      def convert_erlang_binary(parameter) do
+        :erlang.binary_to_existing_atom(parameter, :utf8)
+      end
+    end
+    """
+    |> to_source_file()
+    |> refute_issues(@described_check)
+  end
+
+  #
+  # cases raising issues
+  #
+
+  test "it should report a violation" do
+    """
+    defmodule CredoSampleModule do
+      def some_function(parameter) do
+        String.to_atom(parameter)
+      end
+    end
+    """
+    |> to_source_file()
+    |> assert_issue(@described_check)
+  end
+
+  test "it should report a violation /2" do
+    """
+    defmodule CredoSampleModule do
+      def some_function(parameter) do
+        List.to_atom(parameter)
+      end
+    end
+    """
+    |> to_source_file()
+    |> assert_issue(@described_check)
+  end
+
+  test "it should report a violation /3" do
+    """
+    defmodule CredoSampleModule do
+      def some_function(parameter) do
+        Module.concat(__MODULE__, parameter)
+      end
+    end
+    """
+    |> to_source_file()
+    |> assert_issue(@described_check)
+  end
+
+  test "it should report a violation /4" do
+    """
+    defmodule CredoSampleModule do
+      def some_function(parameter1, parameter2) do
+        Module.concat([__MODULE__, parameter1, parameter2])
+      end
+    end
+    """
+    |> to_source_file()
+    |> assert_issue(@described_check)
+  end
+
+  test "it should report a violation /5" do
+    """
+    defmodule CredoSampleModule do
+      def some_function(parameter) do
+        :erlang.list_to_atom(parameter)
+      end
+    end
+    """
+    |> to_source_file()
+    |> assert_issue(@described_check)
+  end
+
+  test "it should report a violation /6" do
+    """
+    defmodule CredoSampleModule do
+      def some_function(parameter) do
+        :erlang.binary_to_atom(parameter, :utf8)
+      end
+    end
+    """
+    |> to_source_file()
+    |> assert_issue(@described_check)
+  end
+end