Re: checkNewPassword and ignoreCurrentPassword
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
Re: checkNewPassword and ignoreCurrentPassword
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
Re: checkNewPassword and ignoreCurrentPassword
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 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 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=739738 what is your opinion
about that?
And what is the team's opinion?
Thanks
Jacques
Re: checkNewPassword and ignoreCurrentPassword
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 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=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
Re: checkNewPassword and ignoreCurrentPassword
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 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=739738 what is your
opinion about that?
And what is the team's opinion?
Thanks
Jacques
Re: checkNewPassword and ignoreCurrentPassword
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 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 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=739738 what is your
> opinion about that?
>
> And what is the team's opinion?
>
> Thanks
>
> Jacques
>
>
Re: checkNewPassword and ignoreCurrentPassword
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 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=739738 what is your
> opinion about that?
>
> And what is the team's opinion?
>
> Thanks
>
> Jacques
>
>
checkNewPassword and ignoreCurrentPassword
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 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=739738 what is your opinion
about that?
And what is the team's opinion?
Thanks
Jacques
