diff --git a/lib/ex_saml/core/binding.ex b/lib/ex_saml/core/binding.ex index 2c89df1..6857109 100644 --- a/lib/ex_saml/core/binding.ex +++ b/lib/ex_saml/core/binding.ex @@ -33,6 +33,8 @@ defmodule ExSaml.Core.Binding do configured with `use_redirect_for_req: true`. """ + alias ExSaml.Core.Xml.SafeXml + require Record Record.defrecord(:xmlElement, Record.extract(:xmlElement, from_lib: "xmerl/include/xmerl.hrl")) @@ -60,16 +62,12 @@ defmodule ExSaml.Core.Binding do unzip is attempted, falling back to the raw decoded data if decompression fails. The resulting XML string is then parsed with `:xmerl_scan`. """ - @spec decode_response(binary(), binary()) :: xml() + @spec decode_response(binary(), binary()) :: {:ok, xml()} | {:error, :invalid_xml} def decode_response(@deflate, saml_response) do - xml_data = - saml_response - |> :base64.decode() - |> :zlib.unzip() - |> :binary.bin_to_list() - - {xml, _} = :xmerl_scan.string(xml_data, namespace_conformant: true, allow_entities: false) - xml + saml_response + |> :base64.decode() + |> :zlib.unzip() + |> SafeXml.scan() end def decode_response(_encoding, saml_response) do @@ -77,15 +75,14 @@ defmodule ExSaml.Core.Binding do xml_data = try do - :zlib.unzip(data) |> :binary.bin_to_list() + :zlib.unzip(data) rescue - _e -> :binary.bin_to_list(data) + _e -> data catch - _kind, _reason -> :binary.bin_to_list(data) + _kind, _reason -> data end - {xml, _} = :xmerl_scan.string(xml_data, namespace_conformant: true, allow_entities: false) - xml + SafeXml.scan(xml_data) end @doc """ diff --git a/lib/ex_saml/core/sp.ex b/lib/ex_saml/core/sp.ex index f382448..a2b2357 100644 --- a/lib/ex_saml/core/sp.ex +++ b/lib/ex_saml/core/sp.ex @@ -32,7 +32,8 @@ defmodule ExSaml.Core.Sp do SpMetadata, Subject, Util, - Xml.Dsig + Xml.Dsig, + Xml.SafeXml } @type xml :: record(:xmlElement) @@ -298,18 +299,13 @@ defmodule ExSaml.Core.Sp do [{:namespace, ns}] ) do [encrypted] when Record.is_record(encrypted, :xmlElement) -> - try do - decrypted = decrypt_assertion(encrypted, sp) - true = Record.is_record(decrypted, :xmlElement) - - case :xmerl_xpath.string(~c"/saml:Assertion", decrypted, [{:namespace, ns}]) do - [a] -> {:ok, a} - _ -> {:error, :bad_assertion} - end - rescue + with {:ok, decrypted} <- decrypt_assertion(encrypted, sp), + true <- Record.is_record(decrypted, :xmlElement), + [assertion] <- + :xmerl_xpath.string(~c"/saml:Assertion", decrypted, [{:namespace, ns}]) do + {:ok, assertion} + else _ -> {:error, :bad_assertion} - catch - _, _ -> {:error, :bad_assertion} end _ -> @@ -516,13 +512,11 @@ defmodule ExSaml.Core.Sp do assertion_xml = block_decrypt(to_string(algorithm), symmetric_key, cipher_value) - {assertion, _} = - :xmerl_scan.string(:binary.bin_to_list(assertion_xml), - namespace_conformant: true, - allow_entities: false - ) - - assertion + SafeXml.scan(assertion_xml) + rescue + _ -> {:error, :decrypt_failed} + catch + _, _ -> {:error, :decrypt_failed} end defp decrypt_key_info(encrypted_data, key) do diff --git a/lib/ex_saml/core/xml/safe_xml.ex b/lib/ex_saml/core/xml/safe_xml.ex new file mode 100644 index 0000000..f1c1cdd --- /dev/null +++ b/lib/ex_saml/core/xml/safe_xml.ex @@ -0,0 +1,97 @@ +defmodule ExSaml.Core.Xml.SafeXml do + @moduledoc """ + Single entry point for parsing untrusted SAML/XML payloads with `:xmerl_scan`. + + All calls go through this module so that the XXE-safe options + (`allow_entities: false`, `namespace_conformant: true`) and the + binary→charlist conversion are applied **once**, in one place. Callers do + not pass scan options — they cannot weaken the defaults. + + Centralising the wrapper means a future hardening change (new xmerl option, + stricter limits, alternative parser) is a one-file diff. It also + centralises the binary→charlist conversion that previously had to be + patched at every call site (cf. UTF-8 SAMLResponse handling). + + ## Error reporting hook + + An arity-1 function may be configured to receive parse-failure context. + Typical use: forward to a SIEM / OCSF pipeline. + + config :ex_saml, :safe_xml, + on_error: &MyApp.SIEM.report_xml_error/1 + + The handler is invoked **after** the `{:error, :invalid_xml}` tuple is + produced and receives a map: + + %{ + reason: :invalid_xml, + kind: :error | :exit | :throw, + payload: Exception.t() | term() + } + + Any exception raised by the handler is silently caught — the handler must + never be able to mask the original parse failure or alter the caller's + control flow. + """ + + @scan_opts [namespace_conformant: true, allow_entities: false, quiet: true] + + @type xml :: term() + @type error_context :: %{ + required(:reason) => :invalid_xml, + required(:kind) => :error | :exit | :throw, + required(:payload) => Exception.t() | term() + } + + @doc """ + Parse `xml` with `:xmerl_scan` using the hardened option set. + + Accepts either a UTF-8 binary or a charlist. Returns the parsed root on + success or `{:error, :invalid_xml}` on any parse failure. + + Invokes the configured `:on_error` hook (if any) before returning the + error tuple. + """ + @spec scan(binary() | charlist()) :: {:ok, xml()} | {:error, :invalid_xml} + def scan(xml) when is_binary(xml), do: xml |> :binary.bin_to_list() |> scan() + + def scan(xml) when is_list(xml) do + {root, _rest} = :xmerl_scan.string(xml, @scan_opts) + {:ok, root} + rescue + e -> + notify(%{reason: :invalid_xml, kind: :error, payload: e}) + {:error, :invalid_xml} + catch + :exit, reason -> + notify(%{reason: :invalid_xml, kind: :exit, payload: reason}) + {:error, :invalid_xml} + + :throw, value -> + notify(%{reason: :invalid_xml, kind: :throw, payload: value}) + {:error, :invalid_xml} + end + + # --------------------------------------------------------------------------- + # Error reporting hook + # --------------------------------------------------------------------------- + + defp notify(context) do + case Application.get_env(:ex_saml, :safe_xml, [])[:on_error] do + fun when is_function(fun, 1) -> + safe_invoke(fun, context) + + _ -> + :ok + end + end + + defp safe_invoke(fun, context) do + fun.(context) + :ok + rescue + _ -> :ok + catch + _, _ -> :ok + end +end diff --git a/lib/ex_saml/helper.ex b/lib/ex_saml/helper.ex index ab6f327..909baa3 100644 --- a/lib/ex_saml/helper.ex +++ b/lib/ex_saml/helper.ex @@ -108,9 +108,9 @@ defmodule ExSaml.Helper do end defp decode_saml_payload(saml_encoding, saml_payload) do - xml = Core.Binding.decode_response(saml_encoding, saml_payload) - {:ok, xml} - rescue - error -> {:error, {:invalid_response, "#{inspect(error)}"}} + case Core.Binding.decode_response(saml_encoding, saml_payload) do + {:ok, xml} -> {:ok, xml} + {:error, reason} -> {:error, {:invalid_response, reason}} + end end end diff --git a/lib/ex_saml/metadata.ex b/lib/ex_saml/metadata.ex index c83b1cb..fa8906f 100644 --- a/lib/ex_saml/metadata.ex +++ b/lib/ex_saml/metadata.ex @@ -32,6 +32,7 @@ defmodule ExSaml.Metadata do See `ExSaml.Metadata.ValidationResult` for the shape of the returned struct. """ + alias ExSaml.Core.Xml.SafeXml alias ExSaml.Metadata.ValidationResult require Record @@ -100,26 +101,16 @@ defmodule ExSaml.Metadata do # --------------------------------------------------------------------------- defp parse(xml) do - charlist = :binary.bin_to_list(xml) - - try do - {root, _rest} = - :xmerl_scan.string( - charlist, - namespace_conformant: true, - allow_entities: false, - quiet: true - ) - - if Record.is_record(root, :xmlElement) do - {:ok, root} - else + case SafeXml.scan(xml) do + {:ok, root} -> + if Record.is_record(root, :xmlElement) do + {:ok, root} + else + {:error, invalid_xml_violation()} + end + + {:error, _} -> {:error, invalid_xml_violation()} - end - rescue - _ -> {:error, invalid_xml_violation()} - catch - _kind, _reason -> {:error, invalid_xml_violation()} end end diff --git a/test/ex_saml/core/binding_test.exs b/test/ex_saml/core/binding_test.exs index a36d155..f55a1d9 100644 --- a/test/ex_saml/core/binding_test.exs +++ b/test/ex_saml/core/binding_test.exs @@ -101,7 +101,7 @@ defmodule ExSaml.Core.BindingTest do [_, b64_payload] = Regex.run(~r/name="SAMLRequest" value="([^"]+)"/, html) # decode_response with nil encoding (non-deflate path) should parse it - decoded = Binding.decode_response("", b64_payload) + {:ok, decoded} = Binding.decode_response("", b64_payload) assert Record.is_record(decoded, :xmlElement) assert xmlElement(decoded, :name) == :AuthnRequest end @@ -170,7 +170,7 @@ defmodule ExSaml.Core.BindingTest do compressed = :zlib.zip(xml_str) b64 = Base.encode64(compressed) - decoded = + {:ok, decoded} = Binding.decode_response( "urn:oasis:names:tc:SAML:2.0:bindings:URL-Encoding:DEFLATE", b64 @@ -184,7 +184,7 @@ defmodule ExSaml.Core.BindingTest do xml_str = "hello" b64 = Base.encode64(xml_str) - decoded = Binding.decode_response("", b64) + {:ok, decoded} = Binding.decode_response("", b64) assert Record.is_record(decoded, :xmlElement) assert xmlElement(decoded, :name) == :Response @@ -234,7 +234,7 @@ defmodule ExSaml.Core.BindingTest do xml = "" <> sample <> "" b64 = Base.encode64(xml) - decoded = Binding.decode_response("", b64) + {:ok, decoded} = Binding.decode_response("", b64) assert Record.is_record(decoded, :xmlElement) assert xmlElement(decoded, :name) == :Response @@ -248,7 +248,7 @@ defmodule ExSaml.Core.BindingTest do xml = " attr <> "\">x" b64 = Base.encode64(xml) - decoded = Binding.decode_response("", b64) + {:ok, decoded} = Binding.decode_response("", b64) assert Record.is_record(decoded, :xmlElement) assert attribute_value(decoded, :label) == unquote(expected) @@ -261,7 +261,7 @@ defmodule ExSaml.Core.BindingTest do xml = "" <> sample <> "" b64 = xml |> :zlib.zip() |> Base.encode64() - decoded = + {:ok, decoded} = Binding.decode_response( "urn:oasis:names:tc:SAML:2.0:bindings:URL-Encoding:DEFLATE", b64 @@ -283,7 +283,7 @@ defmodule ExSaml.Core.BindingTest do "" b64 = Base.encode64(xml) - decoded = Binding.decode_response("", b64) + {:ok, decoded} = Binding.decode_response("", b64) assert Record.is_record(decoded, :xmlElement) assert xmlElement(decoded, :name) == :"samlp:Response" diff --git a/test/ex_saml/core/xml/safe_xml_test.exs b/test/ex_saml/core/xml/safe_xml_test.exs new file mode 100644 index 0000000..50a9e1d --- /dev/null +++ b/test/ex_saml/core/xml/safe_xml_test.exs @@ -0,0 +1,121 @@ +defmodule ExSaml.Core.Xml.SafeXmlTest do + use ExUnit.Case, async: false + + alias ExSaml.Core.Xml.SafeXml + + require Record + + Record.defrecord(:xmlElement, Record.extract(:xmlElement, from_lib: "xmerl/include/xmerl.hrl")) + + # `async: false` because tests in the `on_error hook` describe mutate + # `Application.put_env(:ex_saml, :safe_xml, ...)`, which is process-global. + + describe "scan/1" do + test "parses a well-formed XML binary" do + assert {:ok, root} = SafeXml.scan("hello") + assert Record.is_record(root, :xmlElement) + assert xmlElement(root, :name) == :root + end + + test "parses a charlist input identically to a binary input" do + xml = "" + assert {:ok, from_binary} = SafeXml.scan(xml) + assert {:ok, from_charlist} = SafeXml.scan(String.to_charlist(xml)) + assert xmlElement(from_binary, :name) == xmlElement(from_charlist, :name) + end + + test "accepts UTF-8 multibyte content in element values" do + assert {:ok, _root} = SafeXml.scan("éàü — ✓") + end + + test "returns {:error, :invalid_xml} on malformed input" do + assert {:error, :invalid_xml} = SafeXml.scan("") + end + + test "returns {:error, :invalid_xml} on empty input" do + assert {:error, :invalid_xml} = SafeXml.scan("") + end + + test "rejects DOCTYPE with an external entity (XXE)" do + xxe = """ + + + ]> + &xxe; + """ + + assert {:error, :invalid_xml} = SafeXml.scan(xxe) + end + + test "rejects DOCTYPE with an internal entity expansion" do + doc = """ + + + ]> + &hello; + """ + + assert {:error, :invalid_xml} = SafeXml.scan(doc) + end + end + + describe ":on_error hook" do + setup do + previous = Application.get_env(:ex_saml, :safe_xml) + + on_exit(fn -> + if previous do + Application.put_env(:ex_saml, :safe_xml, previous) + else + Application.delete_env(:ex_saml, :safe_xml) + end + end) + + :ok + end + + test "is invoked once per parse failure with a context map" do + test_pid = self() + + Application.put_env(:ex_saml, :safe_xml, + on_error: fn ctx -> send(test_pid, {:ctx, ctx}) end + ) + + assert {:error, :invalid_xml} = SafeXml.scan(" send(test_pid, :called) end) + + assert {:ok, _} = SafeXml.scan("") + + refute_receive :called, 50 + end + + test "swallows handler exceptions and still returns the error tuple" do + Application.put_env(:ex_saml, :safe_xml, on_error: fn _ctx -> raise "handler boom" end) + + assert {:error, :invalid_xml} = SafeXml.scan(" :wrong_arity end) + assert {:error, :invalid_xml} = SafeXml.scan("