Jun 9, 2021 · May 19, 2021 · May 19, 2021 · May 19, 2021 · May 19, 2021 · May 19, 2021
diff --git a/src/node/routes/login.ts b/src/node/routes/login.ts
Original file line number	Diff line number	Diff line change
Expand Up		@@ -77,7 +77,12 @@ router.post("/", async (req, res) => {
		? isHashLegacyMatch(req.body.password, req.args["hashed-password"])
		: req.args.password && safeCompare(req.body.password, req.args.password)
		) {
	const hashedPassword = req.args["hashed-password"] ? hashLegacy(req.body.password) : hash(req.body.password)
	// NOTE@jsjoeio:
	// We store the hashed password as a cookie. In order to be backwards-comptabile for the folks
	// using sha256 (the original hashing algorithm), we need to check the hashed-password in the req.args
	// TODO all of this logic should be cleaned up honestly. The current implementation only checks for a hashed-password
	// but doesn't check which algorithm they are using.
	const hashedPassword = req.args["hashed-password"] ? hashLegacy(req.body.password) : await hash(req.body.password)
		// 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, hashedPassword, {
Copy link @oxy oxy Jun 8, 2021 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. While hashing is a major step forward, the problem with this approach is that it still allows attackers who have access to the hash to just submit it as-is and gain access to code-server - effectively not very different from storing it in plaintext. We should definitely look into replacing this with something more robust so that password hashing isn't just a placebo - now or in a future version is up to you, but IMO we should tackle this before asking users to upgrade to Argon2. jsjoeio reacted with thumbs up emoji Copy link Contributor Author @jsjoeio jsjoeio Jun 8, 2021 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. effectively not very different from storing it in plaintext. I'm really glad you mention that and agree. We should definitely look into replacing this with something more robust so that password hashing isn't just a placebo Also agree! now or in a future version is up to you, but IMO we should tackle this before asking users to upgrade to Argon2 I would argue that it falls out of the scope of this PR. If we close this PR without merging, we're in the same situation we're discussing right now (see code) because we're doing the same thing - hashing the password and storing as a cookie. The scope of this PR was to replace the hashing algorithm (not to fix the problem with attackers who have the hash). Therefore, I think it makes sense to separate the two concerns and tackle them in separate PRs. I will create a new issue to track that though! RA80533 reacted with thumbs up emoji Copy link Contributor Author @jsjoeio jsjoeio Jun 8, 2021 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. #3576 Copy link @RA80533 RA80533 Jun 8, 2021 Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. Totally get it. Sometimes PRs bring in a lot of heat just because they touch an area that would otherwise have a lot of attention drawn towards. This one specifically has allowed us to become aware of other things in the codebase we want to change. 🙏
Expand Down