From 56d66642ecc633cff0606927601e81cdac361370 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Wed, 10 Apr 2024 12:47:04 +0200 Subject: [PATCH] Merge pull request from GHSA-9wwp-q7wq-jx35 * Add anti session theft provisions Signed-off-by: Matteo Collina * Update README.md Co-authored-by: James Sumners <321201+jsumners@users.noreply.github.com> Signed-off-by: Matteo Collina * Apply suggestions from code review Signed-off-by: Manuel Spigolon --------- Signed-off-by: Matteo Collina Signed-off-by: Matteo Collina Signed-off-by: Manuel Spigolon Co-authored-by: James Sumners <321201+jsumners@users.noreply.github.com> Co-authored-by: Manuel Spigolon --- README.md | 10 +++- index.js | 30 ++++++++++-- package.json | 1 + test/anti-reuse-15-min.js | 62 +++++++++++++++++++++++++ test/anti-reuse.js | 62 +++++++++++++++++++++++++ test/http-only.js | 96 +++++++++++++++++++++++++++++++++++++++ 6 files changed, 255 insertions(+), 6 deletions(-) create mode 100644 test/anti-reuse-15-min.js create mode 100644 test/anti-reuse.js create mode 100644 test/http-only.js diff --git a/README.md b/README.md index 25749c9..dd0e936 100644 --- a/README.md +++ b/README.md @@ -53,6 +53,9 @@ fastify.register(require('@fastify/secure-session'), { cookieName: 'my-session-cookie', // adapt this to point to the directory where secret-key is located key: fs.readFileSync(path.join(__dirname, 'secret-key')), + // the amount of time the session is considered valid; this is different from the cookie options + // and based on value wihin the session. + expiry: 24 * 60 * 60, // Default 1 day cookie: { path: '/' // options for setCookie, see https://github.com/fastify/fastify-cookie @@ -365,9 +368,12 @@ fastify.get('/', (request, reply) => { }) ``` -## TODO +## Security Notice -- [ ] add an option to just sign, and do not encrypt +`@fastify/secure-session` stores the session within a cookie, and as a result an attacker could impersonate a user +if the cookie is leaked. The maximum expiration time of the session is set by the `expiry` option, which has default +1 day. Adjust this parameter accordingly. +Moreover, to protect users from further attacks, all cookies are created as "http only" if not specified otherwise. ## License diff --git a/index.js b/index.js index 756ddbc..64e996e 100644 --- a/index.js +++ b/index.js @@ -50,8 +50,13 @@ function fastifySecureSession (fastify, options, next) { for (const sessionOptions of options) { const sessionName = sessionOptions.sessionName || 'session' const cookieName = sessionOptions.cookieName || sessionName + const expiry = sessionOptions.expiry || 86401 // 24 hours const cookieOptions = sessionOptions.cookieOptions || sessionOptions.cookie || {} + if (cookieOptions.httpOnly === undefined) { + cookieOptions.httpOnly = true + } + let key if (sessionOptions.secret) { if (Buffer.byteLength(sessionOptions.secret) < 32) { @@ -120,7 +125,8 @@ function fastifySecureSession (fastify, options, next) { sessionNames.set(sessionName, { cookieName, cookieOptions, - key + key, + expiry }) if (!defaultSessionName) { @@ -139,7 +145,7 @@ function fastifySecureSession (fastify, options, next) { throw new Error('Unknown session key.') } - const { key } = sessionNames.get(sessionName) + const { key, expiry } = sessionNames.get(sessionName) // do not use destructuring or it will deopt const split = cookie.split(';') @@ -184,8 +190,15 @@ function fastifySecureSession (fastify, options, next) { return null } + const parsed = JSON.parse(msg) + if ((parsed.__ts + expiry) * 1000 - Date.now() <= 0) { + // maximum validity is reached, resetting + log.debug('@fastify/secure-session: expiry reached') + return null + } const session = new Proxy(new Session(JSON.parse(msg)), sessionProxyHandler) session.changed = signingKeyRotated + return session }) @@ -228,7 +241,7 @@ function fastifySecureSession (fastify, options, next) { const cookie = request.cookies[cookieName] const result = fastify.decodeSecureSession(cookie, request.log, sessionName) - request[sessionName] = new Proxy((result || new Session({})), sessionProxyHandler) + request[sessionName] = result || new Proxy(new Session({}), sessionProxyHandler) } next() @@ -275,6 +288,10 @@ class Session { this[kCookieOptions] = null this.changed = false this.deleted = false + + if (this[kObj].__ts === undefined) { + this[kObj].__ts = Math.round(Date.now() / 1000) + } } get (key) { @@ -296,7 +313,12 @@ class Session { } data () { - return this[kObj] + const copy = { + ...this[kObj] + } + + delete copy.__ts + return copy } touch () { diff --git a/package.json b/package.json index 66b348d..f6a24c3 100644 --- a/package.json +++ b/package.json @@ -32,6 +32,7 @@ "homepage": "https://github.com/fastify/fastify-secure-session#readme", "devDependencies": { "@fastify/pre-commit": "^2.0.2", + "@sinonjs/fake-timers": "^11.2.2", "@types/node": "^20.1.0", "cookie": "^0.6.0", "fastify": "^4.0.0", diff --git a/test/anti-reuse-15-min.js b/test/anti-reuse-15-min.js new file mode 100644 index 0000000..c4af204 --- /dev/null +++ b/test/anti-reuse-15-min.js @@ -0,0 +1,62 @@ +'use strict' + +const t = require('tap') +const fastify = require('fastify')({ logger: false }) +const sodium = require('sodium-native') +const FakeTimers = require('@sinonjs/fake-timers') +const clock = FakeTimers.install({ + shouldAdvanceTime: true, + now: Date.now() +}) + +const key = Buffer.alloc(sodium.crypto_secretbox_KEYBYTES) +sodium.randombytes_buf(key) + +fastify.register(require('../'), { + key, + expiry: 15 * 60 // 15 minutes +}) + +fastify.post('/', (request, reply) => { + request.session.set('some', request.body.some) + request.session.set('some2', request.body.some2) + reply.send('hello world') +}) + +t.teardown(fastify.close.bind(fastify)) +t.plan(5) + +fastify.get('/', (request, reply) => { + const some = request.session.get('some') + const some2 = request.session.get('some2') + reply.send({ some, some2 }) +}) + +fastify.inject({ + method: 'POST', + url: '/', + payload: { + some: 'someData', + some2: { a: 1, b: undefined, c: 3 } + } +}, (error, response) => { + t.error(error) + t.equal(response.statusCode, 200) + t.ok(response.headers['set-cookie']) + + clock.jump('00:15:01') // default validity is 24 hours + + fastify.inject({ + method: 'GET', + url: '/', + headers: { + cookie: response.headers['set-cookie'] + } + }, (error, response) => { + t.error(error) + t.same(JSON.parse(response.payload), {}) + clock.reset() + clock.uninstall() + fastify.close() + }) +}) diff --git a/test/anti-reuse.js b/test/anti-reuse.js new file mode 100644 index 0000000..e6edc40 --- /dev/null +++ b/test/anti-reuse.js @@ -0,0 +1,62 @@ +'use strict' + +const t = require('tap') +const fastify = require('fastify')({ logger: false }) +const sodium = require('sodium-native') +const FakeTimers = require('@sinonjs/fake-timers') +const clock = FakeTimers.install({ + shouldAdvanceTime: true, + now: Date.now() +}) + +const key = Buffer.alloc(sodium.crypto_secretbox_KEYBYTES) + +sodium.randombytes_buf(key) + +fastify.register(require('../'), { + key +}) + +fastify.post('/', (request, reply) => { + request.session.set('some', request.body.some) + request.session.set('some2', request.body.some2) + reply.send('hello world') +}) + +t.teardown(fastify.close.bind(fastify)) +t.plan(6) + +fastify.get('/', (request, reply) => { + const some = request.session.get('some') + const some2 = request.session.get('some2') + reply.send({ some, some2 }) +}) + +fastify.inject({ + method: 'POST', + url: '/', + payload: { + some: 'someData', + some2: { a: 1, b: undefined, c: 3 } + } +}, (error, response) => { + t.error(error) + t.equal(response.statusCode, 200) + t.ok(response.headers['set-cookie']) + t.equal(response.headers['set-cookie'].split(';')[1].trim(), 'HttpOnly') + + clock.jump('24:01:00') // default validity is 24 hours + + fastify.inject({ + method: 'GET', + url: '/', + headers: { + cookie: response.headers['set-cookie'] + } + }, (error, response) => { + t.error(error) + t.same(JSON.parse(response.payload), {}) + clock.reset() + clock.uninstall() + }) +}) diff --git a/test/http-only.js b/test/http-only.js new file mode 100644 index 0000000..721db98 --- /dev/null +++ b/test/http-only.js @@ -0,0 +1,96 @@ +'use strict' + +const tap = require('tap') +const Fastify = require('fastify') +const SecureSessionPlugin = require('../') +const sodium = require('sodium-native') +const key = Buffer.alloc(sodium.crypto_secretbox_KEYBYTES) +sodium.randombytes_buf(key) + +tap.test('http-only override', async t => { + const fastify = Fastify({ logger: false }) + t.teardown(fastify.close.bind(fastify)) + t.plan(3) + + await fastify.register(SecureSessionPlugin, { + key, + cookie: { + path: '/', + httpOnly: false + } + }) + + fastify.post('/login', (request, reply) => { + request.session.set('user', request.body.email) + reply.send('Welcome back!') + }) + + const loginResponse = await fastify.inject({ + method: 'POST', + url: '/login', + payload: { + email: 'me@here.fine' + } + }) + + t.equal(loginResponse.statusCode, 200) + t.ok(loginResponse.headers['set-cookie']) + t.not(loginResponse.headers['set-cookie'].split(';')[1].trim(), 'HttpOnly') +}) + +tap.test('Override global options does not change httpOnly default', t => { + t.plan(8) + const fastify = Fastify() + fastify.register(SecureSessionPlugin, { + key, + cookieOptions: { + maxAge: 42, + path: '/' + } + }) + + fastify.post('/', (request, reply) => { + request.session.set('data', request.body) + request.session.options({ maxAge: 1000 * 60 * 60 }) + reply.send('hello world') + }) + + t.teardown(fastify.close.bind(fastify)) + + fastify.get('/', (request, reply) => { + const data = request.session.get('data') + + if (!data) { + reply.code(404).send() + return + } + reply.send(data) + }) + + fastify.inject({ + method: 'POST', + url: '/', + payload: { + some: 'data' + } + }, (error, response) => { + t.error(error) + t.equal(response.statusCode, 200) + t.ok(response.headers['set-cookie']) + const { maxAge, path } = response.cookies[0] + t.equal(maxAge, 1000 * 60 * 60) + t.equal(response.headers['set-cookie'].split(';')[3].trim(), 'HttpOnly') + t.equal(path, '/') + + fastify.inject({ + method: 'GET', + url: '/', + headers: { + cookie: response.headers['set-cookie'] + } + }, (error, response) => { + t.error(error) + t.same(JSON.parse(response.payload), { some: 'data' }) + }) + }) +})