small medium large xlarge

Generic-user-small
15 Apr 2017, 07:37
Hubert Łępicki (2 posts)

So I just had a scan through the B2.0 version of the e-book and what does scream to me was the casual use of String.to_atom. For example on page 168 of the PDF we have such code:

def handle_in("set_island_coordinates", payload, socket) do
  %{"player" => player, "island" => island, "coordinates" => coordinates} = payload
  player = String.to_atom(player)
  island = String.to_atom(island)
  ...

correct me if I’m wrong, but we are taking here payload from the user, convert it to atoms. I do not think atoms get ever garbage collected so they stay in memory for ever. So we just created a slowly growing memory leak / vector of attack that could crash our VM.

Generic-user-small
16 Apr 2017, 20:25
Lance Halvorsen (28 posts)

You’re right in that this is a potential attack vector. If a malicious person were to repeatedly push messages down the channel with random strings for players, besides “player1” and “player2”, they would eventually crash the BEAM. The same goes for islands. I’ll definitely fix this for the next beta. Thanks!

Generic-user-small
05 May 2017, 20:11
Peer Reynders (29 posts)

Apart from String.to_existing_atom/1 another approach is to use maps containing valid string to atom transforms:

defmodule IslandsInterface.GameChannel do
  use IslandsInterface.Web, :channel

  alias IslandsEngine.Game
  alias IslandsInterface.Presence

  @letters ~W(a b c d e f g h i j)
  @numbers [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
  @keys for letter <- @letters, number <- @numbers, do: "#{letter}#{number}"
  @players [:player1, :player2]
  @islands [:atoll, :dot, :l_shape, :s_shape, :square]
  @s_transform fn name -> {name, String.to_atom(name)} end
  @a_transform fn atom -> {Atom.to_string(atom), atom} end
  @imap Map.new(@islands, @a_transform)
  @kmap Map.new(@keys, @s_transform)
  @pmap Map.new(@players, @a_transform)

  def to_atom(value, map, default \\ :none) do
    case Map.fetch(map, value) do
      {:ok, atom} ->
        atom
      _ ->
        default
    end
  end

  def to_island_atom(name), do: to_atom(name, @imap)

  def to_player_atom(name), do: to_atom(name, @pmap)

  def to_coordinate_atom(name), do: to_atom(name, @kmap)

  # ...

  def handle_in("set_island_coordinates", payload, socket) do
    %{"player" => player_value, "island" => island_value, "coordinates" => coordinates_value} = payload
    player = to_player_atom(player_value)
    island = to_island_atom(island_value)
    coordinates = Enum.map(coordinates_value, &to_coordinate_atom/1)
    case Game.set_island_coordinates({:global, socket.topic}, player, island, coordinates) do
      :ok -> {:reply, :ok, socket}
      :error -> {:reply, :error, socket}
    end
  end

  # ...

Of course it would likely be an improvement if the various to_atom functionalities could just be moved to their respective modules.

You must be logged in to comment