haiwen / seafile-client

Seafile desktop client.
http://seafile.com
Apache License 2.0
474 stars 279 forks source link

clone.db leaks passwords of encrypted libraries....in plain text #1122

Open RoestVrijStaal opened 6 years ago

RoestVrijStaal commented 6 years ago

Steps to reproduce:

  1. Open <directory where you picked to put your Seafile folder there>/Seafile/.seafile-data/clone.db with your favourite text editor which isn't picky or a hex viewer,
  2. Search on your account name to find your passwords next to it, or search on parts of your password.
  3. Call an ambulance for the heart attack you've unlocked by doing step 2.

The fishy part is when opening clone.db with a database tool like "DB Browser for SQLite", none of that data is seen during browsing trough the database with that tool.

I know implementing methods to "keep the app know" the passwords could be tricky, but is this intentional?

Affected OS: Windows 7 x64 SeaFile Client: v6.1.8

egroeper commented 6 years ago

What specifically are you worrying about? The security of the encrypted data or the security of the password, that could be reused somewhere else?

For the first problem the only real solution would be implementing a credential container and asking the user at each start of Seafile to enter the master password to be able to access the credentials encrypted in that container. This enhances security, but with obvious usability caveats. Perhaps there are some machines out there, that should be running Seafile unattended. So this clearly would need to be optional.

For the second problem the devs could think about only storing a derived key based on the password (as is already done in repo.db?).

Redsandro commented 6 years ago

Plain text? :see_no_evil:

Hash. Salt. There is no excuse.

Salt in configuration, not in db.

shoeper commented 6 years ago

You're mixing up different things here. Both links are unrelated.

