Harden and reorganize the auth filters

2 files changed, 667 insertions, 334 deletions

diff --git a/extensions/auth-file.lua b/extensions/auth-file.lua
index 71c8bc6..5415ca7 100644
--- a/extensions/auth-file.lua
+++ b/extensions/auth-file.lua
This diff is too large to be rendered inline. View it on its own page.
diff --git a/extensions/auth-inline.lua b/extensions/auth-inline.lua
index b0d37f1..168a444 100644
--- a/extensions/auth-inline.lua
+++ b/extensions/auth-inline.lua
@@ -1,159 +1,175 @@
--- cgit auth-filter that gates repositories behind a login form and a
--- signed session cookie.
+-- cgit auth-filter that gates repositories behind a login form and a signed
+-- session cookie.
 --
--- This is the INLINE variant. The user accounts and the per-repository
--- access lists are written directly in this script, in the two tables
--- below. Edit them here and reload. This suits a small fixed set of
--- users that rarely changes.
+-- This is the INLINE variant. The user accounts and the per-repository access
+-- lists are written directly in this script, in the two tables in the
+-- CONFIGURATION block below. Edit them here and reload. This suits a small
+-- fixed set of users that rarely changes.
 --
--- The companion auth-file.lua behaves identically but reads its accounts
--- and access lists from files on disk instead, which suits larger or
--- externally managed user sets.
+-- The companion auth-file.lua behaves identically but reads its accounts and
+-- access lists from files on disk instead, which suits larger or externally
+-- managed user sets.
 --
 -- Enable it in cgitrc with
--- 	auth-filter=lua:/path/to/auth-inline.lua
+--     auth-filter=lua:/path/to/auth-inline.lua
 --
--- Requirements
--- 	luaossl
--- 	<http://25thandclement.com/~william/projects/luaossl.html>
--- 	luaposix
--- 	<https://github.com/luaposix/luaposix>
+-- SUPPORTED LUA
 --
+-- Lua 5.1, 5.2, 5.3, 5.4 and LuaJIT. Lua 5.5 is not supported, because luaossl
+-- has no 5.5 build. Match the runtime to the Lua that cgit is built against.
+--
+-- HTTPS IS RECOMMENDED
+--
+-- Serve cgit over HTTPS. Terminate TLS at the web server in front of cgit. The
+-- session cookie is marked Secure by default, so a browser only sends it back
+-- over HTTPS. If you genuinely run cgit over plain HTTP with no TLS anywhere,
+-- set cookie_insecure below, otherwise the cookie is never returned and login
+-- appears to loop.
+--
+-- DEPENDENCIES
+--
+--     luaossl    OpenSSL binding, provides openssl.rand and openssl.hmac
+--                <https://github.com/wahern/luaossl>
+--     luaposix   POSIX binding, provides posix.sys.stat and posix.unistd
+--                <https://github.com/luaposix/luaposix>
+--
+-- The reliable cross-platform install is LuaRocks, matched to your Lua
+-- version. luaossl also needs the OpenSSL development headers present.
+--
+--     # Debian and Ubuntu
+--     sudo apt install luarocks libssl-dev
+--     sudo luarocks --lua-version 5.1 install luaossl
+--     sudo luarocks --lua-version 5.1 install luaposix
+--
+--     # Fedora
+--     sudo dnf install luarocks openssl-devel
+--     sudo luarocks --lua-version 5.1 install luaossl luaposix
+--
+--     # Alpine
+--     sudo apk add luarocks openssl-dev
+--     sudo luarocks-5.1 install luaossl luaposix
+--
+--     # macOS with Homebrew
+--     brew install luarocks openssl
+--     luarocks install luaossl OPENSSL_DIR="$(brew --prefix openssl)"
+--     luarocks install luaposix
+--
+-- Some distributions also package these, for example lua-luaossl and lua-posix
+-- on Debian. If you use a distribution package, make sure it is built for the
+-- same Lua version as cgit.
+--
+-- SECURITY NOTES
+--
+-- The cookie carries only a username with no server-side session store, so
+-- deleting an account does not revoke a cookie already issued until it
+-- expires, and instances that share a secret file accept each other's cookies.
+-- The login form carries no CSRF token. Both are acceptable for gating read
+-- access to a git browser. Weigh them before guarding anything more sensitive.
+
 local sysstat = require("posix.sys.stat")
 local unistd = require("posix.unistd")
 local rand = require("openssl.rand")
 local hmac = require("openssl.hmac")
 
 --
---
--- Configure these variables for your settings.
---
+-- ========================= CONFIGURATION =========================
+-- Edit the values in this block. Nothing below it needs changing for
+-- ordinary use.
 --
 
--- A list of password protected repositories along with the users who can access them.
+-- Protected repositories and the users allowed into each. A repository listed
+-- here is protected. One not listed is public. Keys and user names are matched
+-- case-insensitively. REPLACE THE EXAMPLES BELOW, they are commented out so an
+-- unedited copy protects nothing and grants no accounts.
 local protected_repos = {
-	glouglou	= { laurent = true, jason = true },
-	qt		= { jason = true, bob = true }
+	-- ["secret-repo"] = { alice = true, bob = true },
+	-- ["another"]     = { alice = true },
 }
 
--- A list of users and hashes, generated with `mkpasswd -m sha-512 -R 300000`.
+-- Accounts as name = hash. Generate a hash with
+--     mkpasswd -m sha-512 -R 300000
+-- REPLACE THE EXAMPLES BELOW. The commented lines are not real credentials and
+-- must not be deployed as-is.
 local users = {
-	jason		= "$6$rounds=300000$YYJct3n/o.ruYK$HhpSeuCuW1fJkpvMZOZzVizeLsBKcGA/aF2UPuV5v60JyH2MVSG6P511UMTj2F3H75.IT2HIlnvXzNb60FcZH1",
-	laurent		= "$6$rounds=300000$dP0KNHwYb3JKigT$pN/LG7rWxQ4HniFtx5wKyJXBJUKP7R01zTNZ0qSK/aivw8ywGAOdfYiIQFqFhZFtVGvr11/7an.nesvm8iJUi.",
-	bob		= "$6$rounds=300000$jCLCCt6LUpTz$PI1vvd1yaVYcCzqH8QAJFcJ60b6W/6sjcOsU7mAkNo7IE8FRGW1vkjF8I/T5jt/auv5ODLb1L4S2s.CAyZyUC"
+	-- alice = "$6$rounds=300000$REPLACE_THIS_SALT$REPLACE_THIS_HASH",
+	-- bob   = "$6$rounds=300000$REPLACE_THIS_SALT$REPLACE_THIS_HASH",
 }
 
--- Set this to a path this script can write to for storing a persistent
--- cookie secret, which should be guarded.
+-- Where the cookie-signing secret is stored. It is created on first use. It
+-- must be persistent and writable by cgit. Prefer a path OUTSIDE the cache
+-- root, because pruning the cache would delete a secret kept inside it and
+-- invalidate every live session. This file should not be world-readable.
 local secret_filename = "/var/cache/cgit/auth-secret"
 
---
---
--- Authentication functions follow below. Swap these out if you want different authentication semantics.
---
---
-
--- Sets HTTP cookie headers based on post and sets up redirection.
-function authenticate_post()
-	local hash = users[post["username"]]
-	local redirect = validate_value("redirect", post["redirect"])
-
-	if redirect == nil then
-		not_found()
-		return 0
-	end
-
-	redirect_to(redirect)
+-- How long a login stays valid, in seconds. Default one week.
+local session_seconds = 7 * 24 * 60 * 60
 
-	if hash == nil or hash ~= unistd.crypt(post["password"], hash) then
-		set_cookie("cgitauth", "")
-	else
-		-- One week expiration time
-		local username = secure_value("username", post["username"], os.time() + 604800)
-		set_cookie("cgitauth", username)
-	end
-
-	html("\n")
-	return 0
-end
-
-
--- Returns 1 if the cookie is valid and 0 if it is not.
-function authenticate_cookie()
-	accepted_users = protected_repos[cgit["repo"]]
-	if accepted_users == nil then
-		-- We return as valid if the repo is not protected.
-		return 1
-	end
+-- Name of the session cookie.
+local cookie_name = "cgitauth"
 
-	local username = validate_value("username", get_cookie(http["cookie"], "cgitauth"))
-	if username == nil or not accepted_users[username:lower()] then
-		return 0
-	else
-		return 1
-	end
-end
+-- Path the cookie is scoped to. "/" covers the whole host. Set it to the cgit
+-- root to scope the cookie more tightly.
+local cookie_path = "/"
 
--- Prints the html for the login form.
-function body()
-	html("<h2>Authentication Required</h2>")
-	html("<form method='post' action='")
-	html_attr(cgit["login"])
-	html("'>")
-	html("<input type='hidden' name='redirect' value='")
-	html_attr(secure_value("redirect", cgit["url"], 0))
-	html("' />")
-	html("<table>")
-	html("<tr><td><label for='username'>Username:</label></td><td><input id='username' name='username' autofocus /></td></tr>")
-	html("<tr><td><label for='password'>Password:</label></td><td><input id='password' name='password' type='password' /></td></tr>")
-	html("<tr><td colspan='2'><input value='Login' type='submit' /></td></tr>")
-	html("</table></form>")
+-- Leave false so the cookie is marked Secure and only travels over HTTPS. Set
+-- it true ONLY if cgit is served over plain HTTP with no TLS anywhere, see the
+-- HTTPS note in the header.
+local cookie_insecure = false
 
-	return 0
-end
+--
+-- =================================================================
+--
 
+-- 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 username exists.
+local dummy_hash = "$6$rounds=300000$0000000000000000$"
 
+-- Module state shared across the open, write and close calls of one request.
+local action, http, cgit, post
 
 --
 --
--- Wrapper around filter API, exposing the http table, the cgit table, and the post table to the above functions.
+-- Account and access-list storage. This is the ONLY part that differs from
+-- auth-file.lua. Swap these two functions to change where accounts live.
 --
 --
 
-local actions = {}
-actions["authenticate-post"] = authenticate_post
-actions["authenticate-cookie"] = authenticate_cookie
-actions["body"] = body
-
-function filter_open(...)
-	action = actions[select(1, ...)]
-
-	http = {}
-	http["cookie"] = select(2, ...)
-	http["method"] = select(3, ...)
-	http["query"] = select(4, ...)
-	http["referer"] = select(5, ...)
-	http["path"] = select(6, ...)
-	http["host"] = select(7, ...)
-	http["https"] = select(8, ...)
-
-	cgit = {}
-	cgit["repo"] = select(9, ...)
-	cgit["page"] = select(10, ...)
-	cgit["url"] = select(11, ...)
-	cgit["login"] = select(12, ...)
-
+-- Fold the configured tables to lowercased user names once, so lookups match
+-- case-insensitively the same way auth-file.lua does. Repository names keep
+-- their case.
+do
+	local folded = {}
+	for name, hash in pairs(users) do
+		folded[tostring(name):lower()] = hash
+	end
+	users = folded
+	for repo, set in pairs(protected_repos) do
+		local fs = {}
+		for name, allowed in pairs(set) do
+			fs[tostring(name):lower()] = allowed
+		end
+		protected_repos[repo] = fs
+	end
 end
 
-function filter_close()
-	return action()
+-- Return the stored password hash for a user, or nil.
+function account_hash(user)
+	if user == nil then
+		return nil
+	end
+	return users[user:lower()]
 end
 
