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
Re: Welcome Arun Patidar as new PMC member
Congratulations and welcome Arun! Thanks Sharan On 2020/07/06 07:00:08, Jacques Le Roux wrote: > The OFBiz PMC has invited Arun Patidar to become member of the committee and > we are glad to announce that he has accepted the nomination. > > On behalf of the OFBiz PMC, welcome on board Arun! > >
Re: Welcome Suraj Khurana as new PMC member
Congratulations Suraj! Thanks Sharan On 2020/07/04 11:52:15, Jacques Le Roux wrote: > The OFBiz PMC has invited Suraj Khurana to become member of the committee and > we are glad to announce that he has accepted the nomination. > > On behalf of the OFBiz PMC, welcome on board Suraj! > >
Re: Welcome Aditya Sharma as new PMC member
Congratulations and welcome Aditya :-) Thanks Sharan On 2020/07/05 16:53:22, Jacques Le Roux wrote: > The OFBiz PMC has invited Aditya Sharma to become member of the committee and > we are glad to announce that he has accepted the nomination. > > On behalf of the OFBiz PMC, welcome on board Aditya! > >
