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

Cloak password fields in config #4638

Closed
wiswedel opened this issue Nov 18, 2020 · 6 comments
Closed

Cloak password fields in config #4638

wiswedel opened this issue Nov 18, 2020 · 6 comments
Labels
Milestone

Comments

@wiswedel
Copy link
Contributor

Is your feature request related to a problem? Please describe.
Disclosing saved passwords can be a security risk. Credentials for 3rd party systems are widely visible once someone is in the admin group.
In other parts of Nextcloud (LDAP config, sharing with password, password-protected Talk conversations, Email server settings on basic settings page), passwords are not visible.

Describe the solution you'd like
Cloak saved passwords for Signaling servers and TURN servers on Talk configuration page

Describe alternatives you've considered

@PVince81
Copy link
Member

PVince81 commented Dec 8, 2020

assuming you mean hiding the field in the UI, not in the output of occ config:system
and with passwords you probably mean the shared secrets ?

there are at least three levels of protection that could be made:

  1. changing the fields to type=password: in this case the letters are replaced with stars. one can still use the browser's debugger to find the value in the field. This is only protection against "direct looks" in case of screenshots or someone just scrolling through the page.

  2. Like 1 but with an eye icon: purpose of the eye is to make it easier to manually check the shared secret, for example for verifying if it matches the one that was setup in the HPB.

  3. Password field but without the actual password there. So it visually looks like a secret is set but no one can read it. The field's purpose is only to be able to set a new one. This makes it impossible to verify the shared secret like in level 2, so would require the admin to re-set the secret to confirm that it's the same.

depending on the level of protection required and use case, we can go with either level

@PVince81 PVince81 added this to the 💔 Backlog milestone Dec 8, 2020
@wiswedel
Copy link
Contributor Author

wiswedel commented Dec 8, 2020

Thanks for laying out the options in detail, @PVince81.
The use case in mind requires option 3. If the Nextcloud admin does not own the 3rd party components (like in this case the TURN server and the HPB), they should not be entitled to see the access credentials to those components.

It would in case of doubt be ok if the shared secrets were still in the config or in the database in plain text if necessary, but since Nextcloud lacks more granular admin roles, a (let's call them) application front-end admin won't reach there anyway while nonetheless having duties like automated tagging, user administration or group folders adminstration on the UI.

@nickvergessen
Copy link
Member

The use case in mind requires option 3. If the Nextcloud admin does not own the 3rd party components (like in this case the TURN server and the HPB), they should not be entitled to see the access credentials to those components.

They can use the support app to generate a config report and get all data, or they install a custom app they published to the appstore which does that for them. Or they make an API request to get the given config.

There are really many ways to get to "secret" values and I don't see this happening anywhere soon.

@nickvergessen nickvergessen added feature: settings ⚙️ Settings and config related issues and removed security labels Apr 8, 2021
@LukasReschke
Copy link
Member

LukasReschke commented Apr 8, 2021

Agreeing with @nickvergessen here. If we consider this a priority, my POV would be that we start in the Server by adding support for storing sensitive config values (and disallowing accessing them by a user).

But randomly patching apps one by one and leaving gaps open, seems contraproductive.

@wiswedel
Copy link
Contributor Author

They can use the support app to generate a config report and get all data

There are a lot of other entries in the system report that only say ***REMOVED SENSITIVE VALUE***.

they install a custom app they published to the appstore which does that for them

That's not the attack vector in question (see OP and #4638 (comment))

Or they make an API request to get the given config.

How do you do that?

@nickvergessen
Copy link
Member

Hiding is done via #12812

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants