R72: validate combined cookie chunk metadata

review_r72_combined_test.go

@@ -0,0 +1,99 @@
+package traefikoidc
+
+import (
+	"crypto/rand"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/gorilla/sessions"
+)
+
+// TestCombinedChunk_MixedGenerationIndexRejected is a regression for a
+// combined-session read-path gap: loadFromCombinedCookies accepted any
+// present chunk set without verifying each chunk's own stored index (i) and
+// count (n). An interrupted save could leave a mixed-generation cookie set
+// that reassembled into valid-looking but wrong session data. The read
+// path must now reject a chunk whose metadata disagrees with its cookie
+// position (clean re-auth) instead of silently decoding.
+func TestCombinedChunk_MixedGenerationIndexRejected(t *testing.T) {
+	sm := createTestSessionManager(t)
+	defer sm.Shutdown()
+
+	// Build a session large enough to split into multiple combined chunks.
+	req := httptest.NewRequest(http.MethodGet, "/", nil)
+	rw := httptest.NewRecorder()
+	session, err := sm.GetSession(req)
+	if err != nil {
+		t.Fatalf("get session: %v", err)
+	}
+	session.SetUserIdentifier("[email protected]")
+	if err := session.SetAuthenticated(true); err != nil {
+		t.Fatalf("mark authenticated: %v", err)
+	}
+	bulk := make([]byte, 8000)
+	_, _ = rand.Read(bulk)
+	session.mainSession.Values["bulk"] = string(bulk)
+	// Force combined storage so the large payload is split into multiple
+	// combined chunk cookies (matching the real combined-session path)
+	// rather than overflowing the single legacy main cookie.
+	session.useCombinedStorage = true
+	session.combinedChunks = make(map[int]*sessions.Session)
+	if err := session.Save(req, rw); err != nil {
+		t.Fatalf("save session: %v", err)
+	}
+	goodCookies := rw.Result().Cookies()
+
+	// Confirm the save actually produced multiple chunks (so the test is
+	// meaningful) by attempting to load chunk 1.
+	if _, err := sm.store.Get(req, sm.combinedChunkCookieName(1)); err != nil {
+		t.Fatalf("expected a multi-chunk combined session, but chunk 1 is not loadable: %v", err)
+	}
+
+	// Control: the original, unmodified cookie set must load.
+	okReq := httptest.NewRequest(http.MethodGet, "/", nil)
+	for _, c := range goodCookies {
+		okReq.AddCookie(c)
+	}
+	okSess, err := sm.GetSession(okReq)
+	if err != nil {
+		t.Fatalf("get session from original cookies: %v", err)
+	}
+	if okSess.GetUserIdentifier() != "[email protected]" {
+		t.Fatalf("original combined cookie set did not load user identifier")
+	}
+
+	// Tamper: rewrite chunk 1 with an index that disagrees with its
+	// position, giving a "mixed-generation" set.
+	tamperReq := httptest.NewRequest(http.MethodGet, "/", nil)
+	for _, c := range goodCookies {
+		tamperReq.AddCookie(c)
+	}
+	ch1, err := sm.store.Get(tamperReq, sm.combinedChunkCookieName(1))
+	if err != nil {
+		t.Fatalf("get chunk 1 for tampering: %v", err)
+	}
+	ch1.Values["i"] = 7
+	tamperW := httptest.NewRecorder()
+	if err := ch1.Save(tamperReq, tamperW); err != nil {
+		t.Fatalf("save tampered chunk 1: %v", err)
+	}
+	loadReq := httptest.NewRequest(http.MethodGet, "/", nil)
+	for _, c := range goodCookies {
+		if c.Name == sm.combinedChunkCookieName(1) {
+			continue // replaced below with the tampered chunk
+		}
+		loadReq.AddCookie(c)
+	}
+	for _, c := range tamperW.Result().Cookies() {
+		loadReq.AddCookie(c)
+	}
+
+	tamperedSess, err := sm.GetSession(loadReq)
+	if err != nil {
+		t.Fatalf("get session from tampered cookies: %v", err)
+	}
+	if got := tamperedSess.GetUserIdentifier(); got != "" {
+		t.Fatalf("mixed-generation combined cookie set loaded user %q, want clean re-auth (empty)", got)
+	}
+}

session.go

@@ -1156,6 +1156,12 @@ func (sm *SessionManager) loadFromCombinedCookies(r *http.Request, sessionData *
 		sm.logger.Debugf("Invalid combined cookie chunk count: %v", firstChunk.Values["n"])
 		return false
 	}
+	// The first chunk must carry index 0; a set whose metadata disagrees with
+	// its cookie position is a mixed-generation set from an interrupted save.
+	if idx, idxOk := firstChunk.Values["i"].(int); !idxOk || idx != 0 {
+		sm.logger.Debugf("Combined cookie chunk 0 has invalid index: %v", firstChunk.Values["i"])
+		return false
+	}
 
 	// Load all chunks
 	chunkSessions := make([]*sessions.Session, totalChunks)
@@ -1168,6 +1174,16 @@ func (sm *SessionManager) loadFromCombinedCookies(r *http.Request, sessionData *
 			sm.logger.Debugf("Missing combined cookie chunk %d", i)
 			return false
 		}
+		// Verify each chunk's own stored index/count match its cookie
+		// position. Without this a partial write can leave a cookie set of
+		// mixed generations that reassembles and decompresses into
+		// valid-looking but wrong session data.
+		idx, idxOk := chunk.Values["i"].(int)
+		n, nOk := chunk.Values["n"].(int)
+		if !idxOk || idx != i || !nOk || n != totalChunks {
+			sm.logger.Debugf("Combined cookie chunk %d has mismatched metadata (i=%v n=%v, want i=%d n=%d)", i, chunk.Values["i"], chunk.Values["n"], i, totalChunks)
+			return false
+		}
 		chunkSessions[i] = chunk
 		sessionData.combinedChunks[i] = chunk
 	}