From c5de486dba9d13750b767db702b07e992bbd3316 Mon Sep 17 00:00:00 2001 From: Pim Kunis Date: Fri, 13 Aug 2021 17:36:34 +0200 Subject: [PATCH] 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 | 10 +++++ lib/matrix_server/schema/server_key_info.ex | 12 +++-- lib/matrix_server_web/authenticate_server.ex | 18 +++++--- .../federation/query_controller.ex | 8 ++-- lib/matrix_server_web/federation_client.ex | 44 ++++++++----------- 5 files changed, 52 insertions(+), 40 deletions(-) diff --git a/lib/matrix_server.ex b/lib/matrix_server.ex index 06965f9..9442f48 100644 --- a/lib/matrix_server.ex +++ b/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 diff --git a/lib/matrix_server/schema/server_key_info.ex b/lib/matrix_server/schema/server_key_info.ex index b1937b9..4f7a97e 100644 --- a/lib/matrix_server/schema/server_key_info.ex +++ b/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} diff --git a/lib/matrix_server_web/authenticate_server.ex b/lib/matrix_server_web/authenticate_server.ex index cecbd84..195176c 100644 --- a/lib/matrix_server_web/authenticate_server.ex +++ b/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=(?.*),key="(?.*)",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 - # TODO: Only query once per origin. - # TODO: Handle expired keys. - SigningKey.for_server(origin) - |> Enum.find_value(false, fn %SigningKey{signing_key: signing_key} -> - :enacl.sign_verify_detached(raw_sig, encoded_object, signing_key) + {:ok, encoded_object} <- MatrixServer.encode_canonical_json(object), + {:ok, %ServerKeyInfo{signing_keys: keys}} <- + ServerKeyInfo.with_fresh_signing_keys(origin) do + Enum.find_value(keys, false, fn %SigningKey{signing_key: signing_key} -> + with {:ok, decoded_key} <- MatrixServer.decode_base64(signing_key) do + MatrixServer.sign_verify(raw_sig, encoded_object, decoded_key) + else + _ -> false + end end) else _ -> false diff --git a/lib/matrix_server_web/federation/query_controller.ex b/lib/matrix_server_web/federation/query_controller.ex index 0568356..b612905 100644 --- a/lib/matrix_server_web/federation/query_controller.ex +++ b/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) diff --git a/lib/matrix_server_web/federation_client.ex b/lib/matrix_server_web/federation_client.ex index faeff5c..93dd339 100644 --- a/lib/matrix_server_web/federation_client.ex +++ b/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? - # TODO: Should probably catch enacl exceptions and just return error atom, - # create seperate function for this. + path = RouteHelpers.key_path(Endpoint, :get_signing_keys) + with {:ok, %GetSigningKeys{server_name: server_name, verify_keys: verify_keys, signatures: sigs} = - response} <- tesla_request(:get, client, @path, GetSigningKeys), - {:ok, encoded_body} <- MatrixServer.serialize_and_encode(response) do + response} <- tesla_request(:get, client, path, GetSigningKeys), + {: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 - server_sigs = sigs[server_name] - - all_keys_signed? = - Enum.all?(verify_keys, fn {key_id, %{"key" => 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} + Enum.all?(verify_keys, fn {key_id, %{"key" => 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 + MatrixServer.sign_verify(decoded_sig, encoded_body, decoded_key) else - :error + _ -> false end - else - :error - end + end) + |> then(fn + true -> {:ok, response} + false -> :error + end) + else + _ -> :error end end