bfabiszewski / ulogger-server

μlogger • web viewer for tracks uploaded with μlogger mobile client
GNU General Public License v3.0
547 stars 85 forks source link

Hide admin user #74

Closed rahra closed 5 years ago

rahra commented 6 years ago

This patch hides the admin user if unauthenticated view of tracks is allowed.

rahra commented 6 years ago

I just added another fix. Importing GPX tracks fails if the track points have no timestamp. In this case it will be set to 0 in utils/import.php which will in turn cause uPosition::add() (in helpers/positions.php) to fail. Thus, I fixed it by setting it to the "artificial" value 1 (which is 1 Jan 1970 00:00:01....).

bfabiszewski commented 6 years ago

Regarding zero timestamp, I don't see anything wrong about that. Zero is a valid unix timestamp. I deliberately decided to set it to zero, as a workaround for buggy GPX files (time tag is in fact mandatory in GPX specification). I can't reproduce your problem, my µlogger demo server successfully imports tracks without time tags. What exact error did you get? Maybe it is a problem with your database setup?

bfabiszewski commented 6 years ago

Regarding hiding admin user.

I am not sure what your aim is. If I understand correctly, your PR will make it impossible to view tracks of a user that has admin status. This is not always desirable.

Look for example at demo site, admin user also has its own tracks.

On another site I have my user: bfabiszewski – this is normal user with tracks and also site admin. With this change other users will not be able to see my tracks.

rahra commented 6 years ago

The admin user is able to create/delete users. Hiding it reduces the attack surface. If the admin user is shown, an attacker can brute-force the password but if it is hidden, he has to brute-force the username as well which is more difficult.

Regarding the timestamp issue. I tried to track down this more specifically. If the timestamp is 0 the $stmt->execute() in positions.php line numer 110 fails with $stmt->errno== 1292. I used your original ulogger.sql to create the tables. I run mysql 5.6.41. I have no idea yet. But I'll keep you informed.

rahra commented 6 years ago

The timestamp issue is actually a configuration issue of mysql. It's not uncommon, e.g. also found here: https://github.com/github/gh-ost/issues/538 To resolve it, it is necessary to remove the STRICT_TRANS_TABLES from the sql_mode variable of the mysql server.

So actually this is a mysql issue and not a ulogger issue (what I originally thought). The questions is how to deal with this since others may run into the same issue. And although 0 is a perfectly valid Unix timestamp there is no big difference compared to 1 ;-) What do you think?

bfabiszewski commented 6 years ago

You are right. I didn't know MySQL has such limitation on timestamp range:

The TIMESTAMP data type is used for values that contain both date and time parts. TIMESTAMP has a range of '1970-01-01 00:00:01' UTC to '2038-01-19 03:14:07' UTC.

Thanks for catching this problem. I would like to merge it but without admin changes. Could you please isolate this patch into separate PR? Actually this patch also requires change to unit test that now fails (if you will not fix it, I can do it later):

There was 1 failure:
1) ImportTest::testImportNoTime
Wrong actual table data
Failed asserting that false is true.
/home/travis/build/bfabiszewski/ulogger-server/.tests/tests/ImportTest.php:393

The admin patch would make it impossible to view tracks for all users who use same account for privileged and unprivileged user, including me :) This could only be implemented as an optional feature (a config setting?). Anyway I don't think hiding admin user name would make the server much more secure. I think it is enough to set a strong password (and minimum password strength requirement in config) to make it very difficult (or impossible) to brute force it. If attacker is able to brute force a password consisting of upper and lower case letters, digits and other non-letter characters, they will surely have no problem with guessing admin user name.

rahra commented 6 years ago

Hi! I'll remove the admin-user patch and put it into a separate branch. I'm just a very busy in the moment...

bfabiszewski commented 5 years ago

I cherry picked timestamp fix commit from your PR. Thanks again!