-function filter_write(str)
-	post = parse_qs(str)
+-- Return the set of users allowed to access a repository, keyed by lowercased
+-- user name, or nil if the repository is not protected.
+function repo_userset(repo)
+	if repo == nil then
+		return nil
+	end
+	return protected_repos[repo]
 end
 
-
 --
 --
 -- Utility functions based on keplerproject/wsapi.
@@ -180,17 +196,23 @@ function url_encode(str)
 	return str
 end
 
+-- Parse an application/x-www-form-urlencoded body. A value may itself contain
+-- '=', for example a base64 password, so the value runs to the next '&'.
 function parse_qs(qs)
 	local tab = {}
-	for key, val in string.gmatch(qs, "([^&=]+)=([^&=]*)&?") do
+	for key, val in string.gmatch(qs or "", "([^&=]+)=([^&]*)") do
 		tab[url_decode(key)] = url_decode(val)
 	end
 	return tab
 end
 
+-- Return the value of the named cookie, or nil. The stored token was already
+-- url-encoded by secure_value, so it is returned verbatim, which keeps the
+-- write path (set_cookie) and the read path symmetric. Decoding it here would
+-- break the signature check for any value carrying a percent escape.
 function get_cookie(cookies, name)
-	cookies = string.gsub(";" .. cookies .. ";", "%s*;%s*", ";")
-	return url_decode(string.match(cookies, ";" .. name .. "=(.-);"))
+	cookies = string.gsub(";" .. (cookies or "") .. ";", "%s*;%s*", ";")
+	return string.match(cookies, ";" .. name .. "=(.-);")
 end
 
 function tohex(b)
@@ -209,7 +231,9 @@ end
 
 local secret = nil
 
--- Loads a secret from a file, creates a secret, or returns one from memory.
+-- Load the cookie-signing secret, creating it on first use. Failures raise,
+-- which cgit turns into a request error, so a broken secret denies rather than
+-- signs with nothing.
 function get_secret()
 	if secret ~= nil then
 		return secret
@@ -220,27 +244,48 @@ function get_secret()
 		local temporary_filename = secret_filename .. ".tmp." .. tohex(rand.bytes(16))
 		local temporary_file = io.open(temporary_filename, "w")
 		if temporary_file == nil then
-			os.exit(177)
+			sysstat.umask(old_umask)
+			error("cgit auth: cannot create secret file " .. secret_filename)
+		end
+		local wrote = temporary_file:write(tohex(rand.bytes(32)))
+		local closed = temporary_file:close()
+		if not wrote or not closed then
+			os.remove(temporary_filename)
+			sysstat.umask(old_umask)
+			error("cgit auth: failed writing secret file " .. secret_filename)
 		end
-		temporary_file:write(tohex(rand.bytes(32)))
-		temporary_file:close()
-		unistd.link(temporary_filename, secret_filename) -- Intentionally fails in the case that another process is doing the same.
+		unistd.link(temporary_filename, secret_filename) -- Intentionally fails if another worker won the race.
 		unistd.unlink(temporary_filename)
 		sysstat.umask(old_umask)
 		secret_file = io.open(secret_filename, "r")
 	end
 	if secret_file == nil then
-		os.exit(177)
+		error("cgit auth: cannot read secret file " .. secret_filename)
 	end
-	secret = secret_file:read()
+	secret = secret_file:read("*l")
 	secret_file:close()
-	if secret:len() ~= 64 then
-		os.exit(177)
+	if secret == nil or secret:len() ~= 64 then
+		secret = nil
+		error("cgit auth: secret file " .. secret_filename .. " is malformed, expected 64 hex characters")
 	end
 	return secret
 end
 
