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.

Sign up for GitHub

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

Jump to bottom

Password Issue #2394

Closed
bambenek opened this issue Aug 11, 2017 · 44 comments
Closed

Password Issue #2394

bambenek opened this issue Aug 11, 2017 · 44 comments
Labels
T: bug Type: bug report: This issue describes unexpected behaviour

Comments

@bambenek
Copy link

I have a user trying to reset his password, my settings are default (12 characters and /((?=.\d)|(?=.\W+))(?![\n])(?=.[A-Z])(?=.[a-z]).*$/).

He is trying to set this and it doesn't work saying not matching complexity requirements:
mSW%vuc@ot85>?

and

aA9!$!@#$@#aeuc<.a

Related, I can't set a specific password for users as admin because it asks me to enter existing password. As admin I should be able to specify a password, right?

If you would like to report a bug, please fill the template bellow

Work environment

Questions Answers
Type of issue Bug
OS version (server) ubuntu
OS version (client) XP, Seven, 10, Ubuntu, ...
PHP version 5.4, 5.5, 5.6, 7.0, 7.1...
MISP version / git hash 2.4.78
Browser If applicable

Expected behavior

Actual behavior

Steps to reproduce the behavior

Logs, screenshots, configuration dump, ...
@iglocska
Copy link
Member

iglocska commented Aug 12, 2017 via email

@bambenek
Copy link
Author

bambenek commented Aug 12, 2017 via email

@iglocska
Copy link
Member

Weird, just tried the password above in my dev instance with default complexity settings and it seems to work fine...

@bambenek
Copy link
Author

bambenek commented Aug 14, 2017 via email

@RichieB2B
Copy link
Contributor

We are experiencing the same issue after we upgraded MISP to 2.4.78 (from 2.4.75). I think the problem is with the Blowfish patch 3317f56 Even through it contains a verifyPassword() that looks decent it is not always used:

2017-08-16 10:10:01 Warning: Warning (512): Invalid salt: {40xHEX} for blowfish Please visit http://www.php.net/crypt and read the appropriate section for building blowfish salts. in [/var/www/MISP/app/Lib/cakephp/lib/Cake/Utility/Security.php, line 323]
Trace:
ErrorHandler::handleError() - APP/Lib/cakephp/lib/Cake/Error/ErrorHandler.php, line 230
Security::_crypt() - APP/Lib/cakephp/lib/Cake/Utility/Security.php, line 323
Security::hash() - APP/Lib/cakephp/lib/Cake/Utility/Security.php, line 115
BlowfishPasswordHasher::check() - APP/Lib/cakephp/lib/Cake/Controller/Component/Auth/BlowfishPasswordHasher.php, line 45
BaseAuthenticate::_findUser() - APP/Lib/cakephp/lib/Cake/Controller/Component/Auth/BaseAuthenticate.php, line 138
FormAuthenticate::authenticate() - APP/Lib/cakephp/lib/Cake/Controller/Component/Auth/FormAuthenticate.php, line 79
AuthComponent::identify() - APP/Lib/cakephp/lib/Cake/Controller/Component/AuthComponent.php, line 771
AuthComponent::login() - APP/Lib/cakephp/lib/Cake/Controller/Component/AuthComponent.php, line 611
UsersController::login() - APP/Controller/UsersController.php, line 769
ReflectionMethod::invokeArgs() - [internal], line ??
Controller::invokeAction() - APP/Lib/cakephp/lib/Cake/Controller/Controller.php, line 491
Dispatcher::_invoke() - APP/Lib/cakephp/lib/Cake/Routing/Dispatcher.php, line 193
Dispatcher::dispatch() - APP/Lib/cakephp/lib/Cake/Routing/Dispatcher.php, line 167
[main] - APP/webroot/index.php, line 92

@RichieB2B RichieB2B mentioned this issue Aug 16, 2017
Closed
@iglocska
Copy link
Member

iglocska commented Aug 16, 2017 via email

@RichieB2B
Copy link
Contributor

It seems that #2401 was unrelated to this issue after all. We are still experiencing this issue after #2401 was fixed.

@iglocska
Copy link
Member

iglocska commented Aug 17, 2017 via email

@RichieB2B
Copy link
Contributor

A user is complaining about it but I cannot reproduce it. Will troubleshoot some more..

@iglocska
Copy link
Member

Interesting. Just tried it with a read only user too (with the default settings accepted / empty) but same thing. Not sure what's up. Can you ask the user to paste what the flash message says on top after it fails?

@RichieB2B
Copy link
Contributor

The user is hitting /users/change_pw because I reset his password (so the change_pw field is set). He is getting this message:

The password could not be updated. Make sure you meet the minimum password length / complexity requirements.

This is from https://github.com/MISP/MISP/blob/2.4/app/Controller/UsersController.php#L158

@RichieB2B
Copy link
Contributor

Reading $this->User->validationErrors after the $this->User->save($user) reveals the problem:

Array
(
    [newsread] => Array
        (
            [0] => boolean
        )

)

CakePHP is expecting a boolean where the users table is filled with epoch timestamps in this column. How did that happen?

