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