Afaik (my knowledge on cryptography is limited) in this case a key is derived from your password. Salt is completely unrelated here (a salt would brake the whole thing as the password alone wouldn't be enough to derive the key). Hash at least not in the way you think it would be.

This is more about a key for symmetric encryption than a password.

The most significant difference is that the key is not meant to be saved on the server as a normal password is. And yes in some cases it is (or the key derived from it) stored on the server when using seahub, but it does not really matter, because both are enough to decrypt the data.

Redsandro commented 6 years ago

My mistake. I thought this was about the server side in order to 'decrypt server side' as seen in the Android app.

It is however not as unrelated as you think. Plaintext passwords are not cool. Adversaries have to steal, bruteforce or otherwise acquire one thing and they have access. They also have a plaintext password they can try on all your other accounts.

My first thought as a novice - but not as novice as storing passwords in plaintext - is to have a random salt generated on the server for every library. Which is communicated to the client on the initial handshake, and tied to the client's unique uuid, which is not stored in clone.db. Then the password + salt are hashed into a passphrase which is stored in clone.db so the app for "remember me" purposes.

Now an adversary:

shoeper commented 6 years ago

Maybe you know more about it than me... This "password" is used to derive a symmetric key to encrypt the data.

I don't understand what the UUID should be useful for (looks more like security through obscurity).

The salt would slightly increase the security, but how do you make sure the attacker doesn't receive it from the server? It must be possible [to receive it] to derive the key.

The issue opener complains that the "password" (or its derived key - one of the two needs to be stored) is stored in his computer and this must always be the case as long as the user should not enter all passwords again on reboot and after hibernation. And given the key is stored on the computer - no matter in which format - it can be stolen and used to decipher the data. (and the data is stored on the client in plaintext anyway, so why not just stealing it?)

Redsandro commented 6 years ago

@shoeper

I don't understand what the UUID should be useful for (looks more like security through obscurity)

The UUID is used for permission to fetch resources. It's kinda like an automatically generated application-specific password in plain text. This means that when you notice an intrusion, you can retract the UUID using the admin account and generate a new one for your application, so you can shut out the adversary without having to change your password (which is annoying). There is nothing obscure about it. It is in fact the opposite.

Also the adversary will get a different UUID if they just connect with a separate client and a acquired password, so they would need to intercept your network traffic in order to steal the UUID that belongs to the acquired password. This would "slightly increase the security" as much as https is "slightly more secure" than http.

The salt would slightly increase the security, but how do you make sure the attacker doesn't receive it from the server? It must be possible [to receive it] to derive the key.

I think you misunderstand the importance of this salt. First, the adversary will get a different salt if they just connect with a separate client and an acquired password, so they would have to steal this too. But more importantly, it prevents adversaries from being able to do rainbow attacks using dictionaries with tools such as John The Ripper or HashCat on a massive scale.

shoeper commented 6 years ago

This means that when you notice an intrusion, you can retract the UUID using the admin account and generate a new one for your application, so you can shut out the adversary without having to change your password (which is annoying). There is nothing obscure about it. It is in fact the opposite.

In that moment the salt would already be stolen.

I think you misunderstand the importance of this salt. First, the adversary will get a different salt if they just connect with a separate client and an acquired password

I think you misunderstood the use case. All clients actually have to get the same salt otherwise the salt and password combined do not lead to the same key (or at least it would require a KDF that leads to the same key with different salts, not sure if something like that works and it wouldn't help as the key would also be lost when having one of the salts) - thus decryption of the data is not possible.

I'm still not sure if you are in the right context. The "password" in this post is being used to derive a key for symmetric encryption. All clients need to have the same key, otherwise they cannot decipher the data.

This is not about a password that only needs to be checked against to match whether it is correct or not like it is being done when logging in on some service. And as long as Seahub is not being used for the encrypted library the password (/key) also will not be on the server.

Redsandro commented 6 years ago

In that moment the salt would already be stolen.

It doesn't matter. The salt's only goal is to prevent rainbow attacks.

All clients actually have to get the same salt otherwise the salt and password combined do not lead to the same key

No, the unique salt is used to wrap/encrypt the symmetric key using your password. You are correct that this doesn't make you immortal, but it prevents:

We need someone with better knowledge than I to figure out what pieces need to be where (client/server).

shoeper commented 6 years ago

I understood what you mean, now.

The second point still does only work when one salt is being used (for all clients) and involves the risk that there is data loss when the salt is lost.

Protection against rainbow attacks is what the key derivation function is for.

But, I'm still questioning whether it would make the situation besser. The best would imho be to store the encryption key in the database of the client once it has been derived. This way the users password should be save and regarding the decryption of the data: it is available unencrypted on the local disk anyway.

Redsandro commented 6 years ago

The best would imho be to store the encryption key in the database of the client once it has been derived

This is a solution for the first problem, but leaves the second vulnerability. If we can have a passphrase for a symmetric key that is wrapped using a library-specific salt plus your password, wouldn't that solve the second problem?

The library would have to store its salt so new clients can login using the same password.

I apologize for my snappy reply earlier, I misunderstood part of what you said. All clients do indeed have to use the same salt. But the libraries must each have a different random salt. It does not matter if the salt gets stolen. The purpose is to remove one specific (popular) attack vector.

shoeper commented 6 years ago

but leaves the second vulnerability

I'm not sure if a bruteforce still works when only having the derived key. A bruteforce would not be necessary anymore to decipher the data as it already is the key. And to get the password it would be required that there exists a function to reverse the kdf (to compute the password from the key).

The issue I have with the library specific salt is that the salt would be in the database. Thus when the database would be lost but the data would be still there, there would be no way to recover (ok a bruteforce - but given the salt has a larger length the chances are low) because the user does not know the salt. Without the salt it would still be possible to get the library into a new installation and download & decipher the data using the client.

I also found documentation on encrypted libraries: https://manual.seafile.com/security/security_features.html#how-does-an-encrypted-library-work (it works a little bit different than I thought regarding that it generates a key and encrypts it using a key/iv pair derived from the given password).

Looks like the client already is supposed to not store the user password, but looking at the initial issue it seems that it is temporarily stored when downloading the library.

shoeper commented 6 years ago

@egroeper

For the second problem the devs could think about only storing a derived key based on the password (as is already done in repo.db?).

It looks like this is how it is supposed to work, but there seems some bug storing the password temporarily in the database.

shoeper commented 6 years ago

@freeplant

Redsandro commented 6 years ago

Looks like the client already is supposed to not store the user password, but looking at the initial issue it seems that it is temporarily stored when downloading the library.

Interesting. Why would the plain text password be temporarily stored? If I remember correctly, mysqlite items don't really get deleted, much like files on a hard drive. Theoretically, the plain text password could be in the file long after it was deleted. Perhaps something similar is happening to clone.db.

Thus when the database would be lost but the data would be still there, there would be no way to recover

Judging from the documentation, a key that the client depends on (because of the random seed), encrypted by the password, is already stored on the server. When this is lost, so are the files. What's worse, the password is sent to the server in the web client.

The database could store salt together with the key, so that a hash would be communicated rather than the password.

This way, password never has to be stored or send to the server. Only the hash is ever stored or sent.

Update: According to the docs, there is already a salt on the server, and passwords are already hashes. I'm confused. I'm sorry, I might have misspoken. The documentation and the issue report contradict each other a bit.

shoeper commented 6 years ago

Interesting. Why would the plain text password be temporarily stored? If I remember correctly, mysqlite items don't really get deleted, much like files on a hard drive. Theoretically, the plain text password could be in the file long after it was deleted. Perhaps something similar is happening to clone.db.

I don't know. My best guess would be that the clone (initial library download) task is passed from one to another component using the db.

According to the docs, there is already a salt on the server, and passwords are already hashes. I'm confused. I'm sorry, I might have misspoken. The documentation and the issue report contradict each other a bit.

The password for the library is not salted afaik - but is used to derive a key/iv pair to encrypt the "file key" (so the password itself is not stored anywhere, not even salted).

The password to login on Seahub is salted with a 32 Byte salt, maybe that's what is confusing on the page?

RoestVrijStaal commented 6 years ago

Hi @everyone,

Do you two need any input from my side?

I toyed with clones.db and suggested here https://github.com/haiwen/seafile/issues/2088#issuecomment-414664461

I used "Compact Database" function of "DB Browser for SQLite" and all plain-text passwords are gone. So far it looks like the issue is resolved at my end....

I can't remember from which version I used SeaFile, but I wasn't the first user since the project started. Likely I started to use SeaFile in the 3.x.x era.

Questions what remain:

shoeper commented 6 years ago

Would the leak still occur?

You can test it. Add a library, clone it and see if the password can be found in the db, again.

RoestVrijStaal commented 6 years ago

@shoeper My apologies for the late reply. But unfortunately yes, I'm able to reproduce it.

I've added a new password-encrypted library via the web-interface. Created some dummy files. Synchronized the created library with SeaFile desktop client. Opened clone.db in my favorite text/hex editor and found the path-cloned-to, appended with the password of the library, server address, a weird number and at last my username of the SeaFile service.

freeplant commented 5 years ago

The clone.db is used temporary. After downloading the library, it will be cleaned.

This behavior (storing of password in clone.db) can be improved in the future.