eXist-db / exist

eXist Native XML Database and Application Platform
https://exist-db.org
GNU Lesser General Public License v2.1
421 stars 179 forks source link

sm:passwd($user, $pass) inaccessible to non-dba users #2150

Open ahenket opened 6 years ago

ahenket commented 6 years ago

So... after 5 years of update troubles that kept us safely at version 2.2, we are now seriously considering 4.3.1 as our next big migration.

When I revisited https://github.com/eXist-db/exist/issues/35 I found the error changed. When you run sm:passwd as non-dba user on your own account you'll get:

exerr:ERROR org.exist.security.PermissionDeniedException: Permission denied to open collection: /db/system/security/exist/accounts by testuser

It basically means I'm unable to offer a user to change their own password. Is this regression? At the very least there could have been a better error message in place.

duncdrum commented 6 years ago

FYI @JoernT since you recently worked on user-manager features.
Looks like a regression against #36

line-o commented 5 years ago

@ahenket you could still enable users to change their password. Have a specific script run with a sticky user or group bit set or with executing a code block with system:as-user.

joewiz commented 5 years ago

The original fix to this, #36, by @shabanovd, lacked a test, so I can imagine a regression could've slipped in without anyone noticing. I haven't seen Dmitriy lately, so I'm not sure he's available to look into this. @adamretter Since you reviewed and merged #36, do you have any ideas?

ahenket commented 5 years ago

@ahenket you could still enable users to change their password. Have a specific script run with a sticky user or group bit set or with executing a code block with system:as-user.

We run open source code, so I would need something where the dba password doesn't live in the code. Sure it's doable, thanks for pointing out the suggestion, but really a proper fix is preferable.

line-o commented 5 years ago

@ahenket you set the sticky group or user bit in post-install via xmldb:chmod. Since the installer runs with the appropriate permissions no password has to given at that point.

ahenket commented 5 years ago

There’s no installer at play. There are manually registered users with a password set by me because I’m unable to offer them a way to register themselves and determine their own password

line-o commented 5 years ago

You can set the user or group bit in exide as well.

screenshot 2018-11-05 at 23 06 15

PS: A password reset script will always need this or something similar, since the application should not run as an administrator and the user cannot be logged in when the password is unknown.

ahenket commented 5 years ago

Maybe I don't understand the sticky bit but I suppose I set up an xquery with it using this code:

let $privilegeduser := 'dba'
let $privilegedpass := 'dbapass'

let $actualuser := xmldb:get-current-user()
let $actualnewpass := request:get-data()/pass

let $doupdate := system:as-user($privilegeduser, $privilegedpass, sm:passwd($actualuser,$actualnewpass))

How does the sticky bit help me to prevent having to have the 'dbapass' inside my code? I appreciate the idea of a workaround, but I think I'll wait this one out for a real fix in eXist-db

line-o commented 5 years ago

I do understand it is not only useful but necessary for exist-db to allow logged in users to change their passwords. I misread your issue for a password reset function, which has to deal with a user that is not able to log in anymore and therefore needs to 'borrow' privilege for this task. I created a rough sketch of the 'forgot-' or 'reset-password' action here: https://gist.github.com/line-o/0aa2901efd535a931a784180d15cb0f8 It is nowhere near production-ready, but should highlight the sticky bit usage.

JoernT commented 5 years ago

@ahenket i fully agree that this feature is missing in eXist-db. I have a similar issue in an app and will work on it. Maybe i can come up with a solution but can't promise it within days.

adamretter commented 5 years ago

@ahenket I think some things are being confused. I think @line-o means that you should use the setUid/setGid bits, NOT the sticky bit. They allow an XQuery to be executed as a different user, so if a .xq file is owner by a user in the DBA group or by the DBA group, and has the setUid/setGid bit, it will execute as though it has DBA permissions, and then you don't need the system:as-user call at all.

adamretter commented 5 years ago

@ahenket Regards the permissions problem you originally mentioned, can you please tell me the user/group/mode of both /db/system/security/exist/accounts and /db/system/security/exist/accounts/testuser.xml in your system?

adamretter commented 5 years ago

It looks to me like this change happened in 2012 for eXist-db 2.0-RC2 (and was pre-GitHub), see https://github.com/eXist-db/exist/commit/8493b454#diff-e62f7d6102f35961d2376efa40ed5993L40

