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

Web UI Authentication #220

Closed
DeepDiver1975 opened this issue Jan 28, 2016 · 20 comments · Fixed by owncloud/core#22255
Closed

Web UI Authentication #220

DeepDiver1975 opened this issue Jan 28, 2016 · 20 comments · Fixed by owncloud/core#22255
Assignees

Comments

@DeepDiver1975
Copy link
Member

Access to the web based updater has to be protected.

Idea:
Store some user password hash in a file within the updater and display a login form to the user when accessing the updater.

To think about:
smooth integration with owncloud core. we want the admin in the admin section to simply click a link
which opens the updater in a new tab/window.
In an ideal world no login form is displayed since we already know that the admin is authorized.

@DeepDiver1975
Copy link
Member Author

bildschirmfoto von 2016-01-28 15-58-41

@karlitschek
Copy link

screenshots are an interesting way for cross repo issue linking ;-)

@LukasReschke
Copy link
Member

In an ideal world no login form is displayed since we already know that the admin is authorized.

We can write and read from the PHP session. While $_SESSION['encrypted_session_data'] is encrypted the other stuff is not. So something like $_SESSION['is_permitted_to_use_updater']. Probably also including some timestamp to ensure that the permission was there recently. I guess storing the user name in there and actually looking up the permissions in the DB is out of question?

When we're thinking about a shared secret this should go more into the direction of a signature so that the actual shared secret is never exposed to the admin. Or change this for for each update / from time to time to ensure that ex-administrators (e.g. fired employees) cannot access the web UI in the future.

@DeepDiver1975
Copy link
Member Author

I guess storing the user name in there and actually looking up the permissions in the DB is out of question?

yes

@DeepDiver1975
Copy link
Member Author

We can write and read from the PHP session. While $_SESSION['encrypted_session_data'] is encrypted the other stuff is not. So something like $_SESSION['is_permitted_to_use_updater']. Probably also including some timestamp to ensure that the permission was there recently.

I like that

@DeepDiver1975
Copy link
Member Author

But we still require a login form for direct access to the updater. Imagine the instance is already in maintenance mode and you cannot login but want to access the updater.

@karlitschek
Copy link

great idea. Let's use the session :-)

@karlitschek
Copy link

@DeepDiver1975 good point. but where doe the user get the hash from? still config.php ?

@VicDeo
Copy link
Member

VicDeo commented Jan 28, 2016

@karlitschek under these conditions (no info in session and broken OC) we can generate some hash on our own and put it in a file that user can read via FTP

@karlitschek
Copy link

Hmm. This is tricky because this file shouldn't be accessible from the browser which is hard to guarantee if it is not guaranteed if .htaccess works.

@VicDeo
Copy link
Member

VicDeo commented Jan 28, 2016

@karlitschek it can be a *.php file with content as follows

<?php
somesecret

in this case it doesn't matter whether anyone can access this file or not.

@VicDeo VicDeo mentioned this issue Jan 28, 2016
3 tasks
@ghost
Copy link

ghost commented Feb 2, 2016

@LukasReschke Hi, would you be able to help during this week to get this done? Sorry for the late request. @karlitschek would this work?

@LukasReschke
Copy link
Member

@LukasReschke Hi, would you be able to help during this week to get this done? Sorry for the late request. @karlitschek would this work?

Can't promise anything. There is other security related stuff that needs to be done as well. Also I consider 1 week before feature freeze a rather VERY short timeline. But don't get me started ranting here 😉

I can try to take a look and do my best but can't promise anything.

@LukasReschke
Copy link
Member

That said, if the other stuff works and this is just the authentication: Should probably be doable. Will try to spend some time tomorrow…

@karlitschek
Copy link

@cmonteroluque I discussed with @LukasReschke and @DeepDiver1975. We will figure something out here.

@ghost
Copy link

ghost commented Feb 8, 2016

OK thanks. Looking for tomorrow func freeze

@LukasReschke LukasReschke self-assigned this Feb 8, 2016
@LukasReschke
Copy link
Member

Ok. Have been thinking some more. Together with owncloud/core#22238 I'm working on a cookieless solution. As the updater basically does remote execution by design this needs to have proper authentication. PR coming later…

@LukasReschke
Copy link
Member

This is done with owncloud/core#22255, #237 and #233

The authentication is implemented in the following way:

  1. There is a config.php entry updater.secret. This value is not set by default.
  2. If somebody opens the admin interface and clicks "Open updater", the value is set and after 24 hours cleaned (so that somebody with an old token can't do much harm). Also there can always only be one token. If somebody can't use the "Open updater" link for whatever reason they can just write that value into the config file themselves and open the updater URL.
  3. The token is sent with any request to the updater. If it matches the request passes.

I did it implement that way because a leaked token would be very bad. It basically allows remote code execution. The way it is implemented now we don't have to worry about CSRF alike attacks plus old tokens do invalidate.

@VicDeo I'd love to make the expiration a tad smaller than 24 hours. Do we have any expectations for how long the updater app is expected to need for an upgrade?

@LukasReschke
Copy link
Member

Plus, without a token by default we keep the risk of abuse way smaller. 😄

@VicDeo
Copy link
Member

VicDeo commented Feb 9, 2016

@LukasReschke I suppose cron jobs are being skipped in maintenance mode, aren't they?
If so, the token will not be invalidated until maintenance mode is over.

I think we can limit it to 2 hours and strongly advice not to use Web UI for big installations.
Most likely server timeout will happen before the token expiration.

LukasReschke added a commit to owncloud/core that referenced this issue Feb 10, 2016
- Reset tokens after 2 hours as discussed at owncloud/updater#220 (comment)
- Used BCrypt for storing the password in the config.php. This makes it substantially harder in case of a leakage of the token to bruteforce it. In the future we can evaluate also an HMAC including the IP. That's a bit tricker though at the moment considering that we support reverse proxies. Didn't feel brave enough to touch that dragon now as well ;)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants