Java: Extract module HardcodedCredentials from CWE-798

[rvermeulen]

This PR needs some scrutinizing.

In this change we:

• Move the module HardcodedCredentials to an importable location
• Rename the module SensitiveApi to CredentialReceivingApi and move it to an importable location
• Extend the CredentialSink from DataFlow::Expr instead of Expr
• Generalize UsernameVariable and PasswordVariable

[aschackmull]

Suggested change

	abstract class CredentialsSink extends DataFlow::Expr {
	abstract class CredentialsSink extends Expr {

The type Expr is not intentionally exposed through the DataFlow module (though with all the import java statements it's no surprise that it slipped in).

[aschackmull]

What is the use case for this PR? Which of the queries need support for extension and in which ways? And which parts of the moved library are expected to be re-usable in other contexts outside the CWE-798 queries?
Without reasonable use cases, I think we should leave the library where it is.

[rvermeulen]

For this case there is no immediate use case. I'm making all possible extension points (e.g, source, sinks, ...) in the Security/CWE/* available to customization via Customizations.qll to prevent query duplication if we need to extend a CWE query. This should help if there are cases where closed source CredentialSink instances need to be modeled for example.

[aschackmull]

I'm making all possible extension points

In general, queries have an infinite number of possible extension points, although I agree that for most cases it comes down to source and sink extensions.

The way these queries are made, means that there's an implicit assumption that CredentialsSink consists of just two cases: Sinks defined in the source CredentialsSourceSink and sinks defined outside the source CredentialsApiSink. So CredentialsSink shouldn't really be exposed as an abstract class, as this suggests an extension point, which would invalidate this assumption. If a closed-source credential sink needs to be added, then it is the CredentialsApiSink that should be extended.

[rvermeulen]

I'm making all possible extension points

In general, queries have an infinite number of possible extension points, although I agree that for most cases it comes down to source and sink extensions.

True, I indeed mean the more common cases.

The way these queries are made, means that there's an implicit assumption that CredentialsSink consists of just two cases: > Sinks defined in the source CredentialsSourceSink and sinks defined outside the source CredentialsApiSink. So CredentialsSink shouldn't really be exposed as an abstract class, as this suggests an extension point, which would invalidate > this assumption. If a closed-source credential sink needs to be added, then it is the CredentialsApiSink that should be extended.

Clear, I agree that CredentialsApiSink should be the only exposed point of extension. Given how it is now structured I need to give this some more thought since both the abstract CredentialsSink as its subclasses are directly referenced.