Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Commit 717eaa6

Browse files
authored
Merge pull request #3422 from cdr/jsjoeio/fix-password-hash
fix: use sufficient computational effort for password hash
2 parents d8c3ba6 + 1e55a64 commit 717eaa6

File tree

21 files changed

+1083
-69
lines changed

21 files changed

+1083
-69
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ VS Code v0.00.0
5959
- chore: cross-compile docker images with buildx #3166 @oxy
6060
- chore: update node to v14 #3458 @oxy
6161
- chore: update .gitignore #3557 @cuining
62+
- fix: use sufficient computational effort for password hash #3422 @jsjoeio
6263

6364
### Development
6465

‎ci/build/build-standalone-release.sh‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,10 @@
11
#!/usr/bin/env bash
22
set -euo pipefail
33

4+
# This is due to an upstream issue with RHEL7/CentOS 7 comptability with node-argon2
5+
# See: https://github.com/cdr/code-server/pull/3422#pullrequestreview-677765057
6+
export npm_config_build_from_source=true
7+
48
main() {
59
cd "$(dirname "${0}")/../.."
610
source ./ci/lib.sh

‎ci/build/npm-postinstall.sh‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,9 @@ detect_arch() {
1818
}
1919

2020
ARCH="${NPM_CONFIG_ARCH:-$(detect_arch)}"
21+
# This is due to an upstream issue with RHEL7/CentOS 7 comptability with node-argon2
22+
# See: https://github.com/cdr/code-server/pull/3422#pullrequestreview-677765057
23+
export npm_config_build_from_source=true
2124

2225
main() {
2326
# Grabs the major version of node from $npm_config_user_agent which looks like

‎docs/FAQ.md‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -205,17 +205,18 @@ Again, please follow [./guide.md](./guide.md) for our recommendations on setting
205205

206206
Yes you can! Set the value of `hashed-password` instead of `password`. Generate the hash with:
207207

208-
```
209-
printf "thisismypassword" | sha256sum | cut -d' ' -f1
208+
```shell
209+
echo -n "password" | npx argon2-cli -e
210+
$argon2i$v=19$m=4096,t=3,p=1$wst5qhbgk2lu1ih4dmuxvg$ls1alrvdiwtvzhwnzcm1dugg+5dto3dt1d5v9xtlws4
210211
```
211212

212-
Of course replace `thisismypassword` with your actual password.
213+
Of course replace `thisismypassword` with your actual password and **remember to put it inside quotes**!
213214

214215
Example:
215216

216217
```yaml
217218
auth: password
218-
hashed-password: 1da9133ab9dbd11d2937ec8d312e1e2569857059e73cc72df92e670928983ab5 # You got this from the command above
219+
hashed-password: "$argon2i$v=19$m=4096,t=3,p=1$wST5QhBgk2lu1ih4DMuxvg$LS1alrVdIWtvZHwnzCM1DUGg+5DTO3Dt1d5v9XtLws4"
219220
```
220221
221222
## How do I securely access web services?

‎package.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@
8888
},
8989
"dependencies": {
9090
"@coder/logger": "1.1.16",
91+
"argon2": "^0.28.0",
9192
"body-parser": "^1.19.0",
9293
"compression": "^1.7.4",
9394
"cookie-parser": "^1.4.5",

‎src/node/cli.ts‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ const options: Options<Required<Args>> = {
114114
"hashed-password": {
115115
type: "string",
116116
description:
117-
"The password hashed with SHA-256 for password authentication (can only be passed in via $HASHED_PASSWORD or the config file). \n" +
117+
"The password hashed with argon2 for password authentication (can only be passed in via $HASHED_PASSWORD or the config file). \n" +
118118
"Takes precedence over 'password'.",
119119
},
120120
cert: {
@@ -240,6 +240,19 @@ export const optionDescriptions = (): string[] => {
240240
})
241241
}
242242

243+
export function splitOnFirstEquals(str: string): string[] {
244+
// we use regex instead of "=" to ensure we split at the first
245+
// "=" and return the following substring with it
246+
// important for the hashed-password which looks like this
247+
// $argon2i$v=19$m=4096,t=3,p=10ドルqR/o+0t00hsbJFQCKSfdQ$oFcM4rL6o+B7oxpuA4qlXubypbBPsf+8L531U7P9HYY
248+
// 2 means return two items
249+
// Source: https://stackoverflow.com/a/4607799/3015595
250+
// We use the ? to say the the substr after the = is optional
251+
const split = str.split(/=(.+)?/, 2)
252+
253+
return split
254+
}
255+
243256
export const parse = (
244257
argv: string[],
245258
opts?: {
@@ -250,6 +263,7 @@ export const parse = (
250263
if (opts?.configFile) {
251264
msg = `error reading ${opts.configFile}: ${msg}`
252265
}
266+
253267
return new Error(msg)
254268
}
255269

@@ -270,7 +284,7 @@ export const parse = (
270284
let key: keyof Args | undefined
271285
let value: string | undefined
272286
if (arg.startsWith("--")) {
273-
const split = arg.replace(/^--/, "").split("=",2)
287+
const split = splitOnFirstEquals(arg.replace(/^--/, ""))
274288
key = split[0] as keyof Args
275289
value = split[1]
276290
} else {

‎src/node/http.ts‎

Lines changed: 25 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,12 @@ import { field, logger } from "@coder/logger"
22
import * as express from "express"
33
import * as expressCore from "express-serve-static-core"
44
import qs from "qs"
5-
import safeCompare from "safe-compare"
65
import { HttpCode, HttpError } from "../common/http"
76
import { normalize, Options } from "../common/util"
87
import { AuthType, DefaultedArgs } from "./cli"
98
import { commit, rootPath } from "./constants"
109
import { Heart } from "./heart"
11-
import { hash } from "./util"
10+
import { getPasswordMethod,IsCookieValidArgs,isCookieValid,sanitizeString } from "./util"
1211

1312
declare global {
1413
// eslint-disable-next-line @typescript-eslint/no-namespace
@@ -45,8 +44,13 @@ export const replaceTemplates = <T extends object>(
4544
/**
4645
* Throw an error if not authorized. Call `next` if provided.
4746
*/
48-
export const ensureAuthenticated = (req: express.Request, _?: express.Response, next?: express.NextFunction): void => {
49-
if (!authenticated(req)) {
47+
export const ensureAuthenticated = async (
48+
req: express.Request,
49+
_?: express.Response,
50+
next?: express.NextFunction,
51+
): Promise<void> => {
52+
const isAuthenticated = await authenticated(req)
53+
if (!isAuthenticated) {
5054
throw new HttpError("Unauthorized", HttpCode.Unauthorized)
5155
}
5256
if (next) {
@@ -57,20 +61,27 @@ export const ensureAuthenticated = (req: express.Request, _?: express.Response,
5761
/**
5862
* Return true if authenticated via cookies.
5963
*/
60-
export const authenticated = (req: express.Request): boolean => {
64+
export const authenticated = async(req: express.Request): Promise<boolean> => {
6165
switch (req.args.auth) {
62-
case AuthType.None:
66+
case AuthType.None:{
6367
return true
64-
case AuthType.Password:
68+
}
69+
case AuthType.Password: {
6570
// The password is stored in the cookie after being hashed.
66-
return !!(
67-
req.cookies.key &&
68-
(req.args["hashed-password"]
69-
? safeCompare(req.cookies.key, req.args["hashed-password"])
70-
: req.args.password && safeCompare(req.cookies.key, hash(req.args.password)))
71-
)
72-
default:
71+
const hashedPasswordFromArgs = req.args["hashed-password"]
72+
const passwordMethod = getPasswordMethod(hashedPasswordFromArgs)
73+
const isCookieValidArgs: IsCookieValidArgs = {
74+
passwordMethod,
75+
cookieKey: sanitizeString(req.cookies.key),
76+
passwordFromArgs: req.args.password || "",
77+
hashedPasswordFromArgs: req.args["hashed-password"],
78+
}
79+
80+
return await isCookieValid(isCookieValidArgs)
81+
}
82+
default: {
7383
throw new Error(`Unsupported auth type ${req.args.auth}`)
84+
}
7485
}
7586
}
7687

‎src/node/routes/domainProxy.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,14 +32,15 @@ const maybeProxy = (req: Request): string | undefined => {
3232
return port
3333
}
3434

35-
router.all("*", (req, res, next) => {
35+
router.all("*", async(req, res, next) => {
3636
const port = maybeProxy(req)
3737
if (!port) {
3838
return next()
3939
}
4040

4141
// Must be authenticated to use the proxy.
42-
if (!authenticated(req)) {
42+
const isAuthenticated = await authenticated(req)
43+
if (!isAuthenticated) {
4344
// Let the assets through since they're used on the login page.
4445
if (req.path.startsWith("/static/") && req.method === "GET") {
4546
return next()
@@ -73,14 +74,14 @@ router.all("*", (req, res, next) => {
7374

7475
export const wsRouter = WsRouter()
7576

76-
wsRouter.ws("*", (req, _, next) => {
77+
wsRouter.ws("*", async(req, _, next) => {
7778
const port = maybeProxy(req)
7879
if (!port) {
7980
return next()
8081
}
8182

8283
// Must be authenticated to use the proxy.
83-
ensureAuthenticated(req)
84+
awaitensureAuthenticated(req)
8485

8586
proxy.ws(req, req.ws, req.head, {
8687
ignorePath: true,

‎src/node/routes/index.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -98,8 +98,8 @@ export const register = async (
9898
app.all("/proxy/(:port)(/*)?", (req, res) => {
9999
pathProxy.proxy(req, res)
100100
})
101-
wsApp.get("/proxy/(:port)(/*)?", (req) => {
102-
pathProxy.wsProxy(req as pluginapi.WebsocketRequest)
101+
wsApp.get("/proxy/(:port)(/*)?", async(req) => {
102+
awaitpathProxy.wsProxy(req as pluginapi.WebsocketRequest)
103103
})
104104
// These two routes pass through the path directly.
105105
// So the proxied app must be aware it is running
@@ -109,8 +109,8 @@ export const register = async (
109109
passthroughPath: true,
110110
})
111111
})
112-
wsApp.get("/absproxy/(:port)(/*)?", (req) => {
113-
pathProxy.wsProxy(req as pluginapi.WebsocketRequest, {
112+
wsApp.get("/absproxy/(:port)(/*)?", async(req) => {
113+
awaitpathProxy.wsProxy(req as pluginapi.WebsocketRequest, {
114114
passthroughPath: true,
115115
})
116116
})

‎src/node/routes/login.ts‎

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,9 @@ import { Router, Request } from "express"
22
import { promises as fs } from "fs"
33
import { RateLimiter as Limiter } from "limiter"
44
import * as path from "path"
5-
import safeCompare from "safe-compare"
65
import { rootPath } from "../constants"
76
import { authenticated, getCookieDomain, redirect, replaceTemplates } from "../http"
8-
import { hash,humanPath } from "../util"
7+
import { getPasswordMethod,handlePasswordValidation,humanPath,sanitizeString } from "../util"
98

109
export enum Cookie {
1110
Key = "key",
@@ -49,9 +48,9 @@ const limiter = new RateLimiter()
4948

5049
export const router = Router()
5150

52-
router.use((req, res, next) => {
51+
router.use(async(req, res, next) => {
5352
const to = (typeof req.query.to === "string" && req.query.to) || "/"
54-
if (authenticated(req)) {
53+
if (awaitauthenticated(req)) {
5554
return redirect(req, res, to, { to: undefined })
5655
}
5756
next()
@@ -62,24 +61,31 @@ router.get("/", async (req, res) => {
6261
})
6362

6463
router.post("/", async (req, res) => {
64+
const password = sanitizeString(req.body.password)
65+
const hashedPasswordFromArgs = req.args["hashed-password"]
66+
6567
try {
6668
// Check to see if they exceeded their login attempts
6769
if (!limiter.canTry()) {
6870
throw new Error("Login rate limited!")
6971
}
7072

71-
if (!req.body.password) {
73+
if (!password) {
7274
throw new Error("Missing password")
7375
}
7476

75-
if (
76-
req.args["hashed-password"]
77-
? safeCompare(hash(req.body.password), req.args["hashed-password"])
78-
: req.args.password && safeCompare(req.body.password, req.args.password)
79-
) {
77+
const passwordMethod = getPasswordMethod(hashedPasswordFromArgs)
78+
const { isPasswordValid, hashedPassword } = await handlePasswordValidation({
79+
passwordMethod,
80+
hashedPasswordFromArgs,
81+
passwordFromRequestBody: password,
82+
passwordFromArgs: req.args.password,
83+
})
84+
85+
if (isPasswordValid) {
8086
// The hash does not add any actual security but we do it for
8187
// obfuscation purposes (and as a side effect it handles escaping).
82-
res.cookie(Cookie.Key, hash(req.body.password), {
88+
res.cookie(Cookie.Key, hashedPassword, {
8389
domain: getCookieDomain(req.headers.host || "", req.args["proxy-domain"]),
8490
path: req.body.base || "/",
8591
sameSite: "lax",

0 commit comments

Comments
(0)

AltStyle によって変換されたページ (->オリジナル) /