Java: Use of HTTP/FTP to download/upload Maven artifacts

[JLLeitschuh]

This adds a security alert for the use of HTTP or FTP to download or upload artifacts using Maven.

Want to take over the Java ecosystem? All you need is a MITM!

Related to #2377

[JLLeitschuh]

Two additional things:

I've submitted this PR for consideration by the GitHub Security Lab Bounty team here: github/securitylab#21

I have a test project that I used to build this query. I'm more than happy to create a test for this query as well, I just want to make sure that there's nothing special I need to do to enable. As mentioned in #2377 the XML logic is currently not automatically collected when building the QL database.

Here is the test project I used to test this query, happy to port it into a unit test for this codebase.
https://github.com/jlleitschuh/bad-pom

You can see an example of this query here:
https://lgtm.com/query/5517996632759312004/

[hmakholm]

It's pretty dismaying that HTTPS is apparently the only layer of defense for authenticity of huge swathes of the free software supply chain. Considering how large the attack surface for exfiltrating server certificates can be, something better than that is sorely needed.

(Which is of course in no way or shape a point against this query).

[JLLeitschuh]

@hmakholm Your assessment of this quite accurate, I wish we all had a better solution at this time.

[aschackmull]

Nice query! LGTM, just a few minor things.

The PR checks complain that the code is not formatted correctly - could you run autoformat? It's easy to do in e.g. VSCode - just right-click in the editor and select "Format Document" or hit Alt+Shift+F. If you're using Eclipse the short-cut is Ctrl+Shift+F.

Adding a small test would be nice. If you look in https://github.com/Semmle/ql/tree/master/java/ql/test/query-tests/maven-dependencies or https://github.com/Semmle/ql/tree/master/java/ql/test/query-tests/dead-code/DeadMethod there's a couple of existing tests for other queries using xml extraction already, and I don't see anything special in those tests, so I'm guessing that the qltests just use full xml extraction by default.

[aschackmull]

Adding a small test would be nice.

Actually, I believe we don't yet have the tooling in place to allow anyone outside github to run the tests, so it may be a bit tricky to get right, so I can help you out with that part. If you add the test itself to a suitable subdir in java/ql/test/query-tests/, I'll add the required .expected file.

[felicitymay]

Hopefully these small changes will fix the qhelp preview failure.

[JLLeitschuh]

Hey all,

My disconnected communication over the next two weeks is due to my being on vacation till December 7th. I'll see if I can steal some time to clean this up during this time, but if I can't, please understand that I'll get back to it as soon as I'm back home.

I really appreciate the review feedback and suggestions!!

[yo-h]

Nice query!

[JLLeitschuh]

Back from my vacation! 😄

Per the request, I've reformatted the query.

@aschackmull unless I've missed something, this PR should hopefully be in a state where you can add the .expected assertions and this can be merged (assuming there isn't any additional review feedback).

Please let me know if there's anything else that you'd like to see in this change.

[aschackmull]

I've added the missing pieces of the test as a PR against your branch here JLLeitschuh#1

[JLLeitschuh]

Merged your PR! Thanks!

I had 'Allow edits from maintainers.' turned on so you could have pushed directly to my branch if you'd wanted to.

[aschackmull]

LGTM. Anyone have any further comments? I've triggered the CI, so I'll merge on green if there are no further comments.

[felicitymay]

I haven't managed to see the qhelp preview yet. It would be good to have a quick look at that before we merge to check that nothing's broken.

[JLLeitschuh]

@felicitymay I'm not sure how to render a preview of the qhelp document. Is there a document you can point me to that describes how someone can render that document?

[hmakholm]

@JLLeitschuh The QHelp tooling is unfortunately still internal only. It's on my to-do list to add a codeql query qhelp command to the CLI, but there are several things in front of it...

(I think Felicity was not asking you to review it, but commenting for aschackmull and his team).

[aschackmull]

It's being rendered as part of the CI that I triggered, but it failed due to an & that ought to have been an & as per the change suggestion I made above. If you just click commit on that, we'll re-trigger CI and get the qhelp rendered.

[felicitymay]

@JLLeitschuh - many apologies for my unclear comment. We're still working on how to make qhelp previews easy for other people to generate. The CI task seems to need a further fix, suggested by @aschackmull, which I don't think I have permission to merge.

[JLLeitschuh]

Anything I can do to help here?

[aschackmull]

Anything I can do to help here?

Yes. I don't have write access to your branch, so if you could just click the "Commit suggestion" button on the s/Additional Gradle & Maven plugin:/Additional Gradle & Maven plugin: suggested change I posted above.

[JLLeitschuh]

Done. Sorry, didn't realize that was lingering there. Applied now! 😄

[felicitymay]

The query help looks fine. AFAIK, we haven't mentioned the CVSS base score previously, and perhaps we should start doing that for other queries?

[xcorail]

Hi there
@aschackmull is this query ready to be merged now?
Thanks

[yo-h]

@aschackmull, there's not yet a release note for this new query, right? Would you mind adding one?

[yo-h]

I've added a change note in #2615 both for this query as well as the other new one.