Zeroize PrivateKey - #112

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	heapless@​0.9.3
	stellar-strkey@​0.0.16
	zeroize@​1.8.2

View full report

Claude finished @leighmcculloch's task in 3m 59s —— View job

Code review — Zeroize PrivateKey seed and decode buffers

Overall direction is solid: derive Zeroize / ZeroizeOnDrop on PrivateKey, route the encode/decode scratch buffers through Zeroizing<…>, expose a no-residue write_string path, and keep behaviour-parity with the published v0.0.16 via a fuzz cross-check. The # Zeroize docs honestly call out the remaining leak vectors, which is the right move for a no-std crate that can't fully control register/return-value residue.

That said, two of the new pub APIs have a real correctness bug, and one design point on the Display path is worth tightening up.

Blocking — pub fn correctness bugs (already raised by Codex/Copilot, confirmed)

• src/convert.rs:115 encode_zeroizing push_strs into out without clearing first. Reusing a Zeroizing<String<E>> either concatenates or panics on capacity (a 56-char private-key buffer fits exactly one encoding, so the second call panics on valid input).
• src/convert.rs:227 decode_zeroizing has the mirror bug — extend_from_slice without clearing. A fallible pub decoder panicking on valid base32 due to prior buffer state is a particularly bad footgun.

In-tree these are dormant (the only callers, PrivateKey::write_string / PrivateKey::from_slice, always pass a fresh buffer), but both functions are pub and write_string's whole reason to exist is to let callers manage their own buffer — exactly the use-case the bug bites. Fix is one line in each:

out.clear(); // right after the const { … } assertions

Existing tests don't exercise reuse (everything starts from Zeroizing::new(String::new()) / Vec::new()), and fuzz_compare_zeroizing doesn't either — please add a regression test for both that calls into the same buffer twice. See the inline comments for details.

Design suggestion — Display for PrivateKey still leaks via to_string

Display::fmt (src/ed25519.rs line 113-115, unchanged in this PR) does write!(f, "{}", self.to_string()). That goes through the inherent to_string, whose returned String<56> is not Zeroizing — it lives on the Display::fmt stack frame, gets formatted into f, and is dropped un-zeroed. So format!("{key}") / println!("{key}") / the blanket ToString impl all still leave a private-key-shaped 56-byte residue, even though the encoder scratch is now clean.

A small change routes Display through write_string:

impl Display for PrivateKey {
    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
        let mut buf: Zeroizing<String<{ Self::ENCODED_LEN }>> = Zeroizing::new(String::new());
        self.write_string(&mut buf);
        f.write_str(&buf)
    }
}

The formatter target is still out of the crate's control (your # Zeroize docs are accurate that Display doesn't fully zeroize), but this at least removes the un-managed String<56> from the encoder side. Up to you whether that's worth the churn vs. the existing doc note.

Smaller items

• fuzz/fuzz_targets/fuzz_compare_zeroizing.rs:14 — comment typo zeroize_zeroizing (already raised by Copilot). Probably should reference encode_zeroizing / decode_zeroizing explicitly.
• fuzz_compare_v16 is a nice safety net for this kind of behaviour-only refactor — good addition. Worth keeping past this PR.
• Removing Copy from PrivateKey is the right call (the breaking-change note in the PR body covers it). I scanned tests/, src/cli/, and src/strkey.rs for usage and didn't see any spots that would have needed *key-style by-value moves; the existing call sites all go through &key / key.0, so the Clone-only impl is enough.
• PrivateKey::to_string (lines 66-72) does the right thing: scratch goes through a Zeroizing<String>, then a final copy into a plain String<56> for the return value (which is the documented "remaining residue" case). Fine.
• Hash / Ord derives on PrivateKey haven't changed semantics, but Hash will leak the seed into a hasher's internal state if anyone ever puts a PrivateKey in a HashSet / HashMap. Probably out of scope here, just worth being aware of.

Inline comments

I posted inline comments on the two convert.rs issues and the fuzz-target typo (which include out.clear() suggestions and a test-gap note).

Fix encode_zeroizing / decode_zeroizing reset bug →

• Branch: fix-private-key-zeroization

Claude finished @leighmcculloch's task in 1m 48s —— View job

Code review — re-check after sync

Since the prior review (commit 624dba4), the only new changes on this branch are:

• 5a1ba6f add corpus for comparison fuzz — adds 23 corpus files under fuzz/corpus/fuzz_compare_zeroizing/. No new test or source coverage.
• 8e109de Merge branch 'main' into fix-private-key-zeroization — merge only.

git diff 624dba4..HEAD -- src/ fuzz/fuzz_targets/ is empty: no source-side changes. So all findings from the prior review still stand:

Still blocking

• src/convert.rs:115-116 encode_zeroizing does out.push_str(...) without first calling out.clear(). A reused Zeroizing<String<56>> either concatenates (silently producing an invalid strkey) or panics on capacity (a 56-char buffer fits exactly one private-key encoding).
• src/convert.rs:227 decode_zeroizing does out.extend_from_slice(...) without first calling out.clear() — mirror bug, same impact, more dangerous because decode_zeroizing is fallible and a Result-returning pub API panicking on valid base32 input is a particularly nasty footgun.
• Neither path has a regression test for buffer reuse: test_encode_zeroizing_matches_encode / test_decode_zeroizing_matches_decode always start from a fresh Zeroizing::new(...), and fuzz_compare_zeroizing.rs likewise constructs new buffers each iteration (line 26, 36, 60) — which is why the new corpus didn't surface this either.

Fix is one line at the top of each function (after the const { ... } assertions):

out.clear();

Plus a regression test that calls each _zeroizing twice into the same buffer and asserts the second call produces the single-call result, and a second pass in fuzz_compare_zeroizing that exercises buffer reuse.

Fix the reset bug + add tests →

Still open (design / nit)

• src/ed25519.rs:112-116 Display for PrivateKey routes through self.to_string(), whose returned String<56> is plain (not Zeroizing) and lives on the fmt stack frame after formatting — so format!("{key}") / println!("{key}") / the blanket ToString still leave a 56-byte private-key-shaped residue. Routing Display through write_string instead would remove the unmanaged copy. Optional — your # Zeroize doc honestly notes Display doesn't fully zeroize, so this is an explicit-tradeoff call.
• fuzz/fuzz_targets/fuzz_compare_zeroizing.rs:14 — comment typo zeroize_zeroizing (already flagged by Codex/Copilot/me).

What looks good

• The newly-added fuzz_compare_zeroizing corpus (23 files) is a useful seed. Once the buffer-reuse path is exercised in the target itself, the existing corpus + libfuzzer's mutator will pick up reuse-related drift cheaply.
• fuzz_compare_v16 continues to be a great safety net for this kind of behaviour-only refactor.
• All other findings from the prior review remain accurate — to_string (lines 66-72) routes through a Zeroizing<String> and only the deliberate final residue copy is plain; Clone-only PrivateKey is right; no by-value-Copy call sites broke.

Todos

• Sync local branch and check what changed since prior review (no source changes — corpus + workflow only)
• Re-check convert::encode_zeroizing / decode_zeroizing — buffer-reset bugs unaddressed
• Re-check fuzz target typo — still present
• Re-check Display for PrivateKey leak path — still goes through plain to_string
• Look at any newly added corpus / tests for buffer-reuse coverage — none added; corpus seeds don't help because the target itself never reuses buffers
• Run cargo build / cargo test — skipped; no source changed since prior review, so no new behaviour to verify
• Post updated review
  • Branch: fix-private-key-zeroization

Claude finished @leighmcculloch's task in 5m 17s —— View job

Code review — re-check after f995894

The new commit f995894 refactor zeroization handling for key encoding decode addresses the P1 buffer-reset bugs from the prior review. Verified by reading src/convert.rs and fuzz/fuzz_targets/fuzz_compare_zeroizing.rs at HEAD.

Now resolved

• encode_zeroizing clears out before writing — src/convert.rs:101. Comment on lines 98–100 explains why. ✓
• decode_zeroizing clears out before writing — src/convert.rs:208. Same shape, plus a nice property: the out.clear() runs before the early Err returns from the data_len < 3 / decode-base32 paths, so a reused buffer is never left holding stale plaintext after a decode failure. The new test_decode_zeroizing_resets_output_on_error test exercises this. ✓
• Regression tests for buffer reuse — src/convert.rs:336-369 (test_encode_zeroizing_resets_output_on_reuse, test_decode_zeroizing_resets_output_on_reuse) plus the error-path test. Each calls the function twice into the same buffer and asserts the second call equals a single-call result, exactly the gap I flagged. ✓
• Fuzz comment typo — fuzz/fuzz_targets/fuzz_compare_zeroizing.rs:14 now reads `encode_zeroizing` / `decode_zeroizing`. ✓

Still open (design suggestion, not blocking)

• src/ed25519.rs:112-116 — Display for PrivateKey still routes through self.to_string(), whose returned String<56> is plain (not Zeroizing) and lives on the fmt stack frame after formatting. So format!("{key}") / println!("{key}") / the blanket ToString still leave a 56-byte private-key-shaped residue, even though the encoder scratch is now clean.

  Routing Display through write_string removes the unmanaged copy:

  impl Display for PrivateKey {
      fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
          let mut buf: Zeroizing<String<{ Self::ENCODED_LEN }>> = Zeroizing::new(String::new());
          self.write_string(&mut buf);
          f.write_str(&buf)
      }
  }

  Optional — your # Zeroize doc on PrivateKey (src/ed25519.rs:26-31) honestly notes Display doesn't fully zeroize, so this is an explicit-tradeoff call. If you take this route, the doc bullet for Display could be tightened to "the formatter target is outside our control" rather than implying the encoder leaves residue. Apply this →

Nit (low priority)

• fuzz/fuzz_targets/fuzz_compare_zeroizing.rs:26,36,60 — the fuzz target still constructs fresh buffers each iteration, so it doesn't exercise the buffer-reuse path that the new unit tests cover. Now that out.clear() is in place this matters less, but a second pass that reuses the same Zeroizing<String<56>> / Zeroizing<Vec<u8, 32>> across two encode/decode calls in the same iteration would let libfuzzer's mutator hammer reuse-related drift cheaply. Optional.

Verification

• I couldn't run cargo test / cargo build in this sandbox (cargo isn't on the action's allowlist), so the new regression tests are not executed here. By inspection they look correct: each does a single-call baseline via encode::<32, 35, 56> / decode::<32, 35>, then a second call into the same Zeroizing<…> buffer, and asserts the result equals the single-call output. CI should confirm.
• All other findings from the prior review have been addressed.

Net

P1 buffer-reset bugs and the typo are resolved. Only the Display-leak design point remains, and that's a deliberate-tradeoff call documented in the type's own # Zeroize doc. Looks good to me.

Todos

• Sync local branch and check what's changed since prior reviews
• Re-check convert::encode_zeroizing / decode_zeroizing buffer-reset bugs — fixed in f995894
• Re-check Display for PrivateKey leak path — still open (optional)
• Re-check fuzz target typo — fixed
• Look for any new test/source coverage that closes the prior gaps — three new tests added, including reset-on-error
• Run cargo build / cargo test — cargo not in this action's allowlist; CI will verify
• Post updated review
  • Branch: fix-private-key-zeroization

Claude finished @leighmcculloch's task in 4m 43s —— View job

Code review — re-check after b0cf80f

The new commit b0cf80f update display to use zeroizing addresses the only remaining open item from the prior review. I verified by reading src/ed25519.rs, src/convert.rs, and fuzz/fuzz_targets/fuzz_compare_zeroizing.rs at HEAD; full git diff f995894..HEAD is the 4-line change to Display and nothing else.

Now resolved

