Le 12/07/2020 à 13:07, Jacques Le Roux a écrit :
Hi team,
We recently got a security report about checkNewPassword where it was claimed a
CSRF vulnerability because of ignoreCurrentPassword but I rejected it.
I have though added a comment in trunk to allow users to adds OFBiz specific
CSRF defense in case it would be needed (peculiar browsers):
https://github.com/apache/ofbiz-framework/commit/16c2d3521990caf5e8703cbb323ce1146c93b9ce/
The reporter then wrote
"Even if this is not a CSRF vulnerability, I still think it is an insecure
measure to not verify the current password when changing the password
of the system account. What do you think?"
His report was initially roughly :
*If it is a system account, current password would not be checked*
*
*
public static void checkNewPassword(GenericValue userLogin, String
currentPassword, String newPassword, String newPasswordVerify, String
passwordHint, List<String> errorMessageList, boolean ignoreCurrentPassword,
Locale locale) {
Delegator delegator = userLogin.getDelegator();
boolean useEncryption =
"true".equals(EntityUtilProperties.getPropertyValue("security",
"password.encrypt", delegator));
String errMsg = null;
if (!ignoreCurrentPassword) {
// if the password.accept.encrypted.and.plain property in
security is set to true allow plain or encrypted passwords
// if this is a system account don't bother checking the
passwords
boolean passwordMatches =
checkPassword(userLogin.getString("currentPassword"), useEncryption,
currentPassword);
if ((currentPassword == null) || (!passwordMatches)) {
errMsg =
UtilProperties.getMessage(resource,"loginservices.old_password_not_correct_reenter",
locale);
errorMessageList.add(errMsg);
}
if (checkPassword(userLogin.getString("currentPassword"),
useEncryption, newPassword)) {
errMsg =
UtilProperties.getMessage(resource,"loginservices.new_password_is_equal_to_old_password",
locale);
errorMessageList.add(errMsg);
}
}
The code and related calling code is easy to check and I don't really see an
issue with it.
@Jacopo, you put it in with
http://svn.apache.org/viewvc?view=revision&revision=739738 what is your opinion
about that?
And what is the team's opinion?
Thanks
Jacques
Something related I already shared in the security ML:
I guess we don't want to change (I don't mean the NOTE but the feature).
// NOTE: must check permission first so that admin users can set own
password without specifying old password
I also have a question to you Jacopo:
I'm not quite sure you meant with (https://s.apache.org/04dt9)
// TODO: change this security group because we can't use permission groups
defined in the applications from the framework.
Which security group is it about?
Thanks
Jacques
