Merge 1e55a64 into d8c3ba6

Author: jsjoeio

CHANGELOG.md

@@ -59,6 +59,7 @@ VS Code v0.00.0
 - chore: cross-compile docker images with buildx #3166 @oxy
 - chore: update node to v14 #3458 @oxy
 - chore: update .gitignore #3557 @cuining
+- fix: use sufficient computational effort for password hash #3422 @jsjoeio
 
 ### Development
 

ci/build/build-standalone-release.sh

@@ -1,6 +1,10 @@
 #!/usr/bin/env bash
 set -euo pipefail
 
+# This is due to an upstream issue with RHEL7/CentOS 7 comptability with node-argon2
+# See: https://github.com/cdr/code-server/pull/3422#pullrequestreview-677765057
+export npm_config_build_from_source=true
+
 main() {
   cd "$(dirname "${0}")/../.."
   source ./ci/lib.sh

ci/build/npm-postinstall.sh

@@ -18,6 +18,9 @@ detect_arch() {
 }
 
 ARCH="${NPM_CONFIG_ARCH:-$(detect_arch)}"
+# This is due to an upstream issue with RHEL7/CentOS 7 comptability with node-argon2
+# See: https://github.com/cdr/code-server/pull/3422#pullrequestreview-677765057
+export npm_config_build_from_source=true
 
 main() {
   # Grabs the major version of node from $npm_config_user_agent which looks like

docs/FAQ.md

@@ -205,17 +205,18 @@ Again, please follow [./guide.md](./guide.md) for our recommendations on setting
 
 Yes you can! Set the value of `hashed-password` instead of `password`. Generate the hash with:
 
-```
-printf "thisismypassword" | sha256sum | cut -d' ' -f1
+```shell
+echo -n "password" | npx argon2-cli -e
+$argon2i$v=19$m=4096,t=3,p=1$wst5qhbgk2lu1ih4dmuxvg$ls1alrvdiwtvzhwnzcm1dugg+5dto3dt1d5v9xtlws4
 ```
 
-Of course replace `thisismypassword` with your actual password.
+Of course replace `thisismypassword` with your actual password and **remember to put it inside quotes**!
 
 Example:
 
 ```yaml
 auth: password
-hashed-password: 1da9133ab9dbd11d2937ec8d312e1e2569857059e73cc72df92e670928983ab5 # You got this from the command above
+hashed-password: "$argon2i$v=19$m=4096,t=3,p=1$wST5QhBgk2lu1ih4DMuxvg$LS1alrVdIWtvZHwnzCM1DUGg+5DTO3Dt1d5v9XtLws4"
 ```
 
 ## How do I securely access web services?

package.json

@@ -88,6 +88,7 @@
   },
   "dependencies": {
     "@coder/logger": "1.1.16",
+    "argon2": "^0.28.0",
     "body-parser": "^1.19.0",
     "compression": "^1.7.4",
     "cookie-parser": "^1.4.5",

src/node/cli.ts

@@ -114,7 +114,7 @@ const options: Options<Required<Args>> = {
   "hashed-password": {
     type: "string",
     description:
-      "The password hashed with SHA-256 for password authentication (can only be passed in via $HASHED_PASSWORD or the config file). \n" +
+      "The password hashed with argon2 for password authentication (can only be passed in via $HASHED_PASSWORD or the config file). \n" +
       "Takes precedence over 'password'.",
   },
   cert: {
@@ -240,6 +240,19 @@ export const optionDescriptions = (): string[] => {
   })
 }
 
+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
+}
+
 export const parse = (
   argv: string[],
   opts?: {
@@ -250,6 +263,7 @@ export const parse = (
     if (opts?.configFile) {
       msg = `error reading ${opts.configFile}: ${msg}`
     }
+
     return new Error(msg)
   }
 
@@ -270,7 +284,7 @@ export const parse = (
       let key: keyof Args | undefined
       let value: string | undefined
       if (arg.startsWith("--")) {
-        const split = arg.replace(/^--/, "").split("=", 2)
+        const split = splitOnFirstEquals(arg.replace(/^--/, ""))
         key = split[0] as keyof Args
         value = split[1]
       } else {

src/node/http.ts

@@ -2,13 +2,12 @@ import { field, logger } from "@coder/logger"
 import * as express from "express"
 import * as expressCore from "express-serve-static-core"
 import qs from "qs"
-import safeCompare from "safe-compare"
 import { HttpCode, HttpError } from "../common/http"
 import { normalize, Options } from "../common/util"
 import { AuthType, DefaultedArgs } from "./cli"
 import { commit, rootPath } from "./constants"
 import { Heart } from "./heart"
-import { hash } from "./util"
+import { getPasswordMethod, IsCookieValidArgs, isCookieValid, sanitizeString } from "./util"
 
 declare global {
   // eslint-disable-next-line @typescript-eslint/no-namespace
@@ -45,8 +44,13 @@ export const replaceTemplates = <T extends object>(
 /**
  * Throw an error if not authorized. Call `next` if provided.
  */
-export const ensureAuthenticated = (req: express.Request, _?: express.Response, next?: express.NextFunction): void => {
-  if (!authenticated(req)) {
+export const ensureAuthenticated = async (
+  req: express.Request,
+  _?: express.Response,
+  next?: express.NextFunction,
+): Promise<void> => {
+  const isAuthenticated = await authenticated(req)
+  if (!isAuthenticated) {
     throw new HttpError("Unauthorized", HttpCode.Unauthorized)
   }
   if (next) {
@@ -57,20 +61,27 @@ export const ensureAuthenticated = (req: express.Request, _?: express.Response,
 /**
  * Return true if authenticated via cookies.
  */
-export const authenticated = (req: express.Request): boolean => {
+export const authenticated = async (req: express.Request): Promise<boolean> => {
   switch (req.args.auth) {
-    case AuthType.None:
+    case AuthType.None: {
       return true
-    case AuthType.Password:
+    }
+    case AuthType.Password: {
       // The password is stored in the cookie after being hashed.
-      return !!(
-        req.cookies.key &&
-        (req.args["hashed-password"]
-          ? safeCompare(req.cookies.key, req.args["hashed-password"])
-          : req.args.password && safeCompare(req.cookies.key, hash(req.args.password)))
-      )
-    default:
+      const hashedPasswordFromArgs = req.args["hashed-password"]
+      const passwordMethod = getPasswordMethod(hashedPasswordFromArgs)
+      const isCookieValidArgs: IsCookieValidArgs = {
+        passwordMethod,
+        cookieKey: sanitizeString(req.cookies.key),
+        passwordFromArgs: req.args.password || "",
+        hashedPasswordFromArgs: req.args["hashed-password"],
+      }
+
+      return await isCookieValid(isCookieValidArgs)
+    }
+    default: {
       throw new Error(`Unsupported auth type ${req.args.auth}`)
+    }
   }
 }
 

src/node/routes/domainProxy.ts

@@ -32,14 +32,15 @@ const maybeProxy = (req: Request): string | undefined => {
   return port
 }
 
-router.all("*", (req, res, next) => {
+router.all("*", async (req, res, next) => {
   const port = maybeProxy(req)
   if (!port) {
     return next()
   }
 
   // Must be authenticated to use the proxy.
-  if (!authenticated(req)) {
+  const isAuthenticated = await authenticated(req)
+  if (!isAuthenticated) {
     // Let the assets through since they're used on the login page.
     if (req.path.startsWith("/static/") && req.method === "GET") {
       return next()
@@ -73,14 +74,14 @@ router.all("*", (req, res, next) => {
 
 export const wsRouter = WsRouter()
 
-wsRouter.ws("*", (req, _, next) => {
+wsRouter.ws("*", async (req, _, next) => {
   const port = maybeProxy(req)
   if (!port) {
     return next()
   }
 
   // Must be authenticated to use the proxy.
-  ensureAuthenticated(req)
+  await ensureAuthenticated(req)
 
   proxy.ws(req, req.ws, req.head, {
     ignorePath: true,

src/node/routes/index.ts

@@ -98,8 +98,8 @@ export const register = async (
   app.all("/proxy/(:port)(/*)?", (req, res) => {
     pathProxy.proxy(req, res)
   })
-  wsApp.get("/proxy/(:port)(/*)?", (req) => {
-    pathProxy.wsProxy(req as pluginapi.WebsocketRequest)
+  wsApp.get("/proxy/(:port)(/*)?", async (req) => {
+    await pathProxy.wsProxy(req as pluginapi.WebsocketRequest)
   })
   // These two routes pass through the path directly.
   // So the proxied app must be aware it is running
@@ -109,8 +109,8 @@ export const register = async (
       passthroughPath: true,
     })
   })
-  wsApp.get("/absproxy/(:port)(/*)?", (req) => {
-    pathProxy.wsProxy(req as pluginapi.WebsocketRequest, {
+  wsApp.get("/absproxy/(:port)(/*)?", async (req) => {
+    await pathProxy.wsProxy(req as pluginapi.WebsocketRequest, {
       passthroughPath: true,
     })
   })

src/node/routes/login.ts

@@ -2,10 +2,9 @@ import { Router, Request } from "express"
 import { promises as fs } from "fs"
 import { RateLimiter as Limiter } from "limiter"
 import * as path from "path"
-import safeCompare from "safe-compare"
 import { rootPath } from "../constants"
 import { authenticated, getCookieDomain, redirect, replaceTemplates } from "../http"
-import { hash, humanPath } from "../util"
+import { getPasswordMethod, handlePasswordValidation, humanPath, sanitizeString } from "../util"
 
 export enum Cookie {
   Key = "key",
@@ -49,9 +48,9 @@ const limiter = new RateLimiter()
 
 export const router = Router()
 
-router.use((req, res, next) => {
+router.use(async (req, res, next) => {
   const to = (typeof req.query.to === "string" && req.query.to) || "/"
-  if (authenticated(req)) {
+  if (await authenticated(req)) {
     return redirect(req, res, to, { to: undefined })
   }
   next()
@@ -62,24 +61,31 @@ router.get("/", async (req, res) => {
 })
 
 router.post("/", async (req, res) => {
+  const password = sanitizeString(req.body.password)
+  const hashedPasswordFromArgs = req.args["hashed-password"]
+
   try {
     // Check to see if they exceeded their login attempts
     if (!limiter.canTry()) {
       throw new Error("Login rate limited!")
     }
 
-    if (!req.body.password) {
+    if (!password) {
       throw new Error("Missing password")
     }
 
-    if (
-      req.args["hashed-password"]
-        ? safeCompare(hash(req.body.password), req.args["hashed-password"])
-        : req.args.password && safeCompare(req.body.password, req.args.password)
-    ) {
+    const passwordMethod = getPasswordMethod(hashedPasswordFromArgs)
+    const { isPasswordValid, hashedPassword } = await handlePasswordValidation({
+      passwordMethod,
+      hashedPasswordFromArgs,
+      passwordFromRequestBody: password,
+      passwordFromArgs: req.args.password,
+    })
+
+    if (isPasswordValid) {
       // The hash does not add any actual security but we do it for
       // obfuscation purposes (and as a side effect it handles escaping).
-      res.cookie(Cookie.Key, hash(req.body.password), {
+      res.cookie(Cookie.Key, hashedPassword, {
         domain: getCookieDomain(req.headers.host || "", req.args["proxy-domain"]),
         path: req.body.base || "/",
         sameSite: "lax",