From e5811cf9cf98cde451214da6038f70b6e90eead4 Mon Sep 17 00:00:00 2001 From: Finnegan's Owner <44065187+pacmano1@users.noreply.github.com> Date: Thu, 10 Sep 2026 12:20:34 -0600 Subject: [PATCH] Stop failed logins from revealing which usernames exist The lockout paths in DefaultUserController.authorizeUser named the account and reported how long the lock had left to run, so an unauthenticated caller could tell a valid username from an invalid one. Both sites now return the generic "Incorrect username or password." unless password.allowdetailedautherrors is set, which defaults to false. Fixing the messages alone left a much louder signal. The password digest is only reached for a valid, unlocked user: an unknown username never enters the block and a locked account returns before it, so both skipped 600k PBKDF2 iterations. Measured on this branch with generic messages enabled, n=250 per cohort, the medians were 230.90 ms for a valid unlocked user against 0.49 ms locked and 0.76 ms unknown, with no overlap between the distributions. One timed request classified any username, whatever the response said. Both miss paths now run a throwaway digest and discard the result. The hash is generated once from the live digester so it follows the configured algorithm and iteration count. Re-measured the same way, the three cohorts land at 236.84, 237.74 and 237.05 ms, within a millisecond of each other and inside the run-to-run noise. Every path that skips the real digest now equalizes: an unknown username, a locked account, an account that exists but has never had a password set, and a failed pre-2.2 hash check. The last two are reachable in normal use, since an admin can create an account before provisioning it and legacy hashes survive upgrades. This still does not make the paths identical. A valid user costs one extra credentials query that a miss does not. That measured under a millisecond against a 237 ms floor, so the practical oracle is closed rather than the timing being constant. Locked accounts are no longer cheap to reject, which gives up a property lockout previously provided. An attacker gains nothing by locking an account, since every username now costs the same, but the tradeoff is deliberate rather than overlooked. Adopts the approach from #241 by Mitch Gaffigan, reworked for the current source layout. That branch predates the move to src/main/java and no longer applies. Takes the allowDetailedAuthErrors name Tony Germano suggested on that thread, and sets the field explicitly in both constructors as he asked, rather than widening the eleven-argument constructor and churning its callers. DefaultUserControllerAuthorizeUserTest covers the message behaviour: a locked account is generic by default, reports detail when the property is set, and is indistinguishable from an unknown username. The first and third fail without this change. It binds mock ControllerFactory and SqlConfig through Guice static injection, the way DefaultHttpConfigurationTest already does, so no database is involved. Nothing asserts the timing, which is measured against a running server. Also renames PasswordRequirementsTests to PasswordRequirementsTest. The build includes '**/*Test.class' only, so the plural-named class had never been run; the rename puts its existing cases into the suite along with the two new ones. Closes #240 Signed-off-by: Finnegan's Owner <44065187+pacmano1@users.noreply.github.com> --- server/conf/mirth.properties | 5 + .../connect/model/PasswordRequirements.java | 11 ++ .../controllers/DefaultUserController.java | 68 +++++++- .../util/PasswordRequirementsChecker.java | 2 + ...efaultUserControllerAuthorizeUserTest.java | 153 ++++++++++++++++++ ...sts.java => PasswordRequirementsTest.java} | 16 +- 6 files changed, 249 insertions(+), 6 deletions(-) create mode 100644 server/src/test/java/com/mirth/connect/server/controllers/DefaultUserControllerAuthorizeUserTest.java rename server/src/test/java/com/mirth/connect/server/util/{PasswordRequirementsTests.java => PasswordRequirementsTest.java} (84%) 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