--- Returns value of cookie if cookie is valid. Otherwise returns nil.
+-- A redirect target is unsafe if a browser would read it as another origin.
+-- The only such form cgit can be tricked into signing is a scheme-relative
+-- "//host" or "/\host". Everything else stays on this host.
+function is_safe_redirect(url)
+	if type(url) ~= "string" then
+		return false
+	end
+	local head = url:sub(1, 2)
+	if head == "//" or head == "/\\" then
+		return false
+	end
+	return true
+end
+
+-- Return the value carried by a signed cookie, or nil if it does not verify.
 function validate_value(expected_field, cookie)
 	local i = 0
 	local value = ""
@@ -259,10 +304,13 @@ function validate_value(expected_field, cookie)
 		elseif i == 1 then
 			value = component
 		elseif i == 2 then
-			expiration = tonumber(component)
-			if expiration == nil then
-				expiration = -1
+			-- The expiration must be a plain integer. Rejecting other forms
+			-- keeps the signed bytes canonical, since tonumber and tostring of
+			-- "1e9" or "100.0" differ across Lua versions.
+			if not string.match(component, "^%d+$") then
+				return nil
 			end
+			expiration = tonumber(component)
 		elseif i == 3 then
 			salt = component
 		elseif i == 4 then
@@ -282,7 +330,8 @@ function validate_value(expected_field, cookie)
 		return nil
 	end
 
-	if expiration == -1 or (expiration ~= 0 and expiration <= os.time()) then
+	-- An expiration of 0 never expires and is used for the redirect token.
+	if expiration ~= 0 and expiration <= os.time() then
 		return nil
 	end
 
@@ -291,8 +340,8 @@ function validate_value(expected_field, cookie)
 	end
 
 	local decoded = url_decode(value)
-	-- Reject values carrying control characters so a signed cookie or
-	-- redirect target cannot smuggle CR or LF into a response header.
+	-- Reject values carrying control characters so a signed value cannot
+	-- smuggle CR or LF into a response header.
 	if decoded:find("%c") then
 		return nil
 	end
@@ -304,11 +353,10 @@ function secure_value(field, value, expiration)
 		return ""
 	end
 
-	local authstr = ""
 	local salt = tohex(rand.bytes(16))
 	value = url_encode(value)
 	field = url_encode(field)
-	authstr = field .. "|" .. value .. "|" .. tostring(expiration) .. "|" .. salt
+	local authstr = field .. "|" .. value .. "|" .. tostring(expiration) .. "|" .. salt
 	authstr = authstr .. "|" .. tohex(hmac.new(get_secret(), "sha256"):final(authstr))
 	return authstr
 end
@@ -318,27 +366,31 @@ function strip_ctl(s)
 	return (string.gsub(s or "", "%c", ""))
 end
 
--- Compare two strings without stopping at the first mismatch, so the time
--- taken does not reveal how many leading bytes already matched.
+-- Compare two strings in time that does not depend on how many leading bytes
+-- match, so a mismatch position is not revealed by timing.
 function constant_equals(a, b)
-	if a == nil or b == nil or #a ~= #b then
+	if type(a) ~= "string" or type(b) ~= "string" or #a ~= #b then
 		return false
 	end
 	local diff = 0
 	for i = 1, #a do
-		if a:byte(i) ~= b:byte(i) then
-			diff = diff + 1
-		end
+		local d = a:byte(i) - b:byte(i)
+		diff = diff + d * d
 	end
 	return diff == 0
 end
 
 function set_cookie(cookie, value)
-	html("Set-Cookie: " .. cookie .. "=" .. strip_ctl(value) .. "; HttpOnly; SameSite=Lax")
-	if http["https"] == "yes" or http["https"] == "on" or http["https"] == "1" then
-		html("; secure")
+	local attrs = "; HttpOnly; SameSite=Lax; Path=" .. cookie_path
+	if not cookie_insecure then
+		attrs = attrs .. "; Secure"
 	end
