diff --git a/api/src/auth.js b/api/src/auth.js index 03f8c268..dfed23fb 100644 --- a/api/src/auth.js +++ b/api/src/auth.js @@ -213,19 +213,6 @@ function verifyToken(token, secret) { return readToken(token, secret)?.sessionId ?? null; } -/* Decode without throwing. A stray '%' in any cookie on the domain would - otherwise fail every authenticated request. */ -function parseCookies(req) { - const header = req.headers.cookie || ''; - /* Null prototype. The keys are cookie names straight off the request. */ - const out = Object.create(null); - for (const part of header.split(';')) { - const [k, ...v] = part.trim().split('='); - if (k) out[k.trim()] = decodeOrRaw(v.join('=')); - } - return out; -} - const TRUST_PROXY = process.env.TRUST_PROXY === 'true'; /* A Secure cookie set over plain HTTP, the normal case on a LAN, is dropped by @@ -326,14 +313,27 @@ function stripDisabledCredentials(auth) { return had; } +/* Every `ds` the request carries. Do not keep only one: a host on the parent + domain can set its own, and the owner then fails every request. Decode + without throwing: a stray '%' in any cookie on the domain would otherwise + fail every authenticated request. */ +function sessionTokens(req) { + const out = []; + for (const part of (req.headers.cookie || '').split(';')) { + const [k, ...v] = part.trim().split('='); + if (k.trim() === 'ds') out.push(decodeOrRaw(v.join('='))); + } + return out; +} + function readSession(req, cfg) { const auth = cfg.settings?.auth; if (!auth?.secret) return null; - const token = parseCookies(req).ds; - if (!token) return null; - const read = readToken(token, auth.secret); - if (!read || Object.hasOwn(auth.revoked || {}, read.sessionId)) return null; - return read; + for (const token of sessionTokens(req)) { + const read = readToken(token, auth.secret); + if (read && !Object.hasOwn(auth.revoked || {}, read.sessionId)) return read; + } + return null; } /* A record is pruned only once every copy of its session has expired. Pruning @@ -400,7 +400,7 @@ module.exports = { readToken, refreshSession, revokeSession, - parseCookies, + sessionTokens, setSessionCookie, clearSessionCookie, isSecureRequest, diff --git a/api/test/auth.test.js b/api/test/auth.test.js index ad80c4d9..fb613914 100644 --- a/api/test/auth.test.js +++ b/api/test/auth.test.js @@ -9,7 +9,7 @@ const { verifyToken, hashPassword, verifyPassword, - parseCookies, + sessionTokens, rateLimit, registerLoginAttempt, clearAttempts, @@ -87,10 +87,10 @@ test('verifyPassword rejects a malformed hash', async () => { assert.equal(await verifyPassword('anything', 'not-a-valid-hash'), false); }); -test('parseCookies parses the session cookie', () => { - const c = parseCookies({ headers: { cookie: 'ds=abc123; other=x' } }); - assert.equal(c.ds, 'abc123'); - assert.equal(c.other, 'x'); +test('sessionTokens returns every session cookie, in order', () => { + assert.deepEqual(sessionTokens({ headers: { cookie: 'ds=abc123; other=x' } }), ['abc123']); + assert.deepEqual(sessionTokens({ headers: { cookie: 'ds=a; other=x; ds=b' } }), ['a', 'b']); + assert.deepEqual(sessionTokens({ headers: {} }), []); }); test('rateLimit allows up to the max then blocks', () => { diff --git a/api/test/connections-vpn-data.test.js b/api/test/connections-vpn-data.test.js index 22b5f470..d9b76ce1 100644 --- a/api/test/connections-vpn-data.test.js +++ b/api/test/connections-vpn-data.test.js @@ -70,3 +70,16 @@ test('a NetBird peer reply that is not a list shows an error', async () => { assert.equal(r.errorKind, 'upstream'); assert.equal(r.connected, false); }); + +test('a blocked address is not named in the reply', async () => { + const { SsrfBlockedError } = require('../src/proxy'); + const r = await dataFn({ + ...ctxFor({}), + fetchJSON: async () => { + throw new SsrfBlockedError('Blocked: gluetun.lan resolves to private IP 10.0.0.5.', 'private_address'); + }, + }); + assert.equal(r.errorKind, 'blocked'); + assert.equal(r.error, 'The request was blocked.'); + assert.doesNotMatch(JSON.stringify(r), /10\.0\.0\.5|gluetun\.lan/); +}); diff --git a/api/test/percent-decode.test.js b/api/test/percent-decode.test.js index 0d1e41ed..a2aa9720 100644 --- a/api/test/percent-decode.test.js +++ b/api/test/percent-decode.test.js @@ -8,8 +8,7 @@ const assert = require('node:assert/strict'); const http = require('node:http'); const { tryDecode, decodeOrRaw } = require('../src/percent-decode'); -const { parseCookies, hashPassword, makeToken } = require('../src/auth'); -const { plain } = require('../test-support/plain'); +const { sessionTokens, hashPassword, makeToken } = require('../src/auth'); /* Values decodeURIComponent throws on. */ const MALFORMED = ['%', '%z', '%zz', '%2', 'a%', '%%', '%E0%A4%A', 'ok-%-then', '%C0%80'.slice(0, 5)]; @@ -52,29 +51,25 @@ test('both tolerate non-string input', () => { /* ── cookies ──────────────────────────────────────────────────────────────── */ -/* Copied onto an ordinary prototype: parseCookies builds a null-prototype - object, since its keys are cookie names off the request. */ -const cookies = header => plain(parseCookies({ headers: { cookie: header } })); +const tokens = header => sessionTokens({ headers: { cookie: header } }); test('a malformed cookie value is kept rather than throwing', () => { - assert.doesNotThrow(() => cookies('ds=%')); - assert.deepEqual(cookies('ds=%'), { ds: '%' }); + assert.doesNotThrow(() => tokens('ds=%')); + assert.deepEqual(tokens('ds=%'), ['%']); }); /* The case that made this reachable without a session: the broken cookie need not be ours. */ -test('a malformed unrelated cookie does not break the ones that matter', () => { - const out = cookies('other=%zz; ds=abc.123.def'); - assert.equal(out.ds, 'abc.123.def'); - assert.equal(out.other, '%zz'); +test('a malformed unrelated cookie does not break the session cookie', () => { + assert.deepEqual(tokens('other=%zz; ds=abc.123.def'), ['abc.123.def']); }); -test('ordinary cookies still decode', () => { - assert.deepEqual(cookies('a=one%20two; b=x'), { a: 'one two', b: 'x' }); +test('ordinary values still decode', () => { + assert.deepEqual(tokens('a=x; ds=one%20two'), ['one two']); }); test('a value containing = survives intact', () => { - assert.equal(cookies('t=a=b=c').t, 'a=b=c'); + assert.deepEqual(tokens('ds=a=b=c'), ['a=b=c']); }); /* ── over HTTP ────────────────────────────────────────────────────────────── */ diff --git a/api/test/session-cookie-shadow.test.js b/api/test/session-cookie-shadow.test.js new file mode 100644 index 00000000..8730153f --- /dev/null +++ b/api/test/session-cookie-shadow.test.js @@ -0,0 +1,35 @@ +const { tmpPath } = require('../test-support/tmp'); +process.env.CONFIG_PATH = tmpPath('apps.json'); + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const { saveConfig } = require('../src/config'); +const { isAuthenticated, makeToken } = require('../src/auth'); + +const SECRET = 'b'.repeat(64); +saveConfig({ items: [], settings: { auth: { enabled: true, secret: SECRET, passwordHash: '$scrypt$x' } } }); +const valid = makeToken('owner-session', SECRET); +const as = cookie => isAuthenticated({ headers: { cookie } }); + +/* A host on the same parent domain can set `ds` with Domain=, and the browser + sends both. Either order must keep the owner signed in. */ +test('a planted session cookie does not hide the real one', () => { + assert.equal(as(`ds=${valid}; ds=planted`), true); + assert.equal(as(`ds=planted; ds=${valid}`), true); +}); + +test('only planted or revoked cookies are still refused', () => { + assert.equal(as('ds=planted; ds=other'), false); + saveConfig({ + items: [], + settings: { + auth: { + enabled: true, + secret: SECRET, + passwordHash: '$scrypt$x', + revoked: { 'owner-session': Date.now() + 60_000 }, + }, + }, + }); + assert.equal(as(`ds=planted; ds=${valid}`), false); +}); diff --git a/ui/widgets/connections/data.js b/ui/widgets/connections/data.js index fd0c77a0..ed79e5bd 100644 --- a/ui/widgets/connections/data.js +++ b/ui/widgets/connections/data.js @@ -92,8 +92,7 @@ const MAP_DEFAULT_COLOR = { const TIMEOUT_CODES = new Set(['ETIMEDOUT', 'ESOCKETTIMEDOUT', 'UND_ERR_HEADERS_TIMEOUT']); function errorText(e) { if (e && e.vouchedMessage) return e.vouchedMessage; - /* Already a sentence this repo wrote. */ - if (e && e.name === 'SsrfBlockedError') return e.message; + if (e && e.name === 'SsrfBlockedError') return 'The request was blocked.'; const code = e && e.code; if (code === 'ECONNREFUSED') return 'Connection refused'; if (code === 'ENOTFOUND' || code === 'EAI_AGAIN') return 'Host not found';