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.

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

[Feature] Implement background wallet sync via view keys on Monerujo #785

Open
sethforprivacy opened this issue Sep 30, 2021 · 5 comments
Open

Comments

@sethforprivacy
Copy link

A bounty has been opened for this feature request, with funding being provided via donations by the Monero community:

https://bounties.monero.social/posts/10/implement-background-wallet-sync-via-view-keys-on-monerujo

Copying description here from that bounty:

The current user experience on mobile wallets without light-wallet servers is very poor unless a user regularly opens his wallet to allow it to sync.

A strong solution for this would be to allow mobile wallets to perform background syncs (especially when on wifi/plugged in, like at night) using only view keys, so no spend keys are kept "hot" without the user's active consent/auth.

Once this is implemented, the user can expect to at most have to sync <1d of blocks before spending.

Payout criteria:

  • Wallet is able to sync up in the background, without the wallet open
  • User can specify to only sync on wifi and/or when plugged in
  • Code is open-sourced
@hyc
Copy link

hyc commented Nov 25, 2021

Copying a comment from the bounty:

Some technical issues to keep in mind: the wallet cache is currently encrypted with a secret passphrase. It is risky to keep the wallet cache in decrypted state long term, and it must be in decrypted state to update it.

It seems to me the only safe way to implement this is by introducing a separate background-sync-cache. That can be encrypted with a separate symmetric encryption key. The background sync only needs the private viewkey, so overall this information is slightly less sensitive and so this separate encryption key could be safe to maintain in memory longterm.

When the wallet runs again in foreground and the passphrase is entered to unlock the main wallet cache, it can then decrypt and import the background-sync-cache at that time.

@hyc
Copy link

hyc commented Nov 25, 2021

For the record, I think the base functionality belongs in the core Monero libwallet source, and so this feature request is in the wrong repo. Other wallet UIs can then wrap whatever user interface around it as desired.

@m2049r
Copy link
Owner

m2049r commented Nov 26, 2021

For the record, I think the base functionality belongs in the core Monero libwallet source, and so this feature request is in the wrong repo. Other wallet UIs can then wrap whatever user interface around it as desired.

I agree 100% - this is a cross-wallet core feature. Any monerujo-specific implementation will be just a hack introducing a bunch of unnecessary attack vectors in addition to bad UX.

Thank you @hyc for opening monero-project/monero#8082

@j-berman
Copy link
Contributor

Hitting a wall on the core libwallet implementation. I have little Android dev experience, so I'm probably misunderstanding things as a result.

Monerujo already implements a way to keep scanning in the background even while the user is not using the app; AFAIU it achieves this by keeping the wallet open in a foreground service. It seems like this can be done more securely via (approach 1) a background sync mode that wipes the spend key from memory and scans with just the view key, after a user has already input their password. Notably, adding this to the core libwallet code would not enable an improved UX in Monerujo, only a plausible security benefit.

However, @m2049r noted to me this may not be of much of a security benefit anyway, since strings in Java are immutable and can't be wiped from memory with certainty; everywhere strings are used for sensitive key material would therefore still be an open vector. Strings can be replaced with char arrays that are explicitly overwritten with 0's, but this still wouldn't be perfect if e.g. the char array is swapped out to disk and doesn't get garbage collected. So the security benefit of a background sync mode that wipes the spend key from memory would seem to be marginal (but still, non-0).

There is another way this background sync seems it can be implemented in libwallet that could yield a UX benefit at cost of security: (approach 2) leaving the view key decrypted on disk always. This way even if the app is closed, it seems the device can feasibly still trigger and run the background sync process without the user needing to input their password. Keep in mind I'm also leaning toward keeping the wallet cache decrypted and accessible as well, as explained here.

I feel at an impasse:

  1. Does it still make sense to go with approach 1 (wipe the spend key from memory) even though the security benefit may be marginal?
  2. Perhaps it make sense to instead go with approach 2 where the view key is always decrypted on disk, at cost of security?

Feedback would be appreciated

@j-berman
Copy link
Contributor

j-berman commented Mar 26, 2022

Learning as I go... (sorry for the walls of text)

I'm planing to proceed with Approach 1 in libwallet (wiping the spend key from memory). I think it is still a reasonable decision to wipe the spend key from memory, even if the password isn't wiped perfectly in Java. I also think it would pair well with #823: upon triggering an inactivity timeout, the app could call a wallet.enableBackgroundSyncMode(), which wipes the spend key from memory (and the wallet will continue scanning with just view key in the background). Then when the user returns to the app, they input their password and the app could call wallet.disableBackgroundSyncMode(password), which under the hood retrieves and decrypts the spend key from disk (and the wallet continues scanning normally again).


My reasoning for why this is still a security improvement, even if the password remains in memory...

AFAICT, the only time the spend key makes it over to Java-land is when the user explicitly sees it in the UI (rare). Otherwise it's fairly well protected in the C++:

The secret key type is defined here.

Given the above protection measures already in place for the spend key, I figure explicitly wiping the spend key from memory would give a solid degree of confidence the spend key is no longer accessible without the password + access to the app's internal storage. I also figure getting access to both the password from memory and the app's internal storage would be more difficult than just getting access to the spend key from memory. Even with Android's security measures, I figure identifying two exploits is more challenging than one. Since wiping the spend key from memory would require two exploits to pull off to get to the spend key instead of just one, then wiping the spend key from memory when the spend key isn't needed seems a tangible security improvement, even if the password isn't wiped from memory perfectly.


If possible, it also seems like it may be worthwhile to eventually migrate to using char arrays everywhere for passwords (and key material) that get wiped even in Java too, especially considering it looks like mlock is available to Android devs. Although maybe Java's VM will do some funky things that side-step efforts at using it correctly.. plausibly worth exploring further.


I decided against Approach 2 from the comment above, since it feels wrong to leave the view key in decrypted state on disk on a phone forever. Onwards -- woohoo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants