[BUG] Recreating session (cookie) with securecookie enabled passes plain cookie value sometimes #1929

mblaschke · 2022-07-16T13:31:15Z

Describe the bug
When using securecookie for session it fails sometime because plain (decoded) cookie value is passed to decode function.

securecookie decode fails with "securecookie: the value is not valid" on MAC check/decode: https://github.com/gorilla/securecookie/blob/master/securecookie.go#L323

To Reproduce

package main

import (
	"net/http"

	"github.com/gorilla/securecookie"
	"github.com/kataras/iris/v12"
	"github.com/kataras/iris/v12/context"
	"github.com/kataras/iris/v12/sessions"
)

const cookieNameForSessionID = "session_id_cookie"

var sess *sessions.Sessions

func secret(ctx iris.Context) {
	session := getSession(ctx, false)

	// Check if user is authenticated
	if auth, _ := session.GetBoolean("authenticated"); !auth {
		ctx.StatusCode(iris.StatusForbidden)
		ctx.ContentType("text/html")
		ctx.WriteString(`Not logged in or session invalid <a href="/logout">logout</a>`)
		return
	}

	// Print secret message
	ctx.ContentType("text/html")
	ctx.WriteString(`The cake is a lie! <a href="/logout">logout</a>`)
}

func login(ctx iris.Context) {
	session := getSession(ctx, false)
	// Authentication goes here
	// ...

	// Set user as authenticated
	session = getSession(ctx, true)
	session.Set("authenticated", true)
	ctx.StatusCode(http.StatusTemporaryRedirect)
	ctx.ContentType("text/html")
	ctx.Header("Location", "/secret")
}

func logout(ctx iris.Context) {
	sess.Destroy(ctx)
	ctx.ContentType("text/html")
	ctx.WriteString(`<a href="/login">login</a>`)
}

func getSession(ctx iris.Context, recreate bool) *sessions.Session {
	cookieOptionList := []context.CookieOption{
		func(ctx *context.Context, cookie *http.Cookie, op uint8) {
			// c.applySessionSetting(ctx, cookie, op)
		},
	}

	if recreate {
		sess.Destroy(ctx)
	}
	s := sess.Start(ctx, cookieOptionList...)
	return s
}

func main() {
	app := iris.New()
	secureCookie := securecookie.New(
		[]byte("IGVneS5eZ0b5jTgwe0l38nON0bW5awxr"),
		[]byte("r4GFiLvBtYCohUxt"),
	)

	sess = sessions.New(sessions.Config{
		Cookie:                      cookieNameForSessionID,
		Encoding:                    secureCookie,
		CookieSecureTLS:             false,
		AllowReclaim:                true,
		DisableSubdomainPersistence: true,
	})
	app.Use(sess.Handler())
	// ^ or comment this line and use sess.Start(ctx) inside your handlers
	// instead of sessions.Get(ctx).

	app.Get("/secret", secret)
	app.Get("/login", login)
	app.Get("/logout", logout)

	app.Listen(":8080")
}

and add alter Decode function in securecookie:

func (s *SecureCookie) Decode(name, value string, dst interface{}) error {
	fmt.Println(value)

result is:

MTY1Nzk3Nzg4MnxOTy0tV2JmWi1xbExTRlAzQXRMbXJJWV82S0F4dDJ5eGpSZWg5ODRydk1qdDBpVm1ZeXpKWUpMSExCWUY0M2ItZFZfZkQ2bm8yUUU9fAmHNHj68vBKoujiboz1UMf5r2IguTm-zoHQJ0DSJDpM
MTY1Nzk3Nzg4MnxOTy0tV2JmWi1xbExTRlAzQXRMbXJJWV82S0F4dDJ5eGpSZWg5ODRydk1qdDBpVm1ZeXpKWUpMSExCWUY0M2ItZFZfZkQ2bm8yUUU9fAmHNHj68vBKoujiboz1UMf5r2IguTm-zoHQJ0DSJDpM
a95828e7-a194-49e8-89cf-d76a987faa2e
a95828e7-a194-49e8-89cf-d76a987faa2e
a95828e7-a194-49e8-89cf-d76a987faa2e
MTY1Nzk3Nzg4OXxPSGs2YVNhQThFRXYyVmkwWkl5OTQ1bm5vNDVNRmxsUHRZUFNCdGFBRW12aGZIZ3dnaW1naVNDZjkyTXZLRWU1Nkc1QkxTVWdoc2M9fG38bjG-RwEYLoostgZErggXc07I_a28N2vKS-99Uf1P
MTY1Nzk3Nzg4OXxPSGs2YVNhQThFRXYyVmkwWkl5OTQ1bm5vNDVNRmxsUHRZUFNCdGFBRW12aGZIZ3dnaW1naVNDZjkyTXZLRWU1Nkc1QkxTVWdoc2M9fG38bjG-RwEYLoostgZErggXc07I_a28N2vKS-99Uf1P

Desktop (please complete the following information):

OS: osx

iris.Version

v12.2.0-beta3
master

The text was updated successfully, but these errors were encountered:

mblaschke · 2022-07-16T14:15:22Z

I haven't found the code where this happens but my rough guess:
When a new session is created it's passed to the decode function which fails because the session was never encoded.

kataras · 2022-07-16T16:56:06Z

Hello @mblaschke,

In your code example you initialize the session on each handler through:

app.Use(sess.Handler())

Then, on your getSession function, it calls the Destroy and if getSession is called from the login function, it calls the session's Start on the same request-response lifecycle, can you explain me why? Sessions are not supposed to work like this, please describe the issue so I can provide a better solution. Meanwhile, I will keep investigate why the plain cookie value is passed when securecookie is enabled.

mblaschke · 2022-07-16T17:05:49Z

i reused the example from #1877

should i avoid using?

app.Use(sess.Handler())

I tried to verify the issue why sometimes sessions are dropped (still working on an example) so i found the issue behind it that plain text session values are passed to securecookie.

kataras · 2022-07-16T17:09:04Z

You should use (app.Use(sess.Handler()) and sessions.Get) or Start - not both. I can't re-produce the issue. I see encoded values on securecookie.Decode method:

SecureCookie.Encode: name = session_id_cookie, value = "6ef75d2a-adba-4c77-a9e9-645548786364"
SecureCookie.Decode: name = session_id_cookie, value = MTY1Nzk5MTI1N3xwOUdRbDBRS2NWbWVFcmZCcTdfU3REWDEzYmp5Tm83VTRmbko4eDFpSnd4SjBZclJHYzI2VXMybmpFVmRzY2JlREhOMTNWU2R4ZU09fFR9KtqAGcJRW_xQJSmr2pkFjyfxAE-64q2SSYdM582w
SecureCookie.Decode: name = session_id_cookie, value = MTY1Nzk5MTI1N3xwOUdRbDBRS2NWbWVFcmZCcTdfU3REWDEzYmp5Tm83VTRmbko4eDFpSnd4SjBZclJHYzI2VXMybmpFVmRzY2JlREhOMTNWU2R4ZU09fFR9KtqAGcJRW_xQJSmr2pkFjyfxAE-64q2SSYdM582w

kataras · 2022-07-16T17:13:38Z

@mblaschke the code part, the securecookie's Decode is called is at:

iris/context/context.go

Lines 5314 to 5318 in cf42f37

    
           case OpCookieGet: 
        
           	// Should decode, it's a read from the client operation. 
        
           	if err := encoding.Decode(c.Name, c.Value, &c.Value); err != nil { 
        
           		c.Value = "" 
        
           	}

If the decode is failed, the cookie value is set to empty string for security reasons. This may be happening if the previous Encode is not called for some reason, probably because you call Destroy and Start in the same handler for the same request-response lifecycle.

kataras · 2022-07-16T17:34:57Z

@mblaschke this is a working example which destroys and re-initializes the session in the same request-response lifecycle (I don't recommend it) but here you are:

package main

import (
	"fmt"
	"net/http"

	"github.com/gorilla/securecookie"
	"github.com/kataras/iris/v12"
	"github.com/kataras/iris/v12/sessions"
)

const cookieNameForSessionID = "session_id_cookie"

func secret(ctx iris.Context) {
	session := getSession(ctx, false)

	// Check if user is authenticated
	if auth, _ := session.GetBoolean("authenticated"); !auth {
		ctx.StatusCode(iris.StatusForbidden)
		ctx.ContentType("text/html")
		ctx.WriteString(`Not logged in or session invalid <a href="/logout">logout</a>`)
		return
	}

	// Print secret message
	ctx.ContentType("text/html")
	ctx.WriteString(`The cake is a lie! <a href="/logout">logout</a>`)
}

func login(ctx iris.Context) {
	// Authentication goes here
	// ...

	// Set user as authenticated
	session := getSession(ctx, true)
	session.Set("authenticated", true)
	ctx.StatusCode(http.StatusTemporaryRedirect)
	ctx.ContentType("text/html")
	ctx.Header("Location", "/secret")
}

func logout(ctx iris.Context) {
	sessions.Get(ctx).Destroy()
	ctx.ContentType("text/html")
	ctx.WriteString(`<a href="/login">login</a>`)
}

func getSession(ctx iris.Context, recreate bool) *sessions.Session {
	// cookieOptionList := []context.CookieOption{
	// 	func(ctx *context.Context, cookie *http.Cookie, op uint8) {
	// 		// c.applySessionSetting(ctx, cookie, op)
	// 	},
	// }

	if recreate {
		sessions.Get(ctx).Destroy()
		fmt.Println("getSession.recreate: destroy called")
	}

	//	s := sess.Start(ctx, cookieOptionList...)
	//	return s
	return sessions.Get(ctx)
}

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

	secureCookie := securecookie.New(
		[]byte("IGVneS5eZ0b5jTgwe0l38nON0bW5awxr"),
		[]byte("r4GFiLvBtYCohUxt"),
	)

	sess := sessions.New(sessions.Config{
		Cookie:                      cookieNameForSessionID,
		Encoding:                    secureCookie,
		CookieSecureTLS:             false,
		AllowReclaim:                true,
		DisableSubdomainPersistence: true,
	})
	app.Use(sess.Handler())

	app.Get("/secret", secret)
	app.Get("/login", login)
	app.Get("/logout", logout)

	app.Listen(":8080")
}

It's the same but you use app.Use/sessions.Get(ctx) and sessions.Get(ctx).Destroy instead.

kataras · 2022-07-16T17:47:14Z

@mblaschke,

I found the origin of the "issue". This is happening because the login handler has decoded the request cookie's value already - but this is not a problem, the server never sends the decoded value back to the client.

mblaschke · 2022-07-17T14:14:57Z

I can confirm the session id is not send back to the client but the session is invalidated at that point.

The reason for recreation of session is https://cheatsheetseries.owasp.org/cheatsheets/Session_Management_Cheat_Sheet.html#renew-the-session-id-after-any-privilege-level-change

Also I want to change cookies samesite mode from lax (for oauth login) to strict (for application).

kataras · 2022-07-22T09:28:36Z

Hello @mblaschke,

Also I want to change cookies samesite mode from lax (for oauth login) to strict (for application).

That's easy, you can do it using a middleware (for all cookies) and cookie options via ctx.AddCookieOptions or pass the cookie option(s) when calling the sessions.Handler method. There is a builtin cookie option called CookieSameSite in the context sub-package but you can define your own, example:

var strictSameSiteOpt = func(_ iris.Context, c *http.Cookie, op uint8) {
	if op == 1 /* context.OpCookieSet */ {
            c.SameSite = http.SameSiteStrictMode
        }
}

	sess := sessions.New(sessions.Config{
		Cookie:                      cookieNameForSessionID,
		Encoding:                    secureCookie,
		CookieSecureTLS:             false,
		AllowReclaim:                true,
		DisableSubdomainPersistence: true,
	})
	


	app.Use(sess.Handler(strictSameSiteOpt))

The reason for recreation of session is https://cheatsheetseries.owasp.org/cheatsheets/Session_Management_Cheat_Sheet.html#renew-the-session-id-after-any-privilege-level-change

Can you run this and provide some feedback? It works here:

package main

import (
	"github.com/gorilla/securecookie"
	"github.com/kataras/iris/v12"
	"github.com/kataras/iris/v12/sessions"
)

var strictSameSiteOpt = func(_ iris.Context, c *http.Cookie, op uint8) {
	if op == 1 /* context.OpCookieSet */ {
            c.SameSite = http.SameSiteStrictMode
        }
}

var sessManager *sessions.Sessions

const sessionCookieName = "session_id"

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

	secureCookie := securecookie.New(
		[]byte("IGVneS5eZ0b5jTgwe0l38nON0bW5awxr"),
		[]byte("r4GFiLvBtYCohUxt"),
	)

	sessManager = sessions.New(sessions.Config{
		Cookie:                      sessionCookieName,
		Encoding:                    secureCookie,
		CookieSecureTLS:             false,
		AllowReclaim:                true,
		DisableSubdomainPersistence: true,
	})
	app.Use(sessManager.Handler(strictSameSiteOpt))

	app.Get("/login", login)
	app.Get("/logout", logout)
	app.Get("/secret", secret)

	app.Listen(":8080")
}

func login(ctx iris.Context) {
	// Authentication goes here
	session := sessions.Get(ctx)
	// ...
	oldSessionID := session.ID()
	ctx.Application().Logger().Infof("old session id = %s", oldSessionID)

	// Set user as authenticated
	// If AllowReclaim is false:
	// manually remove the request cookie, so the new Start creates a new session:
	// ctx.Request().Header.Del(sessionCookieName)

	// Use session's manager instead of session.Destroy so
	// it destroys the response AND request (if AllowReclaim is true)
	// cookie information.
	sessManager.Destroy(ctx)

	session = sessManager.Start(ctx, strictSameSiteOpt)
	session.Set("authenticated", true)

	ctx.Application().Logger().Infof("new session id = %s", session.ID())

	ctx.Redirect("/secret", iris.StatusTemporaryRedirect)
}

func logout(ctx iris.Context) {
	sessManager.Destroy(ctx)

	ctx.HTML(`<a href="/login">login</a>`)
}

func secret(ctx iris.Context) {
	session := sessions.Get(ctx)

	// Check if user is authenticated.
	if auth, _ := session.GetBoolean("authenticated"); !auth {
		ctx.StatusCode(iris.StatusForbidden)
		ctx.HTML(`Not logged in or session invalid <a href="/logout">logout</a>`)
		return
	}

	// Print secret message.
	ctx.HTML(`The cake is a lie! <a href="/logout">logout</a>`)
}

mblaschke · 2022-07-23T15:37:52Z

It's working but i found a way to reproduce (but not with an example app) it so i'm currently digging into the iris code (12.2.0-beta4) to find a clue what's happening.

If the user doesn't have a session, everything is fine.

When the user is redirected to oauth provider and I restart the app (using internal session storage) the login fails.
with the error message i'm using c.session.Destroy(ctx) to destroy the session but the session is NOT destroyed and instead creates a new session:

Set-Cookie: sid=29b213ad-e8a4-4208-a0d7-d4f5e5f76500; Path=/; Expires=Sat, 23 Jul 2022 17:12:36 GMT; Max-Age=7199; HttpOnly; SameSite=Strict

on the login page (where i start oauth and redirect to user to the provider) i then get a for whatever reason:

Set-Cookie: devconsole-sid=; Path=/; Expires=Tue, 10 Nov 2009 23:00:00 GMT; Max-Age=0; HttpOnly

so the user is trapped in an endless invalid session loop.

also app.Use(sessManager.Handler(strictSameSiteOpt)) sets session ids for app.HandleDir requests which should never happen. I guess that's a major issue because that recreates a strict cookie again.

while trying several things sometimes i encounter a invalid memory address or nil pointer dereference:

runtime error: invalid memory address or nil pointer dereference
/opt/homebrew/Cellar/go/1.18.4/libexec/src/runtime/panic.go:838
/opt/homebrew/Cellar/go/1.18.4/libexec/src/runtime/panic.go:220
/opt/homebrew/Cellar/go/1.18.4/libexec/src/runtime/signal_unix.go:818
/app/vendor/github.com/kataras/iris/v12/sessions/session.go:63
/app/vendor/github.com/kataras/iris/v12/sessions/session.go:135

