Skip to content

signedDownloadUrl uses a bare SHA-256 over a secret, not an HMAC #90

Description

@ranshufang55

packages/console/src/features/sandboxes/envd-client.ts:227-248:

export async function signedDownloadUrl(
  auth: EnvdAuth,
  path: string,
): Promise<string> {
  const exp = Math.floor(Date.now() / 1000) + SIGNED_URL_TTL_SECONDS;
  const material = `${path}:read::${auth.envdAccessToken}:${exp}`;
  const digest = await crypto.subtle.digest(
    'SHA-256',
    new TextEncoder().encode(material),
  );

sha256(secret || data) is the textbook construction HMAC exists to replace. Merkle–Damgård hashes are length-extendable: given H(m) and len(m), an attacker can compute H(m || pad || suffix) without knowing m.

Is it exploitable today? Probably not, and I want to be precise rather than alarmist about why: the secret is at position four of five, the trailing :${exp} is attacker-visible, and the verifier (server/e2b/signing.ts, per the comment at line 213) reconstructs the string from parsed query params rather than accepting an arbitrary tail — so there is no obvious place to graft an extension. The concern is that the reason it is safe is an accident of field ordering in a format that is duplicated across two codebases and pinned by an e2e test. Reorder the fields, add a sixth optional component, or let any component be attacker-controlled and open-ended, and the construction becomes forgeable — with no test that would notice, because a forged signature is by definition one the verifier accepts.

Two more properties of the current scheme worth weighing:

  • No revocation. Once minted, the URL is valid for its full 15 minutes (SIGNED_URL_TTL_SECONDS, line 210) against the daemon's public origin (line 248: ${window.location.origin}/files?...), for anyone who has the string, with the wildcard-CORS door in server/e2b/cors.ts:17 open to any origin. Destroying the sandbox or rotating the API token does not invalidate it — only the clock does. That is a deliberate design (the comment at 206-209 argues the TTL trade-off well), but it should be stated as "unrevocable capability", not just "过期是一次点击的事".
  • btoa(String.fromCharCode(...new Uint8Array(digest))) (line 237) spreads a 32-byte array, which is fine at this size but is the pattern that blows the stack when someone reuses the helper on a larger buffer.

Recommendation: switch both sides to crypto.subtle.importKey('raw', token, {name:'HMAC', hash:'SHA-256'}) + crypto.subtle.sign. It is the same number of lines, removes the ordering dependency entirely, and the server side (node:crypto's createHmac) is already using HMAC for the session cookie in auth.ts:127 — so the codebase is inconsistent with itself here.

Activity

  1. ursasi commented on Oct 7, 2026

    @ursasi

    I came here expecting to do the HMAC switch, and I don't think it is available
    to us on this wire: the digest isn't our format, it's the one the official SDK
    mints. packages/js-sdk/src/sandbox/signature.ts builds
    path:operation:user:envdAccessToken[:exp] and sends it through
    crypto.subtle.digest('SHA-256', …), and real envd verifies that same digest —
    so an HMAC on the verification side would reject every SDK-minted URL, which is
    the compatibility this project is built around. That pinning is already stated
    in signing.ts's header and envd/files.ts, and compat.test.ts has a test
    that accepts an SDK-minted signature.

    On the two other points:

    • The length-extension worry has no graft point here, and not just because of
      where the token sits: the verifier rebuilds the material from parsed query
      params in a fixed order, and the only component that can follow the token
      must parse as a number, so there is nowhere for an attacker-chosen suffix to
      survive. Agreed that the reasoning should be written down rather than
      re-derived by every reader.
    • "No test would notice" a reordering is the one claim I'd push back on:
      app.test.ts rewrites the SDK's formula, and compat.test.ts,
      sandbox-proxy.test.ts and e2e/src/console.test.ts mint it, so a reorder
      breaks several. I added the case that was actually missing — a reordered
      material and one with an extra component, both refused.
    • The btoa(String.fromCharCode(...bytes)) note was right and is worth its own
      line: at 400 KB the spread throws RangeError.

    #105 does that, plus states the unrevocable-capability property in the TTL
    comment. If a signature scheme of our own is wanted on top of the
    SDK-compatible one, I think it has to be a second URL form rather than a change
    to this one — happy to work on that separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions