Re: checkNewPassword and ignoreCurrentPassword

[Jacques Le Roux]

Hi Girish,

Le 13/07/2020 à 05:48, Girish Vasmatkar a écrit :

Hi Jacques

I think the vulnerability does not exist if the CSRF defence is in place.

Yes I already answered the same to the reporter and he agreed.

If there is no defence in place, there is a possibility of using system
account session to change the admin password.

All our supported versions have the cookie same attribute in place, so no 
worries

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.

Yes, my thoughts too

Thanks

Jacques

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