Skip to content

Conversation

luchua-bc
Copy link
Contributor

@luchua-bc luchua-bc commented May 17, 2020

JavaMail is widely used in Java applications for sending emails. There are some other popular third-party libraries like Apache Commons Email which are built on JavaMail and facilitate the integration. Authenticated mail sessions require user credentials and mail sessions can require SSL/TLS authentication. It is a common security vulnerability that host-specific certificate data is not validated or is incorrectly validated. Failing to validate the certificate makes the SSL session susceptible to a man-in-the-middle attack.

This query validates configuration of both JavaMail and SimpleMail. I've tested the query against some GitHub projects, and two of them are:

  1. Apache Log4J project using JavaMail. The version 2.13.1 has Add support for specifying an SSL configuration for SmtpAppender (CVE-2020-9488) (not reported by me)
  2. cronomst/simplemail project using Apache Commons Email

Please consider to merge the PR. Thanks.

@luchua-bc

aschackmull
aschackmull previously approved these changes Jun 8, 2020
@aschackmull
Copy link
Contributor

Looks like the autoformat PR check is failing. Could you autoformat the QL code, please?

@luchua-bc
Copy link
Contributor Author

Sorry @aschackmull . I did autoformat the QL code in the previous commit then removed the ending blank line since it caused an autoformat issue with a previous PR.

I just committed a new version with the autoformatted code from the CodeQL plugin of VS Code on Mac, which contains an ending blank line. Let's see whether this helps. If it doesn't, I will try to remove the ending line in GitHub GUI directly.

@aschackmull
Copy link
Contributor

The last character in a text file should be a newline character (and the github ui will show a small icon when that isn't the case). The trouble is that some editors don't follow this convention and show this as a blank line at the end of the file (Eclipse has this behaviour), so this sometimes results in some confusion about whether or not there is a blank line at the end of a file.

In any case, the autoformat check passed and now continued to run the newly added test (we greatly appreciate tests for new queries, so thanks for adding one!). However, the test failed, since such tests need to be self-contained and cannot rely on external libraries. We usually handle this by stubbing any needed dependencies (see the java/ql/test/stubs dir).

@luchua-bc
Copy link
Contributor Author

Thanks @aschackmull . I've added stub classes of Apache Commons Email for the test case. Hope the Java check can pass now.

@aschackmull
Copy link
Contributor

The added Email.java doesn't look like a stub, but rather a complete copy. We prefer stubs. Also, the test doesn't quite work yet as there are some more dependencies missing. You can try to execute the test locally to verify that it works through the Test Explorer in VSCode.

@luchua-bc
Copy link
Contributor Author

luchua-bc commented Jun 9, 2020

Thanks @aschackmull for the advice, I've added all dependencies as class stubs. I used to use binary JAR files directly during my local testing. Now the Java check shall pass.

Copy link
Contributor

@aschackmull aschackmull left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM.

@aschackmull aschackmull merged commit 4b3ca13 into github:master Jun 10, 2020
@luchua-bc luchua-bc deleted the java-insecure-smtp-ssl branch June 10, 2020 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants