Java: Insecure JXBrowser

[intrigus-lgtm]

JXBrowser is a Java library that allows to embed the Chromium browser inside Java applications.
The version 6.x.x by default ignores any HTTPS certificate errors thereby allowing man-in-the-middle attacks.

This PR detects this case.

[intrigus-lgtm]

It just came to my attention that this has been fixed in version 6.24.
I've assumed that the latest version is 6.23.1 as this is the version of the javadocs I found...

I will now detect version 6.24 by the existence of the addBoundsListener method on the com.teamdev.jxbrowser.chromium.Browser class.
As far as I know CodeQL extracts all (public) methods of a class.
And the Browser class is always used in this vulnerability, so it will always be extracted and the existence of the method is therefore a reliable indicator for the version.

[smowton]

Could you also add a test using v6.24, to show this is correctly not flagged? By making your test project not reference the addBoundsListener method you can also ensure your detection method works when that method is not called.

[intrigus-lgtm]

Could you also add a test using v6.24, to show this is correctly not flagged? By making your test project not reference the addBoundsListener method you can also ensure your detection method works when that method is not called.

How can I use different stubs for different tests? The options file is used for the whole folder, isn't it?
Is there a way to use these options only for a specific test file? Or should I have to create two folders for the different versions?

[smowton]

Two folders looks like the right option to me

[intrigus-lgtm]

I've added tests for both versions.

What do you think, would this query need major changes to become a non-experimental query?

[smowton]

I don't think there's anything I'd change about the code; the only possible obstacle would be FPs if any

[intrigus-lgtm]

@smowton @aschackmull just checking, is there anything missing I should improve?

[intrigus-lgtm]

Hmm, looks like some tests are failing.

[aschackmull]

Looks like the alert message in the query doesn't match the expected output in the test.

Also, the qhelp apparently fails to render:

  Error: 2-05 09:00:23] [ERROR] generate query-help> ql/java/ql/src/experimental/Security/CWE/CWE-295/JxBrowserWithoutCertValidation.qhelp:14:5: element "li" not allowed here; expected the element end-tag, text or element "a", "b", "code", "em", "i", "img", "strong", "sub", "sup" or "tt"
  Error: 2-05 09:00:23] [ERROR] generate query-help> ql/java/ql/src/experimental/Security/CWE/CWE-295/JxBrowserWithoutCertValidation.qhelp:15:5: element "li" not allowed here; expected the element end-tag, text or element "a", "b", "code", "em", "i", "img", "strong", "sub", "sup" or "tt"

[intrigus-lgtm]

@aschackmull I hope I fixed both the QHelp and the tests.

[aschackmull]

[2021-02-10 12:14:46] This is codeql generate query-help -vvv --log-to-stderr --output out --format markdown --search-path . -- ql/java/ql/src/experimental/Security/CWE/CWE-295/JxBrowserWithoutCertValidation.qhelp
  Error: 2-10 12:14:46] [ERROR] generate query-help> ql/java/ql/src/experimental/Security/CWE/CWE-295/JxBrowserWithoutCertValidation.qhelp:14:5: element "ul" not allowed here; expected the element end-tag, text or element "a", "b", "code", "em", "i", "img", "strong", "sub", "sup" or "tt"

[intrigus-lgtm]

Sorry for all the back and forth.
This time it should work for real...
OOI: Why is this qhelp preview check not run by default and public?

[2021-02-10 14:03:18] This is codeql generate query-help -vvv --log-to-stderr --output out.md --format markdown --search-path . -- java/ql/src/experimental/Security/CWE/CWE-295/JxBrowserWithoutCertValidation.qhelp
[2021-02-10 14:03:18] Terminating normally.

[aschackmull]

OOI: Why is this qhelp preview check not run by default and public?

Definitely historical reasons. I'm unsure whether those reasons still apply, and it would be nice if it was automatic and public, so I'll raise the question internally.