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

Limit maximum length of password #836

Merged
merged 6 commits into from
Dec 14, 2020
Merged

Conversation

fiammybe
Copy link
Member

Limit the length of the password to 99 positions. If the password is longer (and it can be sent using a direct request, the verification algorithm can take a long time to parse the value, leading to a potential DOS attack.

Issue identified as #1033373 by f1v3 on Cloudflare

@fiammybe fiammybe added the security vulnerability Security vulnerability detected by WhiteSource label Dec 10, 2020
@fiammybe fiammybe added this to the 1.4.2 milestone Dec 10, 2020
@fiammybe fiammybe requested a review from MekDrop December 10, 2020 22:38
@fiammybe fiammybe self-assigned this Dec 10, 2020
Copy link
Member

@MekDrop MekDrop left a comment

Choose a reason for hiding this comment

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

I think for this we will need to make some extra changes. F.e.: this should work also in password changing area and new user registration too. Also, not in only backend but also in frontend (see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/password#maxlength )

htdocs/include/checklogin.php Outdated Show resolved Hide resolved
@fiammybe fiammybe marked this pull request as draft December 14, 2020 20:40
@fiammybe
Copy link
Member Author

I noticed in the frontend that the password length fields are almost all - except the password reset one, that was sloppy :-). So we can safely assume that those that are using the system normally will not have a password length over 32. I will update the allowed length from 99 to 32 then.

@fiammybe fiammybe requested a review from MekDrop December 14, 2020 21:50
@fiammybe fiammybe marked this pull request as ready for review December 14, 2020 21:50
@fiammybe fiammybe merged commit 3aa86b2 into branches/impresscms_1.4 Dec 14, 2020
@fiammybe fiammybe deleted the impresscms-834 branch December 14, 2020 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security vulnerability Security vulnerability detected by WhiteSource
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants