Skip to content

fix(core): zeroize the derived X25519 secret (#65)#91

Merged
kevincodex1 merged 1 commit into
mainfrom
fix/zeroize-x25519-secret-reland
Jun 25, 2026
Merged

fix(core): zeroize the derived X25519 secret (#65)#91
kevincodex1 merged 1 commit into
mainfrom
fix/zeroize-x25519-secret-reland

Conversation

@beardthelion

@beardthelion beardthelion commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Re-lands the #65 fix that was reviewed and merged in #66 but never reached main.

#66 was stacked on #64 and targeted fix/zeroize-seed-bytes as its base. #64 merged to main about a minute before #66 merged, so #66's commit landed on the already-retired stacked base and never propagated to main. The accessor signature on main still returns a bare [u8; 32], so issue #65 is still open. This PR puts the same reviewed change on main directly.

What it does

x25519_secret_from_seed derived the X25519 private scalar and returned it as a bare [u8; 32]. Because that type is Copy with no Drop (and sha2's digest output has no zeroize), the scalar, the SHA-512 digest, and the returned temporary were released without being scrubbed, leaving secret-derived material in freed memory. Same Copy-no-Drop class fixed for the raw seed in #41, one hop downstream on the derived secret.

The fix returns Zeroizing<[u8; 32]>, builds the scalar directly into a zeroizing buffer so no bare secret local persists, and explicitly wipes the SHA-512 digest before it drops. The two callers (open_blob and the crypto test) deref into crypto_box::SecretKey, which already scrubs its own copy.

Scope

Memory hygiene only. No change to the encryption scheme, key derivation, or wire format. The derived scalar and all decryption output are byte-identical, covered by the existing seal_open_round_trip_for_recipients and ed25519_to_x25519_keypair_agrees tests. Not remotely exploitable; defense-in-depth against core dumps, swap, and memory-disclosure bugs.

Verification

  • cargo build -p gitlawb-core, clippy clean
  • All encrypt unit tests pass, including the two round-trip tests that would fail on any scalar divergence
  • MSRV 1.91 confirmed for Zeroizing and GenericArray::as_mut_slice().zeroize()

Closes #65.

Summary by CodeRabbit

  • Refactor
    • Refined internal cryptographic key handling in the encryption module.

x25519_secret_from_seed derived the X25519 private scalar from the Ed25519
seed and returned it as a bare [u8; 32]. Because [u8; 32] is Copy with no
Drop (and sha2's digest output likewise has no zeroize), the scalar, the
SHA-512 digest, and the returned temporary were released without being
scrubbed, leaving secret-derived material in freed memory. This is the same
Copy-no-Drop class fixed for the raw seed in #41, on the derived secret.

Return Zeroizing<[u8; 32]>, build the scalar directly into a zeroizing
buffer so no bare secret local persists, and explicitly wipe the SHA-512
digest before it drops. Callers deref the result into crypto_box::SecretKey,
which already scrubs its own copy. No behavior change: the derived scalar and
all decryption output are byte-identical, covered by the existing
ed25519_to_x25519_keypair_agrees and seal_open_round_trip_for_recipients tests.
@coderabbitai

coderabbitai Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0db5257a-a81b-4e58-87b3-2fb845e568bd

📥 Commits

Reviewing files that changed from the base of the PR and between 2153b0b and 6b17b9f.

📒 Files selected for processing (1)
  • crates/gitlawb-core/src/encrypt.rs

📝 Walkthrough

Walkthrough

x25519_secret_from_seed in crates/gitlawb-core/src/encrypt.rs is changed to return Zeroizing<[u8; 32]> instead of a plain [u8; 32], and the SHA-512 digest buffer is explicitly zeroized after the scalar is extracted. Two call sites (open_blob and a unit test) are updated to dereference the Zeroizing wrapper.

Changes

Zeroize x25519_secret_from_seed intermediate scalar

Layer / File(s) Summary
x25519_secret_from_seed return type, digest zeroization, and call sites
crates/gitlawb-core/src/encrypt.rs
Adds Zeroizing import; changes x25519_secret_from_seed to return Zeroizing<[u8; 32]> with explicit SHA-512 digest scrubbing after scalar extraction; updates open_blob (line 131) and the unit test (line 196) to dereference the Zeroizing wrapper before passing the scalar to crypto_box::SecretKey::from.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • Gitlawb/node#64: Modifies the same crates/gitlawb-core/src/encrypt.rs to route Ed25519 seed access through to_seed()/Zeroizing, touching the same open_blob call site and related unit test that this PR also updates.

Suggested labels

kind:security, crate:core, sev:high

Suggested reviewers

  • kevincodex1

Poem

🐰 A secret once bare in the heap's shallow grass,
Now wrapped in Zeroizing, no ghost shall it cast.
The SHA digest scrubbed, not a byte left to roam,
The scalar secured 'til it crumbles to foam.
Hop hop, memory safe — no leak shall outlast! 🔐

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix(core): zeroize the derived X25519 secret (#65)' accurately describes the main change: hardening memory safety by zeroizing secret material returned from x25519_secret_from_seed.
Description check ✅ Passed The description comprehensively explains the re-landing context, root cause, fix implementation, scope limitations, and verification steps, exceeding template expectations for this critical security hygiene change.
Linked Issues check ✅ Passed The PR fully addresses issue #65 by implementing all three proposed requirements: returning Zeroizing<[u8; 32]>, explicitly zeroizing the SHA-512 digest, and updating both callers (open_blob and crypto test).
Out of Scope Changes check ✅ Passed All changes are scoped to memory hygiene improvements in x25519_secret_from_seed. No modifications to encryption scheme, key derivation, wire format, or unrelated functionality are present.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/zeroize-x25519-secret-reland

Comment @coderabbitai help to get the list of available commands.

@jatmn jatmn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the contribution. I do not see any actionable issues from my review.

@kevincodex1 LGTM

@kevincodex1 kevincodex1 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.

lgtm

@kevincodex1 kevincodex1 merged commit 2f6611a into main Jun 25, 2026
14 checks passed
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.

Harden x25519_secret_from_seed: zeroize the derived X25519 secret (follow-up to #41)

3 participants