Harden and reorganize the auth filters

1 file changed, 345 insertions, 187 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
@@ -1,204 +1,203 @@
--- 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 FILE-BACKED variant. The user accounts, the groups, and
--- the per-repository access lists are read from files on disk. Set their
--- paths below and edit those files without touching this script. This
--- suits larger or externally managed user sets.
+-- This is the FILE-BACKED variant. The user accounts, the groups, and the
+-- per-repository access lists are read from files on disk, whose paths are set
+-- in the CONFIGURATION block below. Edit those files without touching this
+-- script. This suits larger or externally managed user sets.
 --
--- The companion auth-inline.lua behaves identically but keeps its
--- accounts and access lists inline in the script itself, which suits a
--- small fixed set of users.
+-- The companion auth-inline.lua behaves identically but keeps its accounts and
+-- access lists inline in the script itself, which suits a small fixed set of
+-- users.
 --
 -- Enable it in cgitrc with
--- 	auth-filter=lua:/path/to/auth-file.lua
+--     auth-filter=lua:/path/to/auth-file.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")
 
--- This file should contain a series of lines in the form of:
---	username1:hash1
---	username2:hash2
---	username3:hash3
---	...
--- Hashes can be generated using something like `mkpasswd -m sha-512 -R 300000`.
+--
+-- ========================= CONFIGURATION =========================
+-- Edit the values in this block. Nothing below it needs changing for
+-- ordinary use.
+--
+
+-- Accounts, one per line, as username:hash. Generate a hash with
+--     mkpasswd -m sha-512 -R 300000
 -- This file should not be world-readable.
 local users_filename = "/etc/cgit-auth/users"
 
--- This file should contain a series of lines in the form of:
--- 	groupname1:username1,username2,username3,...
---	...
+-- Group membership, one per line, as groupname:user1,user2,user3,...
 local groups_filename = "/etc/cgit-auth/groups"
 
--- This file should contain a series of lines in the form of:
--- 	reponame1:groupname1,groupname2,groupname3,...
---	...
+-- Per-repository access, one per line, as reponame:group1,group2,...
+-- A repository listed here is protected. One not listed is public.
 local repos_filename = "/etc/cgit-auth/repos"
 
--- Set this to a path this script can write to for storing a persistent
--- cookie secret, which should not be world-readable.
+-- 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"
 
+-- How long a login stays valid, in seconds. Default one week.
+local session_seconds = 7 * 24 * 60 * 60
+
+-- Name of the session cookie.
+local cookie_name = "cgitauth"
+
+-- 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 = "/"
+
+-- 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
+
+--
+-- =================================================================
+--
+
+-- 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
+
 --
 --
--- Authentication functions follow below. Swap these out if you want different authentication semantics.
+-- Account and access-list storage. This is the ONLY part that differs from
+-- auth-inline.lua. Swap these two functions to change where accounts live.
 --
 --
 
--- Looks up a hash for a given user.
-function lookup_hash(user)
-	local line
-	for line in io.lines(users_filename) do
+local function trim(s)
+	return (string.gsub(s, "^%s*(.-)%s*$", "%1"))
+end
+
+-- Return the stored password hash for a user, or nil. Reads the users file
+-- fresh each call. A missing or unreadable file, and any unparsable line, are
+-- skipped rather than fatal.
+function account_hash(user)
+	if user == nil then
+		return nil
+	end
+	local wanted = user:lower()
+	local f = io.open(users_filename, "r")
+	if f == nil then
+		return nil
+	end
+	for line in f:lines() do
 		local u, h = string.match(line, "(.-):(.+)")
-		if u:lower() == user:lower() then
+		if u ~= nil and trim(u):lower() == wanted then
+			f:close()
 			return h
 		end
 	end
+	f:close()
 	return nil
 end
 
--- Looks up users for a given repo.
-function lookup_users(repo)
-	local users = nil
+-- Return the set of users allowed to access a repository, keyed by lowercased
+-- username, or nil if the repository is not protected. A protected repository
+-- whose groups resolve to no users returns an EMPTY table, which denies
+-- everyone rather than falling through to public.
+function repo_userset(repo)
+	if repo == nil then
+		return nil
+	end
 	local groups = nil
-	local line, group, user
-	for line in io.lines(repos_filename) do
-		local r, g = string.match(line, "(.-):(.+)")
-		if r == repo then
-			groups = { }
-			for group in string.gmatch(g, "([^,]+)") do
-				groups[group:lower()] = true
+	local f = io.open(repos_filename, "r")
+	if f ~= nil then
+		for line in f:lines() do
+			local r, g = string.match(line, "(.-):(.+)")
+			if r ~= nil and trim(r) == repo then
+				groups = {}
+				for group in string.gmatch(g, "([^,]+)") do
+					groups[trim(group):lower()] = true
+				end
+				break
 			end
-			break
 		end
+		f:close()
 	end
 	if groups == nil then
 		return nil
 	end
-	for line in io.lines(groups_filename) do
-		local g, u = string.match(line, "(.-):(.+)")
-		if groups[g:lower()] then
-			if users == nil then
-				users = { }
-			end
-			for user in string.gmatch(u, "([^,]+)") do
-				users[user:lower()] = true
+	local users = {}
+	local gf = io.open(groups_filename, "r")
+	if gf ~= nil then
+		for line in gf:lines() do
+			local g, u = string.match(line, "(.-):(.+)")
+			if g ~= nil and groups[trim(g):lower()] then
+				for user in string.gmatch(u, "([^,]+)") do
+					users[trim(user):lower()] = true
+				end
 			end
 		end
+		gf:close()
 	end
 	return users
 end
 
-
--- Sets HTTP cookie headers based on post and sets up redirection.
-function authenticate_post()
-	local hash = lookup_hash(post["username"])
-	local redirect = validate_value("redirect", post["redirect"])
-
-	if redirect == nil then
-		not_found()
-		return 0
-	end
-
-	redirect_to(redirect)
-
-	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 = lookup_users(cgit["repo"])
-	if accepted_users == nil then
-		-- We return as valid if the repo is not protected.
-		return 1
-	end
-
-	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
-
--- 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>")
-
-	return 0
-end
-
-
-
---
---
--- Wrapper around filter API, exposing the http table, the cgit table, and the post table to the above functions.
---
---
-
-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, ...)
-
-end
-
-function filter_close()
-	return action()
-end
-
-function filter_write(str)
-	post = parse_qs(str)
-end
-
-
 --
 --
 -- Utility functions based on keplerproject/wsapi.
@@ -225,17 +224,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)
@@ -254,7 +259,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
@@ -265,27 +272,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 = ""
@@ -304,10 +332,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
@@ -327,7 +358,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
 
@@ -336,8 +368,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
@@ -349,11 +381,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
@@ -363,27 +394,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)
@@ -396,3 +431,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