checkNewPassword and ignoreCurrentPassword

[Jacques Le Roux]

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

[grv]

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 <
[hidden email]> 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
>
>

[James Yong-2]

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 <[hidden email]> 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
>
>

[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 <
> [hidden email]> 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
>>
>>

[Jacques Le Roux]

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

[Jacques Le Roux]

Hi James,

Inline...

Le 13/07/2020 à 08:36, James Yong a écrit :
> 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.

The OFBiz specific CSRF defense exists only in trunk because it was hard to backport. The vulnerability reports are always done against our stable
version.
Anyway, all our supported versions (including trunk) have the cookie same attribute in place. So we have a solid indisputable CSRF defense in place.
As explained in security properties we don't need to activate the OFBiz specific CSRF defense except in 2 cases:

    # -- CSRF defense strategy.
    # -- Because OOTB OFBiz  also sets the SameSite attribute to 'strict' for all cookies,
    # -- which is an effective CSRF defense,
    # -- default is org.apache.ofbiz.security.NoCsrfDefenseStrategy if not specified.
    # -- Use org.apache.ofbiz.security.CsrfDefenseStrategy
    # -- if you need to use a 'lax' for SameSiteCookieAttribute.
    # --
    # -- Or if you, or your users, use, or may use, a browser version that
    # -- is not supporting the SameSite cookie attribute
    # -- You may refer to https://caniuse.com/#feat=same-site-cookie-attribute

> 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

I did not read the article yet, but checking current password  is done but not when the user is an admin or "system" user (see
LoginServices::updatePassword). The question is: should we do the same when the user an admin or "system" user?

Thanks

Jacques

>
> Regards,
> James
>
> On 2020/07/12 11:07:59, Jacques Le Roux <[hidden email]> 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
>>
>>

[Jacques Le Roux]

Le 13/07/2020 à 14:50, Jacques Le Roux a écrit :
> 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 think it's OK as is and with the lazy consensus I will told the reporter so


> 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?
I guess it's the PARTYMGR security group and anyway it's unrelated, forget it

Jacques

[Jacques Le Roux]

Le 18/07/2020 à 11:34, Jacques Le Roux a écrit :

> Le 13/07/2020 à 14:50, Jacques Le Roux a écrit :
>> 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 think it's OK as is and with the lazy consensus I will told the reporter so
>
>
>> 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?
> I guess it's the PARTYMGR security group and anyway it's unrelated, forget it
>
> Jacques
>

TO add to this and thanks to James reference to

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

They say there:

  *

    Is the old password requested to complete the change?

    The most insecure scenario here is if the application permits the change of the password without requesting the current password. Indeed if an
    attacker is able to take control of a valid session they could easily change the victim’s password.

Anyway in case of an admin, if a valid session is in the hand of an attacker it's too late to do anything, all is open!

Jacques