Hi Jacques, There is a number of reports relating to CSRF. To reduce the number of false positive security alerts, I think the CSRF defense should be turned on in the demo.
I feel there should be additional verification like checking current password when the user is doing password change. Please see the section on "Test Password Change" at https://owasp.org/www-project-web-security-testing-guide/latest/4-Web_Application_Security_Testing/04-Authentication_Testing/09-Testing_for_Weak_Password_Change_or_Reset_Functionalities Regards, James On 2020/07/12 11:07:59, Jacques Le Roux <[email protected]> wrote: > 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 > >
