From b6c59fa98e8296cd1aeb700ecd5fe9a659290463 Mon Sep 17 00:00:00 2001 From: dakkar Date: Fri, 9 Feb 2024 18:22:32 +0000 Subject: [PATCH] add a probably-not-terrible string-equality function #407 --- packages/backend/src/core/UserAuthService.ts | 4 +-- .../backend/src/misc/secure-ish-compare.ts | 33 +++++++++++++++++++ .../src/server/ActivityPubServerService.ts | 7 ++-- .../test/unit/misc/secure-ish-compare.ts | 25 ++++++++++++++ 4 files changed, 64 insertions(+), 5 deletions(-) create mode 100644 packages/backend/src/misc/secure-ish-compare.ts create mode 100644 packages/backend/test/unit/misc/secure-ish-compare.ts diff --git a/packages/backend/src/core/UserAuthService.ts b/packages/backend/src/core/UserAuthService.ts index 9d6c8ce63..eb9dcb8c0 100644 --- a/packages/backend/src/core/UserAuthService.ts +++ b/packages/backend/src/core/UserAuthService.ts @@ -11,7 +11,7 @@ import type { MiUserProfile, UserProfilesRepository, UsersRepository } from '@/m import { bindThis } from '@/decorators.js'; import { isDuplicateKeyValueError } from '@/misc/is-duplicate-key-value-error.js'; import type { MiLocalUser } from '@/models/User.js'; -import * as crypto from 'node:crypto'; +import { secureishCompare } from '@/misc/secure-ish-compare.js'; @Injectable() export class UserAuthService { @@ -29,7 +29,7 @@ export class UserAuthService { if (profile.twoFactorBackupSecret?.includes(token)) { await this.userProfilesRepository.update({ userId: profile.userId }, { twoFactorBackupSecret: profile.twoFactorBackupSecret.filter( - (secret) => !crypto.timingSafeEqual(secret, token) + (secret) => !secureishCompare(secret, token) ), }); } else { diff --git a/packages/backend/src/misc/secure-ish-compare.ts b/packages/backend/src/misc/secure-ish-compare.ts new file mode 100644 index 000000000..1a359e799 --- /dev/null +++ b/packages/backend/src/misc/secure-ish-compare.ts @@ -0,0 +1,33 @@ +/* + * SPDX-FileCopyrightText: dakkar and other sharkey contributors + * SPDX-License-Identifier: AGPL-3.0-only + */ + +import * as crypto from 'node:crypto'; + +export function secureishCompare(expected: string, given:string) { + const expectedLen = expected.length; + const givenLen = given.length; + + /* + this ensures that the two strings are the same length, so + `timingSafeEqual` will not throw an error + + notice that we perform the same operations regardless of the + length of the strings, we're trying not to leak timing + information! + */ + const paddedExpected = Buffer.from(expected + given); + const paddedGiven = Buffer.from(given + expected); + + /* + if the two strings were equal, `sortOfEqual` will be true + + if will be true in other cases, too! like `abc` and `abcabc`; but + then, `expectedLen` and `givenLen` would be different, and we'd + return false + */ + const sortOfEqual = crypto.timingSafeEqual(paddedExpected, paddedGiven); + + return sortOfEqual && (givenLen === expectedLen); +} diff --git a/packages/backend/src/server/ActivityPubServerService.ts b/packages/backend/src/server/ActivityPubServerService.ts index e67848482..9a38d5dd3 100644 --- a/packages/backend/src/server/ActivityPubServerService.ts +++ b/packages/backend/src/server/ActivityPubServerService.ts @@ -38,6 +38,7 @@ import type { FastifyInstance, FastifyRequest, FastifyReply, FastifyPluginOption import type { FindOptionsWhere } from 'typeorm'; import type Logger from '@/logger.js'; import { LoggerService } from '@/core/LoggerService.js'; +import { secureishCompare } from '@/misc/secure-ish-compare.js'; const ACTIVITY_JSON = 'application/activity+json; charset=utf-8'; const LD_JSON = 'application/ld+json; profile="https://www.w3.org/ns/activitystreams"; charset=utf-8'; @@ -287,7 +288,7 @@ export class ActivityPubServerService { const hash = crypto.createHash('sha256').update(request.rawBody).digest('base64'); - if (! crypto.timingSafeEqual(hash, digestValue)) { + if (!secureishCompare(hash, digestValue)) { // Invalid digest reply.code(401); return; @@ -795,7 +796,7 @@ export class ActivityPubServerService { fastify.get<{ Params: { user: string; } }>('/users/:user', { constraints: { apOrHtml: 'ap' } }, async (request, reply) => { if (await this.shouldRefuseGetRequest(request, reply, request.params.user)) return; - + vary(reply.raw, 'Accept'); const userId = request.params.user; @@ -811,7 +812,7 @@ export class ActivityPubServerService { fastify.get<{ Params: { user: string; } }>('/@:user', { constraints: { apOrHtml: 'ap' } }, async (request, reply) => { if (await this.shouldRefuseGetRequest(request, reply, request.params.user)) return; - + vary(reply.raw, 'Accept'); const user = await this.usersRepository.findOneBy({ diff --git a/packages/backend/test/unit/misc/secure-ish-compare.ts b/packages/backend/test/unit/misc/secure-ish-compare.ts new file mode 100644 index 000000000..659ffba85 --- /dev/null +++ b/packages/backend/test/unit/misc/secure-ish-compare.ts @@ -0,0 +1,25 @@ +/* + * SPDX-FileCopyrightText: dakkar and other sharkey contributors + * SPDX-License-Identifier: AGPL-3.0-only + */ + +import { secureishCompare } from '@/misc/secure-ish-compare.js'; + +describe(secureishCompare, () => { + it('should return true if strings are equal', () => { + expect(secureishCompare('abc','abc')).toBe(true); + expect(secureishCompare('aaa','aaa')).toBe(true); + }); + it('should return false if strings are different', () => { + expect(secureishCompare('abc','def')).toBe(false); + }); + it('should return false if one is prefix of the other', () => { + expect(secureishCompare('abc','abcabc')).toBe(false); + expect(secureishCompare('abcabc','abc')).toBe(false); + expect(secureishCompare('aaa','aa')).toBe(false); + }); + it('should return false if strings are very different', () => { + expect(secureishCompare('abc','defghi')).toBe(false); + expect(secureishCompare('defghi','abc')).toBe(false); + }); +});