Hi Jacques I think the vulnerability does not exist if the CSRF defence is in place. If there is no defence in place, there is a possibility of using system account session to change the admin password.
As for bypassing current password check if the user is admin, it won't hurt if the check was in place for system account as well to check the current password. I could be wrong so we need others opinion as well. My 2 cents. Best Regards, Girish On Sun, Jul 12, 2020 at 4:38 PM Jacques Le Roux < jacques.le.r...@les7arts.com> 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 > >
