Skip to content

Commit

Permalink
Fixed: Any ecommerce user has the ability to reset anothers password
Browse files Browse the repository at this point in the history
(including admin) via "Forget Your Password"
(OFBIZ-4361)

Currently, any user (via ecommerce "Forget Your Password") has the ability to 
reset another users password, including "admin" without permission.  
By simply entering "admin" and clicking "Email Password", the following is 
displayed:

The following occurred:
A new password has been created and sent to you. Please check your Email.

This now forces the user of the ERP to change their password.  
It is also possible to generate a dictionary attack against ofbiz because there 
is no capta code required.  This is serious security risk.

I have modified the patch following comments I made in the Jira, notably
  Removed unused Java variables
  Removed a check in LoginEvents::forgotPassword which prevented to show error
    messages
  Changed fr and en SecurityExtPasswordSentToYou 
    + SecurityExtThisEmailIsInResponseToYourRequestToHave labels 
    + template PasswordEmail.ftl
    + loginservices.token_incorrect labels
  Added fr and en SecurityExtIgnoreEmail + SecurityExtLinkOnce labels
  Removed changes in general.properties
  I did not remove the 2 GetSecurityQuestion.ftl files (webpos one was still in)

There is still room for improvement. I'll discuss them on the Jira and dev
ML. But this version is already strong enough to not wait that the patch is 
inapplicable!

Thanks: mz4wheeler (Mike Z) for the Jira, Nicolas Malin for the patch, I guess 
with some Gil's help, and all others for comments and ideas

git-svn-id: https://svn.apache.org/repos/asf/ofbiz/ofbiz-framework/trunk@1866478 13f79535-47bb-0310-9956-ffa450edef68
  • Loading branch information
JacquesLeRoux committed Sep 5, 2019
1 parent 180ca71 commit 7d7a441
Show file tree
Hide file tree
Showing 23 changed files with 535 additions and 443 deletions.
32 changes: 7 additions & 25 deletions applications/securityext/config/EmailPasswordUiLabels.xml
Original file line number Diff line number Diff line change
Expand Up @@ -33,36 +33,18 @@
<value xml:lang="zh">一个新的</value>
<value xml:lang="zh-TW">一個新的</value>
</property>
<property key="SecurityExtNewPasswordMssgEncryptionOff">
<value xml:lang="da">Deres adgangskode er:</value>
<value xml:lang="de">Ihr Passwort ist: </value>
<value xml:lang="en">Your password is :- </value>
<value xml:lang="fr">Votre mot de passe est : </value>
<value xml:lang="it">La tua password è :- </value>
<value xml:lang="ja">あなたのパスワードは :-</value>
<value xml:lang="nl">Uw wachtwoord is :</value>
<value xml:lang="pt-BR">Sua senha é:</value>
<value xml:lang="zh">你的密码是:- </value>
<value xml:lang="vi">Mật khẩu của bạn :-</value>
<value xml:lang="zh-TW">你的密碼是 :- </value>
<property key="SecurityExtIgnoreEmail">
<value xml:lang="en">Please ignore this email if you did not request a password change</value>
<value xml:lang="fr">Veuillez ignorer ce courriel si vous n'avez pas demandé un changement de mot de passe</value>
</property>
<property key="SecurityExtNewPasswordMssgEncryptionOn">
<value xml:lang="da">Deres nye adgangskode er :- </value>
<value xml:lang="de">Ihr neues Passwort ist : </value>
<value xml:lang="en">Your new password is :- </value>
<value xml:lang="fr">Votre nouveau mot de passe est : </value>
<value xml:lang="it">La nuova password è :- </value>
<value xml:lang="ja">あなたの新しいパスワードは :-</value>
<value xml:lang="nl">Uw nieuwe wachtwoord is : </value>
<value xml:lang="pt-BR">Sua nova senha é:</value>
<value xml:lang="zh">你的新密码是:- </value>
<value xml:lang="zh-TW">你的新密碼是 :- </value>
<value xml:lang="vi">Mật khẩu mới của bạn :-</value>
<property key="SecurityExtLinkOnce">
<value xml:lang="en">This link can be used only once</value>
<value xml:lang="fr">Ce lien ne peut être utilisé qu'une seule fois</value>
</property>
<property key="SecurityExtPasswordSentToYou">
<value xml:lang="da">kodeord er sendt til Dem</value>
<value xml:lang="de">Passwort das Ihnen zugesendet wurde</value>
<value xml:lang="en">password sent to you</value>
<value xml:lang="en">password</value>
<value xml:lang="fr">mot de passe</value>
<value xml:lang="it">password inviata a te</value>
<value xml:lang="ja">パスワードを送信しました</value>
Expand Down