-	html("\n")
+	if value == "" then
+		attrs = attrs .. "; Max-Age=0"
+	elseif session_seconds > 0 then
+		attrs = attrs .. "; Max-Age=" .. tostring(session_seconds)
+	end
+	html("Set-Cookie: " .. cookie .. "=" .. strip_ctl(value) .. attrs .. "\n")
 end
 
 function redirect_to(url)
@@ -351,3 +403,126 @@ function not_found()
 	html("Status: 404 Not Found\n")
 	html("Cache-Control: no-cache, no-store\n\n")
 end
+
+--
+--
+-- Authentication actions. Identical to auth-inline.lua from here down.
+--
+--
+
+-- Sets HTTP cookie headers based on post and sets up redirection.
+function authenticate_post()
+	local redirect = validate_value("redirect", post["redirect"])
+
+	if redirect == nil or not is_safe_redirect(redirect) then
+		not_found()
+		return 0
+	end
+
+	redirect_to(redirect)
+
+	local username = post["username"]
+	local password = post["password"]
+	local ok = false
+	if username ~= nil and password ~= nil then
+		local hash = account_hash(username)
+		if hash == nil then
+			-- Spend the work anyway, see dummy_hash.
+			unistd.crypt(password, dummy_hash)
+		elseif constant_equals(hash, unistd.crypt(password, hash)) then
+			ok = true
+		end
+	end
+
+	if ok then
+		set_cookie(cookie_name, secure_value("username", username, os.time() + session_seconds))
+	else
+		set_cookie(cookie_name, "")
+	end
+
+	html("\n")
+	return 0
+end
+
+-- Returns 1 if the cookie is valid and 0 if it is not.
+function authenticate_cookie()
+	local accepted_users = repo_userset(cgit["repo"])
+	if accepted_users == nil then
+		-- The repository is not protected.
+		return 1
+	end
+
+	local username = validate_value("username", get_cookie(http["cookie"], cookie_name))
+	if username == nil or not accepted_users[username:lower()] then
+		return 0
+	end
+	return 1
+end
+
+-- Prints the html for the login form.
+function body()
+	local target = cgit["url"]
+	if not is_safe_redirect(target) then
+		target = cgit["login"]
+	end
+
+	html("<h2>Authentication Required</h2>")
+	html("<form method='post' action='")
+	html_attr(cgit["login"])
+	html("'>")
+	html("<input type='hidden' name='redirect' value='")
+	html_attr(secure_value("redirect", target, 0))
+	html("' />")
+	html("<table>")
+	html("<tr><td><label for='username'>Username:</label></td><td><input id='username' name='username' autofocus /></td></tr>")
+	html("<tr><td><label for='password'>Password:</label></td><td><input id='password' name='password' type='password' /></td></tr>")
+	html("<tr><td colspan='2'><input value='Login' type='submit' /></td></tr>")
+	html("</table></form>")
+
+	return 0
+end
+
+--
+--
+-- Wrapper around the filter API, exposing the http, cgit and post tables to
+-- the functions above.
+--
+--
+
+local actions = {}
+actions["authenticate-post"] = authenticate_post
+actions["authenticate-cookie"] = authenticate_cookie
+actions["body"] = body
+
+function filter_open(...)
+	action = actions[select(1, ...)]
+
+	post = {}
+
+	http = {}
+	http["cookie"] = select(2, ...)
+	http["method"] = select(3, ...)
+	http["query"] = select(4, ...)
+	http["referer"] = select(5, ...)
+	http["path"] = select(6, ...)
+	http["host"] = select(7, ...)
+	http["https"] = select(8, ...)
+
+	cgit = {}
+	cgit["repo"] = select(9, ...)
+	cgit["page"] = select(10, ...)
+	cgit["url"] = select(11, ...)
+	cgit["login"] = select(12, ...)
+end
+
+function filter_close()
+	if action == nil then
+		-- Unknown action, deny rather than raise.
+		return 0
+	end
+	return action()
+end
+
+function filter_write(str)
+	post = parse_qs(str)
+end