Skip to content

Generate even and odd safe integers in Crypto random APIs - #6941

Open
fubhy wants to merge 1 commit into
mainfrom
audit/repro-core-crypto-next-int-domain
Open

Generate even and odd safe integers in Crypto random APIs#6941
fubhy wants to merge 1 commit into
mainfrom
audit/repro-core-crypto-next-int-domain

Conversation

@fubhy

@fubhy fubhy commented Aug 4, 2026

Copy link
Copy Markdown
Member

Summary

nextIntUnsafe and randomInt cannot produce any even safe integer, including zero, despite promising the full inclusive safe-integer range.

Important

This PR starts with focused failing reproduction tests. Add the implementation fix to this same branch; CI is expected to fail until that fix is included.

Safe-integer random generation reaches only odd values

Module: Crypto
Audit ID: core-a-f-crypto-safe-integer-domain
Severity / confidence: medium / high

What happens

nextIntUnsafe and randomInt cannot produce any even safe integer, including zero, despite promising the full inclusive safe-integer range.

Why it happens

The mathematical width 2^54 - 1 rounds to 2^54 as a number. Multiplying a 53-bit fraction k / 2^53 therefore produces 2k, and adding odd Number.MIN_SAFE_INTEGER makes every output odd.

Expected behavior

nextIntUnsafe and randomInt generate an integer over the inclusive range from Number.MIN_SAFE_INTEGER through Number.MAX_SAFE_INTEGER.

Relevant implementation

These links and excerpts are pinned to audit base c9b56ab507f224426ee8388dc450da447ec4715f.

View problematic code at packages/effect/src/Crypto.ts:225-233
  const nextDoubleUnsafe = (): number => {
    const bytes = randomBytesUnsafe(7)
    const value = ((bytes[0] & 0x1f) * 2 ** 48) + (bytes[1] * 2 ** 40) + (bytes[2] * 2 ** 32) +
      (bytes[3] * 2 ** 24) + (bytes[4] * 2 ** 16) + (bytes[5] * 2 ** 8) + bytes[6]
    return value / 2 ** 53
  }

  const nextIntUnsafe = (): number =>
    Math.floor(nextDoubleUnsafe() * (Number.MAX_SAFE_INTEGER - Number.MIN_SAFE_INTEGER + 1)) + Number.MIN_SAFE_INTEGER

View exact lines on GitHub

View problematic code at packages/effect/src/Crypto.ts:78-82
  /**
   * Generates a random integer in the range Number.MIN_SAFE_INTEGER to
   * Number.MAX_SAFE_INTEGER (both inclusive).
   */
  nextIntUnsafe(): number

View exact lines on GitHub

View problematic code at packages/effect/src/Crypto.ts:113-117
  /**
   * Generates a cryptographically secure random integer between
   * `Number.MIN_SAFE_INTEGER` and `Number.MAX_SAFE_INTEGER` (both inclusive).
   */
  readonly randomInt: Effect.Effect<number>

View exact lines on GitHub

Reproduction

pnpm test --run packages/effect/test/CryptoNextIntDomain.test.ts

Observed failure: Adjacent 53-bit entropy values skipped the adjacent even safe integer.

Implementation handoff

The initial reproduction tests on this branch are the regression specification for the implementation fix that should follow in this PR.

  1. Start with the pinned implementation excerpts and the Why it happens analysis above.
  2. Change the implementation so it satisfies the stated Expected behavior; do not weaken or remove the reproduction assertions.
  3. Run the focused reproduction command(s) and confirm the observed failures become passing tests:
pnpm test --run packages/effect/test/CryptoNextIntDomain.test.ts
  1. Run the affected package's existing tests, then the repository lint and type checks before requesting review.

Audit provenance

  • Audit base: c9b56ab507f224426ee8388dc450da447ec4715f
  • Reproduction base: c9b56ab507f224426ee8388dc450da447ec4715f
  • Findings: core-a-f-crypto-safe-integer-domain
  • Initial patch: focused reproduction tests; implementation fix pending

@fubhy fubhy added the audit Findings originating from the Effect runtime correctness audit label Aug 4, 2026
@github-project-automation github-project-automation Bot moved this to Discussion Ongoing in PR Backlog Aug 4, 2026
@changeset-bot

changeset-bot Bot commented Aug 4, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: a6e61fb

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@effect-slopcop effect-slopcop Bot added 4.0 bug Something isn't working labels Aug 4, 2026

@pullfrog pullfrog Bot left a comment

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.

Important

This PR adds a focused regression test for the safe-integer domain bug in Crypto, but the implementation fix promised by its title is not present. The reproduction itself is correct, yet a few concrete items need to be addressed before merge.

Reviewed changes

  • packages/effect/test/CryptoNextIntDomain.test.ts — adds deterministic, failing reproduction tests for Crypto.make(...).nextIntUnsafe showing that adjacent 53-bit fractions currently skip the adjacent even safe integer.

⚠️ Implementation fix is missing

The PR body already notes that the fix should follow, but the title "Generate even and odd safe integers in Crypto random APIs" reads like the fix is included. Before merge, packages/effect/src/Crypto.ts:232-233 needs an implementation that samples uniformly across the full [Number.MIN_SAFE_INTEGER, Number.MAX_SAFE_INTEGER] range rather than relying on Math.floor(nextDoubleUnsafe() * (2 ** 54 - 1)), which rounds the width to 2 ** 54 and produces only odd offsets.

I verified the reproduction locally: make(1n).nextIntUnsafe() returns -9007199254740989 (MIN_SAFE_INTEGER + 2) instead of the expected -9007199254740990 (MIN_SAFE_INTEGER + 1).

Existing Crypto tests still pass and pnpm check succeeds, so the change has no other regressions.


Note: 1 inline comment(s) dropped because they did not anchor to lines inside the PR diff:

  • packages/effect/test/CryptoNextIntDomain.test.ts:21 (RIGHT) — line 21 (RIGHT) is not inside a diff hunk

Pullfrog  | ⚠️ this action is pinned to a commit SHA, which freezes the cleanup step — switch to @v0 or keep the SHA fresh with Dependabot | Fix all ➔Fix 👍s ➔View workflow run | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏

Comment on lines +4 to +17
const make = (fraction: bigint) => Crypto.make({
randomBytes: () => Uint8Array.of(
Number((fraction >> 48n) & 0x1fn),
Number((fraction >> 40n) & 0xffn),
Number((fraction >> 32n) & 0xffn),
Number((fraction >> 24n) & 0xffn),
Number((fraction >> 16n) & 0xffn),
Number((fraction >> 8n) & 0xffn),
Number(fraction & 0xffn)
),
digest: (_algorithm, data) => Effect.succeed(data)
})

it("maps adjacent 53-bit fractions to adjacent safe integers", () => {

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.

This block needs dprint fmt; pnpm lint currently fails with Found 1 not formatted file on this file.

Suggested change
const make = (fraction: bigint) => Crypto.make({
randomBytes: () => Uint8Array.of(
Number((fraction >> 48n) & 0x1fn),
Number((fraction >> 40n) & 0xffn),
Number((fraction >> 32n) & 0xffn),
Number((fraction >> 24n) & 0xffn),
Number((fraction >> 16n) & 0xffn),
Number((fraction >> 8n) & 0xffn),
Number(fraction & 0xffn)
),
digest: (_algorithm, data) => Effect.succeed(data)
})
it("maps adjacent 53-bit fractions to adjacent safe integers", () => {
const make = (fraction: bigint) =>
Crypto.make({
randomBytes: () =>
Uint8Array.of(
Number((fraction >> 48n) & 0x1fn),
Number((fraction >> 40n) & 0xffn),
Number((fraction >> 32n) & 0xffn),
Number((fraction >> 24n) & 0xffn),
Number((fraction >> 16n) & 0xffn),
Number((fraction >> 8n) & 0xffn),
Number(fraction & 0xffn)
),
digest: (_algorithm, data) => Effect.succeed(data)
})

@github-project-automation github-project-automation Bot moved this from Discussion Ongoing to Waiting on Author in PR Backlog Aug 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4.0 audit Findings originating from the Effect runtime correctness audit bug Something isn't working

Projects

Status: Waiting on Author

Development

Successfully merging this pull request may close these issues.

1 participant