Skip to content

🛡️ Sentinel: [HIGH] Fix length timing oracle in token matching #481

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Dexploarer wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

🛡️ Sentinel: [HIGH] Fix length timing oracle in token matching #481

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/api/auth-routes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,15 @@ export async function handleAuthRoutes(
const expected = normalizePairingCode(current);
const a = Buffer.from(expected, "utf8");
const b = Buffer.from(provided, "utf8");
if (a.length !== b.length || !crypto.timingSafeEqual(a, b)) {
let match = true;
if (a.length !== b.length) {
const padded = Buffer.alloc(a.length);
b.copy(padded, 0, 0, Math.min(b.length, a.length));
match = crypto.timingSafeEqual(a, padded) && false;
Comment on lines +83 to +85

@codereviewbot-ai codereviewbot-ai Bot Apr 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Security/Logic Issue:

The current implementation always sets match = crypto.timingSafeEqual(a, padded) && false; when buffer lengths differ, which means match is always false regardless of the timing-safe comparison. This defeats the purpose of using timing-safe comparison and may expose timing differences based on input length.

Recommended Solution:
Always pad both buffers to the same length and use crypto.timingSafeEqual directly, without forcibly setting the result to false. For example:

const maxLen = Math.max(a.length, b.length);
const aa = Buffer.alloc(maxLen);
const bb = Buffer.alloc(maxLen);
a.copy(aa);
b.copy(bb);
match = crypto.timingSafeEqual(aa, bb);

This ensures consistent timing and proper comparison.

} else {
match = crypto.timingSafeEqual(a, b);
}
Comment on lines +81 to +88

@gemini-code-assist gemini-code-assist Bot Apr 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

security-high high

The current implementation still contains a timing oracle because of the if (a.length !== b.length) branch. The if branch performs an allocation and a copy, while the else branch does not. This difference in execution time allows an attacker to deduce the expected token length by observing which inputs result in faster responses. To achieve constant-time comparison, the padding and comparison should be performed unconditionally.

    const padded = Buffer.alloc(a.length);
    b.copy(padded, 0, 0, Math.min(b.length, a.length));
    const match = crypto.timingSafeEqual(a, padded) && a.length === b.length;

if (!match) {
error(res, "Invalid pairing code", 403);
return true;
}
Expand Down
7 changes: 6 additions & 1 deletion src/api/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4463,7 +4463,12 @@ export function resolveHyperscapeAuthorizationHeader(
function tokenMatches(expected: string, provided: string): boolean {
const a = Buffer.from(expected, "utf8");
const b = Buffer.from(provided, "utf8");
if (a.length !== b.length) return false;
if (a.length !== b.length) {
// Pad to equal length to avoid length oracle
const padded = Buffer.alloc(a.length);
b.copy(padded, 0, 0, Math.min(b.length, a.length));
return crypto.timingSafeEqual(a, padded) && false;
}
return crypto.timingSafeEqual(a, b);
Comment on lines +4466 to 4472

@gemini-code-assist gemini-code-assist Bot Apr 25, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

security-high high

Similar to the change in auth-routes.ts, this implementation introduces a timing difference between the if and else branches. The extra work performed when lengths differ (allocation and copying) makes the execution time dependent on whether the provided length matches the expected length. This should be refactored to be branchless to truly mitigate the timing oracle.

  const padded = Buffer.alloc(a.length);
  b.copy(padded, 0, 0, Math.min(b.length, a.length));
  return crypto.timingSafeEqual(a, padded) && a.length === b.length;

}

Expand Down
Loading