Add origin checks to web sockets

CHANGELOG.md

@@ -20,6 +20,18 @@ Code v99.99.999
 
 -->
 
+## Unreleased
+
+Code v1.75.1
+
+### Security
+
+Add an origin check to web sockets to prevent a cross-site hijacking attack that
+affects those who use older or niche browsers that do not support SameSite
+cookies and those who access code-server under a shared domain with other users
+on separate sub-domains. The check requires the host header to be set so if you
+use a reverse proxy ensure it forwards that information.
+
 ## [4.10.0](https://github.com/coder/code-server/releases/tag/v4.10.0) - 2023-02-15
 
 Code v1.75.1

src/common/http.ts

@@ -4,6 +4,7 @@ export enum HttpCode {
   NotFound = 404,
   BadRequest = 400,
   Unauthorized = 401,
+  Forbidden = 403,
   LargePayload = 413,
   ServerError = 500,
 }

src/node/cli.ts

@@ -3,7 +3,15 @@ import { promises as fs } from "fs"
 import { load } from "js-yaml"
 import * as os from "os"
 import * as path from "path"
-import { canConnect, generateCertificate, generatePassword, humanPath, paths, isNodeJSErrnoException } from "./util"
+import {
+  canConnect,
+  generateCertificate,
+  generatePassword,
+  humanPath,
+  paths,
+  isNodeJSErrnoException,
+  splitOnFirstEquals,
+} from "./util"
 
 const DEFAULT_SOCKET_PATH = path.join(os.tmpdir(), "vscode-ipc")
 
@@ -292,19 +300,6 @@ export const optionDescriptions = (opts: Partial<Options<Required<UserProvidedAr
   })
 }
 
-export function splitOnFirstEquals(str: string): string[] {
-  // we use regex instead of "=" to ensure we split at the first
-  // "=" and return the following substring with it
-  // important for the hashed-password which looks like this
-  // $argon2i$v=19$m=4096,t=3,p=1$0qR/o+0t00hsbJFQCKSfdQ$oFcM4rL6o+B7oxpuA4qlXubypbBPsf+8L531U7P9HYY
-  // 2 means return two items
-  // Source: https://stackoverflow.com/a/4607799/3015595
-  // We use the ? to say the the substr after the = is optional
-  const split = str.split(/=(.+)?/, 2)
-
-  return split
-}
-
 /**
  * Parse arguments into UserProvidedArgs.  This should not go beyond checking
  * that arguments are valid types and have values when required.

src/node/http.ts

@@ -12,7 +12,15 @@ import { version as codeServerVersion } from "./constants"
 import { Heart } from "./heart"
 import { CoderSettings, SettingsProvider } from "./settings"
 import { UpdateProvider } from "./update"
-import { getPasswordMethod, IsCookieValidArgs, isCookieValid, sanitizeString, escapeHtml, escapeJSON } from "./util"
+import {
+  getPasswordMethod,
+  IsCookieValidArgs,
+  isCookieValid,
+  sanitizeString,
+  escapeHtml,
+  escapeJSON,
+  splitOnFirstEquals,
+} from "./util"
 
 /**
  * Base options included on every page.
@@ -308,3 +316,68 @@ export const getCookieOptions = (req: express.Request): express.CookieOptions =>
 export const self = (req: express.Request): string => {
   return normalize(`${req.baseUrl}${req.originalUrl.endsWith("/") ? "/" : ""}`, true)
 }
+
+function getFirstHeader(req: http.IncomingMessage, headerName: string): string | undefined {
+  const val = req.headers[headerName]
+  return Array.isArray(val) ? val[0] : val
+}
+
+/**
+ * Throw an error if origin checks fail. Call `next` if provided.
+ */
+export function ensureOrigin(req: express.Request, _?: express.Response, next?: express.NextFunction): void {
+  if (!authenticateOrigin(req)) {
+    throw new HttpError("Forbidden", HttpCode.Forbidden)
+  }
+  if (next) {
+    next()
+  }
+}
+
+/**
+ * Authenticate the request origin against the host.
+ */
+export function authenticateOrigin(req: express.Request): boolean {
+  // A missing origin probably means the source is non-browser.  Not sure we
+  // have a use case for this but let it through.
+  const originRaw = getFirstHeader(req, "origin")
+  if (!originRaw) {
+    return true
+  }
+
+  let origin: string
+  try {
+    origin = new URL(originRaw).host.trim().toLowerCase()
+  } catch (error) {
+    return false // Malformed URL.
+  }
+
+  // Honor Forwarded if present.
+  const forwardedRaw = getFirstHeader(req, "forwarded")
+  if (forwardedRaw) {
+    const parts = forwardedRaw.split(/[;,]/)
+    for (let i = 0; i < parts.length; ++i) {
+      const [key, value] = splitOnFirstEquals(parts[i])
+      if (key.trim().toLowerCase() === "host" && value) {
+        return origin === value.trim().toLowerCase()
+      }
+    }
+  }
+
+  // Honor X-Forwarded-Host if present.
+  const xHost = getFirstHeader(req, "x-forwarded-host")
+  if (xHost) {
+    return origin === xHost.trim().toLowerCase()
+  }
+
+  // A missing host likely means the reverse proxy has not been configured to
+  // forward the host which means we cannot perform the check.  Emit a warning
+  // so an admin can fix the issue.
+  const host = getFirstHeader(req, "host")
+  if (!host) {
+    logger.warn(`no host headers found; blocking request to ${req.originalUrl}`)
+    return false
+  }
+
+  return origin === host.trim().toLowerCase()
+}

src/node/routes/domainProxy.ts

@@ -1,6 +1,6 @@
 import { Request, Router } from "express"
 import { HttpCode, HttpError } from "../../common/http"
-import { authenticated, ensureAuthenticated, redirect, self } from "../http"
+import { authenticated, ensureAuthenticated, ensureOrigin, redirect, self } from "../http"
 import { proxy } from "../proxy"
 import { Router as WsRouter } from "../wsRouter"
 
@@ -78,10 +78,8 @@ wsRouter.ws("*", async (req, _, next) => {
   if (!port) {
     return next()
   }
-
-  // Must be authenticated to use the proxy.
+  ensureOrigin(req)
   await ensureAuthenticated(req)
-
   proxy.ws(req, req.ws, req.head, {
     ignorePath: true,
     target: `http://0.0.0.0:${port}${req.originalUrl}`,

src/node/routes/errors.ts

@@ -63,5 +63,11 @@ export const errorHandler: express.ErrorRequestHandler = async (err, req, res, n
 
 export const wsErrorHandler: express.ErrorRequestHandler = async (err, req, res, next) => {
   logger.error(`${err.message} ${err.stack}`)
-  ;(req as WebsocketRequest).ws.end()
+  let statusCode = 500
+  if (errorHasStatusCode(err)) {
+    statusCode = err.statusCode
+  } else if (errorHasCode(err) && notFoundCodes.includes(err.code)) {
+    statusCode = HttpCode.NotFound
+  }
+  ;(req as WebsocketRequest).ws.end(`HTTP/1.1 ${statusCode} ${err.message}\r\n\r\n`)
 }

src/node/routes/pathProxy.ts

@@ -3,7 +3,7 @@ import * as path from "path"
 import * as qs from "qs"
 import * as pluginapi from "../../../typings/pluginapi"
 import { HttpCode, HttpError } from "../../common/http"
-import { authenticated, ensureAuthenticated, redirect, self } from "../http"
+import { authenticated, ensureAuthenticated, ensureOrigin, redirect, self } from "../http"
 import { proxy as _proxy } from "../proxy"
 
 const getProxyTarget = (req: Request, passthroughPath?: boolean): string => {
@@ -50,6 +50,7 @@ export async function wsProxy(
     passthroughPath?: boolean
   },
 ): Promise<void> {
+  ensureOrigin(req)
   await ensureAuthenticated(req)
   _proxy.ws(req, req.ws, req.head, {
     ignorePath: true,

src/node/routes/vscode.ts

@@ -7,7 +7,7 @@ import { WebsocketRequest } from "../../../typings/pluginapi"
 import { logError } from "../../common/util"
 import { CodeArgs, toCodeArgs } from "../cli"
 import { isDevMode } from "../constants"
-import { authenticated, ensureAuthenticated, redirect, replaceTemplates, self } from "../http"
+import { authenticated, ensureAuthenticated, ensureOrigin, redirect, replaceTemplates, self } from "../http"
 import { SocketProxyProvider } from "../socket"
 import { isFile, loadAMDModule } from "../util"
 import { Router as WsRouter } from "../wsRouter"
@@ -173,7 +173,7 @@ export class CodeServerRouteWrapper {
     this.router.get("/", this.ensureCodeServerLoaded, this.$root)
     this.router.get("/manifest.json", this.manifest)
     this.router.all("*", ensureAuthenticated, this.ensureCodeServerLoaded, this.$proxyRequest)
-    this._wsRouterWrapper.ws("*", ensureAuthenticated, this.ensureCodeServerLoaded, this.$proxyWebsocket)
+    this._wsRouterWrapper.ws("*", ensureOrigin, ensureAuthenticated, this.ensureCodeServerLoaded, this.$proxyWebsocket)
   }
 
   dispose() {

src/node/util.ts

@@ -541,3 +541,13 @@ export const loadAMDModule = async <T>(amdPath: string, exportName: string): Pro
 
   return module[exportName] as T
 }
+
+/**
+ * Split a string on the first equals.  The result will always be an array with
+ * two items regardless of how many equals there are.  The second item will be
+ * undefined if empty or missing.
+ */
+export function splitOnFirstEquals(str: string): [string, string | undefined] {
+  const split = str.split(/=(.+)?/, 2)
+  return [split[0], split[1]]
+}

src/node/wsRouter.ts

@@ -32,6 +32,9 @@ export class WebsocketRouter {
   /**
    * Handle a websocket at this route. Note that websockets are immediately
    * paused when they come in.
+   *
+   * If the origin header exists it must match the host or the connection will
+   * be prevented.
    */
   public ws(route: expressCore.PathParams, ...handlers: pluginapi.WebSocketHandler[]): void {
     this.router.get(

test/unit/node/cli.test.ts

@@ -11,7 +11,6 @@ import {
   readSocketPath,
   setDefaults,
   shouldOpenInExistingInstance,
-  splitOnFirstEquals,
   toCodeArgs,
   optionDescriptions,
   options,
@@ -535,31 +534,6 @@ describe("cli", () => {
   })
 })
 
-describe("splitOnFirstEquals", () => {
-  it("should split on the first equals", () => {
-    const testStr = "enabled-proposed-api=test=value"
-    const actual = splitOnFirstEquals(testStr)
-    const expected = ["enabled-proposed-api", "test=value"]
-    expect(actual).toEqual(expect.arrayContaining(expected))
-  })
-  it("should split on first equals regardless of multiple equals signs", () => {
-    const testStr =
-      "hashed-password=$argon2i$v=19$m=4096,t=3,p=1$0qR/o+0t00hsbJFQCKSfdQ$oFcM4rL6o+B7oxpuA4qlXubypbBPsf+8L531U7P9HYY"
-    const actual = splitOnFirstEquals(testStr)
-    const expected = [
-      "hashed-password",
-      "$argon2i$v=19$m=4096,t=3,p=1$0qR/o+0t00hsbJFQCKSfdQ$oFcM4rL6o+B7oxpuA4qlXubypbBPsf+8L531U7P9HYY",
-    ]
-    expect(actual).toEqual(expect.arrayContaining(expected))
-  })
-  it("should always return the first element before an equals", () => {
-    const testStr = "auth="
-    const actual = splitOnFirstEquals(testStr)
-    const expected = ["auth"]
-    expect(actual).toEqual(expect.arrayContaining(expected))
-  })
-})
-
 describe("shouldSpawnCliProcess", () => {
   it("should return false if no 'extension' related args passed in", async () => {
     const args = {}