marius-wieschollek / passwords

A simple, yet feature rich password manager for Nextcloud
GNU Affero General Public License v3.0
201 stars 39 forks source link

[FEATURE]: Reduce password transport through network #614

Closed kochn closed 7 months ago

kochn commented 7 months ago

⚠️ This issue respects the following points: ⚠️

Current Status

Currently all passwords within the selected folder are downloaded to the client. At this moment it is not clear, if the user really need all passwords. An this information is in the browser available, in case for copy the password

Feature Description

Request: GET /apps/passwords/api/1.0/folder/show returns a response with a field: passwords which contains entries of all passwords within the given folder. The secret information of passwords are also transmitted to the client although they are not used. This can be observed in the dev tools of a browser.

Additional Context

For increasing the security, it is recommended to transmit the password only on a request, and not eager. Malicious software on client site could catch alls passwords of the browser. Moreover the passwords are everytime transported, although the user might only navigate through folders. Generating a request for downloading only the password, and only then, if the user wants to copy it, would make the tool much more secure.

marius-wieschollek commented 7 months ago

This feature provides no security benefit at all.

To get the best security for passwords, you should:

kochn commented 7 months ago

Your arguments are generally right, but are not reasons, why this feature would not increase security. If a user would put hundreds of credentials within one folder, all passwords would be downloaded although they would not needed. It is part if security principles like Secure by Design.

Not using the web-interace could be a reason, but this request is just a suggestion to optimize the API which would also improved other clients, not only a web interface. It does not matter, which technology the user uses. There is an option to request with less effort all passwords of a folder. Is there really a user case, to download all passwords and hold them for a longer time in memory? This is an unnecessary risk, which could easy fix.

If the user is compromised, I'm with you, that it doesn't matter, because the attacker just needs to call additional API requests. But also this should not be a reson, to not reduce the transportation of strictly confidential and sensible information to a minimum.

E2EE is a very good apporach to increase security. You should not relay only on one solution. Defense in Depth is a principle wich is part of Secure by Design and also recommended by the BSI compendium.

So generally: yes you are right with all your points and I aggree with your arguments. But I do not agree with youropinion, that this suggestion will not improve security "at all".

marius-wieschollek commented 7 months ago

Let me just play through what the implementation of this feature would look like in the browser extension, the most used client for this app: The browser extension has four relevant main areas where it accesses the passwords: autofill, password mining, basic authentication autofill and password copy/drag&drop.

These four examples show that the most likely way how any client would react to this change is by immediately circumventing it. The only way to provide the existing features is to fetch the list of all password entries as today and then bombard the server with several hundred password requests, potentially bringing it down.

All other clients, including the webapp, have similar scenarios which would lead them to circumvent this change. So if they all are going to do that anyway, the only result is an increase in server requests and code complexity.

This feature has no security benefit at all because there is no realistic attack vector where it would prevent the attacker from gaining access to your passwords. Any fringe edge case i could think of is be better handled by the E2EE since it encrypts all personal data. Even if this feature somehow managed to prevent an inept attacker from learning the password, they still know enough other information to put together a well informed phishing mail.

It is not "Defense in Depth" because it is unable to protect against any attack type. If the attacker can bypass the HTTPS encryption they can also bypass this feature, meaning that it did not provide an additional security layer. It is not part of "Security by Design" because it requires the attacker to not know how the app works in order to be useful. This feature falls under the "Dont's" of "Security by Design": "security through obscurity" and "security theater". It is a security measure which wholly consists of making two requests instead of one without having any additional protection for the second request.

Going further, this feature has the potential to actually undermine the security of an user which has been attacked. If they knew of this feature, they could be lead to think that only passwords they have accessed are compromised and need to be changed. That is incorrect as there are many scenarios where the app would have to fetch the password without the user knowing. E.g. the app only supports updating the whole password entry (because E2EE) which means that moving, tagging, favoriting etc. all require the app to load the password. So any user believing to have been protected by this feature would be at risk of not changing credentials that have actually been compromised.

Lets get to the last point, which really annoys me:

This is an unnecessary risk, which could easy fix.

No. It's not an easy fix. An API is based on the promise that the server will always return a result which follows a predefined format. The password property is a required property, so the promise is to any client that it is always there. Changing that is a breaking change to the API. That means making a version 2.0 of the API, having a 1.0 legacy API and maintaining that for at least one year. It would also require all client applications to be adapted to the new behavior which incurs a great amount of work for all the open source developers maintaining these clients. The change would greatly increase server performance requirements as clients would have to send a lot more requests than before. Not just the ~15% of users with low-power servers would be affected by this, but also companies with lots of users would effectively see their servers DDOSed by the browser extension.

So to summarize: There is no security benefit as this feature does not introduce any additional security check. The effort of implementing this is extremely high as it requires all clients using the API to be changed. The user experience would be degraded as some features like copy would randomly not work and others would be slow and sluggish. And administrators would have to reconsider offering the app due to the traffic increase caused by it.

I get that it can be scary to open the network monitor and see that the app is just transmitting passwords in plain text all the time. It's a well known issue of this app and all other online password managers that you have to trust the server or you just can't use it. That is why we have security measures like the strict HTTPS requirement, the CSP, E2EE and SSEv2. Those are proven security layers that can even still foil attacks even if another layer failed. This feature can't.

And that's why i'm closing this ticket.

Unless anyone can show an attack on the app which would have been stopped by this feature but not the existing security measures provided by the app, i'm not going to reconsider this.