Fix SSL cert check on non-standard ports and login user enumeration#3726
Fix SSL cert check on non-standard ports and login user enumeration#3726sawirricardo wants to merge 2 commits into
Conversation
- Pass port from monitor URL to ssl-checker when checking certificates on non-standard HTTPS ports (fixes bluewave-labs#3646) - Prevent user enumeration in login by returning the same generic error for both non-existent email and wrong password (fixes bluewave-labs#2768)
sawirricardo
requested review from
Br0wnHammer,
ajhollid and
karenvicent
as code owners
ajhollid
left a comment
ajhollid
left a comment
There was a problem hiding this comment.
This doesn't actually resolve the enumeration issue noted in the PR title, please see comments.
In future, one issue per PR please 👍
| try { | ||
| user = await this.usersRepository.findByEmail(email); | ||
| } catch { | ||
| throw new AppError({ message: "Incorrect email or password", service: SERVICE_NAME, status: 401 }); | ||
| } |
There was a problem hiding this comment.
This doesn't resolve the enumeration problem. Attackers can still determine if they have found a correct email credential, as bcrypt is still awaited after the user lookup.
If you want to fully resolve the enumeration issue then a dummy value needs to be hashed on the user not found path.
Additionally, this catch block is too broad, it classifies all errors thrown by the repository as email/password errors. This should be narrowed appropriately.
| const hostname = monitorUrl.hostname; | ||
| const cert = await checker(hostname); | ||
| const port = monitorUrl.port ? parseInt(monitorUrl.port, 10) : undefined; | ||
| const cert = await checker(hostname, { ...(port ? { port } : {}) }); |
There was a problem hiding this comment.
This is a pretty convoluted way of expressing that port should not be added here if not specificed.
checker(hostname, port ? { port } : undefined)
seems clearer to me 🤔
2 participants
Summary
Two small fixes for confirmed open issues:
1. SSL/TLS certificate expiry on non-standard HTTPS ports (#3646)
fetchMonitorCertificatewas only passing the hostname tossl-checker, ignoring the port specified in the monitor URL. This meant certificates on non-standard HTTPS ports (e.g.,https://example.com:8443) were never checked and always showed "N/A".Fix: Extract port from the monitor URL and pass it to the ssl-checker via the
SSLOptions.portparameter (already supported by the library).2. Login error handling / user enumeration (#2768)
The login flow returned different error messages and status codes depending on whether the email existed:
This leaks information about which emails are registered.
Fix: Catch the "user not found" case and return the same generic "Incorrect email or password" (401) message for both scenarios.
Fixes #3646
Fixes #2768