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

🐛 [Bug]: Csrf with Session store does not support secure cookie middleware #2743

Closed
3 tasks done
rngallen opened this issue Nov 27, 2023 · 4 comments · Fixed by #2753
Closed
3 tasks done

🐛 [Bug]: Csrf with Session store does not support secure cookie middleware #2743

rngallen opened this issue Nov 27, 2023 · 4 comments · Fixed by #2753

Comments

@rngallen
Copy link

Bug Description

Store defined as

Store = session.New(
		session.Config{
			Expiration: setups.SessionExpiration,
			KeyLookup:  "header:Session-Id",
			// Storage: ,// Redis on production/Memory on dev
			CookieDomain:   config.Conf.App.Domain,
			CookieSecure:   config.Conf.App.Secure, // True on HTTPS
			CookieHTTPOnly: true,
		},
	)

Csrf Configuration

	app.Use(csrf.New(csrf.Config{
		CookieDomain:      config.Conf.App.Domain,
		CookieSecure:      config.Conf.App.Secure, // HTTPS True
		CookieHTTPOnly:    true,
		CookieSameSite:    "Strict",
		CookieSessionOnly: true, // Set True ignore expiration time
		Session:           Store,
		ErrorHandler:      defaultErrorHandler,
	}))

This works perfect but if i enable secure cookie middleware

	 app.Use(encryptcookie.New(encryptcookie.Config{
	Key: encryptcookie.GenerateKey(),
	}))

csrf always returns forbidden

How to Reproduce

Enable secure cookie middleware when csrf storage uses session store

Expected Behavior

I expect it should also work when secure cookie middleware is in use

Fiber Version

2.51.0

Code Snippet (optional)

package main

import "github.com/gofiber/fiber/v2"
import "log"

func main() {
  app := fiber.New()

  // Steps to reproduce

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

Checklist:

  • I agree to follow Fiber's Code of Conduct.
  • I have checked for existing issues that describe my problem prior to opening this one.
  • I understand that improperly formatted bug reports may be closed without explanation.
@sixcolors
Copy link
Member

@rngallen Here's a working example. Be aware that middleware order matters. Also, if you use Key: encryptcookie.GenerateKey() it will fail between app restarts.

Replace the key in the example with your secret key:

package main

import (
	"fmt"

	"github.com/gofiber/fiber/v2"
	"github.com/gofiber/fiber/v2/middleware/csrf"
	"github.com/gofiber/fiber/v2/middleware/encryptcookie"
	"github.com/gofiber/fiber/v2/middleware/session"
)

func main() {
	app := fiber.New()

	// Example: Encryptcookie middleware
	app.Use(encryptcookie.New(encryptcookie.Config{
		Key: "t+A2pNQ+GM117Uj7AhaHq/BwjWzZwBT9crgOSY6eWjA=",
	}))

	sessionStore := session.New(session.Config{})

	// Example: CSRF middleware
	app.Use(csrf.New(csrf.Config{
		KeyLookup:  "form:csrfToken",
		Session:    sessionStore,
		ContextKey: "csrfToken",
	}))

	// Route handler
	app.Get("/", func(c *fiber.Ctx) error {
		// html page with a form that posts to /test
		// inline javascript that reads the csrf token from the cookie and adds it to the form
		csrfToken := c.Locals("csrfToken")
		page := `
		<html>
			
			<body>
				<form action="/" method="POST">
					<input type="text" name="test" />
					<input type="submit" value="Submit" />
					<input type="hidden" name="csrfToken" value="` + csrfToken.(string) + `" />
					</form>
						</body>
						</html>
			`
		c.Type("html")
		return c.SendString(page)
	})

	app.Post("/", func(c *fiber.Ctx) error {
		csrfToken := c.Locals("csrfToken")
		page := `
		<html>
			
			<body>
				<p>You submitted: ` + c.FormValue("test") + `</p>
				<form action="/" method="POST">
					<input type="text" name="test" />
					<input type="submit" value="Submit" />
					<input type="hidden" name="csrfToken" value="` + csrfToken.(string) + `" />
					</form>
						</body>
						</html>
			`
		c.Type("html")
		return c.SendString(page)
	})

	// Listen on port 3000
	err := app.Listen(":3000")
	if err != nil {
		fmt.Println("Error:", err)
	}
}

@sixcolors
Copy link
Member

sixcolors commented Nov 30, 2023

@rngallen by default, the encryptcookie middleware does not encrypt the csrf_ cookie, see encryptcookie docs. This is required if you're reading the token from the cookie in the front-end or client-side. But if you're including the CSRF token in the HTML (like the example above), you can choose to encrypt the CSRF cookie using the following configuration:

	app.Use(encryptcookie.New(encryptcookie.Config{
		Key: "t+A2pNQ+GM117Uj7AhaHq/BwjWzZwBT9crgOSY6eWjA=",
		Except: []string{
		},
	}))

@rngallen
Copy link
Author

rngallen commented Nov 30, 2023

Thanks but after working around I have discovered the followings

  1. I use sess.Save() on my middleware
func AuthMiddleware(c *fiber.Ctx) error {
	sess, err := Store.Get(c)
	if err != nil {
		return c.Status(fiber.StatusUnauthorized).JSON(response.UnAuthorized(""))
	}
	if sess.Get(AUTH_KEY) == nil {
		return c.Status(fiber.StatusUnauthorized).JSON(response.UnAuthorized(""))
	}
	// Add user to the next handle
	c.Locals("userUid", sess.Get(USER_UID))
	c.Locals("superUser", sess.Get(SUPER_USER))
	c.Locals("sub", sess.Get(SUB))

	// Update user session expiration time
	// Uncomment this to enable coockie to be updated on each request
	// if err := sess.Save(); err != nil {
	// 	return err
	// }
	return c.Next()
}

session was keep updating on every request. And because am using js(next js) on frontend I have to save this cookie(s) on every request, i think it's not a proper way. On top of that session max-age/expires keep extended on each request.
2. Even if I comment sess.Save() but in csrf middleware Session uses Store instead of nil (default) it keep updating session id on every request.
3. if I set Session:nil on csrf middleware and comment sess.Save() session id will not updated on each request. But as per documentation csrf will be effective if it used with session(Store).

Is it the best practice to keep updating session on each request?
What are the drawbacks(security) if I use session but csrf will not use store? as per point3

@sixcolors
Copy link
Member

sixcolors commented Nov 30, 2023

Let's address each point:

  1. If you are only reading values from the session store, there is no requirement to call Save(). You only need to save if you've made changes.
  2. If you are using CSRF with a session be sure the encryptcookie middleware is applied first otherwise the session_id would be encrypted and not found, generating a new session. However, even if applied in the correct order, the csrf middleware will Save the session on each request. This is a known issue, see 🐛 [Bug]: Inconsistency in Session and CSRF Middleware Handling of Timeout and Expiration #2741. We are working to address how Expiration is treated in GoFiber middleware.
  3. You can use the CSRF middleware for protection with the Double Submit Cookie pattern. This is considered a good defence, see https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#naive-double-submit-cookie

The Synchronizer Token Patters is the recommended method if practical, as the double submit cookie pattern has known weaknesses. However when combined with other techniques it is likely sufficient for most use-cases.

I recommend reading both OWASP's defense-in-depth-techniques, our own CSRF#Defense-In-Depth, as well as Googling "is csrf dead" for more about Cookie SameSite and cookie prefixes. Finally, it should go without saying. But, please use HTTPS.

I hope that helps.

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

Successfully merging a pull request may close this issue.

2 participants