From b548968ac6ab5d6c14edcd91c23e4e27dd5d5aa4 Mon Sep 17 00:00:00 2001
From: Tim Wundenberg <tim@wundenbergs.de>
Date: Mon, 2 Sep 2024 11:16:46 +0200
Subject: [PATCH] fix(auth): #121 fix some minor issues and write a few
 comments

---
 service/auth.go | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/service/auth.go b/service/auth.go
index d14efec..55edcc0 100644
--- a/service/auth.go
+++ b/service/auth.go
@@ -1,8 +1,8 @@
 package service
 
 import (
-	"bytes"
 	"crypto/rand"
+	"crypto/subtle"
 	"database/sql"
 	"encoding/base64"
 	"log"
@@ -77,12 +77,22 @@ func HandleSignUp(db *sql.DB) http.HandlerFunc {
 		}
 
 		salt := make([]byte, 16)
-		rand.Read(salt)
+		_, err = rand.Read(salt)
+		if err != nil {
+			log.Printf("Could not generate salt: %v", err)
+			auth.Error("Internal Server Error").Render(r.Context(), w)
+			return
+		}
 
 		hash := getHashPassword(password, salt)
 
 		_, err = db.Exec("INSERT INTO user (user_uuid, email, email_verified, is_admin, password, salt, created_at) VALUES (?, ?, FALSE, FALSE, ?, ?, datetime())", user_uuid, email, hash, salt)
 		if err != nil {
+			// This does leak information about the email being in use, though not specifically stated
+			// It needs to be refacoteres to "If the email is not already in use, an email has been send to your address", or something
+			// The happy path, currently a redirect, needs to send the same message!
+			// Then it is also important to have the same compute time in both paths
+			// Otherwise an attacker could guess emails when comparing the response time
 			if strings.Contains(err.Error(), "email") {
 				auth.Error("Bad Request").Render(r.Context(), w)
 				return
@@ -93,7 +103,7 @@ func HandleSignUp(db *sql.DB) http.HandlerFunc {
 			return
 		}
 
-		result := tryCreateSessionAndSetCookie(w, db, user_uuid)
+		result := tryCreateSessionAndSetCookie(r, w, db, user_uuid)
 		if !result {
 			return
 		}
@@ -122,13 +132,13 @@ func HandleSignIn(db *sql.DB) http.HandlerFunc {
 		if result {
 			new_hash := getHashPassword(password, salt)
 
-			if !bytes.Equal(new_hash, saved_hash) {
+			if subtle.ConstantTimeCompare(new_hash, saved_hash) == 0 {
 				result = false
 			}
 		}
 
 		if result {
-			result := tryCreateSessionAndSetCookie(w, db, user_uuid)
+			result := tryCreateSessionAndSetCookie(r, w, db, user_uuid)
 			if !result {
 				return
 			}
@@ -140,6 +150,7 @@ func HandleSignIn(db *sql.DB) http.HandlerFunc {
 		// If the email is correct, the server will calculate the hash, which will take some time
 		// This way an attacker could guess emails when comparing the response time
 		// Because of that, we cant use WriteHeader in the middle of the function. We have to wait until the end
+		// Unfortunatly this makes the code harder to read
 		time.Sleep(time.Duration(time_to_wait) * time.Millisecond)
 
 		if result {
@@ -202,12 +213,12 @@ func HandleSignOutComp(db *sql.DB) http.HandlerFunc {
 // 	// )
 // )
 
-func tryCreateSessionAndSetCookie(w http.ResponseWriter, db *sql.DB, user_uuid uuid.UUID) bool {
+func tryCreateSessionAndSetCookie(r *http.Request, w http.ResponseWriter, db *sql.DB, user_uuid uuid.UUID) bool {
 	var session_id_bytes []byte = make([]byte, 32)
 	_, err := rand.Reader.Read(session_id_bytes)
 	if err != nil {
 		log.Printf("Could not generate session ID: %v", err)
-		http.Error(w, err.Error(), http.StatusInternalServerError)
+		auth.Error("Internal Server Error").Render(r.Context(), w)
 		return false
 	}
 	session_id := base64.StdEncoding.EncodeToString(session_id_bytes)
@@ -215,7 +226,7 @@ func tryCreateSessionAndSetCookie(w http.ResponseWriter, db *sql.DB, user_uuid u
 	_, err = db.Exec("INSERT INTO session (session_id, user_uuid, created_at) VALUES (?, ?, datetime())", session_id, user_uuid)
 	if err != nil {
 		log.Printf("Could not insert session: %v", err)
-		http.Error(w, err.Error(), http.StatusInternalServerError)
+		auth.Error("Internal Server Error").Render(r.Context(), w)
 		return false
 	}