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

[Security] Use random IV for encryption #204

Open
FlorianPicca opened this issue Feb 4, 2023 · 7 comments · May be fixed by #441
Open

[Security] Use random IV for encryption #204

FlorianPicca opened this issue Feb 4, 2023 · 7 comments · May be fixed by #441
Labels
improvement A general improvement of an existing feature security

Comments

@FlorianPicca
Copy link

Description

Files are encrypted using a key and IV derived from the password.
Because this process is entirely deterministic, every file will be encrypted with the same Key/IV pair, which completely breaks the confidentiality offered by the GCM mode of operation. You can find more information about why this is bad here.

Impact

An attacker with access to encrypted files (which might require root privileges) and knowledge of a single photo, can decrypt and view every other photos without knowing the password.

Recommandation

Use a randomly generated IV for each encryption operation.

@leonlatsch
Copy link
Owner

Thank you for this submission. A similar issue was reported a while back but this one really breaks it down, so I will keep it open.

I may change the title and keep this as a todo for the future.

@leonlatsch leonlatsch added improvement A general improvement of an existing feature security labels Feb 6, 2023
@leonlatsch leonlatsch changed the title [Security] Cryptographic weakness [Security] Use random IV for encryption Feb 6, 2023
@leonlatsch
Copy link
Owner

Was reported in #177.

@leonlatsch
Copy link
Owner

Implementing this means we need some kind of migration for the users files, aswell as a new backup restoring version.

@FlorianPicca
Copy link
Author

I did not see the previous issue, but I do agree with the author.
The other issues discussed there are legit and I didn't report them here because I wanted to focus on the main problem, which is the IV handling.

If you want to use a better key derivation function than SHA256, you might want to look at this ressource which recommends argon2id like mentioned in the previous issue. You might even want to change your password storage mechanisms to use that.

@leonlatsch leonlatsch added this to the Photok 2.0 milestone Feb 7, 2023
@leonlatsch leonlatsch added this to Photok Feb 7, 2023
@leonlatsch leonlatsch moved this to 🔖 Ready in Photok Feb 7, 2023
@anbraten anbraten mentioned this issue May 11, 2023
2 tasks
@leonlatsch leonlatsch removed this from the Photok 2.0 milestone Apr 29, 2024
leonlatsch added a commit that referenced this issue Jul 2, 2024
**Description:**

- Load images as whole bitmap and not as buffered source

**Explaination**
Loading AES/GCM encrypted files buffered/sampled. Causes the stream to
skip bytes which is not allowed in AES/GCM.

Since this raises the issue of crashing with very big images again. This
is once of the reasons I want to overhaul the encryption some day to not
use GCM anymore #204

Fixes #330 

- [x] Confirmed working by @prosch88
@leonlatsch
Copy link
Owner

leonlatsch commented Jul 9, 2024

I have made a concept for a new encryption
image

https://excalidraw.com/#json=nGOvK_cvBiSmj9H2zC_Ms,kfxrwsOVJvm42KVdfAkOmg

@clach04
Copy link

clach04 commented Nov 29, 2024

I have made a concept for a new encryption image

https://excalidraw.com/#json=nGOvK_cvBiSmj9H2zC_Ms,kfxrwsOVJvm42KVdfAkOmg

@leonlatsch do you have the current file format specification documented anywhere? I wasn't able to locate anything other than looking through source code.

If you have that and a proposal for new format that would make it easier to get feedback before you commit to it. There are a couple of reddit crypto/privacy spaces that can be surprisingly friendly with feedback 😀

@leonlatsch
Copy link
Owner

I started looking into this topic again. The main problem caused by the current implementation was fixed now (#198).

What clear is that the hard part will be a migration, but thats step 2.

Step 1:

I (we) need to decide on a V2 of the apps encryption. Meaning:

  • Algorithm / variant
  • key generation / storage
  • How to handle iv / salts

Since we need to do a migration in any case, we have full freedom to choose anything we want.

Current file format / encryption

Algorithm: AES/GCM/NoPadding
Key: Derived from password (plain text -> SHA256 -> Is the key)
IV: Derived from the password (first 16 bytes of the plain text password are the IV)
Storage: Currently NOTHING is stored. Not the IV not the key, not the password.


Problems

  • Find a algorithm and key generation that is suited for password based encryption
    I would like to keep this part as is but I'm open to suggestions. The current key allows to restore any files with just a password. Generating a random key will result in storing it and handling the case of restoring data on a new device. (backup codes, etc..)

  • Handle IV
    If we stick to GCM we need to follow the requirements (random IV for every file). And we need to store it. (at the beginning of the file or in the database).
    Or we find a different algorithm that does not require this mechanism and is safe

  •  Keep future features in mind
    Features like Fingerprint Unlock (Use fingerprint reader on devices that support it #45) or code unlock require the encryption key (random or derived from password) to be stored in the Android keystore.

Step 2: Migration

Once a new good encryption is implemented we need to find a way to migrate users data.

Since were working with potentially very big data, this needs to have a UI aswell.

I can imagine that its worth releasing a V2.0.0 and make it a breaking change, but we still need to find a way.

  • Option 1:
    • Keep the old code and migrate at runtime. This means every file will be decrypted and encrypted with the new code. Similar to when you change your password right now.
  • Option 2:
    • Communicate the breaking change and force a export of all data (using the old code) and resetting the app. This is probably easier for us but I can imagine the users don't really like this way.

Ideas welcome!

Step 3: Backwards compatibility

Especially for backups. The app needs to somehow handle old data when you try to restore a old backup.

Alternative: After updating the app show a notice that old backups are NOT compatible anymore and offer the user to do a new backup now.

@leonlatsch leonlatsch linked a pull request Feb 26, 2025 that will close this issue
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement A general improvement of an existing feature security
Projects
No open projects
Status: 🔖 Ready
3 participants