From 5a727d530beb704c338a82f7c65a05fdcac859a8 Mon Sep 17 00:00:00 2001 From: Philip Molares Date: Thu, 28 Jan 2021 12:18:20 +0100 Subject: [PATCH 1/3] auth: Fix secret length The former length of 64 bytes (512-bit) is transformed into base64url (a 6-bit code) ~86 characters long. This is too long for bcrypt as it ignores any characters beyond the 72th. This fix therefore reduces the amount of generated bytes to 54 (as 72*6/8 = 54) characters. This ensures that removing one character from the token the hash won't be the same anymore. Signed-off-by: Philip Molares --- src/auth/auth.service.spec.ts | 16 ++++++++++++++++ src/auth/auth.service.ts | 10 +++++++++- 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/auth/auth.service.spec.ts b/src/auth/auth.service.spec.ts index 271973dd5..b5a7e2c1d 100644 --- a/src/auth/auth.service.spec.ts +++ b/src/auth/auth.service.spec.ts @@ -13,6 +13,7 @@ import { UsersModule } from '../users/users.module'; import { Identity } from '../users/identity.entity'; import { LoggerModule } from '../logger/logger.module'; import { AuthToken } from './auth-token.entity'; +import { TokenNotValidError } from '../errors/errors'; describe('AuthService', () => { let service: AuthService; @@ -105,6 +106,16 @@ describe('AuthService', () => { .checkPassword(testPassword, hash) .then((result) => expect(result).toBeTruthy()); }); + it('fails, if secret is too short', async () => { + const secret = service.BufferToBase64Url(await service.randomString(54)); + const hash = await service.hashPassword(secret); + service + .checkPassword(secret, hash) + .then((result) => expect(result).toBeTruthy()); + service + .checkPassword(secret.substr(0, secret.length - 1), hash) + .then((result) => expect(result).toBeFalsy()); + }); }); describe('getTokensByUsername', () => { @@ -148,6 +159,11 @@ describe('AuthService', () => { authTokens: [authToken], }); }); + it('fails on too long token', () => { + expect( + service.validateToken(`${authToken.keyId}.${'a'.repeat(73)}`), + ).rejects.toBeInstanceOf(TokenNotValidError); + }); }); describe('removeToken', () => { diff --git a/src/auth/auth.service.ts b/src/auth/auth.service.ts index e6b7c3a94..386ee60eb 100644 --- a/src/auth/auth.service.ts +++ b/src/auth/auth.service.ts @@ -37,6 +37,14 @@ export class AuthService { async validateToken(token: string): Promise { try { const [keyId, secret] = token.split('.'); + 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`, + ); + } const accessToken = await this.getAuthTokenAndValidate(keyId, secret); await this.setLastUsedToken(keyId); return this.usersService.getUserByUsername(accessToken.user.userName); @@ -92,7 +100,7 @@ export class AuthService { `User '${user.userName}' has already 200 tokens and can't have anymore`, ); } - const secret = this.BufferToBase64Url(await this.randomString(64)); + const secret = this.BufferToBase64Url(await this.randomString(54)); const keyId = this.BufferToBase64Url(await this.randomString(8)); const accessToken = await this.hashPassword(secret); let token; From ba517b3cfe0740e43ea5b0763c2b94c2d0edf035 Mon Sep 17 00:00:00 2001 From: Philip Molares Date: Fri, 29 Jan 2021 22:00:47 +0100 Subject: [PATCH 2/3] auth: Fix UnauthorizedException throwing Move conversion of Errors from AuthService to TokenStrategy. This is necessary to correctly test the validateToken method. Signed-off-by: Philip Molares --- src/auth/auth.service.ts | 34 ++++++++++++---------------------- src/auth/token.strategy.ts | 15 +++++++++++++-- 2 files changed, 25 insertions(+), 24 deletions(-) diff --git a/src/auth/auth.service.ts b/src/auth/auth.service.ts index 386ee60eb..3d0d76c5a 100644 --- a/src/auth/auth.service.ts +++ b/src/auth/auth.service.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: AGPL-3.0-only */ -import { Injectable, UnauthorizedException } from '@nestjs/common'; +import { Injectable } from '@nestjs/common'; import { UsersService } from '../users/users.service'; import { User } from '../users/user.entity'; import { AuthToken } from './auth-token.entity'; @@ -35,28 +35,18 @@ export class AuthService { } async validateToken(token: string): Promise { - try { - const [keyId, secret] = token.split('.'); - 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`, - ); - } - const accessToken = await this.getAuthTokenAndValidate(keyId, secret); - await this.setLastUsedToken(keyId); - return this.usersService.getUserByUsername(accessToken.user.userName); - } catch (error) { - if ( - error instanceof NotInDBError || - error instanceof TokenNotValidError - ) { - throw new UnauthorizedException(error.message); - } - throw error; + const [keyId, secret] = token.split('.'); + 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`, + ); } + const accessToken = await this.getAuthTokenAndValidate(keyId, secret); + await this.setLastUsedToken(keyId); + return this.usersService.getUserByUsername(accessToken.user.userName); } async hashPassword(cleartext: string): Promise { diff --git a/src/auth/token.strategy.ts b/src/auth/token.strategy.ts index 4f4f4e002..fe00c8298 100644 --- a/src/auth/token.strategy.ts +++ b/src/auth/token.strategy.ts @@ -6,9 +6,10 @@ import { Strategy } from 'passport-http-bearer'; import { PassportStrategy } from '@nestjs/passport'; -import { Injectable } from '@nestjs/common'; +import { Injectable, UnauthorizedException } from '@nestjs/common'; import { AuthService } from './auth.service'; import { User } from '../users/user.entity'; +import { NotInDBError, TokenNotValidError } from '../errors/errors'; @Injectable() export class TokenStrategy extends PassportStrategy(Strategy, 'token') { @@ -17,6 +18,16 @@ export class TokenStrategy extends PassportStrategy(Strategy, 'token') { } async validate(token: string): Promise { - return this.authService.validateToken(token); + try { + return await this.authService.validateToken(token); + } catch (error) { + if ( + error instanceof NotInDBError || + error instanceof TokenNotValidError + ) { + throw new UnauthorizedException(error.message); + } + throw error; + } } } From 08b3dd5db982efe9fec8ff38384dc0a0444fc4e7 Mon Sep 17 00:00:00 2001 From: Philip Molares Date: Fri, 29 Jan 2021 22:24:19 +0100 Subject: [PATCH 3/3] auth: Fix undefined secret error Signed-off-by: Philip Molares --- src/auth/auth.service.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/auth/auth.service.ts b/src/auth/auth.service.ts index 3d0d76c5a..9662f4c1e 100644 --- a/src/auth/auth.service.ts +++ b/src/auth/auth.service.ts @@ -36,6 +36,9 @@ export class AuthService { async validateToken(token: string): Promise { const [keyId, secret] = token.split('.'); + 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