Large diffs are not rendered by default.

7 changes: 5 additions & 2 deletions applications/securityext/template/email/PasswordEmail.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,15 @@ under the License.
<head>
</head>
<body>
<div>${uiLabelMap.SecurityExtThisEmailIsInResponseToYourRequestToHave} <#if useEncryption>${uiLabelMap.SecurityExtANew}<#else>${uiLabelMap.SecurityExtYour}</#if> ${uiLabelMap.SecurityExtPasswordSentToYou}.</div>
<div>${uiLabelMap.SecurityExtThisEmailIsInResponseToYourRequestToHave} ${uiLabelMap.SecurityExtANew} ${uiLabelMap.SecurityExtPasswordSentToYou}.</div>
<div>${uiLabelMap.SecurityExtIgnoreEmail}.</div>

<br />
<div>
<form method="post" action="${baseEcommerceSecureUrl}/partymgr/control/passwordChange?USERNAME=${userLogin.userLoginId!}&password=${password!}&forgotPwdFlag=true&tenantId=${tenantId!}" name="loginform" id="loginform" target="_blank">
<form method="post" action="${baseEcommerceSecureUrl}/partymgr/control/passwordChange?USERNAME=${userLogin.userLoginId!}&TOKEN=${token!}&forgotPwdFlag=true&tenantId=${tenantId!}" name="loginform" id="loginform" target="_blank">
<input type="submit" name="submit" value="${uiLabelMap.ResetPassword}" />
</form>
${uiLabelMap.SecurityExtLinkOnce}.
</div>
</body>
</html>
2 changes: 1 addition & 1 deletion build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ dependencies {
implementation 'oro:oro:2.0.8'
implementation 'wsdl4j:wsdl4j:1.6.3'
implementation 'org.jsoup:jsoup:1.12.1'
implementation 'io.jsonwebtoken:jjwt:0.9.1'
implementation 'com.auth0:java-jwt:3.8.2'
implementation 'org.json:json:20140107'
testImplementation 'org.hamcrest:hamcrest:2.1'
testImplementation 'org.hamcrest:hamcrest-library:2.1' // Enable junit4 to not depend on hamcrest-1.3
Expand Down
21 changes: 21 additions & 0 deletions framework/common/config/SecurityextUiLabels.xml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,23 @@
under the License.
-->
<resource xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="http://ofbiz.apache.org/dtds/ofbiz-properties.xsd">

<property key="loginevents.change_password_request_error_missing_fields">
<value xml:lang="en">Error accessing password: ${errorMessage}.</value>
<value xml:lang="es">Merci de renseigner tous les champs </value>
</property>
<property key="loginevents.change_password_request_error_not_valid_parameters">
<value xml:lang="en">Error accessing password: ${errorMessage}.</value>
<value xml:lang="fr">La demande de changement de mot de passe n'est pas valide.</value>
</property>
<property key="loginevents.change_password_request_error_technical_error">
<value xml:lang="en">Error accessing password: ${errorMessage}.</value>
<value xml:lang="fr">Une erreur technique c'est produite. La nouveau mot de passe n'a pas été pris en compte</value>
</property>
<property key="loginevents.change_password_request_success">
<value xml:lang="en">Error accessing password: ${errorMessage}.</value>
<value xml:lang="fr">Le nouveau mot de passe est actif</value>
</property>
<property key="loginevents.error_accessing_password">
<value xml:lang="de">Fehler beim Zugriff auf Passwort: ${errorMessage}.</value>
<value xml:lang="en">Error accessing password: ${errorMessage}.</value>
Expand Down Expand Up @@ -844,6 +861,10 @@
<value xml:lang="zh">自${disabledDateTime}以来。</value>
<value xml:lang="zh-TW">從 ${disabledDateTime} 開始.</value>
</property>
<property key="loginservices.token_incorrect">
<value xml:lang="en">Invalid Token</value>
<value xml:lang="fr">Jeton non valide</value>
</property>
<property key="loginservices.user_not_found">
<value xml:lang="de">Benutzer konnte nicht gefunden werden</value>
<value xml:lang="en">User not found.</value>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@

import org.apache.ofbiz.base.crypto.HashCrypt;
import org.apache.ofbiz.base.util.Debug;
import org.apache.ofbiz.base.util.StringUtil;
import org.apache.ofbiz.base.util.UtilCodec;
import org.apache.ofbiz.base.util.UtilDateTime;
import org.apache.ofbiz.base.util.UtilMisc;
import org.apache.ofbiz.base.util.UtilProperties;
Expand All @@ -61,6 +63,7 @@
import org.apache.ofbiz.service.LocalDispatcher;
import org.apache.ofbiz.service.ModelService;
import org.apache.ofbiz.service.ServiceUtil;
import org.apache.ofbiz.webapp.control.JWTManager;
import org.apache.ofbiz.webapp.control.LoginWorker;

import org.apache.tomcat.util.res.StringManager;
Expand Down Expand Up @@ -112,14 +115,18 @@ public static Map<String, Object> userLogin(DispatchContext ctx, Map<String, ?>
if (password == null) {
password = (String) context.get("password");
}
String jwtToken = (String) context.get("login.token");
if (jwtToken == null) {
jwtToken = (String) context.get("token");
}

// get the visitId for the history entity
String visitId = (String) context.get("visitId");

String errMsg = "";
if (UtilValidate.isEmpty(username)) {
errMsg = UtilProperties.getMessage(resource,"loginservices.username_missing", locale);
} else if (UtilValidate.isEmpty(password)) {
} else if (UtilValidate.isEmpty(password) && UtilValidate.isEmpty(jwtToken)) {
errMsg = UtilProperties.getMessage(resource,"loginservices.password_missing", locale);
} else {

Expand Down Expand Up @@ -223,11 +230,17 @@ public static Map<String, Object> userLogin(DispatchContext ctx, Map<String, ?>
// in the usage of userLogin service in ICalWorker.java and XmlRpcEventHandler.java.
useTomcatSSO = useTomcatSSO && (request!=null);

// resolve the key for decrypt the token and control the validity
boolean jwtTokenValid = SecurityUtil.authenticateUserLoginByJWT(delegator, username, jwtToken);

// 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
// if externalAuth passed; this is run as well
if ((!authFatalError && externalAuth) || (useTomcatSSO ? TomcatSSOLogin(request, username, password) : checkPassword(userLogin.getString("currentPassword"), useEncryption, password) )) {
Debug.logVerbose("[LoginServices.userLogin] : Password Matched", module);
if ((!authFatalError && externalAuth)
|| (useTomcatSSO && TomcatSSOLogin(request, username, password))
|| (jwtToken != null && jwtTokenValid)
|| (password != null && checkPassword(userLogin.getString("currentPassword"), useEncryption, password))) {
Debug.logVerbose("[LoginServices.userLogin] : Password Matched or Token Validated", module);

// update the hasLoggedOut flag
if (hasLoggedOut == null || hasLoggedOut) {
Expand Down Expand Up @@ -269,7 +282,8 @@ public static Map<String, Object> userLogin(DispatchContext ctx, Map<String, ?>

Debug.logInfo("[LoginServices.userLogin] : Password Incorrect", module);
// password invalid...
errMsg = UtilProperties.getMessage(resource,"loginservices.password_incorrect", locale);
if (password != null) errMsg = UtilProperties.getMessage(resource,"loginservices.password_incorrect", locale);
else if (jwtToken != null) errMsg = UtilProperties.getMessage(resource,"loginservices.token_incorrect", locale);

// increment failed login count
Long currentFailedLogins = userLogin.getLong("successiveFailedLogins");
Expand Down Expand Up @@ -745,6 +759,16 @@ public static Map<String, Object> updatePassword(DispatchContext ctx, Map<String
userLoginId = loggedInUserLogin.getString("userLoginId");
}

GenericValue userLoginToUpdate;

try {
userLoginToUpdate = EntityQuery.use(delegator).from("UserLogin").where("userLoginId", userLoginId).queryOne();
} catch (GenericEntityException e) {
Map<String, String> messageMap = UtilMisc.toMap("errorMessage", e.getMessage());
errMsg = UtilProperties.getMessage(resource,"loginservices.could_not_change_password_read_failure", messageMap, locale);
return ServiceUtil.returnError(errMsg);
}

// <b>security check</b>: userLogin userLoginId must equal userLoginId, or must have PARTYMGR_UPDATE permission
// NOTE: must check permission first so that admin users can set own password without specifying old password
// TODO: change this security group because we can't use permission groups defined in the applications from the framework.
Expand All @@ -753,6 +777,9 @@ public static Map<String, Object> updatePassword(DispatchContext ctx, Map<String
errMsg = UtilProperties.getMessage(resource,"loginservices.not_have_permission_update_password_for_user_login", locale);
return ServiceUtil.returnError(errMsg);
}
if (UtilValidate.isNotEmpty(context.get("login.token"))) {
adminUser = SecurityUtil.authenticateUserLoginByJWT(delegator, userLoginId, (String) context.get("login.token"));
}
} else {
adminUser = true;
}
Expand All @@ -762,16 +789,6 @@ public static Map<String, Object> updatePassword(DispatchContext ctx, Map<String
String newPasswordVerify = (String) context.get("newPasswordVerify");
String passwordHint = (String) context.get("passwordHint");

GenericValue userLoginToUpdate = null;

try {
userLoginToUpdate = EntityQuery.use(delegator).from("UserLogin").where("userLoginId", userLoginId).queryOne();
} catch (GenericEntityException e) {
Map<String, String> messageMap = UtilMisc.toMap("errorMessage", e.getMessage());
errMsg = UtilProperties.getMessage(resource,"loginservices.could_not_change_password_read_failure", messageMap, locale);
return ServiceUtil.returnError(errMsg);
}

if (userLoginToUpdate == null) {
// this may be a full external authenticator; first try authenticating
boolean authenticated = false;
Expand Down
25 changes: 9 additions & 16 deletions framework/common/webcommon/WEB-INF/common-controller.xml
Original file line number Diff line number Diff line change
Expand Up @@ -85,23 +85,16 @@ under the License.
<event type="java" path="org.apache.ofbiz.securityext.login.LoginEvents" invoke="forgotPassword"/>
<response name="success" type="view" value="forgotPassword"/>
<response name="error" type="view" value="forgotPassword"/>
<response name="auth" type="request-redirect" value="main" />
</request-map>
<request-map uri="forgotPassword_step1">
<security https="true" auth="false"/>
<response name="success" type="view" value="forgotPassword_step1"/>
</request-map>
<request-map uri="forgotPassword_step2">
<security auth="false" https="true"></security>
<response name="success" type="view" value="forgotPassword_step2" />
<request-map uri="forgotPasswordReset">
<security https="true" auth="false" />
<event type="java" path="org.apache.ofbiz.securityext.login.LoginEvents" invoke="changePasswordRequest"/>
<response name="success" type="request-redirect" value="main" />
<response name="error" type="view" value="forgotPassword" />
</request-map>
<request-map uri="forgotPassword_step3">
<security https="true" auth="false"/>
<event type="java" path="org.apache.ofbiz.securityext.login.LoginEvents" invoke="forgotPassword"/>
<response name="success" type="view" value="login"/>
<response name="error" type="view" value="forgotPassword_step2"/>
</request-map>
<request-map uri="passwordChange">
<security https="true" auth="false"/>
<security https="true" auth="true"/>
<response name="success" type="view" value="requirePasswordChange"/>
</request-map>
<request-map uri="view">
Expand Down Expand Up @@ -360,6 +353,6 @@ under the License.
<view-map name="LookupGeo" type="screen" page="component://common/widget/LookupScreens.xml#LookupGeo"/>
<view-map name="LookupGeoName" type="screen" page="component://common/widget/LookupScreens.xml#LookupGeoName"/>
<view-map name="LookupLocale" type="screen" page="component://common/widget/LookupScreens.xml#LookupLocale"/>
<view-map name="forgotPassword_step1" type="screen" page="component://common/widget/CommonScreens.xml#forgotPassword_step1"/>
<view-map name="forgotPassword_step2" type="screen" page="component://common/widget/CommonScreens.xml#forgotPassword_step2"/>
<view-map name="forgotPassword" type="screen" page="component://common/widget/CommonScreens.xml#forgotPassword"/>

</site-conf>
28 changes: 0 additions & 28 deletions framework/common/widget/CommonScreens.xml
Original file line number Diff line number Diff line change
Expand Up @@ -271,34 +271,6 @@ under the License.
</section>
</screen>

<screen name="forgotPassword_step1">
<section>
<widgets>
<include-screen name="MinimalActions" />
<include-screen name="forgotPassword_step1" location="${groovy:commonScreenLocations.forgotPassword_step1?commonScreenLocations.forgotPassword_step1:commonDecoratorLocation}"/>
</widgets>
</section>
</screen>

<screen name="forgotPassword_step2">
<section>
<actions>
<set field="userLoginId" from-field="parameters.USERNAME"/>
<entity-and entity-name="UserLoginSecurityQuestion" list="securityQuestions">
<field-map field-name="userLoginId" />
</entity-and>
<set field="questionEnumId" from-field="securityQuestions[0].questionEnumId" />
<entity-one entity-name="Enumeration" value-field="securityQuestion">
<field-map field-name="enumId" from-field="questionEnumId"/>
</entity-one>
</actions>
<widgets>
<include-screen name="MinimalActions" />
<include-screen name="forgotPassword_step2" location="${groovy:commonScreenLocations.forgotPassword_step2?commonScreenLocations.forgotPassword_step2:commonDecoratorLocation}"/>
</widgets>
</section>
</screen>

<screen name="forgotPassword">
<section>
<widgets>
Expand Down
8 changes: 4 additions & 4 deletions framework/security/config/security.properties
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,6 @@ security.login.authorised.during.impersonate=false
# -- should we encrypt (SHA Hash) the password? --
password.encrypt=true

# -- set requirePasswordChange to true, after emailPassword --
password.email_password.require_password_change=true

# -- specify the type of hash to use for one-way encryption, will be passed to java.security.MessageDigest.getInstance() --
# -- options may include: SHA, PBKDF2WithHmacSHA1, PBKDF2WithHmacSHA256, PBKDF2WithHmacSHA384, PBKDF2WithHmacSHA512 and etc
password.encrypt.hash.type=SHA
Expand All @@ -98,6 +95,9 @@ password.encrypt.pbkdf2.iterations=10000
# -- SHOULD GENERALLY NOT BE TRUE FOR PRODUCTION SITES, but is useful for interim periods when going to password encryption --
password.accept.encrypted.and.plain=false

# -- set request life time after a password change (like email) in hours, set -1 if you want disable it --
password.request.change.timeout=24

# -- should we convert usernames and passwords to lowercase? (useful for case insensitive usernames and passwords) --
username.lowercase=false
password.lowercase=false
Expand Down Expand Up @@ -142,7 +142,7 @@ security.login.externalLoginKey.enabled=true
login.secret_key_string=Secret Key

# -- Time To Live of the token send to the external server in seconds, 10 seconds seems plenty enough OOTB. Custom projects might want set a lower value.
security.jwt.token.expireTime=10
security.jwt.token.expireTime=1800

# -- Enables the internal Single Sign On feature which allows a token based login between OFBiz instances
# -- To make this work you also have to configure a secret key with security.token.key
Expand Down
Loading

0 comments on commit 7d7a441

Please sign in to comment.