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

Backups should not store sensitive data #744

Closed
5 tasks done
Luna712 opened this issue Nov 1, 2023 · 5 comments · Fixed by #979
Closed
5 tasks done

Backups should not store sensitive data #744

Luna712 opened this issue Nov 1, 2023 · 5 comments · Fixed by #979
Labels
bug Something isn't working possible provider issue Assigned by the bot

Comments

@Luna712
Copy link
Contributor

Luna712 commented Nov 1, 2023

Steps to reproduce

Backup and then look at backup

Expected behavior

Sensitive data should not be in there

Actual behavior

Sensitive data is there

For example, "opensubtitles_account_3/open_subtitles_user":"{\"user\":\"<real_username>\",\"pass\":\"<real_password>\",\"access_token\":\"<real_access_token>\"}"

From what I could tell, this is meant to be avoided by this, but it does not seem to work properly:

private val nonTransferableKeys = listOf(
// When sharing backup we do not want to transfer what is essentially the password
ANILIST_TOKEN_KEY,
ANILIST_CACHED_LIST,
ANILIST_UNIXTIME_KEY,
ANILIST_USER_KEY,
MAL_TOKEN_KEY,
MAL_REFRESH_TOKEN_KEY,
MAL_CACHED_LIST,
MAL_UNIXTIME_KEY,
MAL_USER_KEY,
// The plugins themselves are not backed up
PLUGINS_KEY,
PLUGINS_KEY_LOCAL,
OPEN_SUBTITLES_USER_KEY,
"nginx_user", // Nginx user key
)

Additionally we should probably add lockPin to be excluded from that as well. simkl_user maybe also, even though that only stores username and avatar, it can't be used on restore without reauthorization, especially if you restore on a different device. And another issue is when restoring it should only try to enable the plugins from restored repositories that you had enabled before, the only option seems to be none and do all needed knes manually again, or do all of them and remove ones you don't want again.

Also, automatic backups don't seem to always work on my phone. It does on TV and even emulator but on my phone, set to 3 hours and last backup was two days ago. Subscription worker doesn't seem to be triggered either. I notice subscriptions being updated on emulator on PC, but on phone, I never, as if the schedulers just don't work on the phone very often.

Cloudstream version and commit hash

4.2.1 8b73c35

Android version

Android 13

Logcat

No response

Other details

No response

Acknowledgements

  • I have searched the existing issues and this is a new ticket, NOT a duplicate or related to another open issue.
  • I have written a short but informative title.
  • I have updated the app to pre-release version Latest.
  • If related to a provider, I have checked the site and it works, but not the app.
  • I will fill out all of the requested information in this form.
@Luna712 Luna712 added the bug Something isn't working label Nov 1, 2023
@recloudstream
Copy link
Contributor

recloudstream bot commented Nov 1, 2023

Hello Luna712.
Please do not report any provider bugs here. This repository does not contain any providers. Please find the appropriate repository and report your issue there or join the discord.

Found provider name: Loklok

@recloudstream recloudstream bot added the possible provider issue Assigned by the bot label Nov 1, 2023
@fire-light42
Copy link
Collaborator

  1. Ye, this seams like a major bug. However it would be pointless to not store the lockPin as lockPin is only the key to view the rest of the unencrypted data. If you really wanted that, then you would need to encrypt all keys associated with an account with that pin, but that too would be useless as you could brute force 10000 combinations quite easily. So while I agree that opensubs + simkl and stuff should be removed, we cant remove the pin.

  2. What api is your phone? the worker does not work on some lower API vers.

@Luna712
Copy link
Contributor Author

Luna712 commented Nov 1, 2023

  1. Ye, this seams like a major bug. However it would be pointless to not store the lockPin as lockPin is only the key to view the rest of the unencrypted data. If you really wanted that, then you would need to encrypt all keys associated with an account with that pin, but that too would be useless as you could brute force 10000 combinations quite easily. So while I agree that opensubs + simkl and stuff should be removed, we cant remove the pin.

Yeah It would not make much sense now that I think of it, not storing it only introduces another way to bypass the PIN, so nevermind on that part

  1. What api is your phone? the worker does not work on some lower API vers.

API 33

@fire-light42

@CranberrySoup
Copy link
Contributor

CranberrySoup commented Nov 1, 2023

Please do not touch the backup system overly much, my PR reworks a lot there, including fixing sensitive keys.

@Luna712
Copy link
Contributor Author

Luna712 commented Nov 1, 2023

Please do not touch the backup system overly much, my PR reworks a lot there, including fixing this.

@CranberrySoup

Your PR was the reason I created this instead of trying to fix it myself for this exact reason, I did not know it fixed the issue though, sorry about that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working possible provider issue Assigned by the bot
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants