From fd0ca5987bd5a9d65e7e48d84738d27d2774879a Mon Sep 17 00:00:00 2001 From: gavyngong Date: Fri, 1 May 2026 10:26:54 +0800 Subject: [PATCH] fix(nextcloud-talk): replace manual XOR with crypto.timingSafeEqual and fix length leak (#58097) Merged via squash. Prepared head SHA: 3cb82bce40928983cc924e9d519555e88977d562 Co-authored-by: gavyngong <267269824+gavyngong@users.noreply.github.com> Co-authored-by: hxy91819 <8814856+hxy91819@users.noreply.github.com> Reviewed-by: @hxy91819 --- CHANGELOG.md | 1 + extensions/nextcloud-talk/src/core.test.ts | 95 +++++++++++++++++++++- extensions/nextcloud-talk/src/signature.ts | 27 ++++-- 3 files changed, 113 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bd8376ab7cc..2530a4fbde3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ Docs: https://docs.openclaw.ai - Gateway/systemd: exit with sysexits 78 for supervised lock and `EADDRINUSE` conflicts so `RestartPreventExitStatus=78` stops `Restart=always` restart loops instead of repeatedly reloading plugins against an occupied port. Fixes #75115. Thanks @yhyatt. - Agents/runtime: skip blank visible user prompts at the embedded-runner boundary before provider submission while still allowing internal runtime-only turns and media-only prompts, so Telegram/group sessions no longer leak raw empty-input provider errors when replay history exists. Fixes #74137. Thanks @yelog, @Gracker, and @nhaener. - Agents/Codex: isolate local Codex app-server `CODEX_HOME` and `HOME` per agent and add a deliberate Codex migration path with selectable skill copies, so personal Codex CLI skills, plugins, config, and hooks no longer leak into OpenClaw agents unless the operator migrates them into the workspace. Thanks @pashpashpash. +- Security/Nextcloud Talk: make webhook signature validation use the padded timing-safe compare path even when the supplied signature length is wrong, keep normalized header lookup behavior, and extend regression coverage for tampered bodies, wrong secrets, array-backed headers, and truncated signatures. Carries forward earlier contributor work from #50516 by teddytennant. (#58097) Thanks @gavyngong. - Plugins/runtime-deps: replace stale symlinked mirror target roots before writing runtime-mirror temp files and skip rewriting already materialized hardlinks, so cross-version container upgrades no longer crash-loop on read-only image-layer paths while warm mirrors do less churn. Fixes #75108; refs #75069. Thanks @coletebou and @xiaohuaxi. - Auto-reply/group chats: fall back to automatic source delivery when a channel precomputes message-tool-only replies but the `message` tool is unavailable, so Discord/Slack-style group turns do not silently complete without a visible reply. Fixes #74868. Thanks @kagura-agent. - Browser/gateway: share one browser control runtime across the HTTP control server and `browser.request`, and refresh browser profile config from the source snapshot, so CLI status/start honors configured `browser.executablePath`, `headless`, and `noSandbox` instead of falling back to stale auto-detection. Fixes #75087; repairs #73617. Thanks @civiltox and @martingarramon. diff --git a/extensions/nextcloud-talk/src/core.test.ts b/extensions/nextcloud-talk/src/core.test.ts index 10601bcb80a..b6938e03d26 100644 --- a/extensions/nextcloud-talk/src/core.test.ts +++ b/extensions/nextcloud-talk/src/core.test.ts @@ -1,7 +1,7 @@ import { mkdtemp, rm } from "node:fs/promises"; import os from "node:os"; import path from "node:path"; -import { afterEach, describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { looksLikeNextcloudTalkTargetId, normalizeNextcloudTalkMessagingTarget, @@ -131,6 +131,99 @@ describe("nextcloud talk core", () => { ).toBeNull(); }); + it("rejects tampered bodies, wrong secrets, and tampered signatures", () => { + const body = JSON.stringify({ hello: "world" }); + const generated = generateNextcloudTalkSignature({ + body, + secret: "secret-123", + }); + + expect( + verifyNextcloudTalkSignature({ + signature: generated.signature, + random: generated.random, + body: JSON.stringify({ hello: "tampered" }), + secret: "secret-123", + }), + ).toBe(false); + expect( + verifyNextcloudTalkSignature({ + signature: generated.signature, + random: generated.random, + body, + secret: "wrong-secret", + }), + ).toBe(false); + expect( + verifyNextcloudTalkSignature({ + signature: "a".repeat(generated.signature.length), + random: generated.random, + body, + secret: "secret-123", + }), + ).toBe(false); + }); + + it("takes the first value from array-backed headers", () => { + expect( + extractNextcloudTalkHeaders({ + "x-nextcloud-talk-signature": ["sig1", "sig2"], + "x-nextcloud-talk-random": ["rand1", "rand2"], + "x-nextcloud-talk-backend": ["backend1", "backend2"], + }), + ).toEqual({ + signature: "sig1", + random: "rand1", + backend: "backend1", + }); + }); + + it("still runs timingSafeEqual when the supplied signature length mismatches", async () => { + const timingSafeEqualMock = vi.fn(); + + vi.resetModules(); + vi.doMock("node:crypto", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + timingSafeEqual: vi.fn((left: NodeJS.ArrayBufferView, right: NodeJS.ArrayBufferView) => { + timingSafeEqualMock(left, right); + return actual.timingSafeEqual(left, right); + }), + }; + }); + + const { generateNextcloudTalkSignature, verifyNextcloudTalkSignature } = + await import("./signature.js"); + const body = JSON.stringify({ hello: "world" }); + const generated = generateNextcloudTalkSignature({ + body, + secret: "secret-123", + }); + const shortSignature = generated.signature.slice(0, 12); + + expect( + verifyNextcloudTalkSignature({ + signature: shortSignature, + random: generated.random, + body, + secret: "secret-123", + }), + ).toBe(false); + + expect(timingSafeEqualMock).toHaveBeenCalledOnce(); + const [leftBuffer, rightBuffer] = timingSafeEqualMock.mock.calls[0] ?? []; + expect(Buffer.isBuffer(leftBuffer)).toBe(true); + expect(Buffer.isBuffer(rightBuffer)).toBe(true); + if (!Buffer.isBuffer(leftBuffer) || !Buffer.isBuffer(rightBuffer)) { + throw new TypeError("Expected timingSafeEqual to receive Buffer arguments"); + } + expect(leftBuffer).toHaveLength(rightBuffer.length); + + vi.doUnmock("node:crypto"); + vi.resetModules(); + }); + it("persists replay decisions across guard instances and scopes account namespaces", async () => { const stateDir = await makeTempDir(); diff --git a/extensions/nextcloud-talk/src/signature.ts b/extensions/nextcloud-talk/src/signature.ts index 4f84417a82c..3df6e1309c5 100644 --- a/extensions/nextcloud-talk/src/signature.ts +++ b/extensions/nextcloud-talk/src/signature.ts @@ -1,4 +1,4 @@ -import { createHmac, randomBytes } from "node:crypto"; +import { createHmac, randomBytes, timingSafeEqual } from "node:crypto"; import { normalizeLowercaseStringOrEmpty } from "openclaw/plugin-sdk/text-runtime"; import type { NextcloudTalkWebhookHeaders } from "./types.js"; @@ -25,14 +25,23 @@ export function verifyNextcloudTalkSignature(params: { .update(random + body) .digest("hex"); - if (signature.length !== expected.length) { - return false; - } - let result = 0; - for (let i = 0; i < signature.length; i++) { - result |= signature.charCodeAt(i) ^ expected.charCodeAt(i); - } - return result === 0; + const expectedBuf = Buffer.from(expected, "utf8"); + const signatureBuf = Buffer.from(signature, "utf8"); + + // Pad to equal length before constant-time comparison to prevent + // leaking length information via early-return timing. + // Note: digest("hex") always produces lowercase ASCII (64 bytes for SHA-256), + // so expectedBuf is always 64 bytes — no variable-length concern on the expected side. + const maxLen = Math.max(expectedBuf.length, signatureBuf.length); + const paddedExpected = Buffer.alloc(maxLen); + const paddedSignature = Buffer.alloc(maxLen); + expectedBuf.copy(paddedExpected); + signatureBuf.copy(paddedSignature); + + // Use crypto.timingSafeEqual instead of manual XOR loop to avoid + // potential JIT-optimisation timing leaks in the JavaScript engine. + const timingResult = timingSafeEqual(paddedExpected, paddedSignature); + return expectedBuf.length === signatureBuf.length && timingResult; } /**