diff --git a/server/conf/mirth.properties b/server/conf/mirth.properties index 6b71ebdbf3..1188c4be64 100644 --- a/server/conf/mirth.properties +++ b/server/conf/mirth.properties @@ -21,6 +21,11 @@ password.graceperiod = 0 password.reuseperiod = 0 password.reuselimit = 0 +# When false (the default), failed logins return a generic "Incorrect username or password." +# message. Set to true to restore detailed messages that reveal account lockout state and +# remaining attempts. Warning: enabling this lets an attacker enumerate valid usernames. +password.allowdetailedautherrors = false + # Only used for migration purposes, do not modify version = 4.6.0 diff --git a/server/src/main/java/com/mirth/connect/model/PasswordRequirements.java b/server/src/main/java/com/mirth/connect/model/PasswordRequirements.java index c4276ff578..968e059ac8 100644 --- a/server/src/main/java/com/mirth/connect/model/PasswordRequirements.java +++ b/server/src/main/java/com/mirth/connect/model/PasswordRequirements.java @@ -29,6 +29,7 @@ public class PasswordRequirements implements Serializable { private int gracePeriod; private int reusePeriod; private int reuseLimit; + private boolean allowDetailedAuthErrors; public PasswordRequirements() { this.minLength = 0; @@ -42,6 +43,7 @@ public PasswordRequirements() { this.gracePeriod = 0; this.reusePeriod = 0; this.reuseLimit = 0; + this.allowDetailedAuthErrors = false; } public PasswordRequirements(int minLength, int minUpper, int minLower, int minNumeric, int minSpecial, int retryLimit, int lockoutPeriod, int expiration, int gracePeriod, int reusePeriod, int reuseLimit) { @@ -56,6 +58,7 @@ public PasswordRequirements(int minLength, int minUpper, int minLower, int minNu this.gracePeriod = gracePeriod; this.reusePeriod = reusePeriod; this.reuseLimit = reuseLimit; + this.allowDetailedAuthErrors = false; } public int getMinLength() { @@ -145,4 +148,12 @@ public int getReuseLimit() { public void setReuseLimit(int reuseLimit) { this.reuseLimit = reuseLimit; } + + public boolean getAllowDetailedAuthErrors() { + return allowDetailedAuthErrors; + } + + public void setAllowDetailedAuthErrors(boolean allowDetailedAuthErrors) { + this.allowDetailedAuthErrors = allowDetailedAuthErrors; + } } diff --git a/server/src/main/java/com/mirth/connect/server/controllers/DefaultUserController.java b/server/src/main/java/com/mirth/connect/server/controllers/DefaultUserController.java index dbd9a89492..4dcae531d5 100644 --- a/server/src/main/java/com/mirth/connect/server/controllers/DefaultUserController.java +++ b/server/src/main/java/com/mirth/connect/server/controllers/DefaultUserController.java @@ -15,6 +15,7 @@ import java.util.Map; import java.util.Properties; import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; import org.apache.commons.collections4.CollectionUtils; import org.apache.commons.lang3.StringUtils; @@ -43,6 +44,12 @@ public class DefaultUserController extends UserController { public static final String VACUUM_LOCK_PERSON_STATEMENT_ID = "User.vacuumPersonTable"; public static final String VACUUM_LOCK_PREFERENCES_STATEMENT_ID = "User.vacuumPersonPreferencesTable"; + private static final String INCORRECT_CREDENTIALS_MESSAGE = "Incorrect username or password."; + private static final String TIMING_EQUALIZATION_PASSWORD = "password used only to equalize authentication timing"; + + private final Object timingEqualizationLock = new Object(); + private volatile String timingEqualizationHash; + private final AtomicBoolean timingEqualizationFailureLogged = new AtomicBoolean(); private Logger logger = LogManager.getLogger(this.getClass()); private ExtensionController extensionController = null; @@ -301,15 +308,22 @@ public LoginStatus authorizeUser(String username, String plainPassword, String s boolean authorized = false; Credentials credentials = null; LoginRequirementsChecker loginRequirementsChecker = null; + PasswordRequirements passwordRequirements = ControllerFactory.getFactory().createConfigurationController().getPasswordRequirements(); + Digester digester = ControllerFactory.getFactory().createConfigurationController().getDigester(); // Retrieve the matching User User validUser = getUser(null, username); if (validUser != null) { - Digester digester = ControllerFactory.getFactory().createConfigurationController().getDigester(); loginRequirementsChecker = new LoginRequirementsChecker(validUser); if (loginRequirementsChecker.isUserLockedOut()) { - return new LoginStatus(LoginStatus.Status.FAIL_LOCKED_OUT, "User account \"" + username + "\" has been locked. You may attempt to login again in " + loginRequirementsChecker.getPrintableStrikeTimeRemaining() + "."); + equalizeAuthenticationTime(digester, plainPassword); + + if (passwordRequirements.getAllowDetailedAuthErrors()) { + return new LoginStatus(LoginStatus.Status.FAIL_LOCKED_OUT, "User account \"" + username + "\" has been locked. You may attempt to login again in " + loginRequirementsChecker.getPrintableStrikeTimeRemaining() + "."); + } else { + return new LoginStatus(LoginStatus.Status.FAIL, INCORRECT_CREDENTIALS_MESSAGE); + } } loginRequirementsChecker.resetExpiredStrikes(); @@ -322,14 +336,21 @@ public LoginStatus authorizeUser(String username, String plainPassword, String s if (Pre22PasswordChecker.checkPassword(plainPassword, credentials.getPassword())) { checkOrUpdateUserPassword(validUser.getId(), plainPassword); authorized = true; + } else { + // A pre-2.2 hash is checked with a cheap digest, so equalize the miss + equalizeAuthenticationTime(digester, plainPassword); } } else { authorized = digester.matches(plainPassword, credentials.getPassword()); } + } else { + // The account exists but has never had a password set + equalizeAuthenticationTime(digester, plainPassword); } + } else { + equalizeAuthenticationTime(digester, plainPassword); } - PasswordRequirements passwordRequirements = ControllerFactory.getFactory().createConfigurationController().getPasswordRequirements(); LoginStatus loginStatus = null; if (authorized) { @@ -392,12 +413,12 @@ public LoginStatus authorizeUser(String username, String plainPassword, String s } } else { LoginStatus.Status status = LoginStatus.Status.FAIL; - String failMessage = "Incorrect username or password."; + String failMessage = INCORRECT_CREDENTIALS_MESSAGE; if (loginRequirementsChecker != null) { loginRequirementsChecker.incrementStrikes(); - if (loginRequirementsChecker.isLockoutEnabled()) { + if (loginRequirementsChecker.isLockoutEnabled() && passwordRequirements.getAllowDetailedAuthErrors()) { if (loginRequirementsChecker.isUserLockedOut()) { status = LoginStatus.Status.FAIL_LOCKED_OUT; failMessage += " User account \"" + username + "\" has been locked. You may attempt to login again in " + loginRequirementsChecker.getPrintableStrikeTimeRemaining() + "."; @@ -629,6 +650,43 @@ public void removePreference(int id, String name) { } } + /** + * Performs a throwaway password digest so that a failed login costs roughly the same whether or + * not the username exists, and whether or not the account is locked out. The real digest is only + * reached for a valid, unlocked user, so without this the response time alone identifies which + * usernames are real regardless of what the response message says. + * + * Called on every path that skips the real digest: an unknown username, a locked account, an + * account with no stored credentials, and a failed pre-2.2 hash check. + * + * The throwaway hash is generated once from the live digester, so it follows the configured + * algorithm and iteration count rather than a value baked in here. + * + * ponytail: equalizes the digest only, not the extra credentials query a valid user costs. + * That gap measured under 1ms against a 237ms floor. Revisit if the digest cost ever drops + * far enough for a sub-millisecond difference to be readable. + */ + private void equalizeAuthenticationTime(Digester digester, String plainPassword) { + try { + String hash = timingEqualizationHash; + + if (hash == null) { + synchronized (timingEqualizationLock) { + if (timingEqualizationHash == null) { + timingEqualizationHash = digester.digest(TIMING_EQUALIZATION_PASSWORD); + } + hash = timingEqualizationHash; + } + } + + digester.matches(plainPassword, hash); + } catch (Exception e) { + if (timingEqualizationFailureLogged.compareAndSet(false, true)) { + logger.warn("Unable to equalize authentication timing. Until this is resolved, the time taken to reject a login reveals whether the username exists. This is logged once per server run.", e); + } + } + } + private LoginStatus handleSecondaryAuthentication(String username, LoginStatus loginStatus, LoginRequirementsChecker loginRequirementsChecker, String serverURL) { if (loginStatus != null && extensionController.getMultiFactorAuthenticationPlugin() != null && (loginStatus.getStatus() == Status.SUCCESS || loginStatus.getStatus() == Status.SUCCESS_GRACE_PERIOD)) { loginStatus = extensionController.getMultiFactorAuthenticationPlugin().authenticate(username, loginStatus, serverURL); diff --git a/server/src/main/java/com/mirth/connect/server/util/PasswordRequirementsChecker.java b/server/src/main/java/com/mirth/connect/server/util/PasswordRequirementsChecker.java index a099588e24..2197135bbc 100644 --- a/server/src/main/java/com/mirth/connect/server/util/PasswordRequirementsChecker.java +++ b/server/src/main/java/com/mirth/connect/server/util/PasswordRequirementsChecker.java @@ -57,6 +57,7 @@ public class PasswordRequirementsChecker implements Serializable { private static final String PASSWORD_LOCKOUT_PERIOD = "password.lockoutperiod"; private static final String PASSWORD_REUSE_PERIOD = "password.reuseperiod"; private static final String PASSWORD_REUSE_LIMIT = "password.reuselimit"; + private static final String PASSWORD_ALLOW_DETAILED_AUTH_ERRORS = "password.allowdetailedautherrors"; private static PasswordRequirementsChecker instance = null; @@ -88,6 +89,7 @@ public PasswordRequirements loadPasswordRequirements(PropertiesConfiguration sec passwordRequirements.setLockoutPeriod(securityProperties.getInt(PASSWORD_LOCKOUT_PERIOD, 0)); passwordRequirements.setReusePeriod(securityProperties.getInt(PASSWORD_REUSE_PERIOD, 0)); passwordRequirements.setReuseLimit(securityProperties.getInt(PASSWORD_REUSE_LIMIT, 0)); + passwordRequirements.setAllowDetailedAuthErrors(securityProperties.getBoolean(PASSWORD_ALLOW_DETAILED_AUTH_ERRORS, false)); return passwordRequirements; } diff --git a/server/src/test/java/com/mirth/connect/server/controllers/DefaultUserControllerAuthorizeUserTest.java b/server/src/test/java/com/mirth/connect/server/controllers/DefaultUserControllerAuthorizeUserTest.java new file mode 100644 index 0000000000..3d108580bd --- /dev/null +++ b/server/src/test/java/com/mirth/connect/server/controllers/DefaultUserControllerAuthorizeUserTest.java @@ -0,0 +1,153 @@ +// SPDX-License-Identifier: MPL-2.0 +// SPDX-FileCopyrightText: Open Integration Engine + +package com.mirth.connect.server.controllers; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.reset; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.when; + +import java.util.Calendar; + +import org.apache.ibatis.session.Configuration; +import org.apache.ibatis.session.SqlSessionManager; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.Test; +import org.mockito.ArgumentMatchers; + +import com.google.inject.AbstractModule; +import com.google.inject.Guice; +import com.google.inject.Injector; +import com.mirth.commons.encryption.Digester; +import com.mirth.connect.model.LoginStatus; +import com.mirth.connect.model.PasswordRequirements; +import com.mirth.connect.model.User; +import com.mirth.connect.server.util.SqlConfig; + +/** + * Covers what a failed login tells the caller. A locked account used to name itself and report its + * remaining lock time, which let an unauthenticated caller tell a real username from a made-up one. + * + * These assert on the returned LoginStatus only. Whether the two cases take the same amount of time + * is a separate property that is measured against a running server, not asserted here. + */ +public class DefaultUserControllerAuthorizeUserTest { + + private static final String LOCKED_USERNAME = "lockeduser"; + private static final String UNKNOWN_USERNAME = "nosuchuser"; + private static final String WRONG_PASSWORD = "wrongpassword"; + private static final String GENERIC_MESSAGE = "Incorrect username or password."; + + private static ControllerFactory controllerFactory; + private static ConfigurationController configurationController; + private static ExtensionController extensionController; + private static UserController userController; + + private PasswordRequirements passwordRequirements; + private DefaultUserController controller; + + @BeforeClass + public static void setupBeforeClass() { + controllerFactory = mock(ControllerFactory.class); + configurationController = mock(ConfigurationController.class); + extensionController = mock(ExtensionController.class); + userController = mock(UserController.class); + + when(controllerFactory.createConfigurationController()).thenReturn(configurationController); + when(controllerFactory.createExtensionController()).thenReturn(extensionController); + when(controllerFactory.createUserController()).thenReturn(userController); + + /* + * authorizeUser takes a StatementLock, and StatementLock asks DatabaseUtil whether the + * vacuum statement is mapped, which builds a real SqlConfig and its connection pool. Report + * the statement as absent so the lock is a no-op and nothing tries to reach a database. + */ + SqlConfig sqlConfig = mock(SqlConfig.class); + SqlSessionManager sqlSessionManager = mock(SqlSessionManager.class); + Configuration mybatisConfiguration = mock(Configuration.class); + when(sqlConfig.getSqlSessionManager()).thenReturn(sqlSessionManager); + when(sqlSessionManager.getConfiguration()).thenReturn(mybatisConfiguration); + when(mybatisConfiguration.getMappedStatement(ArgumentMatchers.anyString())) + .thenThrow(new IllegalArgumentException("statement not mapped in this test")); + + Injector injector = Guice.createInjector(new AbstractModule() { + @Override + protected void configure() { + requestStaticInjection(ControllerFactory.class); + requestStaticInjection(SqlConfig.class); + bind(ControllerFactory.class).toInstance(controllerFactory); + bind(SqlConfig.class).toInstance(sqlConfig); + } + }); + injector.getInstance(ControllerFactory.class); + } + + @Before + public void setUp() throws Exception { + reset(configurationController, extensionController, userController); + + // No authorization or MFA plugin, so the controller performs authentication itself + when(extensionController.getAuthorizationPlugin()).thenReturn(null); + when(extensionController.getMultiFactorAuthenticationPlugin()).thenReturn(null); + + passwordRequirements = new PasswordRequirements(); + passwordRequirements.setRetryLimit(3); + passwordRequirements.setLockoutPeriod(1); + when(configurationController.getPasswordRequirements()).thenReturn(passwordRequirements); + when(configurationController.getDigester()).thenReturn(mock(Digester.class)); + + controller = spy(new DefaultUserController()); + doReturn(lockedUser()).when(controller).getUser(null, LOCKED_USERNAME); + doReturn(null).when(controller).getUser(null, UNKNOWN_USERNAME); + } + + @Test + public void lockedAccountIsGenericByDefault() throws Exception { + LoginStatus status = controller.authorizeUser(LOCKED_USERNAME, WRONG_PASSWORD, null); + + assertEquals(LoginStatus.Status.FAIL, status.getStatus()); + assertEquals(GENERIC_MESSAGE, status.getMessage()); + } + + @Test + public void lockedAccountReportsDetailWhenAllowed() throws Exception { + passwordRequirements.setAllowDetailedAuthErrors(true); + + LoginStatus status = controller.authorizeUser(LOCKED_USERNAME, WRONG_PASSWORD, null); + + assertEquals(LoginStatus.Status.FAIL_LOCKED_OUT, status.getStatus()); + assertTrue("expected the message to name the account, but was: " + status.getMessage(), + status.getMessage().contains(LOCKED_USERNAME)); + } + + /** + * The #240 regression test. Without the fix the locked account answers FAIL_LOCKED_OUT and names + * itself while the unknown username answers FAIL, so the two responses identify a real account. + */ + @Test + public void lockedAccountIsIndistinguishableFromUnknownUsername() throws Exception { + LoginStatus locked = controller.authorizeUser(LOCKED_USERNAME, WRONG_PASSWORD, null); + LoginStatus unknown = controller.authorizeUser(UNKNOWN_USERNAME, WRONG_PASSWORD, null); + + assertEquals(unknown.getStatus(), locked.getStatus()); + assertEquals(unknown.getMessage(), locked.getMessage()); + } + + /** + * Strikes past the retry limit with the last one just now, so the lockout period has not + * elapsed. LoginRequirementsChecker reads both values straight off the User. + */ + private User lockedUser() { + User user = new User(); + user.setId(1); + user.setUsername(LOCKED_USERNAME); + user.setStrikeCount(passwordRequirements.getRetryLimit() + 1); + user.setLastStrikeTime(Calendar.getInstance()); + return user; + } +} diff --git a/server/src/test/java/com/mirth/connect/server/util/PasswordRequirementsTests.java b/server/src/test/java/com/mirth/connect/server/util/PasswordRequirementsTest.java similarity index 84% rename from server/src/test/java/com/mirth/connect/server/util/PasswordRequirementsTests.java rename to server/src/test/java/com/mirth/connect/server/util/PasswordRequirementsTest.java index e21b4a683e..eeeecba1bc 100644 --- a/server/src/test/java/com/mirth/connect/server/util/PasswordRequirementsTests.java +++ b/server/src/test/java/com/mirth/connect/server/util/PasswordRequirementsTest.java @@ -9,12 +9,14 @@ package com.mirth.connect.server.util; +import org.apache.commons.configuration2.PropertiesConfiguration; + import junit.framework.TestCase; import com.mirth.connect.client.core.ControllerException; import com.mirth.connect.model.PasswordRequirements; -public class PasswordRequirementsTests extends TestCase { +public class PasswordRequirementsTest extends TestCase { protected void setUp() throws Exception { super.setUp(); @@ -79,4 +81,16 @@ public void testAllConditions() throws ControllerException { assertNotNull(PasswordRequirementsChecker.getInstance().doesPasswordMeetRequirements(null, "test", req)); assertNull(PasswordRequirementsChecker.getInstance().doesPasswordMeetRequirements(null, "Th1$isAtestTEST*#", req)); } + + public void testAllowDetailedAuthErrorsDefaultsToFalse() { + PasswordRequirements req = PasswordRequirementsChecker.getInstance().loadPasswordRequirements(new PropertiesConfiguration()); + assertFalse(req.getAllowDetailedAuthErrors()); + } + + public void testAllowDetailedAuthErrorsHonorsProperty() { + PropertiesConfiguration properties = new PropertiesConfiguration(); + properties.setProperty("password.allowdetailedautherrors", true); + PasswordRequirements req = PasswordRequirementsChecker.getInstance().loadPasswordRequirements(properties); + assertTrue(req.getAllowDetailedAuthErrors()); + } } \ No newline at end of file