Java: QL Query Detector for JHipster Generated CVE-2019-16303 by JLLeitschuh · Pull Request #4312 · github/codeql

JLLeitschuh · 2020-09-21T21:44:50Z

This is the query I used to create the list of 5,500 projects used to generate 3,880 pull requests to fix this vulnerability across GitHub.

This query was run against all (21,955) Java projects on LGTM.com and came back with 5,511 results.
https://lgtm.com/query/8455949919266029298

All For One Bounty Submission

This query is submitted as a part of the 'All For One' bounty submission. That submission can be found here:
github/securitylab#180

intrigus-lgtm

Shouldn't this query be in the experimental folder?

felicitymay

Great to see a pull request for a query that's been used to find so many real problems in open source projects.

The Pull-Request-Check/QHelp-Preview failure looks as if it's due to some missing <p> tags. I've added suggestions that I hope will fix this test. Once the preview is available, I can review the help topic.

Co-authored-by: Felicity Chapman <felicitymay@github.com>

JLLeitschuh · 2020-09-22T19:12:55Z

Shouldn't this query be in the experimental folder?
- @intrigus-lgtm

Queries must meet at least the requirements for experimental queries, including at least one useful result on some revision of a real project. Higher bounties will be awarded for queries that also meet additional requirements for supported queries, including query help and tests.
- https://securitylab.github.com/bounties > All for one, one for all > How to participate > 5.

For this submission, it's my goal for this query to meet the additional requirements under the 'supported queries' category by also including help and tests.

JLLeitschuh · 2020-09-22T19:15:12Z

Thank you @felicitymay for the rapid feedback on these pull requests. It's really appreciated!!

Co-authored-by: Felicity Chapman <felicitymay@github.com>

aibaars

Thanks a lot for this contribution and for your efforts to make open source software more secure!

aibaars · 2020-09-25T13:37:40Z

+
+from VulnerableJHipsterRandomUtilMethod the_method
+select the_method,
+  "RandomUtil was generated by JHipster Generator version vulnerable to CVE-2019-16303"


This query catches cases of badly generated RandomUtil classes. However, it would be possible to generalise this query even further. If I am not mistaken the actual problem is the use of java.util.Random (or wrapper classes like RandomStringUtils) in places where you're doing something crypographically sensitive.

It is of course very hard to write a query that finds all "crypographically sensitive" pieces of code. However, we could start by treating methods named generatePassword , generate%Token%, generate%Key, etc. as likely to be sensitive. That way the query should flag up the generated JHipster classes and potentially many more.

@aibaars and I chatted about this privately. I have another query I'm working on that is more general here:
#2694

felicitymay

Hi - thanks for merging the fixes to the qhelp format and allowing us to see the generated output 😄 Thanks also for including a help file for this query, and especially for including example code.

Looking at the references, it strikes me that the help topic covers the vulnerability and potential for attack well, but that it doesn't say much about the underlying cause of poor "random" number generation. I think would be a good idea to update the content to explain briefly what the problem is with the RandomUtil class generated by vulnerable versions of JHipster. This might help people be more aware of the problem as they're working on future projects.

felicitymay · 2020-10-02T10:17:25Z

+
+<example>
+
+<p>The example below shows the vulnerable <code>RandomUtil</code> class generated by JHipster.</p>


Presumably this is what vulnerable versions of JHipster generate? If so, we should mention this.

felicitymay · 2020-10-02T10:23:29Z

+<sample src="JHipsterGeneratedPRNGVulnerable.java" />
+
+<p>Below is a fixed version of the <code>RandomUtil</code> class.</p>
+<sample src="JHipsterGeneratedPRNGFixed.java" />


We usually use comments to highlight the GOOD and BAD code in examples. Is this possible here? If not, it might be worth highlighting the area that's different?

I'm not looking at a diff of the two code examples here, and I'm not a developer, so this may be a daft suggestion.

I can add this.

github#4312 Co-authored-by: Felicity Chapman <felicitymay@github.com> Co-authored-by: Arthur Baars <aibaars@github.com>

felicitymay

Thanks for the most recent updates. Unfortunately, there's a typo 😞

Co-authored-by: Felicity Chapman <felicitymay@github.com>

felicitymay

Thanks for your patience with all the content updates. All LGTM now.

aschackmull · 2020-10-15T08:38:58Z

+/** Definitions related to the Apache Commons Lang library. */
+import semmle.code.java.Type


Looks like there's an autoformat failure in this file:

ql/java/ql/src/semmle/code/java/frameworks/apache/Lang.qll is not correctly formatted

I think it's just missing an empty line between the file-level qldoc and the first import, but autoformat will tell.
Also, while we're at this file, could you replace the import with import java? We prefer always starting with import <lang> as the very first import in each file and this will include semmle.code.java.Type

Why import all of java when I only need the one import? That sounds like creating tighter coupling to more imports than this class truly needs.

Exec.qll also only imports semmle.code.java.Type

I've fixed it the way you've requested, but I'd still like to better understand the rationale here

The optimiser will remove anything that's imported but not used, so no worries there. However, even though the optimiser tries its best to be as deterministic as possible, there are certain choices it has to make that needs some sort of arbitrary tiebreaker, which in the end can come down to the order in which declarations are encountered when traversing imports. Mostly this doesn't matter as the generated code will tend to be equivalent, but it might cause cache misses that can force re-evaluation of large parts of the libraries. We have been bitten by this problem before, and a good rule-of-thumb to avoid it is to start all ql files with import <lang> as the first import.