@iglocska
Copy link
Member

iglocska commented Aug 17, 2017 •
edited
Loading

Interesting. OK, didn't realise it was the change_pw function, I kept using the user edit to change it. Looking into it. I might be a bit slow with it though, stuck in a literal 2 day conf call.

@iglocska
Copy link
Member

Reproduced, fix incoming.

@iglocska
Copy link
Member

Well I have no clue how that happened. Even more so how the newsread field worked in the first place o.O

@RichieB2B
Copy link
Contributor

I've created #2405 so we can use change_pw more. :-)

@bambenek
Copy link
Author

FYI, I'm still having the password reset issue on my MISP instance after pulling from master for latest code.

@iglocska
Copy link
Member

Which part? The regex failing?

@bambenek
Copy link
Author

bambenek commented Aug 24, 2017 via email

@iglocska
Copy link
Member

Could you paste the commit ID you're on? Is it the latest? Do you have the silly

Security.require_password_confirmation

setting enabled?

@bambenek
Copy link
Author

bambenek commented Aug 24, 2017 via email

@RichieB2B
Copy link
Contributor

Check your newsread column in the users MySQL table. If it contains a timestamp you have the same issue I did.

@bambenek
Copy link
Author

bambenek commented Aug 24, 2017 via email

@iglocska
Copy link
Member

Wait, newsread is supposed to contain a timestamp...

@bambenek
Copy link
Author

bambenek commented Aug 24, 2017 via email

@iglocska
Copy link
Member

iglocska commented Aug 24, 2017 •
edited
Loading

Null is weird. Default should be 0, could be a carry-over from old versions, maybe that's indeed the culprit.

Basically the way it works: If user's newsread < timestamp of the last news item, redirect the user to the news section (which updates the user's newsread to the current timestamp). Null might cause issues. Could you try to run:

Update users set newsread = 0 where newsread is NULL;

and see if it fixes it?

@RichieB2B
Copy link
Contributor

Set your newsread to 0 and try to change your password. CakePHP somehow thinks it should be a Boolean value and refuses to save the data if it isn't.

@iglocska
Copy link
Member

Not boolean, but an integer. The validation checks for numeric values, the null must be a carry over from an older version.

@iglocska
Copy link
Member

Just pushed a possible quick fix.

@iglocska
Copy link
Member

Time to hit the sack, will check back in the morning.

@bambenek
Copy link
Author

I both updated mysql and did another pull, verified it works now. Thanks!

@adulau adulau added the T: bug Type: bug report: This issue describes unexpected behaviour label Aug 25, 2017
@RichieB2B
Copy link
Contributor

14d5b04 will prevent NULL values in newsread to become a problem, but you are missing the real issue. In the datamodel newsread is defined as boolean yet timestamps are stored in it. This causes the save() function to fail the data model verification.

I'll do a one liner PR again. :)

RichieB2B added a commit to RichieB2B/MISP that referenced this issue Aug 25, 2017
@RichieB2B
Make newsread numeric instead of boolean
ee5af8d 
Fixes MISP#2394
@RichieB2B RichieB2B mentioned this issue Aug 25, 2017
Merged
@iglocska
Copy link
Member

That is just the name of the verification (which is indeed incorrect), the actual rule used is numeric so it shouldn't make a difference ;)

However, good idea to fix the name!

@RichieB2B
Copy link
Contributor

Then I am totally lost. A user with this problem had a timestamp as newsread. He was unable to change his password. Troubleshooting revealed the failed verification. I set his newsread to 0 and the problem was gone.

@iglocska
Copy link
Member

The validation was fixed like a week ago, was this before or after?

@RichieB2B
Copy link
Contributor

This was last Friday..

@iglocska
Copy link
Member

Hmph. Could you reproduce it now?

@RichieB2B
Copy link
Contributor

Hmph. Nope, even with a timestamp as newsread it now works fine.

@iglocska
Copy link
Member

Good ;)

@RichieB2B
Copy link
Contributor

Ah, now I see: I must have been running without the fix in e0de52a last Friday. I suppose this issue is now dead and buried. ;-)

@iglocska
Copy link
Member

That's how all issues should be, dead and buried that is! :)))

@bambenek
Copy link
Author

bambenek commented Aug 25, 2017 via email

@gecko97
Copy link

gecko97 commented Mar 14, 2018

Hi there,
I am having the same issue when n user tries to set a new password from the GUI.

Currently installed version… v2.4.88
Ubuntu 16.04

The password could not be updated. Make sure you meet the minimum password length / complexity requirements.

despite I am indeed setting a complex password as required, and the mentioned fix (e0de52a) is in place.
I have no errors details in error.log.

What I have noticed is when I hover the mouse on the Password information icon ("i") nothing appears, It looks like the javascript PasswordPopover is not being loaded.

Any ideas?
Thanks

@uberbigun
Copy link

Same issue with MISP on Ubuntu 16.04 and the server built from scratch today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
T: bug Type: bug report: This issue describes unexpected behaviour
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@adulau @RichieB2B @iglocska @bambenek @uberbigun @gecko97