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 decrypt to 256 characters #2685

Merged
merged 6 commits into from
Jan 13, 2023
Merged

Conversation

luigifab
Copy link
Collaborator

@luigifab luigifab commented Nov 6, 2022

Description

Limit decrypt to 256 characters. This fix #2245.
In production since 3 months.

OpenMage 20.0.16 / PHP 8.0.25

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)

@github-actions github-actions bot added the Component: Core Relates to Mage_Core label Nov 6, 2022
@elidrissidev
Copy link
Member

elidrissidev commented Nov 8, 2022

Note that Mage_Core_Helper_Data::validateHash may be used in many places including third-party extensions, it it really safe to assume every "password" must be less than 256 chars?

@luigifab
Copy link
Collaborator Author

luigifab commented Nov 8, 2022

By default it is also used by admin user password validation.
For us, this is not used by our third-party extensions, and we haven't see problems.

luigifab and others added 2 commits November 16, 2022 21:59
Co-authored-by: sv3n <github-sr@hotmail.com>
@sreichel
Copy link
Contributor

sreichel commented Jan 4, 2023

Note that Mage_Core_Helper_Data::validateHash may be used in many places including third-party extensions, it it really safe to assume every "password" must be less than 256 chars?

Do you or your code uses a password longer 256 chars?

kiatng
kiatng previously approved these changes Jan 4, 2023
Copy link
Contributor

@sreichel sreichel left a comment

Choose a reason for hiding this comment

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

Mhhh, better use a new constant?

Mage_Customer_Model_Customer::MAXIMUM_PASSWORD_LENGTH was not my best choice.

@kiatng
Copy link
Collaborator

kiatng commented Jan 4, 2023

Mhhh, better use a new constant?

Mage_Customer_Model_Customer::MAXIMUM_PASSWORD_LENGTH was not my best choice.

You are right. May be add Mage_Core_Model_App::ABSOLUTE_MAX_PASSWORD_LENGTH

@elidrissidev
Copy link
Member

Do you or your code uses a password longer 256 chars?

A hash is not necessary used for a password, can be any other value.

@sreichel
Copy link
Contributor

sreichel commented Jan 4, 2023

A hash is not necessary used for a password, can be any other value.

True , buts only used to validate passwords (and one api-key) in ...

  • Mage_Admin_Model_User
  • Mage_Api_Model_User
  • Mage_Customer_Model_Customer

@kiatng kiatng self-requested a review January 5, 2023 03:48
Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

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

ok by me, let's wait for @sreichel to un-block his change request

@sreichel
Copy link
Contributor

done

@fballiano fballiano merged commit f7be9cb into OpenMage:1.9.4.x Jan 13, 2023
@fballiano
Copy link
Contributor

merged and cherrypicked to v20

@luigifab luigifab deleted the validate-pass branch January 13, 2023 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Core Relates to Mage_Core security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No password length restriction leads to denial of service
6 participants