Java: add some neutral models discovered with heuristics

[jcogs33]

Description

This PR adds some neutral models discovered with heuristics.
The apache model was discovered with a ssrf heuristic. The rest of the models were discovered with a path-injection heuristic.

(cc @atorralba @tiferet)

Consideration

• I put a provenance of "manual" on these since they were manually reviewed after initially being generated by heuristic queries. Let me know if I should use a different provenance for heuristic-generated models (related to this discussion).
• I didn't add a change-note because I don't think neutral models are really a "user-visible" change. Let me know if I'm wrong about that and if I should add a change note for these models.
• I haven't added test cases for neutral models before, but let me know if I should add some "tests" that just have a // no flow comment or something similar.

[atorralba]

Looks good! I think there was a small formatting mistake I added some suggestions for.

I put a provenance of "manual" on these since they were manually reviewed after initially being generated by heuristic queries. Let me know if I should use a different provenance for heuristic-generated models (related to #12228 (comment)).

I think manual is the right provenance according to the discussion.

I didn't add a change-note because I don't think neutral models are really a "user-visible" change. Let me know if I'm wrong about that and if I should add a change note for these models.

This doesn't need a change note indeed.

I haven't added test cases for neutral models before, but let me know if I should add some "tests" that just have a // no flow comment or something similar.

For neutral summaries, it probably doesn't make sense to add tests. But for neutral sinks, which are manually added and from my POV have a stronger and more intentional sense of "this is safe/non-exploitable", I think it could make sense to have tests — so that we get notified if that assumption is no longer valid and someone adds such a sink in the future.

[jcogs33]

For neutral summaries, it probably doesn't make sense to add tests. But for neutral sinks, which are manually added and from my POV have a stronger and more intentional sense of "this is safe/non-exploitable", I think it could make sense to have tests — so that we get notified if that assumption is no loner valid and someone adds such a sink in the future.

Sounds good, I'll add tests for these, but I'll wait until the discussion from elsewhere about exactly how to represent neutral sinks is finalized.
Converting this PR back to a draft until then.

[jcogs33]

Re-opening this PR for review now that #12931 is completed.
As a reminder, this PR is just adding a few neutral sink models that were discovered when reviewing heuristic results, it is not intended to be complete.

Changes since this was last reviewed:

• added "sink" kinds
• updated the provenance to "hq-manual"
• added tests as was recommended above

Regarding the tests, let me know if you think there is a better way to test these. The test case I've added just uses sinkNode to check if a MaD sink exists and will fail if a MaD sink is added for one of the neutrals, but it currently won't fail if a sink is added in regular CodeQL. Let me know if there is a good way to include a check for sinks written in regular CodeQL.
Also, would it make sense to have a more general check/test instead that makes sure that sinks and neutral sinks never overlap? Maybe something like getInvalidNeutralModel added to invalidModelRow?

[atorralba]

Models themselves LGTM.

Let me know if there is a good way to include a check for sinks written in regular CodeQL.

I guess that would only work for queries with extensible sinks (i.e. abstract classes), and you'd need to explicitly use them in tests, which doesn't seem ideal. I think we're fine testing only models-as-data sinks for the moment.

On a different note, you're currently testing that certain callables don't contain sinks, but not that we actually have neutral models for them. Do you think it makes sense to also test that with something like the following?

pragma[nomagic]
predicate isNeutral() { this = any(FlowSummaryImpl::Public::NeutralCallable nsc).asCallable() }

Let me know if you think it's overkill.

[jcogs33]

I guess that would only work for queries with extensible sinks (i.e. abstract classes), and you'd need to explicitly use them in tests, which doesn't seem ideal. I think we're fine testing only models-as-data sinks for the moment.

👍

On a different note, you're currently testing that certain callables don't contain sinks, but not that we actually have neutral models for them. Do you think it makes sense to also test that with something like the following?

pragma[nomagic]
predicate isNeutral() { this = any(FlowSummaryImpl::Public::NeutralCallable nsc).asCallable() }

Let me know if you think it's overkill.

It seems somewhat overkill to me, but I don't have a strong opinion about it 🙂, so I added something to test for this in 82f208c. Note that FlowSummaryImpl::Public::NeutralCallable is specific to neutral summaries, so I wasn't able to use it to test for neutral sinks. What I did instead feels a bit hacky to me, so let me know if you think it isn't worth adding this or if you think there's a better way to write this test case. (I suppose I could create a NeutralSinkCallable class similar to FlowSummaryImpl::Public::NeutralCallable, but I was trying to keep it simple by just using existing classes/predicates).

[atorralba]

(I suppose I could create a NeutralSinkCallable class similar to FlowSummaryImpl::Public::NeutralCallable, but I was trying to keep it simple by just using existing classes/predicates).

I think we should have that class, but that's out of the scope of this PR.

I like that we can test that we're creating the neutrals we're intending to, AND that none of those collides with a sink. Thanks for indulging me!