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>

3 files changed, 131 insertions, 94 deletions

diff --git a/cgit.c b/cgit.cindex c52ef33..be1265d 100644--- a/cgit.c+++ b/cgit.c@@ -614,22 +614,19 @@ static inline void open_auth_filter(struct cgit_context *ctx, const char *functi 		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);-	}--	open_auth_filter(ctx, "authenticate-post"); 	char buffer[MAX_AUTHENTICATION_POST_BYTES]; 	int len;++	open_auth_filter(ctx, "authenticate-post"); 	len = ctx->env.content_length; 	if (len > MAX_AUTHENTICATION_POST_BYTES) 		len = MAX_AUTHENTICATION_POST_BYTES;@@ -637,10 +634,7 @@ static inline void authenticate_post(struct cgit_context *ctx) 		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/cgit.c b/cgit.c
index c52ef33..be1265d 100644
--- a/cgit.c
+++ b/cgit.c
@@ -614,22 +614,19 @@ static inline void open_auth_filter(struct cgit_context *ctx, const char *functi
 		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);
-	}
-
-	open_auth_filter(ctx, "authenticate-post");
 	char buffer[MAX_AUTHENTICATION_POST_BYTES];
 	int len;
+
+	open_auth_filter(ctx, "authenticate-post");
 	len = ctx->env.content_length;
 	if (len > MAX_AUTHENTICATION_POST_BYTES)
 		len = MAX_AUTHENTICATION_POST_BYTES;
@@ -637,10 +634,7 @@ static inline void authenticate_post(struct cgit_context *ctx)
 		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.txtindex c45dbd3..682d8bb 100644--- a/cgitrc.5.txt+++ b/cgitrc.5.txt@@ -662,7 +662,8 @@ auth filter:: 	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/cgitrc.5.txt b/cgitrc.5.txt
index c45dbd3..682d8bb 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -662,7 +662,8 @@ auth filter::
 	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.luaindex 4cd4983..5935d08 100644--- a/filters/simple-authentication.lua+++ b/filters/simple-authentication.lua@@ -33,15 +33,28 @@ local secret = "BE SURE TO CUSTOMIZE THIS STRING TO SOMETHING BIG AND RANDOM" -- -- --- 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 @@ function authenticate_cookie() 		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 @@ function body() 	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 @@ function body() 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 @@ end  -- ----- Utility functions follow below, based on keplerproject/wsapi.+-- Utility functions based on keplerproject/wsapi. -- -- @@ -211,6 +156,16 @@ function url_decode(str) 	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+ function parse_qs(qs) 	local tab = {} 	for key, val in string.gmatch(qs, "([^&=]+)=([^&=]*)&?") do@@ -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
diff --git a/filters/simple-authentication.lua b/filters/simple-authentication.lua
index 4cd4983..5935d08 100644
--- a/filters/simple-authentication.lua
+++ b/filters/simple-authentication.lua
@@ -33,15 +33,28 @@ local secret = "BE SURE TO CUSTOMIZE THIS STRING TO SOMETHING BIG AND RANDOM"
 --
 --
 
--- 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 @@ function authenticate_cookie()
 		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 @@ function body()
 	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 @@ function body()
 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 @@ end
 
 --
 --
--- Utility functions follow below, based on keplerproject/wsapi.
+-- Utility functions based on keplerproject/wsapi.
 --
 --
 
@@ -211,6 +156,16 @@ function url_decode(str)
 	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
+
 function parse_qs(qs)
 	local tab = {}
 	for key, val in string.gmatch(qs, "([^&=]+)=([^&=]*)&?") do
@@ -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