Crypto improvements

[M1nd3r]

No description provided.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 47754f75-2faf-466e-9a63-2bbbb2a7145c

📥 Commits

Reviewing files that changed from the base of the PR and between 585ef07 and db60f71.

📒 Files selected for processing (4)

• crypto/bip32.c
• crypto/cardano.c
• crypto/ecdsa.c
• crypto/ed25519-donna/ed25519.c

🚧 Files skipped from review as they are similar to previous changes (2)

• crypto/ed25519-donna/ed25519.c
• crypto/bip32.c

---

Walkthrough

Replaces scattered early returns across multiple crypto modules with a uniform pattern: introduce a local status/return variable (initialized to failure), set it on success, and jump to a centralized cleanup label before returning. Adds memzero/explicit clearing of sensitive temporaries (private keys, nonces, intermediate big-number buffers, AES contexts) on all paths. Changes touch hdnode NEM encrypt/decrypt, Cardano derivation, ECDSA signing/public-key paths, and ed25519 co-signer signing. No public APIs or signatures were modified.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

🚥 Pre-merge checks | ❌ 3

❌ Failed checks (2 warnings, 1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	No pull request description was provided by the author, missing all required sections from the repository template including development status, project assignment, and QA notes if applicable.	Add a comprehensive description covering the purpose of these crypto improvements, any relevant issue references, and development status. For core developers, ensure PR is properly assigned and added to the Firmware project with appropriate priority and sprint settings.
Docstring Coverage	⚠️ Warning	Docstring coverage is 30.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'Crypto improvements' is vague and overly generic, using a broad term that doesn't convey the specific nature of the changes without reading the detailed summary.	Consider a more specific title that highlights the primary change, such as 'Add memzero cleanup paths to cryptographic functions' or 'Improve memory security in crypto operations'.

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

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch m1nd3r/crypto-improvements

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

en main(all)

model	device_test	click_test	persistence_test
T2T1	test(all) main(all)	test(all) main(all)	test(all) main(all)
T3B1	test(all) main(all)	test(all) main(all)	test(all) main(all)
T3T1	test(all) main(all)	test(all) main(all)	test(all) main(all)
T3W1	test(all) main(all)	test(all) main(all)	test(all) main(all)

Latest CI run: 22671811162

[coderabbitai]

🧹 Nitpick comments (1)

crypto/cardano.c (1)

150-156: Consider zeroing ctx and zl8 in cleanup for completeness.

The HMAC context ctx (line 115) and the zl8 buffer (line 120) are also LOCAL_CONFIDENTIAL and contain sensitive intermediate state, but they are not included in the cleanup section. For consistency with the defensive cleanup pattern applied throughout this PR, consider adding:

 cleanup:
   // making sure to wipe our memory
   memzero(z, sizeof(z));
   memzero(data, sizeof(data));
   memzero(priv_key, sizeof(priv_key));
   memzero(res_key, sizeof(res_key));
+  memzero(zl8, sizeof(zl8));
+  memzero(&ctx, sizeof(ctx));
   return ret;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crypto/cardano.c` around lines 150 - 156, Add zeroing of the HMAC context and
zl8 buffer in the cleanup section: call memzero(&ctx, sizeof(ctx)) (or
memzero(ctx, sizeof *ctx) if ctx is a pointer) and memzero(zl8, sizeof(zl8))
before returning; update the cleanup block that currently memzeros z, data,
priv_key, res_key to also wipe ctx and zl8 to ensure all LOCAL_CONFIDENTIAL
variables are cleared.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@crypto/cardano.c`:
- Around line 150-156: Add zeroing of the HMAC context and zl8 buffer in the
cleanup section: call memzero(&ctx, sizeof(ctx)) (or memzero(ctx, sizeof *ctx)
if ctx is a pointer) and memzero(zl8, sizeof(zl8)) before returning; update the
cleanup block that currently memzeros z, data, priv_key, res_key to also wipe
ctx and zl8 to ensure all LOCAL_CONFIDENTIAL variables are cleared.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bb493c6 and bd093ad.

📒 Files selected for processing (4)

• crypto/bip32.c
• crypto/cardano.c
• crypto/ecdsa.c
• crypto/ed25519-donna/ed25519.c