Skip to content

Add password scanner - #424

Open
mgaffigan wants to merge 1 commit into
OpenIntegrationEngine:mainfrom
mgaffigan:feat/password-scanner
Open

Add password scanner#424
mgaffigan wants to merge 1 commit into
OpenIntegrationEngine:mainfrom
mgaffigan:feat/password-scanner

Conversation

@mgaffigan

Copy link
Copy Markdown
Contributor

Add a password scanner for trivial passwords. Logs to server event log.

WARN 2026-08-27 13:51:36.488 [Password Scanner] com.mirth.connect.plugins.passwordscanner.PasswordScanner: User "admin" has a trivially guessable password and should change it immediately.

Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
@github-actions

Copy link
Copy Markdown

Test Results

686 tests  +11   686 ✅ +11   2m 10s ⏱️ +49s
115 suites + 1     0 💤 ± 0 
115 files   + 1     0 ❌ ± 0 

Results for commit 6c7b2d9. ± Comparison against base commit e8afb74.

@jonbartels jonbartels left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the approach is wrong. This take a known password list and checks against it like an actual login.

Since we control the DB and can read the salts I think it would be more efficient to compute a rainbow table using large known password list like this example. Then compute the hash from that list for each users salt. Then do a lookup by hash.

Also consider my usual comment - is there a library or utility that does this for us? John the Ripper was the tool to use back in the day, but I haven't kept pace.

@mgaffigan

Copy link
Copy Markdown
Contributor Author

compute a rainbow table using large known password list like this example. Then compute the hash from that list for each users salt.

This is the rub. Salts are used to prevent rainbow tables from being used. We can't run a large password list against each user since computing the hash is expensive by design. A small wordlist is the best that can be done in a background scanner.

To check a large table, we would have to do it when we have the cleartext in hand (e.g.: during set or during login). Doing it without the cleartext is not possible to do securely, since any weakened hash (even if not used during login) can also be used to crack passwords.

@goosvorbook

Copy link
Copy Markdown

Good direction overall. One concern worth flagging before this merges: the warning log includes the username of the affected account. Anyone with access to the server logs, whether via a SIEM, a central log management platform, or direct file access, can see which accounts have weak passwords and correlate that against the bundled wordlist. That is information they should not have, and the daily cadence means it persists in archives long after the password has been changed.

A cleaner approach: keep the background scanner for detection, but log only an aggregate count without account names, for example: "2 account(s) have passwords matching known-weak credentials, administrator action required." Combine this with a forced rotation at next login when the stored hash matches. This way dormant accounts are still detected and flagged, but log readers cannot determine which accounts are affected.

This also covers existing installations that PR #421 does not reach, and aligns with NIST SP 800-63B §5.1.1.2.

The mgaffigan/jonbartels debate about salts vs. rainbow tables is largely moot, mgaffigan is correct that salts make precomputed lookups ineffective against expensive hashes.

@mgaffigan

Copy link
Copy Markdown
Contributor Author

@goosvorbook

Anyone with access to the server logs [...] can see which accounts have weak passwords [...]. That is information they should not have

Making it known to log-readers is the cost for keeping the alert actionable. The vulnerability is the weak password, not the warning; an attacker can trivially re-create this information.

Combine this with a forced rotation at next login when the stored hash matches.

Change after login is addressed in #427

This also covers existing installations that PR #421 does not reach, and aligns with NIST SP 800-63B §5.1.1.2.

NIST SP 800-63B §5.1.1.2 password requirement is addressed in #426.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants