Skip to content

keep-mobile: hardening & test-coverage backlog (29 items) #792

Description

@kwsantiago

Backlog of hardening / test-coverage findings in keep-mobile, verified still-open against current code by an automated triage pass. Each is an independent, pickup-able task — pick one and open a PR.

Findings

  • do_start_bunker local-vs-network branch selection untested — The do_start_bunker local_complete branch selection (nip46.rs:553-600) still has no test; frost_signer.rs tests only exercise FrostSigner sign/verify on a 1-of-1 share, not the branch decision.
  • From for KeepMobileError maps to FrostError, now inconsistent with nip46_to_mobile_error — From for KeepMobileError still maps everything to FrostError (keep-mobile/src/error.rs:114-116) while nip46_to_mobile_error maps per-variant (lib.rs:2367); the two inconsistent mappers still coexist.
  • nip46_to_mobile_error drops Timeout/CapacityExceeded/UserRejected semantics — nip46_to_mobile_error now maps Timeout and RateLimited (keep-mobile/src/lib.rs:2371-2372), but UserRejected and CapacityExceeded still fall through to the stringified NetworkError arm (lib.rs:2378), so those two variants' semantics are still lost.
  • did_backup always forced to true on backup restore — restore_backup still hard-codes did_backup: true for both shares and keys, overriding the backup's original value (keep-mobile/src/lib.rs:2145,2157,2192).
  • Deserialized audit entries not re-validated before export — export_json still converts loaded entries to export format without any chain-integrity verification and carries no by-design note (keep-mobile/src/audit.rs:311-320).
  • get_seed_words creates non-zeroizing copy of mnemonic — The extra bytes.to_vec() copy is gone but get_seed_words still returns a non-zeroizing String via s.to_owned() (keep-mobile/src/lib.rs:1047); no Zeroizing internal handling.
  • mnemonic lost on restore from backup — restore_backup still hardcodes plaintext_mnemonic: None (keep-mobile/src/lib.rs:2168) and create_backup's BackupShare never carries the mnemonic, so seed words are still lost across a backup/restore cycle.
  • get_seed_words returns unzeroized String across FFI — get_seed_words still returns Option across the UniFFI boundary (keep-mobile/src/lib.rs:1031) which the foreign caller cannot zeroize; the inherent un-zeroizable return remains.
  • plaintext mnemonic persisted to storage without encryption — plaintext_mnemonic is still serialized as plaintext bytes to SecureStorage with no application-layer encryption (keep-mobile/src/lib.rs:293); only a documenting comment was added, behavior unchanged.
  • Mnemonic copy taken before zeroize may leave residual in allocator — The described pattern is unchanged: mnemonic_bytes = Zeroizing::new(mnemonic.as_bytes().to_vec()) taken before mnemonic.zeroize() at keep-mobile/src/lib.rs:734-737 (informational; finding itself deems it acceptable).
  • NIP-49 export_ncryptsec missing round-trip test — export_ncryptsec (keep-mobile/src/lib.rs:859) still has no round-trip test; grep finds no test invoking export_ncryptsec anywhere in the keep-mobile crate.
  • Mobile fails node init on session store error, desktop degrades gracefully — Mobile still propagates FileDescriptorSessionStore::new failure with map_err(...)? at keep-mobile/src/lib.rs:2656-2661, failing node init, while desktop degrades gracefully at keep-desktop/src/frost.rs:319-327.
  • now_ms as u64 truncation should use try_into — now_ms() at keep-mobile/src/signing_policy.rs:224 still uses .as_millis() as u64 rather than a checked try_from conversion.
  • Duplicate rate limiting with existing VelocityTracker — SigningRateLimiter (signing_policy.rs) and VelocityTracker (velocity.rs) both still exist unconsolidated, though they track different domains (per-package request counts vs Bitcoin spend amounts), so the premise of true duplication is weak.
  • No cert pinning on BunkerHandler relay connections — do_start_bunker still only calls validate_relay_url and never verify_relay_certificate; cert pinning runs only in do_initialize (nip46.rs:500-502 vs lib.rs:2595).
  • No rate limiting on frost_generate and frost_split operations — keep-mobile/src/lib.rs:1049 frost_generate and :1066 frost_split still perform key generation with no rate limiting or concurrency guard.
  • TrustedDealer warning not exposed through UniFFI API — keep-mobile/src/lib.rs:1049 frost_generate still calls TrustedDealer::new with no doc comment or warning surfaced in the UniFFI API; the testing-only warning lives only in keep-core/src/frost/dealer.rs:60.
  • No validation of entry timestamp ordering — keep-mobile/src/audit.rs:83 still uses chrono::Utc::now().timestamp() with no ordering validation in log_event (line 243).
  • Timing side-channel in length checks before constant-time comparison — verify() at keep-mobile/src/audit.rs:124-127 still performs early-return length checks before ct_eq with no documenting note that it is not fully constant-time.
  • Hash chain can be bypassed by malicious storage — AuditStorage trait (keep-mobile/src/audit.rs:179-186) remains foreign-implemented; no signed entries or attestation added, so malicious storage can still drop/tamper entries.
  • Potential sensitive data exposure in details field — details is bounded to 4096 chars (keep-mobile/src/audit.rs:104) but truncation does not prevent a <4096-char secret from being logged/exported and no no-secrets doc was added.
  • Inconsistent error handling silently hides failures in get_active_share — get_active_share still returns Option and silently converts errors to None via ?/.ok()?, so storage corruption is indistinguishable from 'no active share' (keep-mobile/src/lib.rs:893-896, 3449-3453).
  • Callback URL allows https:// scheme without validation — is_safe_callback_url still allows arbitrary https:// URLs via a bare starts_with check with no domain allowlisting or validation (keep-mobile/src/nip55.rs:888-892).
  • Rate limit HashMap unbounded growth potential — evict_stale_entries only removes entries with no recent requests above the 1000 threshold with no hard cap, so a flood of many unique still-recent caller_ids grows the HashMap unbounded (keep-mobile/src/nip55.rs:782-795).
  • Hardcoded x-only pubkey prefix assumption — parse_pubkey_to_compressed still hardcodes compressed[0]=0x02 for 32-byte x-only keys with no documenting comment or on-curve validation as the finding requested (keep-mobile/src/nip55.rs:952-956).
  • Hardcoded even-y prefix in parse_pubkey_to_compressed — parse_pubkey_to_compressed still hardcodes compressed[0] = 0x02 with no explanatory documentation at nip55.rs:954, exactly the undocumented even-y assumption the finding cites.
  • Missing rate limiting on signing requests — The FROST peer signing path still only caps the queue via MAX_PENDING_REQUESTS at lib.rs:3219 with no per-peer rate limiting or backoff; the new SigningRateLimiter guards only the NIP-55/Android caller path, not this network path.
  • Plaintext mnemonic persisted in mobile storage — keep-mobile/src/lib.rs:293 still declares plaintext_mnemonic: Option<Zeroizing<Vec>> in StoredShareData serialized to SecureStorage; the residual was downgraded to P3 but not fixed at the Rust layer.
  • Missing authorization check for clear_entries() — keep-mobile/src/audit.rs:347 clear_entries still only forwards a confirm string with no authorization/authentication check; downgraded to P3 in triage but the trait-level defect remains in code.

Grouped from individual tracked items; close each checkbox as its PR merges.

Metadata

Metadata

Assignees

No one assigned

    Labels

    mobilekeep-mobile UniFFI bindingsp2PrioritysecuritySecurity-related issues

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions