Add cove-keyteleport crate: Key Teleport crypto primitives (mnemonic/xprv)#676
Add cove-keyteleport crate: Key Teleport crypto primitives (mnemonic/xprv)#676pradhyum6144 wants to merge 5 commits into
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a new internal Rust crate Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant ReceiverSession as ReceiverSession
participant Crypto as Crypto
participant BBQr as BBQr
User->>ReceiverSession: generate()
ReceiverSession->>Crypto: sample secp256k1 private key
ReceiverSession->>ReceiverSession: choose numeric_code
User->>ReceiverSession: to_packet()
ReceiverSession->>Crypto: receiver_pubkey_key(numeric_code)
ReceiverSession->>Crypto: aes256ctr(key, pubkey_bytes)
ReceiverSession->>BBQr: encode(encrypted_pubkey, type=R)
BBQr-->>User: B$2R... (QR string)
sequenceDiagram
participant User as Sender App
participant SenderSession as SenderSession
participant Crypto as Crypto
participant BBQr as BBQr
User->>SenderSession: new(receiver_packet, numeric_code)
SenderSession->>Crypto: aes256ctr_decrypt(pubkey_key, encrypted_pubkey)
SenderSession->>Crypto: parse receiver PublicKey
SenderSession->>Crypto: generate ephemeral secret
SenderSession->>Crypto: session_key = ECDH(priv, remote_pub)
SenderSession->>SenderSession: generate teleport_password (Base32)
User->>SenderSession: encrypt(payload)
SenderSession->>Crypto: payload -> to_bytes + checksum
SenderSession->>Crypto: inner_key = pbkdf2_stretch(session_key, teleport_password)
SenderSession->>Crypto: aes256ctr(inner_key, payload_with_checksum)
SenderSession->>Crypto: append checksum, aes256ctr(session_key, outer_bytes)
SenderSession->>BBQr: encode(sender_pubkey || encrypted_body, type=S)
BBQr-->>User: B$2S... (QR string)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Greptile SummaryThis PR introduces the Confidence Score: 4/5Safe to merge for protocol exploration, but two unresolved P1 issues from prior review (silent base58 corruption and modular bias in numeric code) should be addressed before production use. All new findings in this review are P2 (teleport_password not zeroed, Secp256k1 context overhead). However, two P1-level issues flagged in prior review threads remain unaddressed: the silent base58 fallback in Payload::Xprv that can corrupt data without error, and the ~2.3% modular bias in the numeric code generation used to protect the receiver's public key. These keep the score at 4/5. rust/crates/cove-keyteleport/src/payload.rs (silent base58 error) and rust/crates/cove-keyteleport/src/receiver.rs (modular bias in numeric code) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[ReceiverSession::generate] --> B[Random ephemeral keypair]
A --> C[Random 8-digit numeric code]
B --> D[Encrypt pubkey with SHA256 of code]
D --> E[R packet BBQr]
F[SenderSession::new] --> G[Decrypt receiver pubkey using code]
G --> H[Generate ephemeral keypair]
H --> I[ECDH to derive session key]
I --> J[Generate teleport password]
K[SenderSession::encrypt] --> L[Serialize payload with type byte]
L --> M[Append 2-byte checksum]
M --> N[AES-CTR encrypt with PBKDF2 inner key]
N --> O[Append 2-byte outer checksum]
O --> P[AES-CTR encrypt with session key]
P --> Q[S packet BBQr]
R[ReceiverSession::decode] --> S[ECDH to derive session key]
S --> T[Outer AES-CTR decrypt and verify checksum]
T --> U[Derive inner key via PBKDF2]
U --> V[Inner AES-CTR decrypt and verify checksum]
V --> W[Parse Payload Mnemonic or Xprv]
Reviews (2): Last reviewed commit: "Apply rustfmt formatting to cove-keytele..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| aes = "0.8" | ||
| ctr = "0.9" | ||
|
|
||
| # PBKDF2-SHA512 | ||
| pbkdf2 = "0.12" |
There was a problem hiding this comment.
aes = "0.8", ctr = "0.9", and pbkdf2 = "0.12" are declared with inline version specifiers instead of via the workspace. All other crates in this workspace use workspace = true for version management. This inconsistency means these versions won't be bumped uniformly when the workspace is updated and can drift from what other crates' transitive dependencies resolve to. Consider adding them to the root Cargo.toml [workspace.dependencies] section and referencing them as workspace = true here.
| Secp(String), | ||
| } | ||
|
|
||
| impl From<bitcoin::secp256k1::Error> for Error { | ||
| fn from(e: bitcoin::secp256k1::Error) -> Self { | ||
| Error::Secp(e.to_string()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Error::Secp variant and From impl appear to be dead code
No code path in this crate currently propagates a secp256k1::Error via ? — all secp operations (key generation, mul_tweak, pubkey parse) either use .expect() or directly return a domain error. If future callers need this conversion it can be re-added then; leaving unused enum variants and trait impls in a public API can confuse consumers.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
rust/crates/cove-keyteleport/src/payload.rs (1)
12-13: Nit: BIP-39 also allows 15 and 21 words.Doc comment omits 15/21-word mnemonics; consider
(12 / 15 / 18 / 21 / 24 words)to match BIP‑39.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-keyteleport/src/payload.rs` around lines 12 - 13, Update the doc comment for the Mnemonic variant in payload.rs to list all allowed BIP-39 word counts; change the parentheses text on the Mnemonic(Mnemonic) enum variant from "(12 / 18 / 24 words)" to "(12 / 15 / 18 / 21 / 24 words)" so the documentation accurately reflects BIP‑39-supported sizes.rust/crates/cove-keyteleport/src/receiver.rs (1)
25-35: Small: modulo bias and retry loop invariants.
rand::random::<u32>() % (MAX_NUMERIC_CODE + 1)is slightly biased (negligible here — ≤ 2⁻²⁴), butrng.random_range(0..=MAX_NUMERIC_CODE)is both unbiased and clearer. Similarly,SecretKey::from_slicerejects only the zero scalar and values ≥ curve order (~2⁻¹²⁸ probability), so the retry loop is fine but could be swapped forSecretKey::new(&mut rand::rng())from secp256k1's rand feature if you prefer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-keyteleport/src/receiver.rs` around lines 25 - 35, Replace the biased numeric generation in generate() by using the RNG's unbiased range method: instead of rand::random::<u32>() % (MAX_NUMERIC_CODE + 1) use rng.random_range(0..=MAX_NUMERIC_CODE) (or equivalent unbiased API) to remove modulo bias; optionally, for the private key creation you can replace the manual byte-fill + SecretKey::from_slice retry loop by using secp256k1's SecretKey::new(&mut rand::rng()) when the crate's rand feature is enabled, otherwise keep the current loop around SecretKey::from_slice(&key_bytes) which preserves the retry invariant.rust/crates/cove-keyteleport/src/bbqr.rs (1)
45-63: Minor: unwrap onchars.next()relies on byte length vs char count.
rest.len() < 6checks bytes, but the subsequentchars.next().unwrap()s assume ≥2 chars. For a malformed input with multi-byte UTF‑8 the byte check can pass while fewer than 2 chars exist, causing a panic. Uppercasing earlier and restricting to ASCII, or usingchars.next().ok_or_else(...), would make this robust without losing clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-keyteleport/src/bbqr.rs` around lines 45 - 63, In decode, the current byte-length check (rest.len() < 6) can still allow fewer than two Unicode chars and the two calls chars.next().unwrap() (for encoding and file_type) can panic; replace these unwraps with safe conversions: either verify character count via rest.chars().count() >= 2 before creating chars, or call chars.next().ok_or_else(|| Error::InvalidBbqr("missing encoding".into())) and similarly wrap the second chars.next() passed to KtFileType::from_char with ok_or_else to return a clear Error::InvalidBbqr instead of panicking (reference function decode, variable rest, chars.next().unwrap(), and KtFileType::from_char).rust/crates/cove-keyteleport/src/sender.rs (1)
29-60: Document this protocol limitation or add checksum validation to R packet.The current implementation relies solely on
PublicKey::from_slicerejection to detect invalid numeric codes. However, approximately 0.4% of wrong codes will decrypt to syntactically valid compressed pubkeys (first byte 0x02/0x03 with valid x-coordinate), causing the sender to proceed and the receiver to see a confusingChecksumMismatchon the inner payload rather than an immediate "wrong code" error.Two options to address this:
- Spec-level fix: Include a 2-byte checksum or encrypted tag alongside the pubkey in the R packet (similar to the S packet checksum), enabling immediate detection.
- Documentation-level fix: If staying strictly protocol-conformant with the current 33-byte R packet, add a comment documenting this ~0.4% failure mode.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-keyteleport/src/sender.rs` around lines 29 - 60, The R-packet decryption can yield a syntactically valid compressed pubkey ~0.4% of the time even with a wrong numeric_code, so change Sender::new (the function with receiver_pubkey_key, aes256ctr and PublicKey::from_slice) to either (preferred) implement a small explicit check on the R packet (e.g. extend the spec to include a 2-byte checksum/encrypted tag alongside the 33-byte pubkey and verify it after aes256ctr decryption before calling PublicKey::from_slice) or (alternate) add a clear code comment in ReceiverPacket/Sender::new documenting this failure mode and why PublicKey::from_slice is not sufficient; update the docs/spec and error messages accordingly so callers can detect "wrong numeric code" immediately instead of surfacing a later ChecksumMismatch on inner payload.rust/crates/cove-keyteleport/src/lib.rs (1)
69-79: Consider adding an xprv roundtrip test.The PR objectives call out support for both
mnemonicandxprvv1 payloads, but this module only exercisesPayload::Mnemonic. Anxprvroundtrip test here (alongside the 12/24-word cases) would give direct coverage of the second supported variant at the public API boundary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-keyteleport/src/lib.rs` around lines 69 - 79, Add a new test case invoking the existing roundtrip helper to exercise Payload::XPrv: construct a Payload::XPrv (matching the v1 xprv format your public API expects), call roundtrip(payload) and assert the returned Payload equals the original; place this alongside the existing 12/24-word mnemonic tests and reuse the roundtrip function and ReceiverSession/SenderSession flow to cover the xprv variant at the public API boundary.rust/crates/cove-keyteleport/Cargo.toml (1)
17-22: Consider promotingaes,ctr, andpbkdf2to workspace dependencies.All other crypto deps in this crate (
bitcoin,sha2,hmac,rand,zeroize,thiserror,data-encoding,bip39) are consumed viaworkspace = true. Pinningaes = "0.8",ctr = "0.9", andpbkdf2 = "0.12"directly here breaks that convention and will make future RustCrypto version bumps harder to keep consistent across the workspace. Prefer declaring them under[workspace.dependencies]inrust/Cargo.tomland referencing viaworkspace = true.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-keyteleport/Cargo.toml` around lines 17 - 22, The crate is pinning aes, ctr, and pbkdf2 locally which breaks the workspace convention; move these three dependencies into the top-level workspace [workspace.dependencies] in rust/Cargo.toml (declare versions there) and replace their entries in this crate's Cargo.toml with workspace = true for aes, ctr, and pbkdf2 so they are consumed from the workspace and remain version-consistent with other RustCrypto deps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/crates/cove-keyteleport/Cargo.toml`:
- Line 13: Remove the direct hmac dependency from Cargo.toml because crypto.rs
only uses pbkdf2::pbkdf2_hmac and types from sha2, and hmac is provided
transitively by pbkdf2; update Cargo.toml by deleting the line "hmac = {
workspace = true }" so the crate relies on the transitive hmac version, leaving
pbkdf2 and sha2 entries intact and ensuring no code changes in functions like
pbkdf2::pbkdf2_hmac in src/crypto.rs are necessary.
In `@rust/crates/cove-keyteleport/src/payload.rs`:
- Around line 30-38: The XPRV branch (Payload::Xprv) currently uses
bitcoin::base58::decode with a silent unwrap_or_else fallback and plain base58
encoding on serialize/deserialize; change both sides to use
bitcoin::base58::decode_check and bitcoin::base58::encode_check and remove the
UTF-8 fallback so invalid Base58Check XPRV strings surface as errors; update the
code paths (the Payload::Xprv handling in to_bytes/from_bytes or the functions
that serialize/deserialize this variant and any use of TYPE_XPRV) to propagate
or return the decoding/encoding error instead of silently converting to raw
bytes (or alternatively validate XPRV when constructing the Payload::Xprv so
this branch never receives invalid input).
In `@rust/crates/cove-keyteleport/src/sender.rs`:
- Around line 118-126: The test wrong_numeric_code_gives_error is flaky because
random decrypted bytes can sometimes form a valid PublicKey; change the test to
try multiple wrong codes deterministically: call ReceiverSession::generate(),
get r_pkt via to_packet(), then loop a fixed number of different wrong codes
(e.g. 50) and call SenderSession::new(&r_pkt, wrong_code) for each, collecting
results from SenderSession::new (which invokes PublicKey::from_slice); finally
assert that at least one attempt returned Err (or assert a high rejection rate)
to avoid spurious passes while keeping the test deterministic and tied to
numeric_code/to_packet/SenderSession::new.
---
Nitpick comments:
In `@rust/crates/cove-keyteleport/Cargo.toml`:
- Around line 17-22: The crate is pinning aes, ctr, and pbkdf2 locally which
breaks the workspace convention; move these three dependencies into the
top-level workspace [workspace.dependencies] in rust/Cargo.toml (declare
versions there) and replace their entries in this crate's Cargo.toml with
workspace = true for aes, ctr, and pbkdf2 so they are consumed from the
workspace and remain version-consistent with other RustCrypto deps.
In `@rust/crates/cove-keyteleport/src/bbqr.rs`:
- Around line 45-63: In decode, the current byte-length check (rest.len() < 6)
can still allow fewer than two Unicode chars and the two calls
chars.next().unwrap() (for encoding and file_type) can panic; replace these
unwraps with safe conversions: either verify character count via
rest.chars().count() >= 2 before creating chars, or call
chars.next().ok_or_else(|| Error::InvalidBbqr("missing encoding".into())) and
similarly wrap the second chars.next() passed to KtFileType::from_char with
ok_or_else to return a clear Error::InvalidBbqr instead of panicking (reference
function decode, variable rest, chars.next().unwrap(), and
KtFileType::from_char).
In `@rust/crates/cove-keyteleport/src/lib.rs`:
- Around line 69-79: Add a new test case invoking the existing roundtrip helper
to exercise Payload::XPrv: construct a Payload::XPrv (matching the v1 xprv
format your public API expects), call roundtrip(payload) and assert the returned
Payload equals the original; place this alongside the existing 12/24-word
mnemonic tests and reuse the roundtrip function and
ReceiverSession/SenderSession flow to cover the xprv variant at the public API
boundary.
In `@rust/crates/cove-keyteleport/src/payload.rs`:
- Around line 12-13: Update the doc comment for the Mnemonic variant in
payload.rs to list all allowed BIP-39 word counts; change the parentheses text
on the Mnemonic(Mnemonic) enum variant from "(12 / 18 / 24 words)" to "(12 / 15
/ 18 / 21 / 24 words)" so the documentation accurately reflects BIP‑39-supported
sizes.
In `@rust/crates/cove-keyteleport/src/receiver.rs`:
- Around line 25-35: Replace the biased numeric generation in generate() by
using the RNG's unbiased range method: instead of rand::random::<u32>() %
(MAX_NUMERIC_CODE + 1) use rng.random_range(0..=MAX_NUMERIC_CODE) (or equivalent
unbiased API) to remove modulo bias; optionally, for the private key creation
you can replace the manual byte-fill + SecretKey::from_slice retry loop by using
secp256k1's SecretKey::new(&mut rand::rng()) when the crate's rand feature is
enabled, otherwise keep the current loop around
SecretKey::from_slice(&key_bytes) which preserves the retry invariant.
In `@rust/crates/cove-keyteleport/src/sender.rs`:
- Around line 29-60: The R-packet decryption can yield a syntactically valid
compressed pubkey ~0.4% of the time even with a wrong numeric_code, so change
Sender::new (the function with receiver_pubkey_key, aes256ctr and
PublicKey::from_slice) to either (preferred) implement a small explicit check on
the R packet (e.g. extend the spec to include a 2-byte checksum/encrypted tag
alongside the 33-byte pubkey and verify it after aes256ctr decryption before
calling PublicKey::from_slice) or (alternate) add a clear code comment in
ReceiverPacket/Sender::new documenting this failure mode and why
PublicKey::from_slice is not sufficient; update the docs/spec and error messages
accordingly so callers can detect "wrong numeric code" immediately instead of
surfacing a later ChecksumMismatch on inner payload.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2fee2c5c-f013-4d03-b3b5-c9daa815fb36
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
rust/Cargo.tomlrust/crates/cove-keyteleport/Cargo.tomlrust/crates/cove-keyteleport/src/bbqr.rsrust/crates/cove-keyteleport/src/crypto.rsrust/crates/cove-keyteleport/src/error.rsrust/crates/cove-keyteleport/src/lib.rsrust/crates/cove-keyteleport/src/packet.rsrust/crates/cove-keyteleport/src/payload.rsrust/crates/cove-keyteleport/src/receiver.rsrust/crates/cove-keyteleport/src/sender.rs
|
|
||
| # crypto primitives | ||
| sha2 = { workspace = true } | ||
| hmac = { workspace = true } |
There was a problem hiding this comment.
Remove unused hmac direct dependency.
Per rust/crates/cove-keyteleport/src/crypto.rs, only pbkdf2::pbkdf2_hmac and sha2 are used directly; hmac appears solely as a transitive dependency of pbkdf2. Declaring it as a direct dep here adds noise and risks version drift from the transitive one.
Proposed fix
# crypto primitives
sha2 = { workspace = true }
-hmac = { workspace = true }
rand = { workspace = true }
zeroize = { workspace = true, features = ["derive"] }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| hmac = { workspace = true } | |
| # crypto primitives | |
| sha2 = { workspace = true } | |
| rand = { workspace = true } | |
| zeroize = { workspace = true, features = ["derive"] } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/crates/cove-keyteleport/Cargo.toml` at line 13, Remove the direct hmac
dependency from Cargo.toml because crypto.rs only uses pbkdf2::pbkdf2_hmac and
types from sha2, and hmac is provided transitively by pbkdf2; update Cargo.toml
by deleting the line "hmac = { workspace = true }" so the crate relies on the
transitive hmac version, leaving pbkdf2 and sha2 entries intact and ensuring no
code changes in functions like pbkdf2::pbkdf2_hmac in src/crypto.rs are
necessary.
| Payload::Xprv(xprv) => { | ||
| // base58-decoded binary XPRV (78 bytes), prefixed with the type byte | ||
| let decoded = | ||
| bitcoin::base58::decode(xprv).unwrap_or_else(|_| xprv.as_bytes().to_vec()); | ||
| let mut out = Vec::with_capacity(1 + decoded.len()); | ||
| out.push(TYPE_XPRV); | ||
| out.extend_from_slice(&decoded); | ||
| out | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Key Teleport protocol specification xprv payload base58 check encoding format
💡 Result:
The Key Teleport protocol is a feature of COLDCARD Q hardware wallets (firmware v1.3.2Q and later) for securely transferring sensitive data like seeds, XPRVs, secure notes/passwords, PSBTs, or full backups between two COLDCARD Q devices using QR/BBQr codes and/or NFC, protected by ECDH-derived session keys and double AES-256-CTR encryption. The plaintext payload begins with a 1-byte data type code. For XPRV transfer (type 'x'), the format is: 1-byte 'x' + 4 bytes (XPRV details, likely version, depth, parent fingerprint, child number or similar metadata) + binary representation of the base58-decoded XPRV (78 bytes for standard BIP-32 extended private key: version (4), depth (1), parent fingerprint (4), child number (4), chain code (32), private key (33)). The XPRV binary follows standard BIP-32 serialization, which is Base58Check-encoded as a string starting with 'xprv' (mainnet) or 'tprv' (testnet). Base58 decoding yields the 78-byte binary structure plus 4-byte checksum. This payload is then AES-256-CTR encrypted with a random Teleport Key (8-char Base32, stretched via PBKDF2-SHA512), yielding inner ciphertext. Sender's ephemeral public key (33 bytes compressed) is prepended, the result checksummed (last 2 bytes = SHA256(body[:-2])[-2:]), and AES-256-CTR encrypted with ECDH session key (SHA256(X+Y coordinates)), then BBQr-encoded for transfer. Receiver reverses: decrypt outer layer with session key, verify checksum, prompt for Teleport Key, decrypt inner layer, parse type 'x', and import the XPRV. Sources confirm XPRV payload uses base58-decoded binary directly in the typed payload, per COLDCARD firmware docs.
Citations:
- 1: https://coldcard.com/docs/key-teleport/
- 2: https://github.com/Coldcard/firmware/blob/master/docs/key-teleport.md
- 3: https://keyteleport.com/
- 4: https://blog.coinkite.com/ccc-and-keyteleport/
- 5: https://bips.xyz/32
🏁 Script executed:
# Find the payload.rs file and examine its structure
find . -name "payload.rs" -type f | head -20Repository: bitcoinppl/cove
Length of output: 201
🏁 Script executed:
# Search for the Payload type definition and related methods
rg "enum Payload|impl.*Payload" -A 10 -B 2 --type rustRepository: bitcoinppl/cove
Length of output: 3711
🏁 Script executed:
# Check the from_bytes method to understand the reverse operation
rg "from_bytes|from_slice" -B 5 -A 15 --type rust | head -100Repository: bitcoinppl/cove
Length of output: 5942
🏁 Script executed:
# Examine how bitcoin::base58 is used throughout the codebase
rg "bitcoin::base58::" --type rust -B 2 -A 2Repository: bitcoinppl/cove
Length of output: 996
🏁 Script executed:
# Look for TYPE_XPRV constant and validation logic
rg "TYPE_XPRV" -B 5 -A 5 --type rustRepository: bitcoinppl/cove
Length of output: 2818
🏁 Script executed:
# Check bitcoin crate functions for base58 methods
rg "decode_check|encode_check" --type rustRepository: bitcoinppl/cove
Length of output: 41
🏁 Script executed:
# Look for how Payload::Xprv is constructed and if there's any validation
rg "Payload::Xprv" -B 3 -A 3 --type rustRepository: bitcoinppl/cove
Length of output: 1297
🏁 Script executed:
# Check the full payload.rs file to understand construction and validation
cat -n rust/crates/cove-keyteleport/src/payload.rsRepository: bitcoinppl/cove
Length of output: 3844
🏁 Script executed:
# Search for the bitcoin crate's base58 module to understand available functions
rg "pub.*fn.*(decode|encode)" --type rust | grep -i base58Repository: bitcoinppl/cove
Length of output: 41
🏁 Script executed:
# Check the Cargo.toml for the crate to see what bitcoin dependency is used
cat rust/crates/cove-keyteleport/Cargo.tomlRepository: bitcoinppl/cove
Length of output: 605
🏁 Script executed:
# Look for bitcoin crate usage to see what base58 functions are available
fd "Cargo.lock" | xargs grep -A 5 "bitcoin" | head -50Repository: bitcoinppl/cove
Length of output: 723
🏁 Script executed:
# Search for any imports or usage of decode_check or encode_check
rg "decode_check|encode_check|CheckedDecode" --type rust -B 2 -A 2Repository: bitcoinppl/cove
Length of output: 41
🏁 Script executed:
# Search for any usage of base58 functions with more context
rg "bitcoin::base58" -B 5 -A 5 --type rustRepository: bitcoinppl/cove
Length of output: 1950
🏁 Script executed:
# Check if there are any extended key validation functions
rg "ExtendedPrivKey|ExtendedPubKey|decode_check" --type rust -B 2 -A 2Repository: bitcoinppl/cove
Length of output: 462
🏁 Script executed:
# Look for the bitcoin crate documentation or type signatures
find . -name "*.rs" -path "*/bitcoin/*" -o -name "lib.rs" | head -10Repository: bitcoinppl/cove
Length of output: 424
🏁 Script executed:
# Search for any xprv validation patterns in the codebase
rg "xprv|tprv" -i --type rust -B 3 -A 3 | head -60Repository: bitcoinppl/cove
Length of output: 3489
🏁 Script executed:
# Check what functions bitcoin::base58 module exports
rg "pub.*fn" --type rust | grep -i base58 | head -20Repository: bitcoinppl/cove
Length of output: 41
🏁 Script executed:
# Look for base58ck crate usage to understand checksum functions
rg "base58ck|CheckedDecode" --type rust -B 2 -A 2Repository: bitcoinppl/cove
Length of output: 41
🏁 Script executed:
# Web search for bitcoin crate 0.32.8 base58 documentationRepository: bitcoinppl/cove
Length of output: 41
🌐 Web query:
bitcoin rust crate 0.32.8 base58 module API decode encode functions
💡 Result:
The bitcoin Rust crate version 0.32.8 uses the base58ck crate (version 0.1.0) for Base58 encoding and decoding functionality, specifically Bitcoin base58 with checksum support (base58check). This is a direct dependency as shown in the Cargo.toml: base58 = { package = "base58ck", version = "0.1.0", default-features = false }. The base58 module (from base58ck) provides the following key API functions for encode/decode: - encode(data: &[u8]) -> String: Encodes bytes as a base58 string (no checksum). - encode_check(data: &[u8]) -> String: Encodes bytes as a base58check string (includes checksum). - encode_check_to_fmt(data: &[u8], f: &mut impl Write): Encodes bytes as base58check into a formatter (no allocation for string). - decode(s: &str) -> Result<Vec, base58ck::error::Error>: Decodes base58 string to bytes (no checksum verification). - decode_check(s: &str) -> Result<Vec, base58ck::error::Error>: Decodes base58check string to bytes, verifying the checksum. Usage example in the bitcoin crate context (re-exported or directly via base58ck): use base58ck::{encode_check, decode_check}; let data = b"hello bitcoin"; let encoded = encode_check(data); // e.g., "2NEpo7TZRRrMAbN2R5fBj3D2iVPvbhVgdM" let decoded = decode_check(&encoded).unwrap; assert_eq!(data.to_vec, decoded); The crate supports no-std with alloc and uses bitcoin_hashes for checksum calculation (double SHA256). Errors include InvalidCharacterError for invalid chars and checksum mismatch. Official docs: https://docs.rs/base58ck/0.1.0/base58ck (functions listed there). Repository: https://github.com/rust-bitcoin/rust-bitcoin/tree/master/base58.
Citations:
- 1: https://docs.rs/crate/bitcoin/latest
- 2: https://docs.rs/crate/bitcoin/^0.32.8
- 3: https://docs.rs/crate/bitcoin/latest/source/Cargo.toml.orig
- 4: https://crates.io/crates/base58ck
- 5: https://crates.io/crates/base58ck/0.1.0
- 6: https://docs.rs/base58ck/latest/base58ck
- 7: https://docs.rs/base58ck/latest/base58ck/index.html
- 8: https://docs.rs/base58ck/^0.1.0
- 9: https://docs.rs/base58ck
- 10: https://github.com/rust-bitcoin/rust-bitcoin/tree/master/base58
- 11: https://docs.rs/bitcoin/latest/bitcoin
🌐 Web query:
BIP-32 XPRV base58check encoding specification standard
💡 Result:
The BIP-32 specification defines the Base58Check encoding for extended private keys (XPRV) as follows: Serialization format (78-byte payload): - 4 bytes: version bytes (mainnet private: 0x0488ADE4; testnet private: 0x04358394) - 1 byte: depth (0x00 for master nodes, 0x01 for level-1 derived keys, etc.) - 4 bytes: fingerprint of the parent key (0x00000000 for master key) - 4 bytes: child number (32-bit unsigned integer, most significant byte first; 0x00000000 for master key) - 32 bytes: chain code - 33 bytes: private key data (0x00 || 32-byte private key serialized as big-endian) Encoding process: 1. Append 4-byte checksum (first 4 bytes of double SHA-256 hash of the 78-byte payload). 2. Encode the resulting 82-byte structure using Base58 alphabet. 3. Results in a 111-character string starting with "xprv" (mainnet) or "tprv" (testnet). This is the standard from BIP-32 (Final status). Note that some wallets (e.g., Electrum) use alternative version bytes for SegWit compatibility (yprv, zprv), but these are extensions beyond core BIP-32.
Citations:
- 1: https://github.com/Bitcoin/bips/blob/master/bip-0032.mediawiki
- 2: https://bips.xyz/32
- 3: https://bips.dev/32/
- 4: https://bips.dev/32
- 5: https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki
XPRV payload serialization uses plain base58 instead of base58check, creating both a spec violation and silent corruption path.
The Key Teleport spec requires XPRV payloads to use Base58Check encoding (per BIP-32), which includes a 4-byte checksum. The current code violates this:
to_bytes: callsbitcoin::base58::decode(xprv)with a silent fallback to raw UTF-8 bytes on decode failure. This is a critical security issue: a corrupted XPRV string will silently become UTF-8 bytes, then get re-encoded on the receiver side as a valid-looking but completely different XPRV with no error raised.from_bytes: callsbitcoin::base58::encode(bin)without checksum, meaning received payloads are never validated against a checksum.
Replace both with bitcoin::base58::decode_check and bitcoin::base58::encode_check, which are available in the bitcoin crate (v0.32.8). Remove the fallback entirely—if the XPRV is invalid, that's a real error that should propagate. Alternatively, validate the XPRV at Payload::Xprv construction time so this branch is unreachable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/crates/cove-keyteleport/src/payload.rs` around lines 30 - 38, The XPRV
branch (Payload::Xprv) currently uses bitcoin::base58::decode with a silent
unwrap_or_else fallback and plain base58 encoding on serialize/deserialize;
change both sides to use bitcoin::base58::decode_check and
bitcoin::base58::encode_check and remove the UTF-8 fallback so invalid
Base58Check XPRV strings surface as errors; update the code paths (the
Payload::Xprv handling in to_bytes/from_bytes or the functions that
serialize/deserialize this variant and any use of TYPE_XPRV) to propagate or
return the decoding/encoding error instead of silently converting to raw bytes
(or alternatively validate XPRV when constructing the Payload::Xprv so this
branch never receives invalid input).
| #[test] | ||
| fn wrong_numeric_code_gives_error() { | ||
| let receiver = ReceiverSession::generate(); | ||
| let r_pkt = receiver.to_packet(); | ||
| let wrong_code = (receiver.numeric_code() + 1) % 100_000_000; | ||
| // With overwhelming probability, wrong key → invalid pubkey bytes → error | ||
| let result = SenderSession::new(&r_pkt, wrong_code); | ||
| assert!(result.is_err()); | ||
| } |
There was a problem hiding this comment.
Flaky test: wrong_numeric_code_gives_error can spuriously pass.
With a wrong AES key, the 33 decrypted bytes are effectively random. PublicKey::from_slice will accept the result if the first byte is 0x02/0x03 AND the remaining 32 bytes correspond to a valid x‑coordinate on secp256k1 — roughly a ~0.4% success rate. In CI this will occasionally flake.
Either make the test deterministic, or loop a few trials and assert that at least one fails, or run many wrong codes and assert a high rejection rate. A simple fix:
💡 Suggested fix
- #[test]
- fn wrong_numeric_code_gives_error() {
- let receiver = ReceiverSession::generate();
- let r_pkt = receiver.to_packet();
- let wrong_code = (receiver.numeric_code() + 1) % 100_000_000;
- // With overwhelming probability, wrong key → invalid pubkey bytes → error
- let result = SenderSession::new(&r_pkt, wrong_code);
- assert!(result.is_err());
- }
+ #[test]
+ fn wrong_numeric_code_usually_errors() {
+ // ~0.4% of random 33-byte strings parse as valid compressed pubkeys,
+ // so assert a high-but-not-100% rejection rate across many trials.
+ let mut failures = 0usize;
+ const N: usize = 200;
+ for i in 0..N {
+ let receiver = ReceiverSession::generate();
+ let r_pkt = receiver.to_packet();
+ let wrong = receiver.numeric_code().wrapping_add(i as u32 + 1) % 100_000_000;
+ if SenderSession::new(&r_pkt, wrong).is_err() {
+ failures += 1;
+ }
+ }
+ assert!(failures > N * 90 / 100, "expected ≥90% rejection, got {failures}/{N}");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[test] | |
| fn wrong_numeric_code_gives_error() { | |
| let receiver = ReceiverSession::generate(); | |
| let r_pkt = receiver.to_packet(); | |
| let wrong_code = (receiver.numeric_code() + 1) % 100_000_000; | |
| // With overwhelming probability, wrong key → invalid pubkey bytes → error | |
| let result = SenderSession::new(&r_pkt, wrong_code); | |
| assert!(result.is_err()); | |
| } | |
| #[test] | |
| fn wrong_numeric_code_usually_errors() { | |
| // ~0.4% of random 33-byte strings parse as valid compressed pubkeys, | |
| // so assert a high-but-not-100% rejection rate across many trials. | |
| let mut failures = 0usize; | |
| const N: usize = 200; | |
| for i in 0..N { | |
| let receiver = ReceiverSession::generate(); | |
| let r_pkt = receiver.to_packet(); | |
| let wrong = receiver.numeric_code().wrapping_add(i as u32 + 1) % 100_000_000; | |
| if SenderSession::new(&r_pkt, wrong).is_err() { | |
| failures += 1; | |
| } | |
| } | |
| assert!(failures > N * 90 / 100, "expected ≥90% rejection, got {failures}/{N}"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/crates/cove-keyteleport/src/sender.rs` around lines 118 - 126, The test
wrong_numeric_code_gives_error is flaky because random decrypted bytes can
sometimes form a valid PublicKey; change the test to try multiple wrong codes
deterministically: call ReceiverSession::generate(), get r_pkt via to_packet(),
then loop a fixed number of different wrong codes (e.g. 50) and call
SenderSession::new(&r_pkt, wrong_code) for each, collecting results from
SenderSession::new (which invokes PublicKey::from_slice); finally assert that at
least one attempt returned Err (or assert a high rejection rate) to avoid
spurious passes while keeping the test deterministic and tied to
numeric_code/to_packet/SenderSession::new.
…xprv) ECDH session key, AES-256-CTR double-encryption, PBKDF2 stretch, BBQr R/S packet encoding, ReceiverSession and SenderSession. 25 tests, all passing.
praveenperera
force-pushed
the
feat-653-keyteleport-crate
branch
from
d32a1a5 to
526555a
Compare
| impl KtFileType { | ||
| pub fn as_char(self) -> char { |
There was a problem hiding this comment.
I don't like abbreviated variable names and especially not type names, just use KeyTeleportFileType here
| /// Encode binary data as a single-frame BBQr string with the given Key Teleport file type. | ||
| pub fn encode(data: &[u8], file_type: KtFileType) -> String { | ||
| let b32 = BASE32_NOPAD.encode(data); | ||
| // num_parts=01 (1 frame total), part_index=00 (first/only frame) | ||
| format!("B$2{}0100{}", file_type.as_char(), b32) | ||
| } | ||
|
|
||
| /// Decode a single-frame BBQr string, returning the file type and binary payload. | ||
| /// Multi-frame packets are rejected — higher-level transport code handles reassembly. | ||
| pub fn decode(s: &str) -> Result<(KtFileType, Vec<u8>), Error> { | ||
| let s = s.trim().to_uppercase(); | ||
|
|
||
| let rest = | ||
| s.strip_prefix("B$").ok_or_else(|| Error::InvalidBbqr("missing 'B$' header".into()))?; | ||
|
|
||
| if rest.len() < 6 { | ||
| return Err(Error::InvalidBbqr("too short to be a valid BBQr packet".into())); | ||
| } | ||
|
|
||
| let mut chars = rest.chars(); | ||
| let encoding = chars.next().unwrap(); | ||
| if encoding != '2' { | ||
| return Err(Error::InvalidBbqr(format!( | ||
| "unsupported encoding '{encoding}' (only Base32/'2' is supported)" | ||
| ))); | ||
| } | ||
|
|
||
| let file_type = KtFileType::from_char(chars.next().unwrap())?; | ||
|
|
||
| // num_parts and part_index are 2 uppercase hex chars each | ||
| let header_tail: String = chars.take(4).collect(); | ||
| if header_tail.len() != 4 { | ||
| return Err(Error::InvalidBbqr("truncated header".into())); | ||
| } | ||
| let num_parts = u8::from_str_radix(&header_tail[0..2], 16) | ||
| .map_err(|_| Error::InvalidBbqr("bad num_parts".into()))?; | ||
| let part_index = u8::from_str_radix(&header_tail[2..4], 16) | ||
| .map_err(|_| Error::InvalidBbqr("bad part_index".into()))?; | ||
|
|
||
| if num_parts != 1 || part_index != 0 { | ||
| return Err(Error::InvalidBbqr(format!( | ||
| "multi-frame BBQr not supported here (num_parts={num_parts}, part_index={part_index})" | ||
| ))); | ||
| } | ||
|
|
||
| let b32_data = &s[8..]; // "B$" + encoding + type + 4 header chars = 8 | ||
| let data = BASE32_NOPAD | ||
| .decode(b32_data.as_bytes()) | ||
| .map_err(|e| Error::InvalidBbqr(format!("Base32 decode failed: {e}")))?; | ||
|
|
||
| Ok((file_type, data)) | ||
| } |
There was a problem hiding this comment.
we should use the bbqr here, we are already using it in the app
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
rust/crates/cove-keyteleport/src/crypto.rs (1)
65-121: Tests are reasonable butsession_key_is_symmetriccan panic on bad luck.
SecretKey::from_slice(&random_bytes).unwrap()will panic if the 32 random bytes happen to be zero or ≥ curve order. The probability is astronomically small, but if you want a truly flake-proof test, generate via a retry loop orSecretKey::new(&mut rand::rng())equivalent. Not a blocker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-keyteleport/src/crypto.rs` around lines 65 - 121, The session_key_is_symmetric test currently uses SecretKey::from_slice(&random_bytes).unwrap(), which can panic if the random 32 bytes are invalid; modify the test (session_key_is_symmetric) to create valid secret keys deterministically by retrying until SecretKey::from_slice succeeds or by using the SecretKey::new / appropriate RNG constructor to generate keys (e.g., loop generating bytes and call SecretKey::from_slice until Ok or call SecretKey::new(&mut rng)) before deriving public keys and calling session_key(&sk_a, &pk_b) and session_key(&sk_b, &pk_a) to assert equality.rust/crates/cove-keyteleport/src/sender.rs (2)
40-45: Minor: clearer rejection‑sampling loop.The current pattern validates the bytes, then re-parses them with
.expect(...). You can collapse into one parse per iteration and avoid the final redundantfrom_slice:♻️ Proposed refactor
- let mut key_bytes = [0u8; 32]; - rand::rng().fill(&mut key_bytes); - while SecretKey::from_slice(&key_bytes).is_err() { - rand::rng().fill(&mut key_bytes); - } - let privkey = SecretKey::from_slice(&key_bytes).expect("validated above"); + let (privkey, key_bytes) = loop { + let mut bytes = [0u8; 32]; + rand::rng().fill(&mut bytes); + if let Ok(sk) = SecretKey::from_slice(&bytes) { + break (sk, bytes); + } + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-keyteleport/src/sender.rs` around lines 40 - 45, The rejection‑sampling loop currently fills key_bytes, checks SecretKey::from_slice(&key_bytes).is_err(), and then calls SecretKey::from_slice(&key_bytes).expect(...) again; replace this with a single loop that attempts to parse into a SecretKey each iteration (e.g. call SecretKey::from_slice(&key_bytes) and if Ok(sk) break returning sk, otherwise refill key_bytes and retry) so you only parse once per sample and assign directly to privkey (referencing key_bytes, rand::rng().fill, SecretKey::from_slice, and privkey).
81-100: Optional: hoistSecp256k1::new()out ofencrypt.
Secp256k1::new()allocates a full context (precomputation tables) each call. Ifencryptis ever invoked repeatedly, consider usingSecp256k1::signing_only()or a thread-local/global verification context, or passing an existing context in. For the current single-encrypt-per-session usage this is inconsequential.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-keyteleport/src/sender.rs` around lines 81 - 100, The call to Secp256k1::new() in encrypt allocates a full context each invocation; change this to reuse a context by using Secp256k1::signing_only() or by accepting/holding a shared context instead of creating it per call: update the Sender struct (or the encrypt signature) to store or receive a Secp256k1 context and replace the local Secp256k1::new() in encrypt with that reused context so repeated calls avoid expensive reallocation while still obtaining the sender_pubkey via privkey().public_key(&secp).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/crates/cove-keyteleport/src/sender.rs`:
- Around line 14-19: Wrap the teleport_password field in SenderSession with
Zeroizing<String> instead of plain String so its contents are zeroed on drop;
update any accessors (e.g., the teleport_password() method) to return &str via
Deref so existing callers continue to use self.teleport_password.as_str() or
self.teleport_password.as_bytes() (the PBKDF2 usage should continue to work);
ensure you import/use zeroize::Zeroizing and update any
constructors/initializers to wrap the created password string in
Zeroizing::new(...).
- Around line 62-64: The current privkey() recreates and returns a SecretKey by
value from privkey_bytes, leaving unzeroized copies in memory; instead, wrap the
secret in a zeroizing container at construction (use zeroize::Zeroizing) and
store the secret inside the struct (e.g., change privkey_bytes -> Zeroizing<[u8;
32]> or better: store Zeroizing<secp256k1::SecretKey>), then change privkey() to
avoid returning a new SecretKey by value — return a reference (&SecretKey or
&Zeroizing<SecretKey>) or otherwise thread the single stored SecretKey through
callers so no fresh SecretKey is reconstructed on each call; if you must
rebuild, do it inside a Zeroizing temporary and consume it immediately so the
buffer is zeroed.
---
Nitpick comments:
In `@rust/crates/cove-keyteleport/src/crypto.rs`:
- Around line 65-121: The session_key_is_symmetric test currently uses
SecretKey::from_slice(&random_bytes).unwrap(), which can panic if the random 32
bytes are invalid; modify the test (session_key_is_symmetric) to create valid
secret keys deterministically by retrying until SecretKey::from_slice succeeds
or by using the SecretKey::new / appropriate RNG constructor to generate keys
(e.g., loop generating bytes and call SecretKey::from_slice until Ok or call
SecretKey::new(&mut rng)) before deriving public keys and calling
session_key(&sk_a, &pk_b) and session_key(&sk_b, &pk_a) to assert equality.
In `@rust/crates/cove-keyteleport/src/sender.rs`:
- Around line 40-45: The rejection‑sampling loop currently fills key_bytes,
checks SecretKey::from_slice(&key_bytes).is_err(), and then calls
SecretKey::from_slice(&key_bytes).expect(...) again; replace this with a single
loop that attempts to parse into a SecretKey each iteration (e.g. call
SecretKey::from_slice(&key_bytes) and if Ok(sk) break returning sk, otherwise
refill key_bytes and retry) so you only parse once per sample and assign
directly to privkey (referencing key_bytes, rand::rng().fill,
SecretKey::from_slice, and privkey).
- Around line 81-100: The call to Secp256k1::new() in encrypt allocates a full
context each invocation; change this to reuse a context by using
Secp256k1::signing_only() or by accepting/holding a shared context instead of
creating it per call: update the Sender struct (or the encrypt signature) to
store or receive a Secp256k1 context and replace the local Secp256k1::new() in
encrypt with that reused context so repeated calls avoid expensive reallocation
while still obtaining the sender_pubkey via privkey().public_key(&secp).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7f4295d2-59f3-469c-a2ef-c8e0a3b683bc
⛔ Files ignored due to path filters (1)
rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
rust/Cargo.tomlrust/crates/cove-keyteleport/Cargo.tomlrust/crates/cove-keyteleport/src/bbqr.rsrust/crates/cove-keyteleport/src/crypto.rsrust/crates/cove-keyteleport/src/error.rsrust/crates/cove-keyteleport/src/lib.rsrust/crates/cove-keyteleport/src/packet.rsrust/crates/cove-keyteleport/src/payload.rsrust/crates/cove-keyteleport/src/receiver.rsrust/crates/cove-keyteleport/src/sender.rs
✅ Files skipped from review due to trivial changes (1)
- rust/crates/cove-keyteleport/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (7)
- rust/Cargo.toml
- rust/crates/cove-keyteleport/src/error.rs
- rust/crates/cove-keyteleport/src/lib.rs
- rust/crates/cove-keyteleport/src/payload.rs
- rust/crates/cove-keyteleport/src/bbqr.rs
- rust/crates/cove-keyteleport/src/receiver.rs
- rust/crates/cove-keyteleport/src/packet.rs
| fn privkey(&self) -> SecretKey { | ||
| SecretKey::from_slice(&self.privkey_bytes[..]).expect("stored key is always valid") | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does bitcoin::secp256k1::SecretKey (secp256k1 0.29 used by rust-bitcoin 0.32) implement Drop with zeroization of the secret bytes?
💡 Result:
No, bitcoin::secp256k1::SecretKey from rust-secp256k1 0.29 (used by rust-bitcoin 0.32) does not implement Drop with automatic zeroization of the secret bytes.
Citations:
- 1: zeroize secretkey on drop rust-bitcoin/rust-secp256k1#165
- 2: https://docs.rs/secp256k1/latest/secp256k1/struct.SecretKey.html
- 3: https://docs.rs/rs-secp256k1/latest/secp256k1/key/struct.SecretKey.html
- 4: https://github.com/rust-bitcoin/rust-secp256k1
- 5: Zeroing out secrets rust-bitcoin/rust-secp256k1#102
- 6: Add feature zeroize rust-bitcoin/rust-secp256k1#262
- 7: implement
insecure-erasefeature (was:Zeroize) rust-bitcoin/rust-secp256k1#582
privkey() returns an unzeroized SecretKey — critical security issue.
Each call rebuilds a fresh SecretKey from privkey_bytes and returns it by value. bitcoin::secp256k1::SecretKey does NOT implement Drop with automatic zeroization, so secret key bytes remain in memory until garbage collected. This creates a critical security vulnerability when handling Bitcoin private keys.
Do not use this pattern. Instead:
- Wrap
privkey_bytesinZeroizingat construction time and avoid rebuilding theSecretKeyon each call - If rebuilding is necessary, use the
secp256k1::SecretKey::into()or explore zeroizing alternatives from thezeroizecrate - Thread the
SecretKeythrough functions rather than re-parsing on demand to minimize exposure window and intermediate copies in memory
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust/crates/cove-keyteleport/src/sender.rs` around lines 62 - 64, The current
privkey() recreates and returns a SecretKey by value from privkey_bytes, leaving
unzeroized copies in memory; instead, wrap the secret in a zeroizing container
at construction (use zeroize::Zeroizing) and store the secret inside the struct
(e.g., change privkey_bytes -> Zeroizing<[u8; 32]> or better: store
Zeroizing<secp256k1::SecretKey>), then change privkey() to avoid returning a new
SecretKey by value — return a reference (&SecretKey or &Zeroizing<SecretKey>) or
otherwise thread the single stored SecretKey through callers so no fresh
SecretKey is reconstructed on each call; if you must rebuild, do it inside a
Zeroizing temporary and consume it immediately so the buffer is zeroed.
Per mentor review on PR bitcoinppl#676: avoid abbreviated type names. Also wraps teleport_password in Zeroizing<String> so the password is zeroed from memory when the SenderSession is dropped.
Replace hardcoded '2' byte checks with bbqr::encode::Encoding::from_byte() and Encoding::Base32.as_byte() so encoding logic stays in sync with the bbqr crate rather than duplicating the byte mapping.
praveenperera
left a comment
praveenperera
left a comment
There was a problem hiding this comment.
why are we not using bbqr crate here either?
2 participants
Create the internal
cove-keyteleportworkspace crate underrust/crates/cove-keyteleportwith the pure-Rust Key Teleport cryptographic primitives needed for Cove to participate as a real sender and receiver in non-multisig flows.The crate owns the full protocol-layer logic per the COLDCARD spec: ECDH session key (SHA256 of X‖Y), AES-256-CTR double-encryption with zero IV, 2-byte SHA256 checksum, PBKDF2-SHA512 (5000 iter) inner-key derivation, numeric-code pubkey encryption, and 8-char Base32 teleport-password generation. Single-frame BBQr
RandSpacket encoding/decoding is included so the send and receive flows (#654, #655) have everything they need.Exposes
ReceiverSession,SenderSession,Payload(mnemonic/xprv),ReceiverPacket, andSenderPacket. No UniFFI surface, no UI, no persistence — just the reusable primitives.25 tests: unit tests per module, full roundtrips (12- and 24-word mnemonic), BBQr transport roundtrip, wrong-password checksum failure, wrong numeric code failure, and the known keyteleport.com example vector.
Closes #653
Summary by CodeRabbit