gofiber / fiber

⚡️ Express inspired web framework written in Go
https://gofiber.io
MIT License
33.46k stars 1.64k forks source link

SessionID new on each request (session mw) #2793

Closed sixcolors closed 9 months ago

sixcolors commented 9 months ago

if I use SessionStore I found that on each request session expiration is updated as well assessionid` 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 https://github.com/gofiber/fiber/issues/2741#issuecomment-1879799204

@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 commented 9 months ago

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 commented 9 months ago

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

rngallen commented 9 months ago

No, not over https

sixcolors commented 9 months ago

CookieSecure = false CookieSameSite: "Lax"

for both csrf and session.

sixcolors commented 9 months ago

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 commented 9 months ago

The example is running https. https://github.com/gofiber/recipes/blob/master/csrf-with-session/main.go

sixcolors commented 9 months ago

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 commented 9 months ago

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

sixcolors commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

Thanks it's clear now, Kindly close it

sixcolors commented 9 months ago

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.