Fix Android logging signature

[bananabr]

The method signature for the Android Logger methods in the java/ql/src/experimental/semmle/code/java/Logging.qll is wrong and prevents the proper identification of sensitive data logging in Android applications.

[bananabr]

Would a fix like this be a candidate for the One-For-All program?
I found many vulnerable Android apps in LGTM using this fix that were not previously covered by the current query but I am not sure if a fix meets the "improve an existing query and extend its coverage " requirement.

[smowton]

These seem like they're saying the same thing, are you aware of a case that means you need both?

[bananabr]

To be honest I just tried to keep this as close to the originally approved query as possible. Why are these both required in lines 24 and 25? My understanding is also that they are equivalent.

[aschackmull]

There are some differences in relation to generics, in particular how these relations deal with parameterizations, raw types, type variables, and wildcard types. But for something simple like this case, then I expect them to be equivalent (with some possible caveats if there are generic subclasses of android.util.Log).

[bananabr]

Just realized the Log class in Android is final, so neither is required. I will push the update in a minute.

[bananabr]

I simplified the query considering the fact the Log class is final. All my test results remained the same.

[bananabr]

Would a fix like this be a candidate for the One-For-All program?
I found many vulnerable Android apps in LGTM using this fix that were not previously covered by the current query but I am not sure if a fix meets the "improve an existing query and extend its coverage " requirement.

Can someone, please, answer this question? I just want to know if fixes like this one are in scope for the One-for-All program. If the are, I will take the time to submit the required issue. I also would like to mention that I really like codeQL so I will keep contributing to the code any chance I have.

Kudos to the team

[intrigus-lgtm]

Would a fix like this be a candidate for the One-For-All program?
I found many vulnerable Android apps in LGTM using this fix that were not previously covered by the current query but I am not sure if a fix meets the "improve an existing query and extend its coverage " requirement.

Can someone, please, answer this question? I just want to know if fixes like this one are in scope for the One-for-All program.

(NOTE: I'm not affiliated with lgtm.com or the codeql team in any way)

I'd say that this PR should be eligible.
This PR also "only" fixes the detection of an existing query: https://github.com/github/codeql/pull/5349/files#diff-e703c1a656e7f606993e8939b636c98943fe2f46eb6b121fb67444fd8c104b3f
So there is some precedent for such PRs.

And per the bounty rules:

Write a CodeQL query that models a vulnerability class not currently covered by the current queries, or improve an existing query and extend its coverage to detect additional vulnerabilities.

So if you

find at least one CVE that was not previously found by an existing query

it should indeed be eligible.

It might also make sense to add tests for the Logging.qll file, because it seems as if the original PR did not add tests (but I also did not look into it too much).

[smowton]

Sorry I didn't notice the question; yes in principle I think it's eligible though I imagine the bounty would be smaller than most due to the low technical difficulty of the fix. However there's no harm in submitting an application!