Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SessionID new on each request (session mw) #2793

Closed
sixcolors opened this issue Jan 7, 2024 · 13 comments
Closed

SessionID new on each request (session mw) #2793

sixcolors opened this issue Jan 7, 2024 · 13 comments

Comments

@sixcolors
Copy link
Member

sixcolors commented Jan 7, 2024

if I use SessionStore I found that on each request session expiration is updated as well as sessionid` is changed upon each request.
I was expecting sessionid to remain the same throughout the session period before expiration.
Should I open new issue for this?

Originally posted by @rngallen in #2741 (comment)

@rngallen can you please post a sample of your code here? The session mw should not be creating new session id for each session.

@rngallen
Copy link

rngallen commented Jan 7, 2024

I used code from https://github.com/gofiber/recipes/tree/master/csrf-with-session to demonstrate
At first was my mistake i forget to delete some testing codes, now session and csrf works perfectly, I then tested to put encryptcookie middleware https://docs.gofiber.io/api/middleware/encryptcookie#usage-with-other-middlewares-that-reads-or-modify-cookies as per documentation. But now it started to re-create session id on each request.

You can go to Cookies Storage on your browser and you will observe that sessionid is changing on each request. If Js framework like Next Js is used on front it will be hard/unproductive to save/update sessionid on each request. Because after login sessionid and csrf token will be saven on session storage and to be send on each future request(s).

Though it won't affect those using MVC architecture (like example on recipes) because cookie(s) on storage will be updated automatically

package main

import (
	"time"

	"github.com/gofiber/fiber/v2"
	"github.com/gofiber/fiber/v2/log"
	"github.com/gofiber/fiber/v2/middleware/csrf"
	"github.com/gofiber/fiber/v2/middleware/encryptcookie"
	"github.com/gofiber/fiber/v2/middleware/session"
	"github.com/gofiber/template/html/v2"
	"golang.org/x/crypto/bcrypt"
)

// User represents a user in the dummy authentication system
type User struct {
	Username string
	Password string
}

// Dummy user database
var users map[string]User

func main() {

	// In production, run the app on port 443 with TLS enabled
	// or run the app behind a reverse proxy that handles TLS.
	//
	// It is also recommended that the csrf cookie is set to be
	// Secure and HttpOnly and have the SameSite attribute set
	// to Lax or Strict.
	//
	// In this example, we use the "__Host-" prefix for cookie names.
	// This is suggested when your app uses secure connections (TLS).
	// A cookie with this prefix is only accepted if it's secure,
	// comes from a secure source, doesn't have a Domain attribute,
	// and its Path attribute is "/".
	// This makes these cookies "locked" to the domain.
	//
	// See the following for more details:
	// https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html
	//
	// It's recommended to use the "github.com/gofiber/fiber/v2/middleware/helmet"
	// middleware to set headers to help prevent attacks such as XSS, man-in-the-middle,
	// protocol downgrade, cookie hijacking, SSL stripping, clickjacking, etc.

	// Never hardcode passwords in production code
	hashedPasswords := make(map[string]string)
	for username, password := range map[string]string{"user1": "password1", "user2": "password2"} {
		hashedPassword, err := bcrypt.GenerateFromPassword([]byte(password), bcrypt.DefaultCost)
		if err != nil {
			log.Fatal(err)
		}
		hashedPasswords[username] = string(hashedPassword)
	}

	// Used to help prevent timing attacks
	emptyHash, err := bcrypt.GenerateFromPassword([]byte(""), bcrypt.DefaultCost)
	if err != nil {
		log.Fatal(err)
	}
	emptyHashString := string(emptyHash)

	users := make(map[string]User)
	for username, hashedPassword := range hashedPasswords {
		users[username] = User{Username: username, Password: hashedPassword}
	}

	// HTML templates
	engine := html.New("./views", ".html")

	// Create a Fiber app
	app := fiber.New(fiber.Config{
		Views:       engine,
		ViewsLayout: "layouts/main",
	})

	app.Use(encryptcookie.New(encryptcookie.Config{    //<<== will cause sessionid to be updated/changed on each request
		Except: []string{csrf.ConfigDefault.CookieName},
		Key:    "t+A2pNQ+GM117Uj7AhaHq/BwjWzZwBT9crgOSY6eWjA=",
	}))

	// Initialize a session store
	sessConfig := session.Config{
		Expiration:     30 * time.Minute,        // Expire sessions after 30 minutes of inactivity
		KeyLookup:      "cookie:__Host-session", // Recommended to use the __Host- prefix when serving the app over TLS
		CookieSecure:   true,
		CookieHTTPOnly: true,
		CookieSameSite: "Strict",
	}
	store := session.New(sessConfig)

	// CSRF Error handler
	csrfErrorHandler := func(c *fiber.Ctx, err error) error {
		// Log the error so we can track who is trying to perform CSRF attacks
		// customize this to your needs
		log.Warnf("CSRF Error: %v Request: %v From: %v\n", err, c.OriginalURL(), c.IP())

		// check accepted content types
		switch c.Accepts("html", "json") {
		case "json":
			// Return a 403 Forbidden response for JSON requests
			return c.Status(fiber.StatusForbidden).JSON(fiber.Map{
				"error": "403 Forbidden",
			})
		case "html":
			// Return a 403 Forbidden response for HTML requests
			return c.Status(fiber.StatusForbidden).Render("error", fiber.Map{
				"Title":     "Error",
				"Error":     "403 Forbidden",
				"ErrorCode": "403",
			})
		default:
			// Return a 403 Forbidden response for all other requests
			return c.Status(fiber.StatusForbidden).SendString("403 Forbidden")
		}
	}

	// Configure the CSRF middleware
	csrfConfig := csrf.Config{
		Session:    store,
		KeyLookup:  "form:csrf", // In this example, we will be using a hidden input field to store the CSRF token
		CookieName: csrf.ConfigDefault.CookieName,
		// CookieName:     "__Host-csrf", // Recommended to use the __Host- prefix when serving the app over TLS
		CookieSameSite: "Lax", // Recommended to set this to Lax or Strict
		CookieSecure:   true,  // Recommended to set to true when serving the app over TLS
		CookieHTTPOnly: true,  // Recommended, otherwise if using JS framework recomend: false and KeyLookup: "header:X-CSRF-Token"
		ContextKey:     "csrf",
		ErrorHandler:   csrfErrorHandler,
		Expiration:     30 * time.Minute,
	}
	csrfMiddleware := csrf.New(csrfConfig)

	// app.Use(csrf)

	// Route for the root path
	app.Get("/", func(c *fiber.Ctx) error {
		// render the root page as HTML
		return c.Render("index", fiber.Map{
			"Title": "Index",
		})
	})

	// Route for the login page
	app.Get("/login", csrfMiddleware, func(c *fiber.Ctx) error {
		csrfToken, ok := c.Locals("csrf").(string)
		if !ok {
			return c.SendStatus(fiber.StatusInternalServerError)
		}

		return c.Render("login", fiber.Map{
			"Title": "Login",
			"csrf":  csrfToken,
		})
	})

	// Route for processing the login
	app.Post("/login", csrfMiddleware, func(c *fiber.Ctx) error {
		// Retrieve the submitted form data
		username := c.FormValue("username")
		password := c.FormValue("password")

		// Check if the credentials are valid
		user, exists := users[username]
		var checkPassword string
		if exists {
			checkPassword = user.Password
		} else {
			checkPassword = emptyHashString
		}

		if bcrypt.CompareHashAndPassword([]byte(checkPassword), []byte(password)) != nil {
			// Authentication failed
			csrfToken, ok := c.Locals("csrf").(string)
			if !ok {
				return c.SendStatus(fiber.StatusInternalServerError)
			}

			return c.Render("login", fiber.Map{
				"Title": "Login",
				"csrf":  csrfToken,
				"error": "Invalid credentials",
			})
		}

		// Set a session variable to mark the user as logged in
		session, err := store.Get(c)
		if err != nil {
			return c.SendStatus(fiber.StatusInternalServerError)
		}
		if err := session.Reset(); err != nil {
			return c.SendStatus(fiber.StatusInternalServerError)
		}
		session.Set("loggedIn", true)
		if err := session.Save(); err != nil {
			return c.SendStatus(fiber.StatusInternalServerError)
		}

		// Redirect to the protected route
		return c.Redirect("/protected")
	})

	// Route for logging out
	app.Get("/logout", func(c *fiber.Ctx) error {
		// Retrieve the session
		session, err := store.Get(c)
		if err != nil {
			return c.SendStatus(fiber.StatusInternalServerError)
		}

		// Revoke users authentication
		if err := session.Destroy(); err != nil {
			return c.SendStatus(fiber.StatusInternalServerError)
		}

		// Redirect to the login page
		return c.Redirect("/login")
	})

	// Route for the protected content
	app.Get("/protected", csrfMiddleware, func(c *fiber.Ctx) error {
		// Check if the user is logged in
		session, err := store.Get(c)
		if err != nil {
			return c.SendStatus(fiber.StatusInternalServerError)
		}
		loggedIn, _ := session.Get("loggedIn").(bool)

		if !loggedIn {
			// User is not authenticated, redirect to the login page
			return c.Redirect("/login")
		}

		csrfToken, ok := c.Locals("csrf").(string)
		if !ok {
			return c.SendStatus(fiber.StatusInternalServerError)
		}

		return c.Render("protected", fiber.Map{
			"Title": "Protected",
			"csrf":  csrfToken,
		})
	})

	// Route for processing the protected form
	app.Post("/protected", csrfMiddleware, func(c *fiber.Ctx) error {
		// Check if the user is logged in
		session, err := store.Get(c)
		if err != nil {
			return c.SendStatus(fiber.StatusInternalServerError)
		}
		loggedIn, _ := session.Get("loggedIn").(bool)

		if !loggedIn {
			// User is not authenticated, redirect to the login page
			return c.Redirect("/login")
		}

		csrfToken, ok := c.Locals("csrf").(string)
		if !ok {
			return c.SendStatus(fiber.StatusInternalServerError)
		}

		// Retrieve the submitted form data
		message := c.FormValue("message")

		return c.Render("protected", fiber.Map{
			"Title":   "Protected",
			"csrf":    csrfToken,
			"message": message,
		})
	})

	log.Fatal(app.Listen(":3030"))
}

@sixcolors
Copy link
Member Author

sixcolors commented Jan 7, 2024

Are you running the example code over https, eg behind a reverse proxy?

@rngallen
Copy link

rngallen commented Jan 7, 2024

No, not over https

@sixcolors
Copy link
Member Author

sixcolors commented Jan 7, 2024

CookieSecure = false
CookieSameSite: "Lax"

for both csrf and session.

@sixcolors
Copy link
Member Author

and KeyLookup: "cookie:__Host-session", should be KeyLookup: "cookie:session",

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#attributes

You can't use __Host- with non-https.

@sixcolors
Copy link
Member Author

@sixcolors
Copy link
Member Author

sixcolors commented Jan 7, 2024

Also because you are exempting csrf cookie, but not session it's encrypting the cookie value for session. Although the id of the session only changes on auth (per example), it is stable once logged in. It's just the cookie encrypted value that's changing, because it's setting a new cookie each request [even though the id is the same].

	// app.Use(encryptcookie.New(encryptcookie.Config{ //<<== will cause sessionid to be updated/changed on each request
	// 	Except: []string{csrf.ConfigDefault.CookieName},
	// 	Key:    "t+A2pNQ+GM117Uj7AhaHq/BwjWzZwBT9crgOSY6eWjA=",
	// }))

you could:

	app.Use(encryptcookie.New(encryptcookie.Config{ //<<== will cause sessionid to be updated/changed on each request
		Except: []string{csrf.ConfigDefault.CookieName, "session"}, // except csrf and session cookies
		Key:    "t+A2pNQ+GM117Uj7AhaHq/BwjWzZwBT9crgOSY6eWjA=",
	}))

But I would ask, why are you encrypting cookies? There is no security value in encrypting session and csrf cookies.

@rngallen
Copy link

rngallen commented Jan 7, 2024

So what the idea of using encryptCookie if session id will not be encrypted I miss the point here.

@sixcolors
Copy link
Member Author

sixcolors commented Jan 7, 2024

I do not recommend using encryptCookie for session and CSRF tokens or any other token, as it simply replaces one token with another while adding the overhead of encryption and decryption.

Some might choose to use encrypted cookies in cases where they have a cookie whose value contains data they do not want the user to see. Old frameworks from the 2000s, like Ruby on Rails, used this technique to store login and role information in a cookie. However, this use case is no longer considered secure. I cannot think of a valid use case for encrypted cookies, particularly when the server has a database and cache available.

@sixcolors
Copy link
Member Author

You can see the behaviour here:

// ...
app.Use(encryptcookie.New(encryptcookie.Config{ //<<== will cause sessionid to be updated/changed on each request
		Except: []string{csrf.ConfigDefault.CookieName, "session"}, // except csrf and session cookies
		Key:    "t+A2pNQ+GM117Uj7AhaHq/BwjWzZwBT9crgOSY6eWjA=",
	}))

	stableCookieMW := func(c *fiber.Ctx) error {
		// check for cookie
		cookie := c.Cookies("foo")
		if cookie != "" {
			// cookie exists
			log.Info("cookie \"foo\" exists and has value: \"", cookie, "\"")
			// return c.Next() //<<== commenting this out will cause "foo" cookie to be updated/changed on each request
		}

		// set a cookie with name "foo" and value "bar"
		c.Cookie(&fiber.Cookie{
			Name:  "foo",
			Value: "bar",
		})
		return c.Next()
	}

	app.Use(stableCookieMW)
// ...

@sixcolors
Copy link
Member Author

sixcolors commented Jan 7, 2024

The reason for receiving new cookie values on every request where the cookie is set, despite the value string remaining the same, is that the EncryptCookie function is nondeterministic. It uses a random nonce (Number Used Once) each time it performs an encryption operation. The nonce is generated by reading from rand.Reader, which produces a sequence of random bytes. This means that encrypting the same value with the same key will produce different results every time, due to the different nonce used in each encryption operation.

It is possible to write a deterministic encryption function by setting a fixed nonce instead of a random one and using that function in the middleware. However, using a fixed nonce with the same key and value in AES-GCM mode is a serious security risk, as it can lead to the same ciphertext being produced for the same plaintext, which can reveal patterns to an attacker. So this is not recommended for real-world use due to the security implications.

@rngallen
Copy link

rngallen commented Jan 7, 2024

Thanks it's clear now, Kindly close it

@sixcolors
Copy link
Member Author

I would like to amend my previous statement that encrypting tokens has no security value.

Encrypting tokens could help randomize them and mitigate side channel attacks like BREACH from exploiting the compression of HTTP responses.

However, this is not the intended use of the encrypt cookie middleware, and there are other ways to defend against such attacks, such as rate limiting and turning off HTTP compression.

Finally, I want to mention that the reissue of cookies per request with csrf and session is not intentional, but a result of how the middleware interact. This may change when we fix the main issue for this one.

Hence, I still do not advise encrypting these tokens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants