Harden the auth filter headers and session cookie

The filters signed the request url and later wrote it into a Location
header, and the signing step re-encoded a newline that the verifying
step decoded back, so a crafted url could smuggle CR and LF into the
response. The cookie HMAC was also checked with a short-circuiting
comparison and carried no SameSite attribute.

1 file changed, 31 insertions, 5 deletions

diff --git a/extensions/auth-inline.lua b/extensions/auth-inline.lua
index 91e8805..b0d37f1 100644
--- a/extensions/auth-inline.lua
+++ b/extensions/auth-inline.lua
@@ -277,8 +277,8 @@ function validate_value(expected_field, cookie)
 		return nil
 	end
 
-	-- Lua hashes strings, so these comparisons are time invariant.
-	if chmac ~= tohex(hmac.new(get_secret(), "sha256"):final(field .. "|" .. value .. "|" .. tostring(expiration) .. "|" .. salt)) then
+	-- Compare the HMAC without short-circuiting on the first mismatch.
+	if not constant_equals(chmac, tohex(hmac.new(get_secret(), "sha256"):final(field .. "|" .. value .. "|" .. tostring(expiration) .. "|" .. salt))) then
 		return nil
 	end
 
@@ -290,7 +290,13 @@ function validate_value(expected_field, cookie)
 		return nil
 	end
 
-	return url_decode(value)
+	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.
+	if decoded:find("%c") then
+		return nil
+	end
+	return decoded
 end
 
 function secure_value(field, value, expiration)
@@ -307,8 +313,28 @@ function secure_value(field, value, expiration)
 	return authstr
 end
 
+-- Strip control characters that could split an HTTP response header.
+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.
+function constant_equals(a, b)
+	if a == nil or b == nil 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
+	end
+	return diff == 0
+end
+
 function set_cookie(cookie, value)
-	html("Set-Cookie: " .. cookie .. "=" .. value .. "; HttpOnly")
+	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")
 	end
@@ -318,7 +344,7 @@ end
 function redirect_to(url)
 	html("Status: 302 Redirect\n")
 	html("Cache-Control: no-cache, no-store\n")
-	html("Location: " .. url .. "\n")
+	html("Location: " .. strip_ctl(url) .. "\n")
 end
 
 function not_found()