Security Improvements

[beatles1]

Having looked at the code I think there may be a couple of improvements that could be made to the security. I could be wrong about these and happy to be corrected if so.

1. The users password is saved on the server as an SHA256 hash without salt. This should really have a salt and possibly use bcrypt instead?
2. I believe really the IV value should be unique to each OTP "account" rather than to each user.

[beatles1]

To resolve 1 I was looking at the possibility of a simple change to replace the SHA256 hash with the password_hash and password_verify php methods which I thought might be reasonably simple to do. The issue I've noticed is that the tokens are actually encrypted with the hash not the password itself? This means that the key to decrypt a users token is stored in the same database as the encrypted tokens and a breach of the database would leave everything exposed?

I think this needs a reasonably urgent fix. Unless I'm not understanding how everything works again I think we could use the previously mentioned secure php password methods to store the password in the DB for checking. Then we could SHA256 hash the password when it's initially checked and still use that for the actual encryption but without storing it.

EDIT: Actually I wonder if it's an option to never even send the password to the server? More work but ideally I the password should never leave the clients. Maybe you encrypt a known test value so the client can test if it has the correct password or not and the server only deals with already encrypted values.

[matteo-convertino]

Thank you very much, on this topic I really needed someone who could point out these problems to me and who could help me solve them.

As you pointed out, the core of the problem is the fact that I encrypted with the hash of the password and not with the password itself. This led me to not use a value for the salt. This was because I then had difficulty decrypting the secret keys (rightly so, because with salt I was not able to have the same and identical hash). So encrypting the secret keys with the password should be the right way. password_hash and password_verify seem perfect to me.

As for IVs, how should they be managed? Because the clients must be aware of the IV used to encrypt the specific account in order to be able to decrypt it.

EDIT: Actually I wonder if it's an option to never even send the password to the server? More work but ideally I the password should never leave the clients. Maybe you encrypt a known test value so the client can test if it has the correct password or not and the server only deals with already encrypted values.

Do you mean not even doing the /password/check request? The backend eventually already operates with all data encrypted. If you notice accountController->create() (for example) does not perform any encryption because the secret key already arrives encrypted. The two clients (mobile and web) are responsible for doing this. Obviously the only thing that is done is the authentication phase to check if the password is correct at the beginning.

[beatles1]

I think you likely could use a salt for the hash to encrypt the passwords as long as it's sent to the client and used the same as the IV. However I think using the raw password as the key should be fine as far as I'm aware.

For the IVs if it should be a unique IV per account then we'd just need to store that in the database with the encrypted account and send it to the client when it loads the account.

For having it all on the client that is what I was thinking. Currently the server I think needs to know the decryption key for a password change. It might be that we could just move that functionality into the client. Then not even store the hash on the server and just test the password against an encrypted value on the client. Means we don't need to worry that the server ever has the password even in memory.

[matteo-convertino]

To be honest, the fact of having the password only on the clients (as if it were decentralized) doesn't convince me much. For example, when you change your password (a feature that sooner or later will also be available on mobile phones, not just on the web app), how will the other client know that it has changed and update it accordingly? Based on this encrypted test value, if you decrypt it you know the password. So this value should at least be stored in the db, because it must be shared between clients. But this way is not that different from having the hash of the password directly stored. Indeed, if the user's password is not strong there is a greater risk that a brute force will be successful (which practically cannot happen with the hash).

For the IVs if it should be a unique IV per account then we'd just need to store that in the database with the encrypted account and send it to the client when it loads the account.

Perfect, I think this is the way too. Although I was thinking: why not encrypt the IV? because now it seems like a "useless" value that is transported from the client to the server. Whoever intercepts the message (pretending that the communication is in the clear) reads the accounts along with the IV and the only thing to decrypt is the secret key. I believe that encrypting the IV also has a good level of security.

