Skip to content

Fix information disclosure when lockout enabled #241

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mgaffigan wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix information disclosure when lockout enabled #241

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions server/conf/mirth.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ password.expiration = 0
password.graceperiod = 0
password.reuseperiod = 0
password.reuselimit = 0
password.allowusernameenumeration = false
Copy link
Member

@tonygermano tonygermano Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I meant by naming the property with its function and including a warning about a potential consequence of changing the value rather than naming the property after the potential consequence and leaving people to wonder about what it actually does.

Suggested change
password.allowusernameenumeration = false
# Warning: Changing suppressdetailedautherrors to false could allow user enumeration by an attacker
password.suppressdetailedautherrors = true

I don't think we need to worry about not being able to put the warning on setting the values via environment variables, because the only way people would discover that the property exists in the first place is by looking at the mirth.properties file or reading external documentation about the properties, both of which would include the warning.

If you still want to keep the default value of false, then I think allowdetailedautherrors is already enough to give users pause. Using "allow" rather than another word like "show" already implies it is being intentionally withheld. Combined with the warning, it should be sufficiently scary.

If another aspect of preventing user enumeration requires an option in mirth.properties, rather than being always implemented, then I don't think it should share functionality with this property, but should be its own setting, also with an accurate description of what it is doing that a user might be concerned with besides preventing user enumeration. The desire to be scary can't mask the true functionality of the setting.

Copy link
Contributor Author

@mgaffigan mgaffigan Jan 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no further edits planned to the name. If this is blocking, feel free to take ownership of the PR.


# Only used for migration purposes, do not modify
version = 4.5.2
Expand Down
19 changes: 19 additions & 0 deletions server/src/com/mirth/connect/model/PasswordRequirements.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ public class PasswordRequirements implements Serializable {
private int gracePeriod;
private int reusePeriod;
private int reuseLimit;
private boolean allowUsernameEnumeration;

public PasswordRequirements() {
this.minLength = 0;
Expand All @@ -42,9 +43,18 @@ public PasswordRequirements() {
this.gracePeriod = 0;
this.reusePeriod = 0;
this.reuseLimit = 0;
this.allowUsernameEnumeration = false;
}

/**
* @deprecated Use {@link #PasswordRequirements(int, int, int, int, int, int, int, int, int, int, int, boolean)} instead.
*/
@Deprecated
public PasswordRequirements(int minLength, int minUpper, int minLower, int minNumeric, int minSpecial, int retryLimit, int lockoutPeriod, int expiration, int gracePeriod, int reusePeriod, int reuseLimit) {
this(minLength, minUpper, minLower, minNumeric, minSpecial, retryLimit, lockoutPeriod, expiration, gracePeriod, reusePeriod, reuseLimit, 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, boolean allowUsernameEnumeration) {
this.minLength = minLength;
this.minUpper = minUpper;
this.minLower = minLower;
Expand All @@ -56,6 +66,7 @@ public PasswordRequirements(int minLength, int minUpper, int minLower, int minNu
this.gracePeriod = gracePeriod;
this.reusePeriod = reusePeriod;
this.reuseLimit = reuseLimit;
this.allowUsernameEnumeration = allowUsernameEnumeration;
}

public int getMinLength() {
Expand Down Expand Up @@ -145,4 +156,12 @@ public int getReuseLimit() {
public void setReuseLimit(int reuseLimit) {
this.reuseLimit = reuseLimit;
}

public boolean getAllowUsernameEnumeration() {
return allowUsernameEnumeration;
}

public void setAllowUsernameEnumeration(boolean allowUsernameEnumeration) {
this.allowUsernameEnumeration = allowUsernameEnumeration;
}
}
13 changes: 9 additions & 4 deletions server/src/com/mirth/connect/server/controllers/DefaultUserController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
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 Logger logger = LogManager.getLogger(this.getClass());
private ExtensionController extensionController = null;
Expand Down Expand Up @@ -292,6 +293,7 @@ public LoginStatus authorizeUser(String username, String plainPassword, String s
boolean authorized = false;
Credentials credentials = null;
LoginRequirementsChecker loginRequirementsChecker = null;
PasswordRequirements passwordRequirements = ControllerFactory.getFactory().createConfigurationController().getPasswordRequirements();

// Retrieve the matching User
User validUser = getUser(null, username);
Expand All @@ -300,7 +302,11 @@ public LoginStatus authorizeUser(String username, String plainPassword, String s
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() + ".");
if (passwordRequirements.getAllowUsernameEnumeration()) {
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);
Copy link
Member

@tonygermano tonygermano Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Are timing attacks possible?

Since the goal of this PR is to prevent enumeration, does this early return (skipping the db credential lookup and digester match) open up the possibility of timing-based attacks? It is understood that you did not create this situation.

Copy link
Contributor Author

@mgaffigan mgaffigan Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: Timing attacks, absolutely an issue. I recommend a different issue and PR.

}
}

loginRequirementsChecker.resetExpiredStrikes();
Expand All @@ -320,7 +326,6 @@ public LoginStatus authorizeUser(String username, String plainPassword, String s
}
}

PasswordRequirements passwordRequirements = ControllerFactory.getFactory().createConfigurationController().getPasswordRequirements();
LoginStatus loginStatus = null;

if (authorized) {
Expand Down Expand Up @@ -383,12 +388,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.getAllowUsernameEnumeration()) {
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() + ".";
Expand Down
2 changes: 1 addition & 1 deletion server/src/com/mirth/connect/server/servlets/SwaggerExamplesServlet.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1165,7 +1165,7 @@ private List<MetaDataColumn> getMetaDataColumnListExample() {
}

private PasswordRequirements getPasswordRequirementsExample() {
return new PasswordRequirements(8, 1, 1, 1, 1, 3, 0, 0, 0, 0, 3);
return new PasswordRequirements(8, 1, 1, 1, 1, 3, 0, 0, 0, 0, 3, false);
}

private List<String> getPasswordRequirementListExample() {
Expand Down
2 changes: 2 additions & 0 deletions server/src/com/mirth/connect/server/util/PasswordRequirementsChecker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_USERNAME_ENUMERATION = "password.allowusernameenumeration";

private static PasswordRequirementsChecker instance = null;

Expand Down Expand Up @@ -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.setAllowUsernameEnumeration(securityProperties.getBoolean(PASSWORD_ALLOW_USERNAME_ENUMERATION, false));

return passwordRequirements;
}
Expand Down
24 changes: 12 additions & 12 deletions server/test/com/mirth/connect/server/util/PasswordRequirementsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,57 +25,57 @@ protected void tearDown() throws Exception {
}

public void testMinLength() throws ControllerException {
PasswordRequirements req = new PasswordRequirements(10, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0);
PasswordRequirements req = new PasswordRequirements(10, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, false);
assertNotNull(PasswordRequirementsChecker.getInstance().doesPasswordMeetRequirements(null, "test", req));
assertNull(PasswordRequirementsChecker.getInstance().doesPasswordMeetRequirements(null, "testtesttest", req));

}

public void testMinUpper() throws ControllerException {
PasswordRequirements req = new PasswordRequirements(0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0);
PasswordRequirements req = new PasswordRequirements(0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, false);
assertNotNull(PasswordRequirementsChecker.getInstance().doesPasswordMeetRequirements(null, "test", req));
assertNull(PasswordRequirementsChecker.getInstance().doesPasswordMeetRequirements(null, "aTest", req));

PasswordRequirements req2 = new PasswordRequirements(0, -1, 0, 0, 0, 0, 0, 0, 0, 0, 0);
PasswordRequirements req2 = new PasswordRequirements(0, -1, 0, 0, 0, 0, 0, 0, 0, 0, 0, false);
assertNull(PasswordRequirementsChecker.getInstance().doesPasswordMeetRequirements(null, "test", req2));
assertNotNull(PasswordRequirementsChecker.getInstance().doesPasswordMeetRequirements(null, "TESt", req2));
}

public void testMinLower() throws ControllerException {
PasswordRequirements req = new PasswordRequirements(0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0);
PasswordRequirements req = new PasswordRequirements(0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, false);
assertNotNull(PasswordRequirementsChecker.getInstance().doesPasswordMeetRequirements(null, "TEST", req));
assertNull(PasswordRequirementsChecker.getInstance().doesPasswordMeetRequirements(null, "TESt", req));

PasswordRequirements req2 = new PasswordRequirements(0, 0, -1, 0, 0, 0, 0, 0, 0, 0, 0);
PasswordRequirements req2 = new PasswordRequirements(0, 0, -1, 0, 0, 0, 0, 0, 0, 0, 0, false);
assertNull(PasswordRequirementsChecker.getInstance().doesPasswordMeetRequirements(null, "TEST", req2));
assertNotNull(PasswordRequirementsChecker.getInstance().doesPasswordMeetRequirements(null, "TESt", req2));

assertNotNull(PasswordRequirementsChecker.getInstance().doesPasswordMeetRequirements(null, "testBLAH", new PasswordRequirements(0, 0, 5, 0, 0, 0, 0, 0, 0, 0, 0)));
assertNull(PasswordRequirementsChecker.getInstance().doesPasswordMeetRequirements(null, "testtBLAH", new PasswordRequirements(0, 0, 5, 0, 0, 0, 0, 0, 0, 0, 0)));
assertNotNull(PasswordRequirementsChecker.getInstance().doesPasswordMeetRequirements(null, "testBLAH", new PasswordRequirements(0, 0, 5, 0, 0, 0, 0, 0, 0, 0, 0, false)));
assertNull(PasswordRequirementsChecker.getInstance().doesPasswordMeetRequirements(null, "testtBLAH", new PasswordRequirements(0, 0, 5, 0, 0, 0, 0, 0, 0, 0, 0, false)));
}

public void testMinNumeric() throws ControllerException {
PasswordRequirements req = new PasswordRequirements(0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0);
PasswordRequirements req = new PasswordRequirements(0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, false);
assertNotNull(PasswordRequirementsChecker.getInstance().doesPasswordMeetRequirements(null, "TEST", req));
assertNull(PasswordRequirementsChecker.getInstance().doesPasswordMeetRequirements(null, "TEST9", req));

PasswordRequirements req2 = new PasswordRequirements(0, 0, 0, -1, 0, 0, 0, 0, 0, 0, 0);
PasswordRequirements req2 = new PasswordRequirements(0, 0, 0, -1, 0, 0, 0, 0, 0, 0, 0, false);
assertNull(PasswordRequirementsChecker.getInstance().doesPasswordMeetRequirements(null, "TEST", req2));
assertNotNull(PasswordRequirementsChecker.getInstance().doesPasswordMeetRequirements(null, "TESt9", req2));
}

public void testMinSpecial() throws ControllerException {
PasswordRequirements req = new PasswordRequirements(0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0);
PasswordRequirements req = new PasswordRequirements(0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, false);
assertNotNull(PasswordRequirementsChecker.getInstance().doesPasswordMeetRequirements(null, "TEST", req));
assertNull(PasswordRequirementsChecker.getInstance().doesPasswordMeetRequirements(null, "TEST$TEST", req));

PasswordRequirements req2 = new PasswordRequirements(0, 0, 0, 0, -1, 0, 0, 0, 0, 0, 0);
PasswordRequirements req2 = new PasswordRequirements(0, 0, 0, 0, -1, 0, 0, 0, 0, 0, 0, false);
assertNull(PasswordRequirementsChecker.getInstance().doesPasswordMeetRequirements(null, "TEST", req2));
assertNotNull(PasswordRequirementsChecker.getInstance().doesPasswordMeetRequirements(null, "TESt$", req2));
}

public void testAllConditions() throws ControllerException {
PasswordRequirements req = new PasswordRequirements(15, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0);
PasswordRequirements req = new PasswordRequirements(15, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0, false);
assertNotNull(PasswordRequirementsChecker.getInstance().doesPasswordMeetRequirements(null, "test", req));
assertNull(PasswordRequirementsChecker.getInstance().doesPasswordMeetRequirements(null, "Th1$isAtestTEST*#", req));
}
Expand Down