cgit

commit b826537cb4aa2358027ffcb1dd6a87274734e962

Author: Jason A. Donenfeld <Jason@zx2c4.com>

authentication: use hidden form instead of referer

This also gives us some CSRF protection. Note that we make use of the
hmac to protect the redirect value.

Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>

 cgit.c | 22 +--
 cgitrc.5.txt | 3 
 filters/simple-authentication.lua | 200 +++++++++++++++++++-------------


diff --git a/cgit.c b/cgit.c
index c52ef331b76995419aff93935784f1174150a8bc..be1265da6cb03ce99da6e1a68476164d660b10b9 100644
--- a/cgit.c
+++ b/cgit.c
@@ -614,22 +614,19 @@ 		ctx->qry.page ? ctx->qry.page : "",
 		ctx->qry.url ? ctx->qry.url : "");
 }
 
+/* We intentionally keep this rather small, instead of looping and
+ * feeding it to the filter a couple bytes at a time. This way, the
+ * filter itself does not need to handle any denial of service or
+ * buffer bloat issues. If this winds up being too small, people
+ * will complain on the mailing list, and we'll increase it as needed. */
 #define MAX_AUTHENTICATION_POST_BYTES 4096
+/* The filter is expected to spit out "Status: " and all headers. */
 static inline void authenticate_post(struct cgit_context *ctx)
 {
-	if (ctx->env.http_referer && strlen(ctx->env.http_referer) > 0) {
-		html("Status: 302 Redirect\n");
-		html("Cache-Control: no-cache, no-store\n");
-		htmlf("Location: %s\n", ctx->env.http_referer);
-	} else {
-		html("Status: 501 Missing Referer\n");
-		html("Cache-Control: no-cache, no-store\n\n");
-		exit(0);
-	}
+	char buffer[MAX_AUTHENTICATION_POST_BYTES];
+	int len;
 
 	open_auth_filter(ctx, "authenticate-post");
-	char buffer[MAX_AUTHENTICATION_POST_BYTES];
-	int len;
 	len = ctx->env.content_length;
 	if (len > MAX_AUTHENTICATION_POST_BYTES)
 		len = MAX_AUTHENTICATION_POST_BYTES;
@@ -637,10 +634,7 @@ 	if (read(STDIN_FILENO, buffer, len) < 0)
 		die_errno("Could not read POST from stdin");
 	if (write(STDOUT_FILENO, buffer, len) < 0)
 		die_errno("Could not write POST to stdout");
-	/* The filter may now spit out a Set-Cookie: ... */
 	cgit_close_filter(ctx->cfg.auth_filter);
-
-	html("\n");
 	exit(0);
 }
 




diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index c45dbd3681de8f7a258d8f84669d1bc08ca7d6d5..682d8bb46f2202b3df3b469350cdf76974f4ae3b 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -662,7 +662,8 @@ 	the filter action is "authenticate-cookie", this filter must validate
 	the http cookie and return a 0 if it is invalid or 1 if it is invalid,
 	in the exit code / close function. If the filter action is
 	"authenticate-post", this filter receives POST'd parameters on
-	standard input, and should write to output one or more "Set-Cookie"
+	standard input, and should write a complete CGI request, preferably
+	with a 302 redirect, and write to output one or more "Set-Cookie"
 	HTTP headers, each followed by a newline.
 
 	Please see `filters/simple-authentication.lua` for a clear example




diff --git a/filters/simple-authentication.lua b/filters/simple-authentication.lua
index 4cd498313fd72276f760f1ccb23e518e256f36b5..5935d080f87e0388a2dfdcf9b8b1586f93c186d5 100644
--- a/filters/simple-authentication.lua
+++ b/filters/simple-authentication.lua
@@ -33,15 +33,28 @@ -- Authentication functions follow below. Swap these out if you want different authentication semantics.
 --
 --
 
--- Sets HTTP cookie headers based on post
+-- Sets HTTP cookie headers based on post and sets up redirection.
 function authenticate_post()
 	local password = users[post["username"]]
-	-- TODO: Implement time invariant string comparison function to mitigate against timing attack.
+	local redirect = validate_value(post["redirect"])
+
+	if redirect == nil then
+		not_found()
+		return 0
+	end
+
+	redirect_to(redirect)
+
+	-- TODO: Implement time invariant string comparison function to mitigate timing attack.
 	if password == nil or password ~= post["password"] then
-		construct_cookie("", "cgitauth")
+		set_cookie("cgitauth", "")
 	else
-		construct_cookie(post["username"], "cgitauth")
+		-- One week expiration time
+		local username = secure_value(post["username"], os.time() + 604800)
+		set_cookie("cgitauth", username)
 	end
+
+	html("\n")
 	return 0
 end
 
@@ -54,8 +67,8 @@ 		-- We return as valid if the repo is not protected.
 		return 1
 	end
 
-	local username = validate_cookie(get_cookie(http["cookie"], "cgitauth"))
-	if username == nil or not accepted_users[username] then
+	local username = validate_value(get_cookie(http["cookie"], "cgitauth"))
+	if username == nil or not accepted_users[username:lower()] then
 		return 0
 	else
 		return 1