Another topic I would like to open is: how can the "forgot password" functionality be implemented (#31)? Because right now (with this change that will be made) if you don't remember your password you have lost your accounts.

[beatles1]

For example, when you change your password (a feature that sooner or later will also be available on mobile phones, not just on the web app), how will the other client know that it has changed and update it accordingly?

I'd say they just try to decrypt something and if it fails it assumes the password has changed. You could be right about a test value but I think it's actually no different brute forcing a hand value. I think in practice you could likely just try and decrypt an account and it would probably wipe unless you had the valid key?

I don't think it's essential though, as long as the password is saved on the server with a better hash it's all good.

why not encrypt the IV?

Again I'm no expert on this but I think the IV is similar to a salt in that it just ensures that if you encrypt the same value twice with the same key the output is different and you don't know they're the same. I'm pretty sure the IV is fine to be known as long as the key is secret.

Thinking about it given the encrypted values should really always be unique for this application maybe the current implementation is fine.

Another topic I would like to open is: how can the "forgot password" functionality be implemented (#31)?

I don't think that's possible, you have to accept that if you forget your password then you lose the accounts.

To enable that you might as well not encrypt them and accept that the server has access to all the secrets. I guess that's an option but it should be clear if that's the case as the seperate password implies (at least to me) that the server owner has no access.

[matteo-convertino]

Thinking about it given the encrypted values should really always be unique for this application maybe the current implementation is fine.

Are you referring to having an IV for each user (as it is now) instead of having it for each account?

Again I'm no expert on this but I think the IV is similar to a salt in that it just ensures that if you encrypt the same value twice with the same key the output is different and you don't know they're the same. I'm pretty sure the IV is fine to be known as long as the key is secret.

Yes, actually by just searching for a moment it seems that you can easily get the IV in the clear. (Why is the IV passed in the clear when it can be easily encrypted?) (Sending IV along with cipher text, safe?)

I don't think that's possible, you have to accept that if you forget your password then you lose the accounts.

For the moment this seems the only reasonable thing. I will insert a warning message where I urge you to memorize your password and where I make it clear that if you lose your password you will lose your accounts.

Perhaps the only salvation in this case could be the mobile app: since it knows the password because it is stored locally, implementing the password change directly from the app, when I ask you to enter the old password I also ask for the fingerprint (exactly as happens now when opening the mobile app). In this way I always have confirmation that it is really you who is changing the password but at the same time you are recovering it because you don't remember it (whoever has a mobile phone without a fingerprint reader is screwed :grimacing:). Tell me what you think.

So in summary, the change to make is not to encrypt with the hash of the password, but directly with the password before hashing it. Let me know if there are any other things you think are problematic.

[beatles1]

Are you referring to having an IV for each user (as it is now) instead of having it for each account?

Yes, I think in the real world this is probably fine now I've thought about it more.

Perhaps the only salvation in this case could be the mobile app: since it knows the password because it is stored locally, implementing the password change directly from the app, when I ask you to enter the old password I also ask for the fingerprint (exactly as happens now when opening the mobile app). In this way I always have confirmation that it is really you who is changing the password but at the same time you are recovering it because you don't remember it (whoever has a mobile phone without a fingerprint reader is screwed 😬). Tell me what you think.

That's a really good point, maybe even let the user export from the app offline so they can export a backup and reset then import again if everything becomes really screwed up? That would also be useful if the Nextcloud server got ransomewared or otherwise broken.

So in summary, the change to make is not to encrypt with the hash of the password, but directly with the password before hashing it. Let me know if there are any other things you think are problematic.

I think that sounds good to me as long as that hash uses a salt (like hash_password does automatically). Thanks for listening to the feedback and sorry for being a bit unclear with some of it.

[matteo-convertino]

I just released a new release. This issue should be resolved. Right now the logic I'm using is

unencrypted user password -> hash (sha256) -> password_hash() -> save on the database

This is simply because to encrypt with aes256 I need a 256 bit key, so I do that intermediate step to get a key of the correct length. The change I made with this new release was simply to apply password_hash to the password already present in the db and modify a couple of methods so that it is managed in this way.

Let me know if there is anything you think could be done better and thanks again for helping me with this issue.

[beatles1]

Looks great, thanks for sorting this