Skip to content

fix(primitives): gate PrivateKey raw access and zeroize via derive#2994

Open
chefsale wants to merge 2 commits into
masterfrom
claude/privatekey-zeroization-fix-4iz2d9
Open

fix(primitives): gate PrivateKey raw access and zeroize via derive#2994
chefsale wants to merge 2 commits into
masterfrom
claude/privatekey-zeroization-fix-4iz2d9

Conversation

@chefsale

Copy link
Copy Markdown
Member

Two security issues in PrivateKey (crates/primitives/src/identity.rs):

  1. Raw bytes were exposed via Deref<Target=[u8;32]>, AsRef<[u8;32]>
    and a derived BorshSerialize/BorshDeserialize. Any of these made it
    trivial to copy or serialize the secret into a buffer that is never
    zeroized, silently defeating the zeroize-on-drop guarantee.

    • Drop the blanket Deref and AsRef impls and the Borsh derives.
    • Gate raw access behind a single audited as_bytes(&self) -> &[u8; 32]
      accessor, documented as the one reviewed entry point to the secret.
    • Update all call sites: in-place crypto uses (SigningKey::from_bytes,
      SharedKey) and the storage layers that persist an explicit [u8; 32]
      copy now go through as_bytes().
  2. The Drop impl zeroized via an unsafe cast of *mut Hash to *mut u8
    over size_of::<Hash>(), which would silently miss key material if Hash
    ever gained padding or fields.

    • Change the inner type to a plain [u8; 32] and #[derive(ZeroizeOnDrop)],
      removing the hand-rolled unsafe Drop and tracking field layout
      automatically. Enables zeroize's derive feature on calimero-primitives.

Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_014gfW6RgRfT6z5VDPg4dXQW

Two security issues in `PrivateKey` (crates/primitives/src/identity.rs):

1. Raw bytes were exposed via `Deref<Target=[u8;32]>`, `AsRef<[u8;32]>`
   and a derived `BorshSerialize`/`BorshDeserialize`. Any of these made it
   trivial to copy or serialize the secret into a buffer that is never
   zeroized, silently defeating the zeroize-on-drop guarantee.

   - Drop the blanket `Deref` and `AsRef` impls and the Borsh derives.
   - Gate raw access behind a single audited `as_bytes(&self) -> &[u8; 32]`
     accessor, documented as the one reviewed entry point to the secret.
   - Update all call sites: in-place crypto uses (`SigningKey::from_bytes`,
     `SharedKey`) and the storage layers that persist an explicit `[u8; 32]`
     copy now go through `as_bytes()`.

2. The `Drop` impl zeroized via an `unsafe` cast of `*mut Hash` to `*mut u8`
   over `size_of::<Hash>()`, which would silently miss key material if `Hash`
   ever gained padding or fields.

   - Change the inner type to a plain `[u8; 32]` and `#[derive(ZeroizeOnDrop)]`,
     removing the hand-rolled `unsafe` Drop and tracking field layout
     automatically. Enables zeroize's `derive` feature on calimero-primitives.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014gfW6RgRfT6z5VDPg4dXQW

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

🤖 MeroReviewer

Reviewed by 1 agents | Quality score: 28% | Review time: 65.5s

💡 1 suggestions. See inline comments.


🤖 Generated by MeroReviewer | Review ID: review-2dd88236

Comment thread crates/primitives/src/identity.rs
@meroreviewer

meroreviewer Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Documentation Review

The following documentation may need updates based on the changes in this PR:

  • 🟡 AGENTS.md: AGENTS.md exists but was not updated — consider updating it to reflect the architecture changes in this PR.
  • 🟡 CONTRIBUTING.md: CONTRIBUTING.md exists but was not updated — consider updating it to reflect the architecture changes in this PR.
  • 🟡 architecture/auto-follow.html: Files matching crates/governance-store/** were changed but architecture/auto-follow.html was not updated (per source_to_docs_mapping).
  • 🟡 architecture/crates/context.html: Files matching crates/context/** were changed but architecture/crates/context.html was not updated (per source_to_docs_mapping).
  • 🟡 architecture/crates/node.html: Files matching crates/node/** were changed but architecture/crates/node.html was not updated (per source_to_docs_mapping).
  • 🟡 architecture/crates/server.html: Files matching crates/server/** were changed but architecture/crates/server.html was not updated (per source_to_docs_mapping).
  • 🟡 architecture/crates/sync.html: Files matching crates/node/** were changed but architecture/crates/sync.html was not updated (per source_to_docs_mapping).
  • 🟡 architecture/local-governance.html: Files matching crates/governance-store/** were changed but architecture/local-governance.html was not updated (per source_to_docs_mapping).
  • 🟡 architecture/membership-and-leave.html: Files matching crates/context/** were changed but architecture/membership-and-leave.html was not updated (per source_to_docs_mapping).
  • 🟡 docs/: Static HTML docs in docs/ may need updating — architecture-impacting changes detected. On merge, update-docs will scan this directory and open a PR if any pages need to change.

@chefsale chefsale changed the title Harden PrivateKey against secret exfiltration and fragile zeroize fix(primitives): gate PrivateKey raw access and zeroize via derive Jun 27, 2026

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

🤖 AI Code Reviewer

Reviewed by 2 agents | Quality score: 54% | Review time: 1244.3s

🟡 1 warnings. See inline comments.


🤖 Generated by AI Code Reviewer | Review ID: review-dea067e6

Comment thread crates/context/src/handlers/create_context.rs Outdated
- create_context: pass `&identity_secret` directly to
  `sign_apply_and_publish` instead of `PrivateKey::from(*..as_bytes())`,
  which created an extra live, separately-dropped copy of the secret.
- identity: document that `Clone`/`Copy` are deliberately not derived on
  `PrivateKey`, since either would hand out an untracked copy.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014gfW6RgRfT6z5VDPg4dXQW
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.

2 participants