Java: Add query - insecure environment configuration during JMX/RMI server init

[timoles]

This query detects if an RMI/JMX server is initialised with a potentially dangerous environment.

By default the RMI/JMX authentication method is vulnerable to an unauthenticated deserialization attack, as it accepts arbitrary objects as username and password. This can be mitigated by setting an appropriate filter which prevents the deserialization to unwanted classes.

I hope overall the query fits all the requirements, and has all the potential files (... and I hope not to much is missing so it could potentially get out of experimental). Of course, I'm always happy about any feedback to improve the PR.

For the unit tests. I couldn't import import javax.management.remote.rmi.RMIConnectorServer. For this reason I was unable to execute proper tests for the RMIConnector. But maybe there's an obvious issue I'm missing. It feels like there is some kind of dependency problem, but I couldn't really make sense of the errors...

More info in the corresponding issue which I will open now.

Happy to hear your feedback :)

[Marcono1234]

Maybe the following review comments are helpful.
Note that I am not a member of this project, so feel free to treat all of the comments as suggestions only. The maintainers will probably be able to give you more technical feedback.

[JarLob]

Hi @timoles, do you plan to apply for SecLab bug bounty?

[timoles]

Hi @timoles, do you plan to apply for SecLab bug bounty?

Hey @JarLob, sorry for not following up on the bug bounty issue, some unexpected work came up. I created an all for one issue here.

[timoles]

Maybe the following review comments are helpful.
Note that I am not a member of this project, so feel free to treat all of the comments as suggestions only. The maintainers will probably be able to give you more technical feedback.