mblaschke · 2022-07-23T15:43:28Z

to fix the endless invalid session loop i've moved RemoveCookie up before the cookieValue check. this at least solved to kill the invalid session.

github.com/kataras/iris/v12/sessions/sessions.go:

// Destroy removes the session data, the associated cookie
// and the Context's session value.
// Next calls of `sessions.Get` will occur to a nil Session,
// use `Sessions#Start` method for renewal
// or use the Session's Destroy method which does keep the session entry with its values cleared.
func (s *Sessions) Destroy(ctx *context.Context) {
	cookieValue := s.getCookieValue(ctx, nil)
	ctx.RemoveCookie(s.config.Cookie, s.cookieOptions...)

	if cookieValue == "" { // nothing to destroy
		return
	}

	ctx.Values().Remove(sessionContextKey)
	//ctx.RemoveCookie(s.config.Cookie, s.cookieOptions...)


	s.provider.Destroy(cookieValue)
}

mblaschke · 2022-07-24T23:01:50Z

found one issue:
i've forgot to enable AllowReclaim. Then it's working but only without securecookie 🤔

kataras · 2022-09-22T21:00:38Z

also app.Use(sessManager.Handler(strictSameSiteOpt)) sets session ids for app.HandleDir requests which should never happen. I guess that's a major issue because that recreates a strict cookie again.

It should happen if you have allow it, you should register the middleware after the app.HandleDir to prevent it from running to your app.HandleDir.

kataras · 2022-09-22T21:02:01Z

i've forgot to enable AllowReclaim. Then it's working but only without securecookie 🤔

You mean that the loop you said, is still valid? If so, could you please post an example with steps to re-produce it and test the solution by myself?

mblaschke assigned kataras Jul 16, 2022

kataras added 🐞 type:bug 🚨 prio:high 🕵🏽‍♀️ status:need-investigation labels Jul 16, 2022

kataras removed 🐞 type:bug 🚨 prio:high 🕵🏽‍♀️ status:need-investigation labels Jul 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUG] Recreating session (cookie) with securecookie enabled passes plain cookie value sometimes #1929

[BUG] Recreating session (cookie) with securecookie enabled passes plain cookie value sometimes #1929

mblaschke commented Jul 16, 2022

mblaschke commented Jul 16, 2022

kataras commented Jul 16, 2022

mblaschke commented Jul 16, 2022

kataras commented Jul 16, 2022

kataras commented Jul 16, 2022 •

edited

Loading

kataras commented Jul 16, 2022

kataras commented Jul 16, 2022

mblaschke commented Jul 17, 2022

kataras commented Jul 22, 2022

mblaschke commented Jul 23, 2022

mblaschke commented Jul 23, 2022 •

edited

Loading

mblaschke commented Jul 24, 2022

kataras commented Sep 22, 2022

kataras commented Sep 22, 2022

[BUG] Recreating session (cookie) with securecookie enabled passes plain cookie value sometimes #1929

[BUG] Recreating session (cookie) with securecookie enabled passes plain cookie value sometimes #1929

Comments

mblaschke commented Jul 16, 2022

mblaschke commented Jul 16, 2022

kataras commented Jul 16, 2022

mblaschke commented Jul 16, 2022

kataras commented Jul 16, 2022

kataras commented Jul 16, 2022 • edited Loading

kataras commented Jul 16, 2022

kataras commented Jul 16, 2022

mblaschke commented Jul 17, 2022

kataras commented Jul 22, 2022

mblaschke commented Jul 23, 2022

mblaschke commented Jul 23, 2022 • edited Loading

mblaschke commented Jul 24, 2022

kataras commented Sep 22, 2022

kataras commented Sep 22, 2022

kataras commented Jul 16, 2022 •

edited

Loading

mblaschke commented Jul 23, 2022 •

edited

Loading