Handle enacl exceptions during signature checks

Fix usage of undecoded signing key during server authentication
Fix several bugs in profile query endpoint

lib/matrix_server.ex

@@ -116,4 +116,14 @@ defmodule MatrixServer do
 
     Ecto.Changeset.validate_change(changeset, field, augmented_func)
   end
+
+  # Returns a Boolean whether the signature is valid.
+  # Also returns false on ArgumentError.
+  def sign_verify(sig, text, key) do
+    try do
+      :enacl.sign_verify_detached(sig, text, key)
+    rescue
+      ArgumentError -> false
+    end
+  end
 end

lib/matrix_server/schema/server_key_info.ex

@@ -33,10 +33,16 @@ defmodule MatrixServer.ServerKeyInfo do
   end
 
   defp refresh_signing_keys(server_name) do
+    # TODO: Handle expired keys.
     in_a_week = System.os_time(:millisecond) + 1000 * 60 * 60 * 24 * 7
     client = FederationClient.client(server_name)
 
-    with {:ok, %GetSigningKeys{verify_keys: verify_keys, valid_until_ts: valid_until}} <-
+    with {:ok,
+          %GetSigningKeys{
+            server_name: server_name,
+            verify_keys: verify_keys,
+            valid_until_ts: valid_until
+          }} <-
            FederationClient.get_signing_keys(client) do
       signing_keys =
         Enum.map(verify_keys, fn {key_id, %{"key" => key}} ->
@@ -47,7 +53,7 @@ defmodule MatrixServer.ServerKeyInfo do
       ski = %ServerKeyInfo{server_name: server_name, valid_until: min(valid_until, in_a_week)}
 
       case upsert_multi(server_name, ski, signing_keys) |> Repo.transaction() do
-        {:ok, %{ski: ski}} -> {:ok, ski}
+        {:ok, %{new_ski: ski}} -> {:ok, ski}
         {:error, _} -> :error
       end
     else
@@ -63,7 +69,7 @@ defmodule MatrixServer.ServerKeyInfo do
       conflict_target: [:server_name]
     )
     |> Multi.insert_all(:insert_keys, SigningKey, signing_keys, on_conflict: :nothing)
