Security: accept any valid session cookie and hide blocked addresses

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,

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', () => {

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/);
+});

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 ────────────────────────────────────────────────────────────── */

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);
+});

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';