@@ -68,6 +81,9 @@ 	html("

Authentication Required

") html("<form method='post' action='") html_attr(cgit["login"]) html("'>") + html("<input type='hidden' name='redirect' value='") + html_attr(secure_value(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>") @@ -78,81 +94,10 @@ return 0 end --- --- --- Cookie construction and validation helpers. --- --- - -local crypto = require("crypto") - --- Returns username of cookie if cookie is valid. Otherwise returns nil. -function validate_cookie(cookie) - local i = 0 - local username = "" - local expiration = 0 - local salt = "" - local hmac = "" - - if cookie:len() < 3 or cookie:sub(1, 1) == "|" then - return nil - end - - for component in string.gmatch(cookie, "[^|]+") do - if i == 0 then - username = component - elseif i == 1 then - expiration = tonumber(component) - if expiration == nil then - expiration = 0 - end - elseif i == 2 then - salt = component - elseif i == 3 then - hmac = component - else - break - end - i = i + 1 - end - - if hmac == nil or hmac:len() == 0 then - return nil - end - - -- TODO: implement time invariant comparison to prevent against timing attack. - if hmac ~= crypto.hmac.digest("sha1", username .. "|" .. tostring(expiration) .. "|" .. salt, secret) then - return nil - end - - if expiration <= os.time() then - return nil - end - - return username:lower() -end - -function construct_cookie(username, cookie) - local authstr = "" - if username:len() > 0 then - -- One week expiration time - local expiration = os.time() + 604800 - local salt = crypto.hex(crypto.rand.bytes(16)) - - authstr = username .. "|" .. tostring(expiration) .. "|" .. salt - authstr = authstr .. "|" .. crypto.hmac.digest("sha1", authstr, secret) - end - - html("Set-Cookie: " .. cookie .. "=" .. authstr .. "; HttpOnly") - if http["https"] == "yes" or http["https"] == "on" or http["https"] == "1" then - html("; secure") - end - html("\n") -end -- -- --- Wrapper around filter API follows below, exposing the http table, the cgit table, and the post table to the above functions. +-- Wrapper around filter API, exposing the http table, the cgit table, and the post table to the above functions. -- -- @@ -197,7 +142,7 @@ -- -- --- Utility functions follow below, based on keplerproject/wsapi. +-- Utility functions based on keplerproject/wsapi. -- -- @@ -208,6 +153,16 @@ end str = string.gsub(str, "+", " ") str = string.gsub(str, "%%(%x%x)", function(h) return string.char(tonumber(h, 16)) end) str = string.gsub(str, "\r\n", "\n") + return str +end + +function url_encode(str) + if not str then + return "" + end + str = string.gsub(str, "\n", "\r\n") + str = string.gsub(str, "([^%w ])", function (c) return string.format("%%%02X", string.byte(c)) end) + str = string.gsub(str, " ", "+") return str end @@ -223,3 +178,90 @@ function get_cookie(cookies, name) cookies = string.gsub(";" .. cookies .. ";", "%s*;%s*", ";") return url_decode(string.match(cookies, ";" .. name .. "=(.-);")) end + + +-- +-- +-- Cookie construction and validation helpers. +-- +-- + +local crypto = require("crypto") + +-- Returns value of cookie if cookie is valid. Otherwise returns nil. +function validate_value(cookie) + local i = 0 + local value = "" + local expiration = 0 + local salt = "" + local hmac = "" + + if cookie == nil or cookie:len() < 3 or cookie:sub(1, 1) == "|" then + return nil + end + + for component in string.gmatch(cookie, "[^|]+") do + if i == 0 then + value = component + elseif i == 1 then + expiration = tonumber(component) + if expiration == nil then + expiration = 0 + end + elseif i == 2 then + salt = component + elseif i == 3 then + hmac = component + else + break + end + i = i + 1 + end + + if hmac == nil or hmac:len() == 0 then + return nil + end + + -- TODO: implement time invariant comparison to prevent against timing attack. + if hmac ~= crypto.hmac.digest("sha1", value .. "|" .. tostring(expiration) .. "|" .. salt, secret) then + return nil + end + + if expiration ~= 0 and expiration <= os.time() then + return nil + end + + return url_decode(value) +end + +function secure_value(value, expiration) + if value == nil or value:len() <= 0 then + return "" + end + + local authstr = "" + local salt = crypto.hex(crypto.rand.bytes(16)) + value = url_encode(value) + authstr = value .. "|" .. tostring(expiration) .. "|" .. salt + authstr = authstr .. "|" .. crypto.hmac.digest("sha1", authstr, secret) + return authstr +end + +function set_cookie(cookie, value) + html("Set-Cookie: " .. cookie .. "=" .. value .. "; HttpOnly") + if http["https"] == "yes" or http["https"] == "on" or http["https"] == "1" then + html("; secure") + end + html("\n") +end + +function redirect_to(url) + html("Status: 302 Redirect\n") + html("Cache-Control: no-cache, no-store\n") + html("Location: " .. url .. "\n") +end + +function not_found() + html("Status: 404 Not Found\n") + html("Cache-Control: no-cache, no-store\n\n") +end