ahenket commented 5 years ago

sm:get-permissions(xs:anyURI('/db/system/security/exist/accounts')) and sm:get-permissions(xs:anyURI('/db/system/security/exist/accounts/testuser.xml'))

<sm:permission xmlns:sm="http://exist-db.org/xquery/securitymanager" 
  owner="SYSTEM" group="dba" mode="rwxrwx---">
  <sm:acl entries="0"/>
</sm:permission>

@adamretter Setting the setUid/setGid bit on the xquery like you describe just might work. Maybe that's what @line-o meant too, in which case I just misunderstood. I'll see where that takes me.

adamretter commented 5 years ago

So indeed, currently the way eXist-db is setup by default, only a DBA can access user-credentials. You can use a setUid/setGid XQuery to manage password changes though...

Whether the current way eXist-db is setup is certainly up for discussion. However, it looks like we pretty much kinda follow what current modern Linux does in an eXist-db way.

For example, on Ubuntu 18.04, the user credentials are stored in two files:

$ ls -la /etc/passwd /etc/shadow
-rw-r--r-- 1 root root   1353 Jul  8 12:53 /etc/passwd
-rw-r----- 1 root shadow  814 Jul  8 12:53 /etc/shadow

/etc/passwd is publicly readable, but does not contain the passwords. /etc/shadow contains the password hashes but is only readable by the root user or the shadow group.

So how does a self-service password change work on Linux, if we look at the passwd command, we see this:

ls -la /usr/bin/passwd
-rwsr-xr-x 1 root root 59640 Jan 25  2018 /usr/bin/passwd

Note that the setUid bit is set on /usr/bin/passwd, so the command when run will actually execute with the effective user set as root, which is how a user can modify their own password entry. I think the internals of the command itself likely prevent one user from modifying another users password. This is very similar to how you could create an XQuery Script with the setUid bit set for self-service password management.

In eXist-db we want to avoid two things:

  1. Exposing password hashes, as this would make the system vulnerable to users cracking other users passwords via brute-force or Rainbow Tables etc, or impersonating them via replay attacks.
  2. Letting one user discover what other users are present in the system. This is permitted in Linux as all user accounts are stored in the same file, we could relax this constraint, but it is rather a nice thing to have.

So, we could modify the default permissions on the /db/system/security/exist/accounts Collection or on the individual user accounts stored in the documents within, but I would want to make sure the above constraints hold.

dizzzz commented 5 years ago

@adamretter knowing userids is half of the 'attack vector' ; i'd day try to keep it shielded as we server everything via an easily accessible HTTP(s) interface. Accessing a Linux box via SSH already provides added security probably.

adamretter commented 4 years ago

@ahenket Can we close this one now?

ahenket commented 4 years ago

The default on Linux systems is that users can always change their own password. I think that is an important feature for eXist too. So if the default settings of eXist behave like the default settings in eXist-db 5.2.x, then this ticket may be closed, unless you have a different ticket tracking that change. Could you point me to the ticket that tracks the change if relevant?

adamretter commented 4 years ago

So if the default settings of eXist behave like the default settings in eXist-db 5.2.x

@ahenket I am not sure I follow what you are saying here

line-o commented 4 years ago

@adamretter I understand that it should be possible for user A to call

sm:passwd('A', 'xxxxxxxxxxxxxxxx')

while logged in as A and the function to check if a) the user is part of DBA or b) be logged in as that user. If true, then change the password.

ahenket commented 4 years ago

Default for eXist-db is dba only. I though we were all in agreement that this should be changed the way @line-o stated.

adamretter commented 4 years ago

@line-o @ahenket eXist-db already allows a non-DBA user to change their own password using sm:passwd - https://github.com/eXist-db/exist/blob/develop/exist-core/src/main/java/org/exist/xquery/functions/securitymanager/AccountManagementFunction.java#L162

So I think we are good.

ahenket commented 4 years ago

@adamretter:

sm:passwd('testuser', 'test2')

yields:

exerr:ERROR org.exist.security.PermissionDeniedException: Permission denied to open collection: /db/system/security/exist/accounts by testuser [at line 3, column 1]

I don't think it was fully implemented or did regression kick in? eXist-db 5.2