Spend a missing account's login on a real hash

The fixed SHA-512 stand-in matched only the documented setup, so a site
hashing with yescrypt, other rounds or bcrypt answered for missing and
real names in different times, and on OpenBSD it failed at once.

4 files changed, 99 insertions, 57 deletions

diff --git a/custom/extensions/auth-file.lua b/custom/extensions/auth-file.luaindex f43eb82..59e05cc 100644--- a/custom/extensions/auth-file.lua+++ b/custom/extensions/auth-file.lua@@ -90,11 +90,6 @@ local cookie_insecure = false -- the password's length, and one longer than this is turned down unchecked. local max_password_bytes = 256 --- A throwaway hash of the documented shape, used only to spend the same work--- on a missing account as on a present one, so a failed login does not reveal--- by timing whether the user name exists.-local dummy_hash = "$6$rounds=300000$0000000000000000$"- -- One request reaches this script as an open, a write and a close, so what the -- open decodes is kept here for the calls that follow. local action, http, cgit, post@@ -129,6 +124,10 @@ end -- The hash is trimmed as well as the name because reading by line strips -- the newline but not a carriage return, so a CRLF users file would hand -- crypt a hash with a trailing \r and fail every login.+--+-- A name with no account comes back as nil and another account's hash, so+-- a failed login can spend the same work on a hash of the same kind and+-- cost, and its timing does not tell whether the name exists. function account_hash(user) 	if user == nil then 		return nil@@ -138,15 +137,19 @@ function account_hash(user) 	if users_file == nil then 		return nil 	end+	local stand_in = nil 	for line in users_file:lines() do 		local name, hash = string.match(line, "(.-):(.+)")-		if name ~= nil and trim(name):lower() == wanted then-			users_file:close()-			return trim(hash)+		if name ~= nil then+			if trim(name):lower() == wanted then+				users_file:close()+				return trim(hash)+			end+			stand_in = stand_in or trim(hash) 		end 	end 	users_file:close()-	return nil+	return nil, stand_in end  -- The users allowed into a repository come back keyed by lowercased name, and@@ -454,10 +457,12 @@ function authenticate_post() 	local password = post["password"] 	local ok = false 	if username ~= nil and password ~= nil and #password <= max_password_bytes then-		local hash = account_hash(username)+		local hash, stand_in = account_hash(username) 		if hash == nil then-			-- Spend the work anyway, see dummy_hash.-			unistd.crypt(password, dummy_hash)+			-- Spend the work anyway, see account_hash.+			if stand_in ~= nil then+				unistd.crypt(password, stand_in)+			end 		elseif constant_equals(hash, unistd.crypt(password, hash)) then 			ok = true 		end
diff --git a/custom/extensions/auth-file.lua b/custom/extensions/auth-file.lua
index f43eb82..59e05cc 100644
--- a/custom/extensions/auth-file.lua
+++ b/custom/extensions/auth-file.lua
@@ -90,11 +90,6 @@ local cookie_insecure = false
 -- the password's length, and one longer than this is turned down unchecked.
 local max_password_bytes = 256
 
--- A throwaway hash of the documented shape, used only to spend the same work
--- on a missing account as on a present one, so a failed login does not reveal
--- by timing whether the user name exists.
-local dummy_hash = "$6$rounds=300000$0000000000000000$"
-
 -- One request reaches this script as an open, a write and a close, so what the
 -- open decodes is kept here for the calls that follow.
 local action, http, cgit, post
@@ -129,6 +124,10 @@ end
 -- The hash is trimmed as well as the name because reading by line strips
 -- the newline but not a carriage return, so a CRLF users file would hand
 -- crypt a hash with a trailing \r and fail every login.
+--
+-- A name with no account comes back as nil and another account's hash, so
+-- a failed login can spend the same work on a hash of the same kind and
+-- cost, and its timing does not tell whether the name exists.
 function account_hash(user)
 	if user == nil then
 		return nil
@@ -138,15 +137,19 @@ function account_hash(user)
 	if users_file == nil then
 		return nil
 	end
+	local stand_in = nil
 	for line in users_file:lines() do
 		local name, hash = string.match(line, "(.-):(.+)")
-		if name ~= nil and trim(name):lower() == wanted then
-			users_file:close()
-			return trim(hash)
+		if name ~= nil then
+			if trim(name):lower() == wanted then
+				users_file:close()
+				return trim(hash)
+			end
+			stand_in = stand_in or trim(hash)
 		end
 	end
 	users_file:close()
-	return nil
+	return nil, stand_in
 end
 
 -- The users allowed into a repository come back keyed by lowercased name, and
@@ -454,10 +457,12 @@ function authenticate_post()
 	local password = post["password"]
 	local ok = false
 	if username ~= nil and password ~= nil and #password <= max_password_bytes then
-		local hash = account_hash(username)
+		local hash, stand_in = account_hash(username)
 		if hash == nil then
-			-- Spend the work anyway, see dummy_hash.
-			unistd.crypt(password, dummy_hash)
+			-- Spend the work anyway, see account_hash.
+			if stand_in ~= nil then
+				unistd.crypt(password, stand_in)
+			end
 		elseif constant_equals(hash, unistd.crypt(password, hash)) then
 			ok = true
 		end
diff --git a/custom/extensions/auth-inline.lua b/custom/extensions/auth-inline.luaindex 53b1efc..3355b5c 100644--- a/custom/extensions/auth-inline.lua+++ b/custom/extensions/auth-inline.lua@@ -90,11 +90,6 @@ local cookie_insecure = false -- the password's length, and one longer than this is turned down unchecked. local max_password_bytes = 256 --- A throwaway hash of the documented shape, used only to spend the same work--- on a missing account as on a present one, so a failed login does not reveal--- by timing whether the user name exists.-local dummy_hash = "$6$rounds=300000$0000000000000000$"- -- One request reaches this script as an open, a write and a close, so what the -- open decodes is kept here for the calls that follow. local action, http, cgit, post@@ -120,11 +115,19 @@ do 	end end +-- A name with no account comes back as nil and another account's hash, so+-- a failed login can spend the same work on a hash of the same kind and+-- cost, and its timing does not tell whether the name exists. function account_hash(user) 	if user == nil then 		return nil 	end-	return users[user:lower()]+	local hash = users[user:lower()]+	if hash ~= nil then+		return hash+	end+	local _, stand_in = next(users)+	return nil, stand_in end  -- The users allowed into a repository come back keyed by lowercased name, and@@ -416,10 +419,12 @@ function authenticate_post() 	local password = post["password"] 	local ok = false 	if username ~= nil and password ~= nil and #password <= max_password_bytes then-		local hash = account_hash(username)+		local hash, stand_in = account_hash(username) 		if hash == nil then-			-- Spend the work anyway, see dummy_hash.-			unistd.crypt(password, dummy_hash)+			-- Spend the work anyway, see account_hash.+			if stand_in ~= nil then+				unistd.crypt(password, stand_in)+			end 		elseif constant_equals(hash, unistd.crypt(password, hash)) then 			ok = true 		end
diff --git a/custom/extensions/auth-inline.lua b/custom/extensions/auth-inline.lua
index 53b1efc..3355b5c 100644
--- a/custom/extensions/auth-inline.lua
+++ b/custom/extensions/auth-inline.lua
@@ -90,11 +90,6 @@ local cookie_insecure = false
 -- the password's length, and one longer than this is turned down unchecked.
 local max_password_bytes = 256
 
--- A throwaway hash of the documented shape, used only to spend the same work
--- on a missing account as on a present one, so a failed login does not reveal
--- by timing whether the user name exists.
-local dummy_hash = "$6$rounds=300000$0000000000000000$"
-
 -- One request reaches this script as an open, a write and a close, so what the
 -- open decodes is kept here for the calls that follow.
 local action, http, cgit, post
@@ -120,11 +115,19 @@ do
 	end
 end
 
+-- A name with no account comes back as nil and another account's hash, so
+-- a failed login can spend the same work on a hash of the same kind and
+-- cost, and its timing does not tell whether the name exists.
 function account_hash(user)
 	if user == nil then
 		return nil
 	end
-	return users[user:lower()]
+	local hash = users[user:lower()]
+	if hash ~= nil then
+		return hash
+	end
+	local _, stand_in = next(users)
+	return nil, stand_in
 end
 
 -- The users allowed into a repository come back keyed by lowercased name, and
@@ -416,10 +419,12 @@ function authenticate_post()
 	local password = post["password"]
 	local ok = false
 	if username ~= nil and password ~= nil and #password <= max_password_bytes then
-		local hash = account_hash(username)
+		local hash, stand_in = account_hash(username)
 		if hash == nil then
-			-- Spend the work anyway, see dummy_hash.
-			unistd.crypt(password, dummy_hash)
+			-- Spend the work anyway, see account_hash.
+			if stand_in ~= nil then
+				unistd.crypt(password, stand_in)
+			end
 		elseif constant_equals(hash, unistd.crypt(password, hash)) then
 			ok = true
 		end
diff --git a/tests/extensions/harness.lua b/tests/extensions/harness.luaindex fc83a73..3df4719 100644--- a/tests/extensions/harness.lua+++ b/tests/extensions/harness.lua@@ -78,6 +78,23 @@ function harness.load(script) 	dofile(script) end +-- Load a script with some of its own text replaced first, for checks that+-- need the commented-out examples of its configuration switched on. Each+-- replacement has to match exactly once, so a reworded example fails here+-- rather than leaving the check to test the unedited configuration.+function harness.load_with(script, replacements)+	local file = assert(io.open(script, "rb"))+	local source = file:read("*a")+	file:close()+	for _, pair in ipairs(replacements) do+		local start = source:find(pair[1], 1, true)+		assert(start and not source:find(pair[1], start + 1, true),+			"expected exactly one " .. pair[1])+		source = source:sub(1, start - 1) .. pair[2] .. source:sub(start + #pair[1])+	end+	assert((loadstring or load)(source, "@" .. script))()+end+ local unpack_args = unpack or table.unpack  -- One whole filter round trip, an open with the given arguments, a write per
diff --git a/tests/extensions/harness.lua b/tests/extensions/harness.lua
index fc83a73..3df4719 100644
--- a/tests/extensions/harness.lua
+++ b/tests/extensions/harness.lua
@@ -78,6 +78,23 @@ function harness.load(script)
 	dofile(script)
 end
 
+-- Load a script with some of its own text replaced first, for checks that
+-- need the commented-out examples of its configuration switched on. Each
+-- replacement has to match exactly once, so a reworded example fails here
+-- rather than leaving the check to test the unedited configuration.
+function harness.load_with(script, replacements)
+	local file = assert(io.open(script, "rb"))
+	local source = file:read("*a")
+	file:close()
+	for _, pair in ipairs(replacements) do
+		local start = source:find(pair[1], 1, true)
+		assert(start and not source:find(pair[1], start + 1, true),
+			"expected exactly one " .. pair[1])
+		source = source:sub(1, start - 1) .. pair[2] .. source:sub(start + #pair[1])
+	end
+	assert((loadstring or load)(source, "@" .. script))()
+end
+
 local unpack_args = unpack or table.unpack
 
 -- One whole filter round trip, an open with the given arguments, a write per
diff --git a/tests/extensions/test-auth.lua b/tests/extensions/test-auth.luaindex 97638bd..e0d2fe5 100644--- a/tests/extensions/test-auth.lua+++ b/tests/extensions/test-auth.lua@@ -5,11 +5,11 @@ -- 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--- the account and access lookups are populated through each variant's own--- channel, the documented account_hash and repo_userset swap points for the--- inline script and fixture files reached through a redirected io.open for--- the file one. Both variants carry the same data so the flow checks read--- identically for the two.+-- 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+-- variants carry the same data so the flow checks read identically for the+-- two.  local test_directory = arg[0]:match("^(.*)/") or "." local h = dofile(test_directory .. "/harness.lua")@@ -43,31 +43,31 @@ if variant == "file" then 	h.redirect_file("/etc/cgit-auth/repos", "auth-repos") end -h.load(script)+if variant == "inline" then+	h.load_with(script, {+		{ "local protected_repos = {\n", "local protected_repos = {\n" ..+			"\t[\"secret-repo\"] = { alice = true, Bob = true },\n" ..+			"\t[\"empty-repo\"] = {},\n" ..+			"\t[\"bare-repo\"] = {},\n" },+		{ "local users = {\n", "local users = {\n" ..+			"\tAlice = \"" .. stored_hash .. "\",\n" },+	})+else+	h.load(script)+end  function get_secret() 	return string.rep("0123456789abcdef", 4) end -if variant == "inline" then-	local accounts = { alice = stored_hash }-	local access = {-		["secret-repo"] = { alice = true, bob = true },-		["empty-repo"] = {},-		["bare-repo"] = {},-	}-	function account_hash(user)-		if user == nil then-			return nil-		end-		return accounts[user:lower()]-	end-	function repo_userset(repo)-		if repo == nil then-			return nil-		end-		return access[repo]-	end+-- crypt is watched through its stand-in module, so a check can see whether+-- a login spent it and on which hash.+local unistd = require("posix.unistd")+local real_crypt = unistd.crypt+local crypt_settings = {}+unistd.crypt = function(trypass, setting)+	crypt_settings[#crypt_settings + 1] = setting+	return real_crypt(trypass, setting) end  -- The lookups themselves, which are the part the two variants do not share.@@ -75,6 +75,8 @@ end local hash = account_hash("ALICE") h.equals("an account is found whatever its case", hash, stored_hash) h.equals("an unknown account is not", account_hash("nobody"), nil)+local _, stand_in = account_hash("nobody")+h.equals("it brings another account's hash to spend instead", stand_in, stored_hash) h.equals("a nil user is not", account_hash(nil), nil)  local userset = repo_userset("secret-repo")@@ -278,27 +280,40 @@ out, ret = run_action("authenticate-post", { }) h.contains("an unknown user is told nothing different", out, "Set-Cookie: cgitauth=; ") +-- A failed login for a name with no account spends crypt on a configured+-- hash, the same work a real check costs, so timing tells nothing.+crypt_settings = {}+run_action("authenticate-post", {+	method = "POST",+	body = "username=nobody&password=x&redirect=" .. url_encode(way_back),+})+h.equals("a missing account spends crypt on a configured hash", crypt_settings[1], stored_hash)++if variant == "file" then+	h.redirect_file("/etc/cgit-auth/users", "auth-users-missing")+	crypt_settings = {}+	run_action("authenticate-post", {+		method = "POST",+		body = "username=nobody&password=x&redirect=" .. url_encode(way_back),+	})+	h.equals("with no accounts at all nothing is spent", #crypt_settings, 0)+	h.redirect_file("/etc/cgit-auth/users", "auth-users")+end+ -- crypt's work grows with the password, so one past the cap is turned down -- before crypt sees it.-local unistd = require("posix.unistd")-local real_crypt = unistd.crypt-local crypt_calls = 0-unistd.crypt = function(...)-	crypt_calls = crypt_calls + 1-	return real_crypt(...)-end+crypt_settings = {} out, ret = run_action("authenticate-post", { 	method = "POST", 	body = "username=Alice&password=" .. string.rep("a", 257) .. "&redirect=" .. url_encode(way_back), }) h.contains("an overlong password is turned down", out, "Set-Cookie: cgitauth=; ")-h.equals("without spending crypt on it", crypt_calls, 0)-out, ret = run_action("authenticate-post", {+h.equals("without spending crypt on it", #crypt_settings, 0)+run_action("authenticate-post", { 	method = "POST", 	body = "username=Alice&password=" .. string.rep("a", 256) .. "&redirect=" .. url_encode(way_back), })-h.equals("a password at the cap is still checked", crypt_calls, 1)-unistd.crypt = real_crypt+h.equals("a password at the cap is still checked", #crypt_settings, 1)  out, ret = run_action("authenticate-post", { 	method = "POST",
diff --git a/tests/extensions/test-auth.lua b/tests/extensions/test-auth.lua
index 97638bd..e0d2fe5 100644
--- a/tests/extensions/test-auth.lua
+++ b/tests/extensions/test-auth.lua
@@ -5,11 +5,11 @@
 -- 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
--- the account and access lookups are populated through each variant's own
--- channel, the documented account_hash and repo_userset swap points for the
--- inline script and fixture files reached through a redirected io.open for
--- the file one. Both variants carry the same data so the flow checks read
--- identically for the two.
+-- 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
+-- variants carry the same data so the flow checks read identically for the
+-- two.
 
 local test_directory = arg[0]:match("^(.*)/") or "."
 local h = dofile(test_directory .. "/harness.lua")
@@ -43,31 +43,31 @@ if variant == "file" then
 	h.redirect_file("/etc/cgit-auth/repos", "auth-repos")
 end
 
-h.load(script)
+if variant == "inline" then
+	h.load_with(script, {
+		{ "local protected_repos = {\n", "local protected_repos = {\n" ..
+			"\t[\"secret-repo\"] = { alice = true, Bob = true },\n" ..
+			"\t[\"empty-repo\"] = {},\n" ..
+			"\t[\"bare-repo\"] = {},\n" },
+		{ "local users = {\n", "local users = {\n" ..
+			"\tAlice = \"" .. stored_hash .. "\",\n" },
+	})
+else
+	h.load(script)
+end
 
 function get_secret()
 	return string.rep("0123456789abcdef", 4)
 end
 
-if variant == "inline" then
-	local accounts = { alice = stored_hash }
-	local access = {
-		["secret-repo"] = { alice = true, bob = true },
-		["empty-repo"] = {},
-		["bare-repo"] = {},
-	}
-	function account_hash(user)
-		if user == nil then
-			return nil
-		end
-		return accounts[user:lower()]
-	end
-	function repo_userset(repo)
-		if repo == nil then
-			return nil
-		end
-		return access[repo]
-	end
+-- crypt is watched through its stand-in module, so a check can see whether
+-- a login spent it and on which hash.
+local unistd = require("posix.unistd")
+local real_crypt = unistd.crypt
+local crypt_settings = {}
+unistd.crypt = function(trypass, setting)
+	crypt_settings[#crypt_settings + 1] = setting
+	return real_crypt(trypass, setting)
 end
 
 -- The lookups themselves, which are the part the two variants do not share.
@@ -75,6 +75,8 @@ end
 local hash = account_hash("ALICE")
 h.equals("an account is found whatever its case", hash, stored_hash)
 h.equals("an unknown account is not", account_hash("nobody"), nil)
+local _, stand_in = account_hash("nobody")
+h.equals("it brings another account's hash to spend instead", stand_in, stored_hash)
 h.equals("a nil user is not", account_hash(nil), nil)
 
 local userset = repo_userset("secret-repo")
@@ -278,27 +280,40 @@ out, ret = run_action("authenticate-post", {
 })
 h.contains("an unknown user is told nothing different", out, "Set-Cookie: cgitauth=; ")
 
+-- A failed login for a name with no account spends crypt on a configured
+-- hash, the same work a real check costs, so timing tells nothing.
+crypt_settings = {}
+run_action("authenticate-post", {
+	method = "POST",
+	body = "username=nobody&password=x&redirect=" .. url_encode(way_back),
+})
+h.equals("a missing account spends crypt on a configured hash", crypt_settings[1], stored_hash)
+
+if variant == "file" then
+	h.redirect_file("/etc/cgit-auth/users", "auth-users-missing")
+	crypt_settings = {}
+	run_action("authenticate-post", {
+		method = "POST",
+		body = "username=nobody&password=x&redirect=" .. url_encode(way_back),
+	})
+	h.equals("with no accounts at all nothing is spent", #crypt_settings, 0)
+	h.redirect_file("/etc/cgit-auth/users", "auth-users")
+end
+
 -- crypt's work grows with the password, so one past the cap is turned down
 -- before crypt sees it.
-local unistd = require("posix.unistd")
-local real_crypt = unistd.crypt
-local crypt_calls = 0
-unistd.crypt = function(...)
-	crypt_calls = crypt_calls + 1
-	return real_crypt(...)
-end
+crypt_settings = {}
 out, ret = run_action("authenticate-post", {
 	method = "POST",
 	body = "username=Alice&password=" .. string.rep("a", 257) .. "&redirect=" .. url_encode(way_back),
 })
 h.contains("an overlong password is turned down", out, "Set-Cookie: cgitauth=; ")
-h.equals("without spending crypt on it", crypt_calls, 0)
-out, ret = run_action("authenticate-post", {
+h.equals("without spending crypt on it", #crypt_settings, 0)
+run_action("authenticate-post", {
 	method = "POST",
 	body = "username=Alice&password=" .. string.rep("a", 256) .. "&redirect=" .. url_encode(way_back),
 })
-h.equals("a password at the cap is still checked", crypt_calls, 1)
-unistd.crypt = real_crypt
+h.equals("a password at the cap is still checked", #crypt_settings, 1)
 
 out, ret = run_action("authenticate-post", {
 	method = "POST",