Skip to content

feat: implement constant time equality checks#12

Closed
frectonz wants to merge 4 commits into
better-auth:mainfrom
frectonz:feat/safe-equal
Closed

feat: implement constant time equality checks#12
frectonz wants to merge 4 commits into
better-auth:mainfrom
frectonz:feat/safe-equal

Conversation

@frectonz
Copy link
Copy Markdown

This PR implements support for constant time equality checks that don't leak information, via early exits. It defaults to crypto.timingSafeEqual if it is available, if not it will default to a JS implementation of constant time equality check.

@jazzberry-ai
Copy link
Copy Markdown

jazzberry-ai Bot commented Aug 18, 2025

Bug Report

Name Severity Example test case Description
Timing vulnerability due to length check Low Compare "test" and "testing" multiple times. The safeEqual function checks the lengths of the input strings before calling crypto.timingSafeEqual. This length check is not constant time and could be exploited in a timing attack, especially with very precise timing measurements.

Comments? Email us.

Comment thread src/equal.ts Outdated
return result === 0;
}

export async function safeEqual(a: string, b: string): Promise<boolean> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal of this library is to be runtime-agnostic and just web APIs. Importing crypto here would break that, and using Buffer.from would break it as well

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i can remove the code that tries to use crypto.timingSafeEqual and we can just use the JS only implementation.

@jazzberry-ai
Copy link
Copy Markdown

jazzberry-ai Bot commented Aug 18, 2025

Bug Report

Name Severity Example test case Description
Potential Timing Attack Vulnerability Medium safeEqual("aaaaaaaa", "aaaaaaab") The safeEqual function might be vulnerable to timing attacks due to JavaScript engine optimizations. Although the code is intended to be constant-time, optimizations such as short-circuiting or speculative execution could introduce timing variations that reveal information about the input strings.

Comments? Email us.

@jazzberry-ai
Copy link
Copy Markdown

jazzberry-ai Bot commented Aug 18, 2025

Bug Report

Name Severity Example test case Description
Integer Overflow in safeEqual High const longString1 = 'A'.repeat(2147483647); expect(safeEqual(longString1, longString1)).toBe(true); The result variable in safeEqual is vulnerable to integer overflow due to the XOR and OR operations. When comparing very long strings, the initial XOR of lengths and subsequent XORs of byte values can lead to a value that exceeds the maximum safe integer, causing incorrect comparison results. JavaScript uses 32-bit integers for bitwise operations which exacerbates this issue.

Comments? Email us.

@jazzberry-ai
Copy link
Copy Markdown

jazzberry-ai Bot commented Aug 18, 2025

Bug Report

Name Severity Example test case Description
Timing vulnerability in safeEqual Medium Measure the execution time of safeEqual with slightly different inputs. The bitwise OR operation in the loop might be subject to timing variations, potentially leaking information about the input strings.

Comments? Email us.

Comment thread src/equal.ts
const bBytes = new TextEncoder().encode(b);

let len = Math.max(aBytes.length, bBytes.length);
let result = BigInt(aBytes.length ^ bBytes.length);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need bigint here? I think the number is enough?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i did that to fix this warning by the ai #12 (comment)

Copy link
Copy Markdown
Contributor

@himself65 himself65 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I feel like the API is unnecessary. Like #12 (comment) said.

Since this is a library for the web... but you know, there's no need for security on the web

@frectonz
Copy link
Copy Markdown
Author

Oh, I didn't know we had constantTimeEqual in Better Auth. I guess this PR is unnecessary then.

@frectonz frectonz closed this Aug 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants