Skip to content

Verify SMTP server hostname on SSL/TLS connections - #527

Open
NicoPiel wants to merge 1 commit into
OpenIntegrationEngine:mainfrom
NicoPiel:fix/smtp-hostname-verification
Open

NicoPiel wants to merge 1 commit into
OpenIntegrationEngine:mainfrom
NicoPiel:fix/smtp-hostname-verification

Conversation

@NicoPiel

Copy link
Copy Markdown
Contributor

What

Calls email.setSSLCheckServerIdentity(true) wherever SMTP is set up with SSL or STARTTLS:

  • DefaultSmtpConfiguration (SMTP Sender connector)
  • DefaultConfigurationController (server email settings / test email)
  • ServerSMTPConnection

Why

SonarQube flags these as java:S5527 (6 critical issues). By default commons-email checks the certificate chain but never matches the certificate to the host. So anyone in the network path with any valid certificate could intercept SMTP traffic, including the login credentials.

Reviewer notes

  • Behavior change: if a setup reaches its mail server by IP address, or the server's certificate doesn't match the configured hostname, sending mail will now fail during the TLS handshake. Worth a line in the release notes.
  • Verified with ./gradlew :server:compileJava on JDK 17. No automated tests cover these paths, and I haven't tested it against a real TLS mail server.

🤖 Generated with Claude Code

commons-email only checks the certificate chain by default, so any
valid certificate was accepted regardless of host. Enable server
identity checking wherever SSL or STARTTLS is configured.

Signed-off-by: Nico Piel <nico.piel@hotmail.de>
@github-actions

Copy link
Copy Markdown

Test Results

130 files  ±0  130 suites  ±0   4m 7s ⏱️ + 1m 5s
789 tests ±0  789 ✅ ±0  0 💤 ±0  0 ❌ ±0 
915 runs  ±0  909 ✅ ±0  6 💤 ±0  0 ❌ ±0 

Results for commit fbfc446. ± Comparison against base commit a403039.

@mgaffigan mgaffigan 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.

This would be exploitable with MITM. Probably needs a CVE filed.

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.

2 participants