-    |> Multi.run(:ski, fn _, _ ->
+    |> Multi.run(:new_ski, fn _, _ ->
       case with_signing_keys(server_name) do
         nil -> {:error, :ski}
         ski -> {:ok, ski}

lib/matrix_server_web/authenticate_server.ex

@@ -1,7 +1,7 @@
 defmodule MatrixServerWeb.AuthenticateServer do
   import MatrixServerWeb.Plug.Error
 
-  alias MatrixServer.SigningKey
+  alias MatrixServer.{SigningKey, ServerKeyInfo}
 
   @auth_header_regex ~r/^X-Matrix origin=(?<origin>.*),key="(?<key>.*)",sig="(?<sig>.*)"$/
 
@@ -27,18 +27,22 @@ defmodule MatrixServerWeb.AuthenticateServer do
   end
 
   defp authenticate_with_headers(headers, object_fun) do
+    # TODO: Only query once per origin.
     headers
     |> parse_authorization_headers()
     |> Enum.find(:error, fn {origin, _, sig} ->
       object = object_fun.(origin)
 
       with {:ok, raw_sig} <- MatrixServer.decode_base64(sig),
-           {:ok, encoded_object} <- MatrixServer.encode_canonical_json(object) do
+           {:ok, encoded_object} <- MatrixServer.encode_canonical_json(object),
-        # TODO: Only query once per origin.
+           {:ok, %ServerKeyInfo{signing_keys: keys}} <-
-        # TODO: Handle expired keys.
+             ServerKeyInfo.with_fresh_signing_keys(origin) do
-        SigningKey.for_server(origin)
+        Enum.find_value(keys, false, fn %SigningKey{signing_key: signing_key} ->
-        |> Enum.find_value(false, fn %SigningKey{signing_key: signing_key} ->
+          with {:ok, decoded_key} <- MatrixServer.decode_base64(signing_key) do
-          :enacl.sign_verify_detached(raw_sig, encoded_object, signing_key)
+            MatrixServer.sign_verify(raw_sig, encoded_object, decoded_key)
+          else
+            _ -> false
+          end
         end)
       else
         _ -> false

lib/matrix_server_web/federation/query_controller.ex

@@ -23,7 +23,7 @@ defmodule MatrixServerWeb.Federation.QueryController do
       |> cast(params, [:user_id, :field])
       |> validate_required([:user_id])
       |> validate_inclusion(:field, ["displayname", "avatar_url"])
-      |> tap(fn
+      |> then(fn
         %Ecto.Changeset{valid?: true} = cs -> {:ok, apply_changes(cs)}
         _ -> :error
       end)
@@ -31,7 +31,7 @@ defmodule MatrixServerWeb.Federation.QueryController do
   end
 
   def profile(conn, params) do
-    with %ProfileRequest{user_id: user_id} <- ProfileRequest.validate(params) do
+    with {:ok, %ProfileRequest{user_id: user_id}} <- ProfileRequest.validate(params) do
       if MatrixServer.get_domain(user_id) == MatrixServer.server_name() do
         localpart = MatrixServer.get_localpart(user_id)
 
@@ -43,10 +43,10 @@ defmodule MatrixServerWeb.Federation.QueryController do
             |> json(%{})
 
           nil ->
-            put_error(:not_found, "User does not exist.")
+            put_error(conn, :not_found, "User does not exist.")
         end
       else
-        put_error(:not_found, "Wrong server name.")
+        put_error(conn, :not_found, "Wrong server name.")
       end
     else
       _ -> put_error(conn, :bad_json)

lib/matrix_server_web/federation_client.ex

@@ -17,39 +17,31 @@ defmodule MatrixServerWeb.FederationClient do
     Tesla.client([{Tesla.Middleware.BaseUrl, "http://" <> server_name} | @middleware], @adapter)
   end
 
-  @path RouteHelpers.key_path(Endpoint, :get_signing_keys)
   def get_signing_keys(client) do
-    # TODO: Which server_name should we take?
+    path = RouteHelpers.key_path(Endpoint, :get_signing_keys)
-    # TODO: Should probably catch enacl exceptions and just return error atom,
+
-    #       create seperate function for this.
     with {:ok,
           %GetSigningKeys{server_name: server_name, verify_keys: verify_keys, signatures: sigs} =
-            response} <- tesla_request(:get, client, @path, GetSigningKeys),
+            response} <- tesla_request(:get, client, path, GetSigningKeys),
-         {:ok, encoded_body} <- MatrixServer.serialize_and_encode(response) do
+         {:ok, encoded_body} <- MatrixServer.serialize_and_encode(response),
+         server_sigs when not is_nil(server_sigs) <- sigs[server_name] do
       # For each verify key, check if there is a matching signature.
       # If not, invalidate the whole response.
-      if Map.has_key?(sigs, server_name) do
+      Enum.all?(verify_keys, fn {key_id, %{"key" => key}} ->
-        server_sigs = sigs[server_name]
+        with true <- Map.has_key?(server_sigs, key_id),
-
+             {:ok, decoded_key} <- MatrixServer.decode_base64(key),
-        all_keys_signed? =
+             {:ok, decoded_sig} <- MatrixServer.decode_base64(server_sigs[key_id]) do
-          Enum.all?(verify_keys, fn {key_id, %{"key" => key}} ->
+          MatrixServer.sign_verify(decoded_sig, encoded_body, decoded_key)
-            with true <- Map.has_key?(server_sigs, key_id),
-                 {:ok, decoded_key} <- MatrixServer.decode_base64(key),
-                 {:ok, decoded_sig} <- MatrixServer.decode_base64(server_sigs[key_id]) do
-              :enacl.sign_verify_detached(decoded_sig, encoded_body, decoded_key)
-            else
-              _ -> false
-            end
-          end)
-
-        if all_keys_signed? do
-          {:ok, response}
         else
-          :error
+          _ -> false
         end
-      else
+      end)
-        :error
+      |> then(fn
-      end
+        true -> {:ok, response}
+        false -> :error
+      end)
+    else
+      _ -> :error
     end
   end