Fix information disclosure when lockout enabled

[mgaffigan]

When lockout is enabled, enhanced response error messages allow an attacker to enumerate valid usernames. Change adds new configuration option to disable username enumeration by returning the same error message for both invalid usernames and locked out accounts.

The option is enabled by default. To revert, specify password.allowEnumeration=true in mirth.properties.

Closes #240

[Copilot]

Pull request overview

This PR fixes an information disclosure vulnerability when account lockout is enabled. The vulnerability allowed attackers to enumerate valid usernames by observing different error messages for invalid usernames versus locked out accounts. The fix introduces a new configuration option password.allowEnumeration (defaulting to false for security) that controls whether detailed error messages are shown.

Changes:

• Added allowEnumeration boolean field to PasswordRequirements model with default value of false
• Added configuration loading for password.allowEnumeration property in PasswordRequirementsChecker
• Modified authentication logic in DefaultUserController to return generic error messages when enumeration is disabled

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
server/src/com/mirth/connect/model/PasswordRequirements.java	Added allowEnumeration field with getter/setter methods to support the new configuration option
server/src/com/mirth/connect/server/util/PasswordRequirementsChecker.java	Added constant and loading logic for password.allowEnumeration property with default value of false
server/src/com/mirth/connect/server/controllers/DefaultUserController.java	Modified authorization logic to return generic error messages for locked accounts when allowEnumeration is false, preventing username enumeration

Comments suppressed due to low confidence (1)

server/src/com/mirth/connect/model/PasswordRequirements.java:61

• The parameterized constructor does not initialize the new allowEnumeration field. This means that when this constructor is used (for example in SwaggerExamplesServlet.java line 1168), the allowEnumeration field will default to false, which may not be the intended behavior. Consider adding allowEnumeration as a parameter to this constructor or explicitly setting it to false in the constructor body for clarity.

    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 = minLength;
        this.minUpper = minUpper;
        this.minLower = minLower;
        this.minNumeric = minNumeric;
        this.minSpecial = minSpecial;
        this.retryLimit = retryLimit;
        this.lockoutPeriod = lockoutPeriod;
        this.expiration = expiration;
        this.gracePeriod = gracePeriod;
        this.reusePeriod = reusePeriod;
        this.reuseLimit = reuseLimit;
    }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The property name "password.allowEnumeration" uses camelCase for "allowEnumeration", which is inconsistent with the existing naming convention for other password properties (e.g., "password.minlength", "password.lockoutperiod", "password.reuselimit"). Consider using "password.allowenumeration" instead for consistency.

Suggested change

	private static final String PASSWORD_ALLOW_ENUMERATION = "password.allowEnumeration";
	private static final String PASSWORD_ALLOW_ENUMERATION = "password.allowenumeration";

[mgaffigan]

Updated.

[tonygermano]

Do you need to push? I don't see it updated.

[tonygermano]

suggestion: add default value to mirth.properties

I think the default should probably be added to the /server/conf/mirth.properties file as part of this change.

---

question (non-blocking): do we need to improve event logging with this change?

It kind of sucks prior to your changes when lockout is enabled. The event just shows the failed login attempts with the username as the only property. There's no indication to someone viewing the log that the account has become locked (you might be able to guess if you see repeated failed attempts.) Should we fire off another event when incrementing the strikes if it causes the account to lock? Putting the unlock time as a property of the server event would be helpful, I think.

This would also allow a future PR to send an email triggered by the event to the user notifying them of their account being locked now that it just appears they keep entering their credentials incorrectly.

Additionally, there doesn't appear to be a way for another admin to unlock it, and the lock duration is specified in hours.

[tonygermano]

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.

[tonygermano]

comment (non-blocking): yuck

I don't like that the property is added to this class, but I guess it's fine. This class already seems to be doing too much stuff not related to password requirements. The first 5 "min" properties all have to do with password complexity, the "reuse" ones still constitute what makes a valid password, but the rest of them are more related to login security policies. I suppose this new property fits in with the last group, but we might want to split them off at some point in the future.

[tonygermano]

Do you need to push? I don't see it updated.

[tonygermano]

suggestion: rename this property

Can we rename the property to something more descriptive of what it is doing, like allowDetailedAuthErrors?

We need to consider how it will be initialized if someone calls the non-default constructor. I think an uninitialized boolean defaults to false, but all other properties are explicitly set in both constructors.