JLLeitschuh · 2020-10-15T15:55:24Z

Woot! All the checks have passed!

JLLeitschuh · 2020-10-16T13:55:46Z

😄 Woot!!!

JLLeitschuh requested review from a team and felicitymay as code owners September 21, 2020 21:44

github-actions Bot added documentation Java labels Sep 21, 2020

JLLeitschuh force-pushed the feat/JLL/java/jhipster_CVE-2019-16303 branch 2 times, most recently from 2ad0c8b to b86e88e Compare September 21, 2020 21:52

JLLeitschuh mentioned this pull request Sep 21, 2020

Java: QL Query Detector for JHipster Generated CVE-2019-16303 github/securitylab#180

Closed

1 task

Java: QL Query Detector for JHipster Generated CVE-2019-16303

ab618dc

JLLeitschuh force-pushed the feat/JLL/java/jhipster_CVE-2019-16303 branch from b86e88e to ab618dc Compare September 21, 2020 22:46

intrigus-lgtm reviewed Sep 21, 2020

View reviewed changes

Comment thread java/ql/src/semmle/code/java/frameworks/apache/Lang.qll Outdated

felicitymay reviewed Sep 22, 2020

View reviewed changes

Comment thread java/ql/src/Security/CWE/CWE-338/JHipsterGeneratedPRNG.qhelp Outdated

Comment thread java/ql/src/Security/CWE/CWE-338/JHipsterGeneratedPRNG.qhelp Outdated

Comment thread java/ql/src/Security/CWE/CWE-338/JHipsterGeneratedPRNG.qhelp Outdated

Apply suggestions from code review

24fe3d0

Co-authored-by: Felicity Chapman <felicitymay@github.com>

felicitymay reviewed Sep 22, 2020

View reviewed changes

Comment thread java/ql/src/Security/CWE/CWE-338/JHipsterGeneratedPRNG.qhelp

JLLeitschuh and others added 2 commits September 22, 2020 15:02

Update java/ql/src/Security/CWE/CWE-338/JHipsterGeneratedPRNG.qhelp

8578bc5

Co-authored-by: Felicity Chapman <felicitymay@github.com>

Fix documentation in apache/Lang.qll

645d7c8

felicitymay reviewed Sep 23, 2020

View reviewed changes

Comment thread java/ql/src/Security/CWE/CWE-338/JHipsterGeneratedPRNG.qhelp Outdated

Update java/ql/src/Security/CWE/CWE-338/JHipsterGeneratedPRNG.qhelp

17603c8

Co-authored-by: Felicity Chapman <felicitymay@github.com>

aibaars reviewed Sep 25, 2020

View reviewed changes

aschackmull reviewed Sep 30, 2020

View reviewed changes

Update JHipster CodeQL query from code review

ab3772e

aibaars previously approved these changes Oct 2, 2020

View reviewed changes

aibaars reviewed Oct 2, 2020

View reviewed changes

Comment thread java/ql/src/semmle/code/java/frameworks/apache/Lang.qll Outdated

felicitymay reviewed Oct 2, 2020

View reviewed changes

Apply suggestions from code review

8272d59

github#4312 Co-authored-by: Felicity Chapman <felicitymay@github.com> Co-authored-by: Arthur Baars <aibaars@github.com>

JLLeitschuh dismissed aibaars’s stale review via 8272d59 October 5, 2020 18:12

aibaars previously approved these changes Oct 12, 2020

View reviewed changes

JHipster Vuln: Add GOOD/BAD & release note links

895f4d0

JLLeitschuh dismissed aibaars’s stale review via 895f4d0 October 12, 2020 15:00

felicitymay reviewed Oct 12, 2020

View reviewed changes

Apply suggestions from code review

48f4b6c

Co-authored-by: Felicity Chapman <felicitymay@github.com>

aschackmull previously approved these changes Oct 13, 2020

View reviewed changes

felicitymay previously approved these changes Oct 13, 2020

View reviewed changes

JLLeitschuh mentioned this pull request Oct 13, 2020

3,880 Pull Requests Generated to fix JHipster RNG Vulnerability CVE-2019-16303 github/securitylab#191

Closed

1 task

aschackmull reviewed Oct 15, 2020

View reviewed changes

Fix formatting in Lang.qll

a9c5551

JLLeitschuh dismissed stale reviews from felicitymay and aschackmull via a9c5551 October 15, 2020 12:52

aschackmull approved these changes Oct 16, 2020

View reviewed changes

aschackmull merged commit a806a4f into github:main Oct 16, 2020

aschackmull added a commit to aschackmull/ql that referenced this pull request Oct 29, 2020

Java: Add change note for github#4312.

7f97139


		<example>

		<p>The example below shows the vulnerable <code>RandomUtil</code> class generated by JHipster.</p>

		/** Definitions related to the Apache Commons Lang library. */
		import semmle.code.java.Type

Conversation

JLLeitschuh commented Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All For One Bounty Submission

Uh oh!

intrigus-lgtm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

felicitymay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JLLeitschuh commented Sep 22, 2020

Uh oh!

JLLeitschuh commented Sep 22, 2020

Uh oh!

Uh oh!

aibaars left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

felicitymay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

felicitymay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

felicitymay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JLLeitschuh commented Oct 15, 2020

Uh oh!

JLLeitschuh commented Oct 16, 2020

Uh oh!

JLLeitschuh commented Sep 21, 2020 •

edited

Loading