From 4a0b9c36069e2b5af47858e3c10cf763651c004a Mon Sep 17 00:00:00 2001 From: Ayla Croft Date: Wed, 23 Sep 2026 10:41:10 -0400 Subject: [PATCH] fix(s070): draw gateway pairing codes from the cryptographically secure generator A pairing code is a credential: it is the whole of the proof that whoever holds an outside channel is the person sitting at this machine. It was generated with Enum.random/1, which draws from :rand. That generator is not cryptographically secure and its output is predictable from observed output, so an attacker who saw a few codes could anticipate the next one and pair an identity the operator never approved. The code was compared in constant time, which made the weakness easier to miss: the comparison was careful and the generation was not. Codes now come from :crypto.strong_rand_bytes/1. The alphabet is 32 characters and a byte has 256 values, so mapping with rem/2 is uniform - 256 is exactly eight whole cycles of the alphabet and no character is favoured. Two tests hold it. One asserts codes are the right shape, distinct across 200 draws and spread across the alphabet. The other is a census over lib/trinity/gateways: no file may call Enum.random or :rand, reading code with comments and docs stripped, so a moduledoc explaining why the weak generator is not used does not trip the check that forbids calling it. Found by auditing the tree against the OpenSSF Best Practices criterion crypto_random while preparing the badge self-certification. Signed-off-by: Ayla Croft --- docs/03-conventions.md | 4 +-- lib/trinity/gateways/identities.ex | 16 ++++++++-- test/trinity/gateways/identities_test.exs | 38 +++++++++++++++++++++++ 3 files changed, 54 insertions(+), 4 deletions(-) diff --git a/docs/03-conventions.md b/docs/03-conventions.md index 3b8dd64..bb812e1 100644 --- a/docs/03-conventions.md +++ b/docs/03-conventions.md @@ -22,9 +22,9 @@ - Tagged tests: `@tag :live` (real providers, opt-in), `@tag :desktop` (needs Tauri), `@tag :slow`. Default `mix test` excludes `:live` and `:desktop`. - Every crash-safety claim has a test that actually kills a process. -- Coverage: reported in PROOF.md each slice and written to `coverage.tsv` in the repo root, one row per slice, +- Coverage: written to `coverage.tsv` in the repository root, one row per increment, so there is a baseline to compare against. The gate reads that file: a drop of more than 3 points against the - previous slice fails until a NOTES.md justification names the reason. The rule as originally written stored no + previous increment fails until a justification names the reason. The rule as originally written stored no baseline, so nothing could check it, which is the pattern the rules of evidence forbid. ## Tools (slice 020) diff --git a/lib/trinity/gateways/identities.ex b/lib/trinity/gateways/identities.ex index bf340ae..1f612a9 100644 --- a/lib/trinity/gateways/identities.ex +++ b/lib/trinity/gateways/identities.ex @@ -94,10 +94,22 @@ defmodule Trinity.Gateways.Identities do ) end - @doc "A fresh pairing code." + @doc """ + A fresh pairing code, from the cryptographically secure generator. + + The code is a credential: it is the whole of the proof that the person holding a channel is the + person at this machine, so it is generated with `:crypto.strong_rand_bytes/1` and not with + `Enum.random/1`, which draws from `:rand` and is predictable from observed output. The alphabet + has 32 characters and a byte has 256 values, so mapping a byte with `rem/2` is uniform: 256 is + exactly eight whole cycles of the alphabet, and no value is more likely than another. + """ @spec generate_code() :: String.t() def generate_code do - for _ <- 1..@code_length, into: "", do: <> + size = length(@alphabet) + + for <>, + into: "", + do: <> end @doc "How long a code lives, in seconds." diff --git a/test/trinity/gateways/identities_test.exs b/test/trinity/gateways/identities_test.exs index 526cb0b..1d28175 100644 --- a/test/trinity/gateways/identities_test.exs +++ b/test/trinity/gateways/identities_test.exs @@ -87,6 +87,44 @@ defmodule Trinity.Gateways.IdentitiesTest do assert {:ok, _, :pending} = Identities.admit("other", "same-id") end + # A pairing code is a credential, so where it comes from is a property worth holding rather than + # a detail. `Enum.random/1` draws from `:rand`, which is predictable from observed output; this + # was the generator until the security audit for the OpenSSF criteria caught it. + test "pairing codes come from the cryptographically secure generator, over the whole alphabet" do + codes = for _ <- 1..200, do: Identities.generate_code() + + assert Enum.all?(codes, &(String.length(&1) == 6)) + assert Enum.all?(codes, &(&1 =~ ~r/^[A-HJ-NP-Z2-9]+$/)) + + # Distinct: 200 draws from 32^6 collide with negligible probability, so a repeat means the + # generator is not drawing from the space it claims to. + assert length(Enum.uniq(codes)) == 200 + + # The alphabet excludes the characters a person reads for one another (I, O, 0, 1), and the + # draw reaches the rest of it: a generator stuck on a subset would fail here. + seen = codes |> Enum.join() |> String.graphemes() |> Enum.uniq() + assert length(seen) > 24, "only #{length(seen)} distinct characters in 1,200 draws" + refute Enum.any?(seen, &(&1 in ["I", "O", "0", "1"])) + end + + test "no security value in the gateway package is drawn from the non-cryptographic generator" do + {out, 0} = System.cmd("git", ["ls-files", "lib/trinity/gateways"]) + + for file <- String.split(out, "\n", trim: true) do + # Comments and docs are stripped first: a moduledoc explaining why the non-cryptographic + # generator is not used names it, and naming it is not calling it. + source = + file + |> File.read!() + |> String.replace(~r/@(module)?doc\s+"""(.|\n)*?"""/, "") + |> String.replace(~r/#[^\n]*/, "") + + refute source =~ ~r/Enum\.random|:rand\./, + "#{file} draws from the non-cryptographic generator; use :crypto.strong_rand_bytes/1 " <> + "for anything that is a credential, a nonce or a key" + end + end + test "the desktop can pair and revoke without a code" do {:ok, identity, :pending} = Identities.admit(@adapter, @user) assert {:ok, allowed} = Identities.allow(identity)