From b4a65b47f0c40f32732f7a2b7a3a28b5f3ce957a Mon Sep 17 00:00:00 2001 From: David Mehren Date: Thu, 9 Dec 2021 23:04:00 +0100 Subject: [PATCH] fix(auth): use sha-512 for auth tokens Bcrypt hashes are too slow to be validated on every request. As our tokens are random and have a fixed length, it is reasonable to use SHA-512 instead. SHA-512 is recommended as cryptographically strong by the BSI: https://www.bsi.bund.de/SharedDocs/Downloads/EN/BSI/Publications/TechGuidelines/TG02102/BSI-TR-02102-1.pdf?__blob=publicationFile Fixes https://github.com/hedgedoc/hedgedoc/issues/1881 Signed-off-by: David Mehren --- src/auth/auth.service.spec.ts | 16 ++++++++--- src/auth/auth.service.ts | 42 ++++++++++++++++------------- test/private-api/tokens.e2e-spec.ts | 2 +- 3 files changed, 37 insertions(+), 23 deletions(-) diff --git a/src/auth/auth.service.spec.ts b/src/auth/auth.service.spec.ts index 21a39d752..f9d5249bc 100644 --- a/src/auth/auth.service.spec.ts +++ b/src/auth/auth.service.spec.ts @@ -7,6 +7,7 @@ import { ConfigModule } from '@nestjs/config'; import { PassportModule } from '@nestjs/passport'; import { Test, TestingModule } from '@nestjs/testing'; import { getRepositoryToken } from '@nestjs/typeorm'; +import crypto from 'crypto'; import { Repository } from 'typeorm'; import appConfigMock from '../config/mock/app.config.mock'; @@ -86,7 +87,10 @@ describe('AuthService', () => { describe('getAuthToken', () => { const token = 'testToken'; it('works', async () => { - const accessTokenHash = await hashPassword(token); + const accessTokenHash = crypto + .createHash('sha512') + .update(token) + .digest('hex'); jest.spyOn(authTokenRepo, 'findOne').mockResolvedValueOnce({ ...authToken, user: user, @@ -162,8 +166,12 @@ describe('AuthService', () => { describe('validateToken', () => { it('works', async () => { - const token = 'testToken'; - const accessTokenHash = await hashPassword(token); + const testSecret = + 'gNrv_NJ4FHZ0UFZJQu_q_3i3-GP_d6tELVtkYiMFLkLWNl_dxEmPVAsCNKxP3N3DB9aGBVFYE1iptvw7hFMJvA'; + const accessTokenHash = crypto + .createHash('sha512') + .update(testSecret) + .digest('hex'); jest.spyOn(userRepo, 'findOne').mockResolvedValueOnce({ ...user, authTokens: [authToken], @@ -179,7 +187,7 @@ describe('AuthService', () => { return authToken; }); const userByToken = await service.validateToken( - `${authToken.keyId}.${token}`, + `${authToken.keyId}.${testSecret}`, ); expect(userByToken).toEqual({ ...user, diff --git a/src/auth/auth.service.ts b/src/auth/auth.service.ts index 789e9b7fb..e69f98e18 100644 --- a/src/auth/auth.service.ts +++ b/src/auth/auth.service.ts @@ -6,7 +6,7 @@ import { Injectable } from '@nestjs/common'; import { Cron, Timeout } from '@nestjs/schedule'; import { InjectRepository } from '@nestjs/typeorm'; -import { randomBytes } from 'crypto'; +import crypto, { randomBytes } from 'crypto'; import { Repository } from 'typeorm'; import { @@ -17,11 +17,7 @@ import { import { ConsoleLoggerService } from '../logger/console-logger.service'; import { User } from '../users/user.entity'; import { UsersService } from '../users/users.service'; -import { - bufferToBase64Url, - checkPassword, - hashPassword, -} from '../utils/password'; +import { bufferToBase64Url } from '../utils/password'; import { TimestampMillis } from '../utils/timestamp'; import { AuthTokenWithSecretDto } from './auth-token-with-secret.dto'; import { AuthTokenDto } from './auth-token.dto'; @@ -43,13 +39,10 @@ export class AuthService { if (!secret) { throw new TokenNotValidError('Invalid AuthToken format'); } - if (secret.length > 72) { - // Only the first 72 characters of the tokens are considered by bcrypt - // This should prevent strange corner cases - // At the very least it won't hurt us - throw new TokenNotValidError( - `AuthToken '${secret}' is too long the be a proper token`, - ); + if (secret.length != 86) { + // We always expect 86 characters, as the secret is generated with 64 bytes + // and then converted to a base64url string + throw new TokenNotValidError(`AuthToken '${token}' has incorrect length`); } const accessToken = await this.getAuthTokenAndValidate(keyId, secret); await this.setLastUsedToken(keyId); @@ -70,9 +63,12 @@ export class AuthService { `User '${user.username}' has already 200 tokens and can't have anymore`, ); } - const secret = bufferToBase64Url(randomBytes(54)); + const secret = bufferToBase64Url(randomBytes(64)); const keyId = bufferToBase64Url(randomBytes(8)); - const accessToken = await hashPassword(secret); + const accessTokenHash = crypto + .createHash('sha512') + .update(secret) + .digest('hex'); let token; // Tokens can only be valid for a maximum of 2 years const maximumTokenValidity = @@ -82,7 +78,7 @@ export class AuthService { keyId, user, identifier, - accessToken, + accessTokenHash, new Date(maximumTokenValidity), ); } else { @@ -90,7 +86,7 @@ export class AuthService { keyId, user, identifier, - accessToken, + accessTokenHash, new Date(validUntil), ); } @@ -122,7 +118,17 @@ export class AuthService { if (accessToken === undefined) { throw new NotInDBError(`AuthToken '${token}' not found`); } - if (!(await checkPassword(token, accessToken.accessTokenHash))) { + // Hash the user-provided token + const userHash = Buffer.from( + crypto.createHash('sha512').update(token).digest('hex'), + ); + const dbHash = Buffer.from(accessToken.accessTokenHash); + if ( + // Normally, both hashes have the same length, as they are both SHA512 + // This is only defense-in-depth, as timingSafeEqual throws if the buffers are not of the same length + userHash.length !== dbHash.length || + !crypto.timingSafeEqual(userHash, dbHash) + ) { // hashes are not the same throw new TokenNotValidError(`AuthToken '${token}' is not valid.`); } diff --git a/test/private-api/tokens.e2e-spec.ts b/test/private-api/tokens.e2e-spec.ts index b49e769cf..d3b140932 100644 --- a/test/private-api/tokens.e2e-spec.ts +++ b/test/private-api/tokens.e2e-spec.ts @@ -48,7 +48,7 @@ describe('Tokens', () => { expect(response.body.label).toBe(tokenName); expect(response.body.validUntil).toBe(null); expect(response.body.lastUsed).toBe(null); - expect(response.body.secret.length).toBe(84); + expect(response.body.secret.length).toBe(98); }); it(`GET /tokens`, async () => {