From 814abc2c9de4276109f1cb5f13ccc64422c324c1 Mon Sep 17 00:00:00 2001 From: Bryce Kwon Date: Sat, 7 Nov 2026 13:57:12 -1000 Subject: Check that the auth secret is made safely The checks replaced get_secret with a constant, so its success path never ran, and the luaposix stand-in had no link or unlink and a umask that ignored its argument. Dropping the tightened mask, which leaves the secret readable by everyone under the usual 022, or accepting a short secret, passed. The real get_secret now makes the secret in the run's directory through stand-ins that behave like the calls they replace, and the checks cover the mask and its restoration, the link race, the temporary file, reuse and a secret written by hand. --- tests/extensions/test-auth.lua | 90 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 82 insertions(+), 8 deletions(-) (limited to 'tests/extensions/test-auth.lua') diff --git a/tests/extensions/test-auth.lua b/tests/extensions/test-auth.lua index 97639ea..506babf 100644 --- a/tests/extensions/test-auth.lua +++ b/tests/extensions/test-auth.lua @@ -5,7 +5,7 @@ -- load, so the harness meets both with deterministic stand-ins, which keeps -- every check about this script's own logic, the cookie layout, the expiry -- and field rules, the redirect vetting and the action flows, rather than --- about OpenSSL. The signing secret is pinned by replacing get_secret, and +-- about OpenSSL. The signing secret is made in the run's directory, and -- the accounts and access lists are filled through each variant's own -- channel, its commented-out examples switched on for the inline script and -- fixture files reached through a redirected io.open for the file one. Both @@ -58,8 +58,13 @@ local inline_config = { "\tcarol = \"" .. stored_hash .. "\",\n" }, } -local function load_script(changes) - local all = {} +-- Every load keeps its signing secret in the run's directory, at the path +-- given or else at one shared by all of them. +local function load_script(changes, secret_path) + local all = { + { "local secret_filename = \"/var/lib/cgit/auth-secret\"", + "local secret_filename = " .. string.format("%q", secret_path or directory .. "/auth-secret") }, + } if variant == "inline" then for _, change in ipairs(inline_config) do all[#all + 1] = change @@ -69,9 +74,6 @@ local function load_script(changes) all[#all + 1] = change end h.load_with(script, all) - function get_secret() - return string.rep("0123456789abcdef", 4) - end end load_script() @@ -89,6 +91,79 @@ unistd.crypt = function(trypass, setting) return real_crypt(trypass, setting) end +local function read_file(path) + local file = io.open(path, "rb") + if file == nil then + return nil + end + local text = file:read("*a") + file:close() + return text +end + +local function write_file(path, text) + local file = assert(io.open(path, "wb")) + file:write(text) + file:close() +end + +-- The secret is made on first use, so the masks it is made under and the +-- temporary files it is linked from are watched the same way. A secret set +-- in race_winner is written just ahead of the link, the way another request +-- making the file at the same moment would. +local sysstat = require("posix.sys.stat") +local real_umask = sysstat.umask +local umasks = {} +sysstat.umask = function(mask) + local old = real_umask(mask) + umasks[#umasks + 1] = { mask = mask, old = old } + return old +end +local real_link = unistd.link +local linked = {} +local race_winner = nil +unistd.link = function(from, to) + linked[#linked + 1] = from + if race_winner then + write_file(to, race_winner) + end + return real_link(from, to) +end + +local _, made = pcall(get_secret) +h.check("a new secret is 64 hex characters", + type(made) == "string" and #made == 64 and not made:find("%X"), tostring(made)) +h.equals("and is what its file holds", read_file(directory .. "/auth-secret"), made) +h.equals("it is written under a mask that keeps everyone else out", umasks[1] and umasks[1].mask, 63) +h.check("and the mask before it goes back after", + #umasks == 2 and umasks[2].mask == umasks[1].old) +h.equals("it reaches its name through one link", #linked, 1) +h.equals("and the temporary file it was linked from is removed", linked[1] and read_file(linked[1]), nil) +load_script() +h.equals("a later load takes the same secret from the file", select(2, pcall(get_secret)), made) +h.equals("without making another", #linked, 1) + +-- The request that made the secret first keeps it, since it may have +-- signed sessions with it already. +race_winner = string.rep("9", 64) +load_script(nil, directory .. "/raced-secret") +h.equals("a secret made meanwhile by another request wins", select(2, pcall(get_secret)), race_winner) +h.equals("and is left as it was", read_file(directory .. "/raced-secret"), race_winner) +h.equals("while the temporary file is still removed", read_file(linked[#linked]), nil) +race_winner = nil + +-- A secret made by hand is read as its first line, so one saved with a +-- newline still serves, while one of the wrong length is refused. +write_file(directory .. "/typed-secret", made .. "\n") +load_script(nil, directory .. "/typed-secret") +h.equals("a secret ending in a newline is read without it", select(2, pcall(get_secret)), made) +write_file(directory .. "/short-secret", "0123456789\n") +load_script(nil, directory .. "/short-secret") +local short_ok, short_err = pcall(get_secret) +h.check("a secret of the wrong length is refused", not short_ok) +h.contains("as malformed", tostring(short_err), "is malformed, expected 64 hex characters") +load_script() + -- The lookups themselves, which are the part the two variants do not share. local hash = account_hash("ALICE") @@ -323,8 +398,7 @@ h.equals("and leaves the flag out of the way back", way_back_in(out), "/repo/log -- The real get_secret, pointed at a directory nobody made, cannot sign the -- way back, so the page says so whole rather than breaking off in the form. -h.load_with(script, { { "local secret_filename = \"/var/lib/cgit/auth-secret\"", - "local secret_filename = " .. string.format("%q", directory .. "/no-such-directory/auth-secret") } }) +load_script(nil, directory .. "/no-such-directory/auth-secret") out = run_action("body", {}) h.contains("a secret that cannot be made leaves a page saying so", out, "") -- cgit 2.12.1