From e68f20907c6cf26038d3c607d806c596d1008c02 Mon Sep 17 00:00:00 2001 From: Nils1729 <45318774+Nils1729@users.noreply.github.com> Date: Sat, 16 Sep 2023 13:06:14 +0200 Subject: [PATCH] Merge pull request from GHSA-jvmr-5fpx-r76j * Enforce allowlist for query parameters Also mitigate other argument injection * Add allow-remote-command option --- src/main.ts | 7 ++++- src/server.ts | 12 ++------ src/server/command.ts | 53 +++++++++++++++++++-------------- src/server/command/address.ts | 37 ++++++++++++++--------- src/server/command/ssh.ts | 40 ++++++++++--------------- src/server/shared/shell.spec.ts | 5 ++++ src/server/shared/shell.ts | 2 +- src/shared/config.ts | 1 + src/shared/defaults.ts | 1 + src/shared/interfaces.ts | 1 + 10 files changed, 85 insertions(+), 74 deletions(-) diff --git a/src/main.ts b/src/main.ts index 77fd6136..1edb7251 100644 --- a/src/main.ts +++ b/src/main.ts @@ -102,7 +102,12 @@ const opts = yargs(hideBin(process.argv)) }) .option('allow-remote-hosts', { description: - 'Allow WeTTY to use the `host` param in a url as ssh destination', + 'Allow WeTTY to use the `host` and `port` params in a url as ssh destination', + type: 'boolean', + }) + .option('allow-remote-command', { + description: + 'Allow WeTTY to use the `command` and `path` params in a url as command and working directory on ssh host', type: 'boolean', }) .option('log-level', { diff --git a/src/server.ts b/src/server.ts index fa612e3a..d061fdef 100644 --- a/src/server.ts +++ b/src/server.ts @@ -6,9 +6,7 @@ import express from 'express'; import gc from 'gc-stats'; import { Gauge, collectDefaultMetrics } from 'prom-client'; import { getCommand } from './server/command.js'; -import { login } from './server/login.js'; import { gcMetrics } from './server/metrics.js'; -import { escapeShell } from './server/shared/shell.js'; import { server } from './server/socketServer.js'; import { spawn } from './server/spawn.js'; import { @@ -75,17 +73,11 @@ export async function decorateServerWithSsh( * @name connection */ logger.info('Connection accepted.'); - const [args, sshUser] = getCommand(socket, ssh, command, forcessh); - const cmd = args.join(' '); - logger.debug('Command Generated', { user: sshUser, cmd }); wettyConnections.inc(); try { - if (!sshUser) { - const username = await login(socket); - args[1] = `${escapeShell(username.trim())}@${args[1]}`; - logger.debug('Spawning term', { username: username.trim(), cmd }); - } + const args = await getCommand(socket, ssh, command, forcessh); + logger.debug('Command Generated', { cmd: args.join(' ') }); await spawn(socket, args); } catch (error) { logger.info('Disconnect signal sent', { err: error }); diff --git a/src/server/command.ts b/src/server/command.ts index 0b630003..242e4afc 100644 --- a/src/server/command.ts +++ b/src/server/command.ts @@ -14,17 +14,23 @@ const localhost = (host: string): boolean => const urlArgs = ( referer: string | undefined, - def: { [s: string]: string }, + { + allowRemoteCommand, + allowRemoteHosts, + }: { + allowRemoteCommand: boolean; + allowRemoteHosts: boolean; + }, ): { [s: string]: string } => - Object.assign(def, url.parse(referer || '', true).query); + _.pick( + _.pickBy(url.parse(referer || '', true).query, _.isString), + ['pass'], + allowRemoteCommand ? ['command', 'path'] : [], + allowRemoteHosts ? ['port', 'host'] : [], + ); -export function getCommand( - { - request: { headers }, - client: { - conn: { remoteAddress }, - }, - }: Socket, +export async function getCommand( + socket: Socket, { user, host, @@ -35,15 +41,22 @@ export function getCommand( knownHosts, config, allowRemoteHosts, + allowRemoteCommand, }: SSH, command: string, - forcessh: boolean, -): [string[], boolean] { - const sshAddress = address(headers, user, host); + forcessh: boolean +): Promise { + const { + request: { headers: { referer } }, + client: { conn: { remoteAddress } }, + } = socket; + if (!forcessh && localhost(host)) { - return [loginOptions(command, remoteAddress), true]; + return loginOptions(command, remoteAddress); } - const args = urlArgs(headers.referer, { + + const sshAddress = await address(socket, user, host); + const args = { host: sshAddress, port: `${port}`, pass: pass || '', @@ -51,13 +64,7 @@ export function getCommand( auth, knownHosts, config: config || '', - }); - if (!allowRemoteHosts) { - args.host = sshAddress; - } - - return [ - sshOptions(args, key), - user !== '' || user.includes('@') || sshAddress.includes('@'), - ]; + ...urlArgs(referer, { allowRemoteHosts, allowRemoteCommand }), + }; + return sshOptions(args, key); } diff --git a/src/server/command/address.ts b/src/server/command/address.ts index ae3ba9c7..30620d4f 100644 --- a/src/server/command/address.ts +++ b/src/server/command/address.ts @@ -1,23 +1,32 @@ import _ from 'lodash'; +import { Socket } from 'socket.io'; +import { login } from '../login.js'; import { escapeShell } from '../shared/shell.js'; -import type { IncomingHttpHeaders } from 'http'; -export function address( - headers: IncomingHttpHeaders, +export async function address( + socket: Socket, user: string, host: string, -): string { +): Promise { // Check request-header for username - const remoteUser = headers['remote-user']; - if (!_.isUndefined(remoteUser) && !Array.isArray(remoteUser)) { - return `${escapeShell(remoteUser)}@${host}`; - } - if (!_.isUndefined(headers.referer)) { - const match = headers.referer.match('.+/ssh/([^/]+)$'); - if (match) { - const username = escapeShell(match[1].split('?')[0]); - return `${username}@${host}`; + const { request: { headers: { + 'remote-user': userFromHeader, + referer + } } } = socket; + + let username: string | undefined; + if (!_.isUndefined(userFromHeader) && !Array.isArray(userFromHeader)) { + username = userFromHeader; + } else { + const userFromPathMatch = referer?.match('.+/ssh/([^/]+)$'); + if (userFromPathMatch) { + // eslint-disable-next-line prefer-destructuring + username = userFromPathMatch[1].split('?')[0]; + } else if (user) { + username = user; + } else { + username = await login(socket); } } - return user ? `${escapeShell(user)}@${host}` : host; + return `${escapeShell(username)}@${host}`; } diff --git a/src/server/command/ssh.ts b/src/server/command/ssh.ts index b01bdd47..160d586a 100644 --- a/src/server/command/ssh.ts +++ b/src/server/command/ssh.ts @@ -17,32 +17,22 @@ export function sshOptions( const cmd = parseCommand(command, path); const hostChecking = knownHosts !== '/dev/null' ? 'yes' : 'no'; logger().info(`Authentication Type: ${auth}`); - let sshRemoteOptsBase = ['ssh', host, '-t']; - if (!isUndefined(config) && config !== '') { - sshRemoteOptsBase = sshRemoteOptsBase.concat(['-F', config]); - } - if (!isUndefined(port) && port !== '') { - sshRemoteOptsBase = sshRemoteOptsBase.concat(['-p', port]); - } - sshRemoteOptsBase = sshRemoteOptsBase.concat([ - '-o', - `PreferredAuthentications=${auth}`, - '-o', - `UserKnownHostsFile=${knownHosts}`, - '-o', - `StrictHostKeyChecking=${hostChecking}`, - ]); - if (!isUndefined(key)) { - return sshRemoteOptsBase.concat(['-i', key, cmd]); - } - if (pass !== '') { - return ['sshpass', '-p', pass].concat(sshRemoteOptsBase, [cmd]); - } - if (auth === 'none') { - sshRemoteOptsBase.splice(sshRemoteOptsBase.indexOf('-o'), 2); - } - return cmd === '' ? sshRemoteOptsBase : sshRemoteOptsBase.concat([cmd]); + return [ + ...pass ? ['sshpass', '-p', pass] : [], + 'ssh', + '-t', + ...config ? ['-F', config] : [], + ...port ? ['-p', port] : [], + ...key ? ['-i', key] : [], + ...auth !== 'none' ? ['-o', `PreferredAuthentications=${auth}`] : [], + '-o', `UserKnownHostsFile=${knownHosts}`, + '-o', `StrictHostKeyChecking=${hostChecking}`, + '-o', 'EscapeChar=none', + '--', + host, + ...cmd ? [cmd] : [], + ]; } function parseCommand(command: string, path?: string): string { diff --git a/src/server/shared/shell.spec.ts b/src/server/shared/shell.spec.ts index 1ea5e272..ac40f4b7 100644 --- a/src/server/shared/shell.spec.ts +++ b/src/server/shared/shell.spec.ts @@ -17,4 +17,9 @@ describe('Values passed to escapeShell should be safe to pass woth sub processes const cmd = escapeShell("-oProxyCommand='bash' -c `wget localhost:2222`"); expect(cmd).to.equal('oProxyCommandbash-cwgetlocalhost2222'); }); + + it('should remove dashes even when there are illegal characters before them', () => { + const cmd = escapeShell("`-oProxyCommand='bash' -c `wget localhost:2222`"); + expect(cmd).to.equal('oProxyCommandbash-cwgetlocalhost2222'); + }); }); diff --git a/src/server/shared/shell.ts b/src/server/shared/shell.ts index d0299f3a..e9a43100 100644 --- a/src/server/shared/shell.ts +++ b/src/server/shared/shell.ts @@ -1,3 +1,3 @@ export const escapeShell = (username: string): string => // eslint-disable-next-line no-useless-escape - username.replace(/^-|[^a-zA-Z0-9_\\\-\.\@-]/g, ''); + username.replace(/[^a-zA-Z0-9_\\\-\.\@-]/g, '').replace(/^-+/g, ''); diff --git a/src/shared/config.ts b/src/shared/config.ts index 51c5a75d..237474ac 100644 --- a/src/shared/config.ts +++ b/src/shared/config.ts @@ -138,6 +138,7 @@ export function mergeCliConf(opts: Arguments, config: Config): Config { pass: opts['ssh-pass'], key: opts['ssh-key'], allowRemoteHosts: opts['allow-remote-hosts'], + allowRemoteCommand: opts['allow-remote-command'], config: opts['ssh-config'], knownHosts: opts['known-hosts'], }) as SSH, diff --git a/src/shared/defaults.ts b/src/shared/defaults.ts index b687ff75..8124d562 100644 --- a/src/shared/defaults.ts +++ b/src/shared/defaults.ts @@ -10,6 +10,7 @@ export const sshDefault: SSH = { port: parseInt(process.env.SSHPORT || '22', 10), knownHosts: process.env.KNOWNHOSTS || '/dev/null', allowRemoteHosts: false, + allowRemoteCommand: false, config: process.env.SSHCONFIG || undefined, }; diff --git a/src/shared/interfaces.ts b/src/shared/interfaces.ts index 4f2cdbbb..4f5e0d39 100644 --- a/src/shared/interfaces.ts +++ b/src/shared/interfaces.ts @@ -8,6 +8,7 @@ export interface SSH { port: number; knownHosts: string; allowRemoteHosts: boolean; + allowRemoteCommand: boolean; pass?: string; key?: string; config?: string;