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/harness.lua | 33 ++++++++++++++-- tests/extensions/test-auth.lua | 90 ++++++++++++++++++++++++++++++++++++++---- 2 files changed, 112 insertions(+), 11 deletions(-) (limited to 'tests/extensions') diff --git a/tests/extensions/harness.lua b/tests/extensions/harness.lua index 86f09a6..1a669ff 100644 --- a/tests/extensions/harness.lua +++ b/tests/extensions/harness.lua @@ -263,15 +263,42 @@ local function fake_crypt(password, setting) return prefix .. password end --- The slices of luaossl and luaposix the auth checks reach. +-- The slices of luaossl and luaposix the auth checks reach. link refuses a +-- target that exists, as link(2) does, since that is how a script tells it +-- lost a race to make a file, and umask hands back the mask it replaces, so +-- a check can see the old one put back. function harness.stub_auth_modules() local rand_counter = 0 + local mask = 18 preload("posix.unistd", { crypt = fake_crypt, + link = function(from, to) + local existing = io.open(to, "rb") + if existing then + existing:close() + return nil, to .. ": File exists", 17 + end + local source = assert(io.open(from, "rb")) + local text = source:read("*a") + source:close() + local target = assert(io.open(to, "wb")) + target:write(text) + target:close() + return 0 + end, + unlink = function(path) + local removed, err = os.remove(path) + if not removed then + return nil, err, 2 + end + return 0 + end, }) preload("posix.sys.stat", { - umask = function() - return 18 + umask = function(new) + local old = mask + mask = new + return old end, }) preload("openssl.rand", { 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