Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Ledger application conducts unnecessary zeroization #6485

Closed
AaronFeickert opened this issue Aug 19, 2024 · 0 comments · Fixed by #6494
Closed

Ledger application conducts unnecessary zeroization #6485

AaronFeickert opened this issue Aug 19, 2024 · 0 comments · Fixed by #6494
Assignees
@hansieodendaal

Comments

@AaronFeickert
Copy link
Collaborator

The Ledger application will often place a RistrettoSecretKey into a Zeroizing wrapper to ensure it is zeroized on drop. However, this is unnecessary, as the RistrettoSecretKey type already zeroizes on drop since it implements SecretKey. All such wrappers can safely be removed for improved efficiency and clarity.
@hansieodendaal hansieodendaal mentioned this issue Aug 21, 2024
Merged
4 tasks
@AaronFeickert AaronFeickert linked a pull request Aug 21, 2024 that will close this issue
feat: various safety improvements to ledger wallet code #6494
Merged
4 tasks
SWvheerden pushed a commit that referenced this issue Aug 21, 2024
@hansieodendaal
feat: various safety improvements to ledger wallet code (#6494)
34eaaec 
Description
---
- Applied proper random nonces in the one-sided metadata signature to be
collision resistant.
- Removed double zeroizing on 'RistrettoSecretKey'.
- Ensured hasher output is zeroized.
- Changed Ledger BIP32 derivation to use 'secp256k1'.

Motivation and Context
---
See #6484, #6485, #6488 and #6490.

How Has This Been Tested?
---
System-level testing using a ledger device with `cargo run --release
--example ledger_demo`

What process can a PR reviewer use to test or verify this change?
---
- Code review
- `cargo run --release --example ledger_demo`

<!-- Checklist -->
<!-- 1. Is the title of your PR in the form that would make nice release
notes? The title, excluding the conventional commit
tag, will be included exactly as is in the CHANGELOG, so please think
about it carefully. -->


Breaking Changes
---

- [ ] None
- [ ] Requires data directory on base node to be deleted
- [ ] Requires hard fork
- [X] Other - Please specify

<!-- Does this include a breaking change? If so, include this line as a
footer -->
BREAKING CHANGE: All ledger derived keys and signatures will be
different.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@hansieodendaal hansieodendaal

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: various safety improvements to ledger wallet code hansieodendaal/tari
2 participants
@hansieodendaal @AaronFeickert