• Display for PrivateKey no longer goes through plain to_string() — src/ed25519.rs:112-118:

  impl Display for PrivateKey {
      fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
          let mut buf: Zeroizing<String<{ Self::ENCODED_LEN }>> = Zeroizing::new(String::new());
          self.write_string(&mut buf);
          f.write_str(&buf)
      }
  }

  This is the form I suggested. The encoder-side residue is now contained: buf is Zeroizing<String<56>> so its 56 bytes are wiped on drop at the end of fmt, and the inner encode_zeroizing scratch is already Zeroizing<…>. Only the formatter target (f's underlying buffer — String for format!, the writer for write!, etc.) remains outside the crate's control, which is the inherent limit of Display and is what the type's # Zeroize doc covers. Drop-on-unwind is preserved if write_string or f.write_str ever panic. ✓

• No regression elsewhere. out.clear() is still in place in encode_zeroizing (src/convert.rs:101) and decode_zeroizing (src/convert.rs:208); the three reuse / reset-on-error tests (src/convert.rs:336-386) are still there; the fuzz comment typo is still fixed.

Smaller items

• src/ed25519.rs:26-31 — the # Zeroize bullet still lists Display alongside to_string and Debug under "do not zero the private key value." That's not wrong (the bytes still hit the un-managed formatter target), but Display and to_string now have meaningfully different residue stories: Display's only remaining leak is the formatter target itself, while to_string additionally returns a plain String<56>. Optional wording tighten: split them, e.g.

  Display and Debug write into a caller-provided Formatter whose backing buffer is outside this crate's control; the encoder-side scratch is zeroed.
  to_string additionally returns a plain String<{ENCODED_LEN}> (use write_string for the no-residue path).

  Skip if the current bullet's brevity is preferred. Apply this →

• fuzz/fuzz_targets/fuzz_compare_zeroizing.rs:26,36,60 — the fuzz target still constructs fresh buffers each iteration. Now that out.clear() is in place this is much less critical, but a second pass exercising buffer reuse would let libfuzzer's mutator hammer reuse-related drift cheaply. Still optional, called out for completeness.

• Hash / Ord derives on PrivateKey haven't changed semantics — out of scope here, but Hash will leak the seed into a hasher's internal state if anyone puts a PrivateKey in a HashMap / HashSet. Worth being aware of; not blocking.

Verification

• Cargo isn't available in this action sandbox, so I couldn't run cargo build / cargo test — verification is by inspection only. The new 4-line Display impl is straightforward and the existing test_private_key_write_string_matches_to_string already covers the write_string path it now delegates to. CI should confirm.
• Re-read all three regression tests in src/convert.rs and the Display/write_string/to_string paths in src/ed25519.rs. No issues spotted.

Net

LGTM. P1 buffer-reset bugs are fixed, the only design suggestion from the prior review is now applied, and the remaining items are minor wording / fuzz-coverage polish that don't need to block the merge.

Todos

• Sync local branch and check what's changed since prior review (f995894 → b0cf80f)
• Re-check Display for PrivateKey — applied as suggested, drop-on-unwind preserved
• Verify the # Zeroize doc bullet is still consistent with the new Display impl — still accurate, optional wording tighten available
• Look for any new tests / coverage and confirm previously-fixed items still hold — no regressions
• Run cargo build / cargo test — cargo not in this action's allowlist; CI will verify
• Post updated review
  • Branch: fix-private-key-zeroization

Claude finished @leighmcculloch's task in 1m 42s —— View job

Code review — re-check after a8d06e1 (merge from main)

git diff b0cf80f..HEAD -- src/ fuzz/ shows the only changes since the prior review are doc-only additions in src/lib.rs, src/strkey.rs, src/decoded_json_format.rs, and two missing top-level doc comments on PublicKey / MuxedAccount in src/ed25519.rs — all from the merged cf1337b "remove zero-allocation path (#110)". No changes to convert.rs, the PrivateKey/Display/write_string paths, or the fuzz target.

So all findings from the prior review still hold. Quick re-confirmation by reading the files at HEAD:

Verified at HEAD

• encode_zeroizing clears out before writing — src/convert.rs:101. ✓
• decode_zeroizing clears out before writing, including before the early Err returns — src/convert.rs:208. ✓
• Buffer-reuse and reset-on-error regression tests — src/convert.rs:336-386. ✓
• Display for PrivateKey routes through write_string with a local Zeroizing<String<56>> — src/ed25519.rs:112-118. ✓
• Fuzz comment typo — fuzz/fuzz_targets/fuzz_compare_zeroizing.rs:14 reads `encode_zeroizing` / `decode_zeroizing`. ✓

Still open (optional polish)

• src/ed25519.rs:26-31 — the # Zeroize bullet list still groups Debug, Display, and to_string together under "do not zero the private key value." After the b0cf80f change, Display's only remaining residue is the formatter target (outside the crate's control), while to_string additionally returns a plain String<56>. Splitting the bullet would be more accurate; the current wording isn't wrong, just a bit conservative. Not blocking.
• fuzz/fuzz_targets/fuzz_compare_zeroizing.rs:26,36,60 — fuzz target still constructs fresh buffers each iteration, so the buffer-reuse path is only covered by the unit tests. Now that out.clear() is in place this matters a lot less, but it's a cheap one-line addition if you want libfuzzer to also explore reuse-related drift. Optional.

Out of scope but worth being aware of

• Hash / Ord derives on PrivateKey are unchanged. Hash will leak the seed into a hasher's internal state if anyone puts a PrivateKey in a HashMap / HashSet — out of scope for this PR.

Verification

Cargo isn't on this action's allowlist, so cargo build / cargo test / fuzz runs are not executed here. The full CI matrix in this repo will cover that. Source review at HEAD: no regressions vs. the prior LGTM commit.

Net

LGTM. P1 buffer-reset bugs and the typo are resolved. Only the Display doc-bullet wording and an optional fuzz-coverage tweak remain, both opt-in polish.

Todos

• Sync local branch and check what's new since prior reviews — a8d06e1 is doc-only (cf1337b merge from main); no source/fuzz changes
• Re-verify convert::encode_zeroizing / decode_zeroizing — out.clear() in place; reuse + reset-on-error tests present
• Re-verify Display for PrivateKey zeroizing path — routes through write_string with a local Zeroizing<String<56>>
• Re-check # Zeroize doc on PrivateKey — still groups Display with to_string/Debug; minor optional polish
• Re-check fuzz target / corpus — typo fixed; reuse-path coverage still optional
• Run cargo build / cargo test — cargo not in this action's allowlist; CI will verify
• Post review

• Branch: fix-private-key-zeroization
•