Thank you very much @Marcono1234 ! Your tips helped a lot and some of the little "tricks" i'll keep in mind for future queries. I implemented most of your suggestions, if you want to you can have a look at it. I hope I didn't spam you too much with notifications, as I underestimated the changes.
Some of the changes I implemented locally and pushed them. On these commits you are not noted as a reference (because I didn't know how). I'm happy to change that if that's somehow possible.

[Marcono1234]

I hope I didn't spam you too much with notifications

No worries, though hopefully we don't spam any other watchers too much 😄

On these commits you are not noted as a reference

That is not a problem at all. If you want you can also use GitHub's suggestion batch commit feature, or perform the changes locally using Git. You don't have to create separate commits for each of my review comments.

[smowton]

I think this could probably be quite a bit simpler:

• Source = the qualifier of Map.put calls that appear to write a relevant key/value
• Sink = any constructor or method that creates an RMI server, as at present

Then simply flag any creation operation that does NOT appear to have a relevant flow?

This would avoid having to special-case null, and the configuration that tracks Map constructors to put operations.

Regarding importing javax.management.remote.JMXConnectorServerFactory, this appears to be a limitation of the Java distribution that ships with our extractor. The simplest short-term solution is to create a stub of the relevant class under java/ql/test/experimental/stubs and add an options file to the test directory adding the stubs to our classpath.

[timoles]

I think this could probably be quite a bit simpler:

• Source = the qualifier of Map.put calls that appear to write a relevant key/value
• Sink = any constructor or method that creates an RMI server, as at present

Then simply flag any creation operation that does NOT appear to have a relevant flow?

This would avoid having to special-case null, and the configuration that tracks Map constructors to put operations.

Regarding the simplification of the query. The simplification suggestion seems to make sense. However, I don't really have a clear coding implementation in my head, and sounds like quite a big change to implement (in terms of time spend, at least for me). Would the modification (and probably speed improvement) be necessary in order to get a merge, or could it stay as-is?

Regarding importing javax.management.remote.JMXConnectorServerFactory, this appears to be a limitation of the Java distribution that ships with our extractor. The simplest short-term solution is to create a stub of the relevant class under java/ql/test/experimental/stubs and add an options file to the test directory adding the stubs to our classpath.

Regarding the stub... I implemented a minimalistic stub, not sure if this is according to the coding and testing guidelines though....

[smowton]

The stub is fine. Please do simplify the query if we can achieve similar results with a much less complicated query: this serves as a better example for others and a more useful starting point to potentially promote to our main query set in the future.

I'd also appreciate a comment on how this PR relates to #5818 (and @artem-smotrakov conversely)

[artem-smotrakov]

I'd also appreciate a comment on how this PR relates to #5818 (and @artem-smotrakov conversely)

I think the root cause of problems is the same -- unsafe deserialization. The query in this pull requests looks for deserialization vulnerabilities in RMI/JMX servers. The query in #5818 looks for vulnerabilities in user's remote objects. By the way, in older Java versions, RMI registry was vulnerable to deserialization attacks itself. That was addressed in JEP 290 by applying deserialization filters for serialized data that is processed by the registry. It might make sense to add a query for that but the query has to make sure that Java version before JEP 290 is used. I am not sure that CodeQL can do that. Can it?

[pwntester]

I'd also appreciate a comment on how this PR relates to #5818 (and @artem-smotrakov conversely)

I think the root cause of problems is the same -- unsafe deserialization. The query in this pull requests looks for deserialization vulnerabilities in RMI/JMX servers. The query in #5818 looks for vulnerabilities in user's remote objects. By the way, in older Java versions, RMI registry was vulnerable to deserialization attacks itself. That was addressed in JEP 290 by applying deserialization filters for serialized data that is processed by the registry. It might make sense to add a query for that but the query has to make sure that Java version before JEP 290 is used. I am not sure that CodeQL can do that. Can it?

Just to add more context, this PR is more about JMX servers (using RMI under the hood) started programmatically and therefore not applying mitigations for CVE-2016-3427.

[smowton]

Java version being used: I'd guess not, as even if the code is built for Java 8, it could be executed against a JVM / standard library that does have the relevant mitigation. We'd probably have to choose between a low-severity warning or not warning at all because we consider mitigation too likely.

[timoles]

I think this could probably be quite a bit simpler:

• Source = the qualifier of Map.put calls that appear to write a relevant key/value
• Sink = any constructor or method that creates an RMI server, as at present

Then simply flag any creation operation that does NOT appear to have a relevant flow?

This would avoid having to special-case null, and the configuration that tracks Map constructors to put operations.

Hey @smowton I tried implementing a more simplified version but just can't get it to work. I mainly tried two different solutions. Having two flows and negating them, or implementing a barrier. Overall I have no clue which approach I should take and why my approaches are not working (because they could never work, or just because of implementation errors...). I would need some advice on how to actually implement it

1. Try to naively have an all init flow and a safe flow and negate them

I have a flow with all server creations and have a flow from Map.put to the server init parameter.
However, I always get to many/little results (For now I only tried to match new RMIConnectorServer calls:

// As a result this flow successfully gives back all new RMIConnector calls
class FlowAllInits extends DataFlow::Configuration {
  FlowAllInits() { this = "FlowAllInits" }

  override predicate isSource(DataFlow::Node source) {
    any()
  }

  override predicate isSink(DataFlow::Node sink) {
    // Match all new RMIConnectorServer calls
    exists(ConstructorCall ccall | sink.asExpr().(ConstructorCall) = ccall |
      isRmiOrJmxServerCreateConstructor(ccall.getConstructor())
    )
  }
}

// This flow successfully maps if a safe map/environment is used during our new RMIConnector call
class SafeFlow extends DataFlow::Configuration {
  SafeFlow() { this = "SafeFlow" }

  override predicate isSource(DataFlow::Node source) {
    putsCredentialtypesKey(source.asExpr()) 
  }

  override predicate isSink(DataFlow::Node sink) {
    exists(ConstructorCall ccall |
      sink.asExpr() = ccall.getArgument(1)
    )
  }
}

From [...]
where flowAllInits.hasFlow(source, sink) and not safeFlow.hasFlow(source, sink) // This returns way to many/little nodes
select source, sink

2. Try to implement a barrier

// Same as in the previous example, but with a barrier

class FlowAllInits extends DataFlow::Configuration {
  FlowAllInits() { this = "FlowAllInits" }

  override predicate isSource(DataFlow::Node source) {
    any()
  }

  override predicate isSink(DataFlow::Node sink) {
    // Match all new RMIConnectorServer calls
    exists(ConstructorCall ccall | sink.asExpr().(ConstructorCall) = ccall |
      isRmiOrJmxServerCreateConstructor(ccall.getConstructor())
    )
  }

  override predicate isBarrier(DataFlow::Node barrier) { 
  // No matter how I tried to implement that it never worked
  // I also have no idea how the barrier node is defined (like, where does the node even come from, is it every edge?
  // But basically what I tried, check if the first argument of the RMIConnectorServer exists, and then check if it has our safe Map.put call
    putsCredentialtypesKey(barrier.asExpr().(ConstructorCall).getArgument(1))
  }
}

From [...]
where flowAllInits.hasFlow(source, sink)
source, sink // The barrier seems to never work

I would be happy with some hints on how to implement it in general (barrier, ...)
Overall the simplified query should be quite easy, but I've sunken more hours into rewriting it then I'm willing to admit....

One more thing... Am I right in the assumption that isAddtionalFlowStep can only be used for adding additional results to the flow and not to restrict the query?

(If wanted I can upload the complete code in a different branch for reference, but for that I would first need to clean up a bunch of trial and error code)

[smowton]

Source (or sink) = any() almost never makes sense, and has potentially very large performance costs.

I think the problem stems from trying to use a path-problem query to identify a problem of absence of flow. Here (smowton@6c679f6) I've adapted the query to have @kind problem instead, and simply return those RMI setup calls that don't have a flow indicating safety.

I've also fixed the static-final-field-read question and added stubs sufficient to test RMIConnectorServer, though our inability to extract projects that use the package javax.management.remote.rmi is a bug we'll need to fix before this query or one like it can do anything useful.

[timoles]

Source (or sink) = any() almost never makes sense, and has potentially very large performance costs.

I think the problem stems from trying to use a path-problem query to identify a problem of absence of flow. Here (smowton@6c679f6) I've adapted the query to have @kind problem instead, and simply return those RMI setup calls that don't have a flow indicating safety.

I've also fixed the static-final-field-read question and added stubs sufficient to test RMIConnectorServer, though our inability to extract projects that use the package javax.management.remote.rmi is a bug we'll need to fix before this query or one like it can do anything useful.

Thank you very much! I didn't expect a fully working solution. Seeing the result it seems... straightforward.

[smowton]

Addendum: the bug regarding javax.management.remote.rmi is actually particular to tests (creating databases for real projects works fine), so the stubs I introduce in my commit in https://github.com/smowton/codeql/tree/insecureJmxRmiServerEnvironment are a sufficient workaround.

I also updated that branch to fix the expected file, which I had forgotten to update. Feel free to pull that branch if you want.

[smowton]

Addendum to the addendum: it's not a bug that codeql test run's extraction routine can't see the RMI module; that was a choice made to shrink the distribution size (cf. when using codeql database create, we intercept a real build and use the real JDK)

[smowton]

Apologies for delay. QHelp needs to be valid XML, otherwise we can merge this

[smowton]

Suggested change

	method, resulting in the attempted deserialization of an attacker-controlled object.
	method, resulting in the attempted deserialization of an attacker-controlled object.
	</p>

[smowton]

And various other places: make sure paragraphs are surrounded by <p></p> and all elements are terminated.

[timoles]

The XML is validated, we also addressed these concerns and overall tried to improve the help

[smowton]

Suggested change

	* Holds if a `put` call on `qualifier` puts a key match
	* into the map.
	* Holds if a `put` call on `qualifier` puts a key into the map that sets some RMI server type filter.

[smowton]

QHelp is still invalid: the code element is an inline element that must fall within <p>, to be used styling a short keyword or snippet. For a multi-line code example use the <sample> tag to include an example from an external file.

[timoles]

Sorry about the qhelp, at first I didn't realise the validator is built into codeql. Now everything should be alright, at least I managed to built it locally.

[smowton]

InsecureRmiJmxEnvironmentConfiguration.ql needs autoformat; then good to go

[timoles]

Pushed the autoformatting. I just hope these two failed action runs don't have anything to do with me?
I guess the one with the HTTP 500 errors I'm not responsible for, but the Java validator might have failed on my added sample files. Should I not name them with a .java extension?

Or should I not do it through the Java files, but similar to this one? (even though there the sample is not within a <p> tag?)

[smowton]

Either way is fine. I actually can't see any failed action runs, and have kicked off our usual test suite.

[timoles]

On that occasion, a big thank you to everyone for helping me figuring things out and making the PR possible.
It was a pleasure getting so much friendly help :)