nextcloud / server

☁️ Nextcloud server, a safe home for all your data
https://nextcloud.com
GNU Affero General Public License v3.0
26.76k stars 4k forks source link

Allow to specify the token of a public share / share renaming #45440

Open artonge opened 4 months ago

artonge commented 4 months ago
szaimen commented 4 months ago

So should this be a separate app so that it can be removed during building the enterprise archive?

jospoortvliet commented 3 months ago

@szaimen yes. Ideally we adapt and include https://apps.nextcloud.com/apps/cfg_share_links or https://apps.nextcloud.com/apps/sharerenamer in the community edition.

artonge commented 3 months ago

@miaulalala could you give an security opinion on that feature?

jancborchardt commented 3 months ago

could be a security issue. Mitigations: add a warning?

A warning is fine, yes. When you want to give a name, you want it to be easier visible.

optionally enforce password protection?

Optional if at all, but again, the point of giving a simple share name is the ability to remember. If you then have to remember a separate password it defeats the purpose and you might as well have pasted the complicated link.

enforce a minimum set of dictionary words? (minimal 2, optionally configurable?)

Straight no on that one. Again if you change the name you want it to be simple. :)

miaulalala commented 3 months ago

Dictionary attacks would become a concern, yes. Token enumeration is already possible if highly unlikely, although I assume the endpoint is brute force protected, so it's expensive and time consuming.

Could also lead to token leaks as two identical tokens aren't possible, so a (guest) user might try creating shares at random to guess at other tokens. If we add the userId to the token, that could work as a preventative measure so each token only needs to be unique to each user space. The token controller will need to first check the userId against the logged-in user so somebody can't fake the userId in a request and try collision based guessing that way, but then the benefit of having a custom token is minimized.

If we do this, I'd recommend enforcing a password.

I'd also suggest a minimum length for the token. Not great if the user chooses something really short.

In short, there are definitely some risks to this, and I can't really see the benefits. Maybe a link shortener integration could be a viable alternative?

jospoortvliet commented 2 months ago

Dictionary attacks would become a concern, yes. Token enumeration is already possible if highly unlikely, although I assume the endpoint is brute force protected, so it's expensive and time consuming.

Could also lead to token leaks as two identical tokens aren't possible, so a (guest) user might try creating shares at random to guess at other tokens. If we add the userId to the token, that could work as a preventative measure so each token only needs to be unique to each user space. The token controller will need to first check the userId against the logged-in user so somebody can't fake the userId in a request and try collision based guessing that way, but then the benefit of having a custom token is minimized.

If we do this, I'd recommend enforcing a password.

I'd also suggest a minimum length for the token. Not great if the user chooses something really short.

In short, there are definitely some risks to this, and I can't really see the benefits. Maybe a link shortener integration could be a viable alternative?

this is basically an accepted risk - it is meant for small home user instances where I might want to share vacation photos with family using an easy to remember link. That that makes the link easy to find is something we will warn about in the UI, but it's inherent to the solution. Adding a (mandatory) password would defeat the purpose of this ;-)

On large instances it's a rather dumb idea to do it, but then we don't intend to promote it for that. It's pure for home users.

FWIW, I use the app that implements this myself.