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

Insecure password change function. Fixes #5538 #5550

Merged
merged 2 commits into from
Apr 13, 2021

Conversation

josegar74
Copy link
Member

  • Update API service to request the old password.
  • Update the Administration UI to allow reset the password of the logged user. Administrators could also use the API service to reset the password of other users, but require to know the old user password, a bit unlikely. I don't see a reason for this as a user can reset his password or recover it.

@josegar74 josegar74 force-pushed the insecure_password_change_function branch from e6a5ed7 to e50e1e8 Compare March 30, 2021 06:26
Copy link
Contributor

@archaeogeek archaeogeek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't had chance to fully test all possible cases but in general it seems to work just fine

@pvgenuchten
Copy link

pvgenuchten commented Mar 31, 2021

A script to add settings to the database is missing
Change password fails with a nullpointer if settings are unavailable
image

@josegar74
Copy link
Member Author

josegar74 commented Mar 31, 2021

@pvgenuchten the change doesn't use any setting. Can you add the stacktrace?

I just notice your UI is not updated, please check the wro4j cache.

@pvgenuchten
Copy link

Administrators could also use the API service to reset the password of other users, but require to know the old user password, a bit unlikely. I don't see a reason for this as a user can reset his password or recover it.

What Jo means is that when somebody walks to your screen, clicks change password and changes the password and can then use another machine to login with that password. For that case it is relevant also to provide the current password

Copy link
Member

@fxprunayre fxprunayre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but if there is no mail server available to use the recover my password then it will not be possible to reset a user password (it will require to go in the DB).

@archaeogeek what do you think about keeping the reset password for administrator users on all users if no mail server available (like https://github.com/geonetwork/core-geonetwork/blob/master/web-ui/src/main/resources/catalog/templates/signin.html#L98) ?

@fxprunayre fxprunayre merged commit 169b290 into geonetwork:4.0.x Apr 13, 2021
josegar74 added a commit that referenced this pull request Apr 21, 2021
* Insecure password change function. Fixes #5538

* Insecure password change function - unit tests. Fixes #5538
josegar74 added a commit to GeoCat/core-geonetwork that referenced this pull request Apr 21, 2021
…work#5550)

* Insecure password change function. Fixes geonetwork#5538

* Insecure password change function - unit tests. Fixes geonetwork#5538
@fxprunayre
Copy link
Member

@archaeogeek @josegar74 any ideas for a workaround for the issue "if there is no mail server available to use the recover my password then it will not be possible to reset a user password (it will require to go in the DB)." ? A number of users get stucked on this when they have no smtp server and no db access.

@archaeogeek
Copy link
Contributor

@fxprunayre I missed your comment before, sorry. Yes, I think that keeping the password reset option for administrators if there is no smtp or database access is fine.

@fxprunayre
Copy link
Member

@archaeogeek discussing with @josegar74 we were thinking adding a setting so we can turn this on or off. Is this fine for you?

@archaeogeek
Copy link
Contributor

@fxprunayre personally I agree. It might not pass a really strict penetration test though, because if an admin account is compromised, this setting could be changed, which would bypass this whole security fix.

@fxprunayre
Copy link
Member

Maybe we could have a setting which is not defined in database by default. So when setting up a catalogue, user can choose to add it:

-- WARNING: Security / Add this settings only if you need to allow admin
-- users to be able to reset user password. If you have mail server configured
-- user can reset password directly. If not, then you may want to add that settings
-- if you don't have access to the database.
-- INSERT INTO Settings (name, value, datatype, position, internal) 
-- VALUES ('system/security/password/allowAdminReset', 'false', 2, 12004, 'n');

What do you think @archaeogeek @josegar74 ?
So an installation without that setting will always be secured from this point of view.

@josegar74
Copy link
Member Author

josegar74 commented Apr 6, 2022

@fxprunayre if we integrate this PR: #6161, I think we can add it with the default false value, so can't be edited in the UI? (instead of requiring to be added manually, no?)

@fxprunayre
Copy link
Member

It is another level in between but if you know the API, you'll still be able to change the value ?

fxprunayre added a commit that referenced this pull request Apr 7, 2022
A new settings allows administrator to reset local user password.

This setting is not added to the database by default (for security
reason - see #5550).

Add it, only if you don't have mail server or if it is difficult to
access to the database.
fxprunayre added a commit that referenced this pull request Apr 7, 2022
A new settings allows administrator to reset local user password.

This setting is not added to the database by default (for security
reason - see #5550).

Add it, only if you don't have mail server or if it is difficult to
access to the database.
josegar74 pushed a commit that referenced this pull request Apr 12, 2022
A new settings allows administrator to reset local user password.

This setting is not added to the database by default (for security
reason - see #5550).

Add it, only if you don't have mail server or if it is difficult to
access to the database.
@juanluisrp juanluisrp mentioned this pull request Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants