Need help on JNDI injection query, does not work for log4j test project

[CaledoniaProject]

I've created a simple log4j project: log4j-test.zip

It uses log4j 2.11 which is vulnerable to JNDI injection, and I've verified the vulnerability exists. Now I need to verify codeql also works. So I created the java database with:

codeql database create java-database -l=java -c="mvn clean install -file pom.xml" --overwrite

Then I opened the starter project and uses ql/java/ql/src/Security/CWE/CWE-074/JndiInjection.ql to test it, but no results came out.

What was wrong?

[CaledoniaProject]

I don't know which part is wrong, I also created another project to test:

public static void main(String[] args) throws Exception {
        Context ctx = new InitialContext();
        Object object = ctx.lookup(args[0]);
}

It does not work either.

[KeuntaeShin]

You may need to consult that the test code has statements and control flows like: ql/java/ql/src/Security/CWE/CWE-074/JndiInjection.java, or not.

[CaledoniaProject]

@KeuntaeShin Thanks for the heads up, this looks exactly same to the second test I've done. I believe the query is suitable for my code too, not sure why it didn't work.

public class JndiInjectionTest {
  @RequestMapping
  public void testInitialContextBad1(@RequestParam String nameStr) throws NamingException {
    Name name = new CompositeName(nameStr);
    InitialContext ctx = new InitialContext();

    ctx.lookup(nameStr); // $hasJndiInjection
    ctx.lookupLink(nameStr); // $hasJndiInjection

[KeuntaeShin]

Additionally, the test code's lookup is one of those?

/** CSV sink models representing methods susceptible to JNDI injection attacks. */
private class DefaultJndiInjectionSinkModel extends SinkModelCsv {
override predicate row(string row) {
row =
[
"javax.naming;Context;true;lookup;;;Argument[0];jndi-injection",
"javax.naming;Context;true;lookupLink;;;Argument[0];jndi-injection",
"javax.naming;Context;true;rename;;;Argument[0];jndi-injection",
"javax.naming;Context;true;list;;;Argument[0];jndi-injection",
"javax.naming;Context;true;listBindings;;;Argument[0];jndi-injection",
"javax.naming;InitialContext;true;doLookup;;;Argument[0];jndi-injection",
"javax.management.remote;JMXConnector;true;connect;;;Argument[-1];jndi-injection",
"javax.management.remote;JMXConnectorFactory;false;connect;;;Argument[0];jndi-injection",
// Spring
"org.springframework.jndi;JndiTemplate;false;lookup;;;Argument[0];jndi-injection",
// spring-ldap 1.2.x and newer
"org.springframework.ldap.core;LdapOperations;true;lookup;;;Argument[0];jndi-injection",
"org.springframework.ldap.core;LdapOperations;true;lookupContext;;;Argument[0];jndi-injection",
"org.springframework.ldap.core;LdapOperations;true;findByDn;;;Argument[0];jndi-injection",
"org.springframework.ldap.core;LdapOperations;true;rename;;;Argument[0];jndi-injection",
"org.springframework.ldap.core;LdapOperations;true;list;;;Argument[0];jndi-injection",
"org.springframework.ldap.core;LdapOperations;true;listBindings;;;Argument[0];jndi-injection",
"org.springframework.ldap.core;LdapOperations;true;search;(Name,String,ContextMapper);;Argument[0];jndi-injection",
"org.springframework.ldap.core;LdapOperations;true;search;(Name,String,int,ContextMapper);;Argument[0];jndi-injection",
"org.springframework.ldap.core;LdapOperations;true;search;(Name,String,int,String[],ContextMapper);;Argument[0];jndi-injection",
"org.springframework.ldap.core;LdapOperations;true;search;(String,String,ContextMapper);;Argument[0];jndi-injection",
"org.springframework.ldap.core;LdapOperations;true;search;(String,String,int,ContextMapper);;Argument[0];jndi-injection",
"org.springframework.ldap.core;LdapOperations;true;search;(String,String,int,String[],ContextMapper);;Argument[0];jndi-injection",
"org.springframework.ldap.core;LdapOperations;true;searchForObject;(Name,String,ContextMapper);;Argument[0];jndi-injection",
"org.springframework.ldap.core;LdapOperations;true;searchForObject;(String,String,ContextMapper);;Argument[0];jndi-injection",
// spring-ldap 1.1.x
"org.springframework.ldap;LdapOperations;true;lookup;;;Argument[0];jndi-injection",
"org.springframework.ldap;LdapOperations;true;lookupContext;;;Argument[0];jndi-injection",
"org.springframework.ldap;LdapOperations;true;findByDn;;;Argument[0];jndi-injection",
"org.springframework.ldap;LdapOperations;true;rename;;;Argument[0];jndi-injection",
"org.springframework.ldap;LdapOperations;true;list;;;Argument[0];jndi-injection",
"org.springframework.ldap;LdapOperations;true;listBindings;;;Argument[0];jndi-injection",
"org.springframework.ldap;LdapOperations;true;search;(Name,String,ContextMapper);;Argument[0];jndi-injection",
"org.springframework.ldap;LdapOperations;true;search;(Name,String,int,ContextMapper);;Argument[0];jndi-injection",
"org.springframework.ldap;LdapOperations;true;search;(Name,String,int,String[],ContextMapper);;Argument[0];jndi-injection",
"org.springframework.ldap;LdapOperations;true;search;(String,String,ContextMapper);;Argument[0];jndi-injection",
"org.springframework.ldap;LdapOperations;true;search;(String,String,int,ContextMapper);;Argument[0];jndi-injection",
"org.springframework.ldap;LdapOperations;true;search;(String,String,int,String[],ContextMapper);;Argument[0];jndi-injection",
"org.springframework.ldap;LdapOperations;true;searchForObject;(Name,String,ContextMapper);;Argument[0];jndi-injection",
"org.springframework.ldap;LdapOperations;true;searchForObject;(String,String,ContextMapper);;Argument[0];jndi-injection",
// Shiro
"org.apache.shiro.jndi;JndiTemplate;false;lookup;;;Argument[0];jndi-injection"
]
}
}

[CaledoniaProject]

@KeuntaeShin This is the full source code I'm using. The item to lookup is dynamic determined by user input:

package Main;

import java.io.*;
import javax.naming.*;
import java.rmi.registry.*;

public class main {
    public static void main(String[] args) throws Exception {
        Context ctx = new InitialContext();
        Object object = ctx.lookup(args[0]);
    }
}

[KeuntaeShin]

Codeql's path-problem depends on a concept of source and sink. It means specific statements such as functions, variables, and etc must be explicitly defined in the query statements or .qlls, and the target source code must meet the conditions and data flows.

I made an example to help your understand. A method - jndiLookup_your_codebase will not be detected like your code, because there is no source the query expect.

public void jndiLookup(HttpServletRequest request) throws NamingException {
    String name = request.getParameter("name");

    Hashtable<String, String> env = new Hashtable<String, String>();
    env.put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.rmi.registry.RegistryContextFactory");
    env.put(Context.PROVIDER_URL, "rmi://trusted-server:1099");
    InitialContext ctx = new InitialContext(env);

    // BAD: User input used in lookup
    ctx.lookup(name);

    // GOOD: The name is validated before being used in lookup
    if (isValid(name)) {
        ctx.lookup(name);
    } else {
        // Reject the request
    }
}
public void jndiLookup_your_codebase() throws NamingException {
    String name = "this will not be identified!";

    Hashtable<String, String> env = new Hashtable<String, String>();
    env.put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.rmi.registry.RegistryContextFactory");
    env.put(Context.PROVIDER_URL, "rmi://trusted-server:1099");
    InitialContext ctx = new InitialContext(env);

    // BAD: User input used in lookup
    ctx.lookup(name);

    // GOOD: The name is validated before being used in lookup
    if (isValid(name)) {
        ctx.lookup(name);
    } else {
        // Reject the request
    }
}

[CaledoniaProject]

Did you mean String[] args is not marked as source?

[KeuntaeShin]

Did you mean String[] args is not marked as source?

Yes. FYI:

class JndiInjectionFlowConfig extends TaintTracking::Configuration {
...
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
}

abstract class RemoteFlowSource extends DataFlow::Node {
/** Gets a string that describes the type of this remote flow source. */
abstract string getSourceType();
}

private class SpringServletInputParameterSource extends RemoteFlowSource {
SpringServletInputParameterSource() {
this.asParameter() = any(SpringRequestMappingParameter srmp | srmp.isTaintedInput())
}

override string getSourceType() { result = "Spring servlet input parameter" }
}

[intrigus-lgtm]

Did you mean String[] args is not marked as source?

String[] args IS a source. It is just NOT a RemoteFlowSource.
Instead, it is a LocalUserInput (I don't know why it is not named local flow source instead, but I will refer to it as a local flow source).

CodeQL distinguishes between sources that can be remotely "reached" and those that can not.
For example, a REST controller implemented using the Spring framework. In this case usually the controller will be remotely reachable. Therefore this is a remote flow source.

On the other hand, there are sources that are NOT remotely reachable.
For example, the String[] args of a Java main method are usually not remotely reachable. To start a Java program you need local access to the computer. And if you have this access, all bets are of.
Other instances of local flow sources are environment variables. These variables can usually only be changed locally, i.e. someone has to have direct access to the computer. Again, all bets are of.

The JNDI query only concerns itself with remote flow sources.
Most CodeQL queries allow to extend sources and sinks - the JNDI query does not allow this (directly).
If we want to include local flow as a source we have to change the code of https://github.com/github/codeql/blob/main/java/ql/src/Security/CWE/CWE-074/JndiInjection.ql
to this:

/**
 * @name JNDI lookup with user-controlled name
 * @description Performing a JNDI lookup with a user-controlled name can lead to the download of an untrusted
 *              object and to execution of arbitrary code.
 * @kind path-problem
 * @problem.severity error
 * @precision high
 * @id java/jndi-injection
 * @tags security
 *       external/cwe/cwe-074
 */

import java
import semmle.code.java.security.JndiInjectionQuery
import DataFlow::PathGraph
import semmle.code.java.dataflow.DataFlow

private class CustomJndiInjectionFlowConfig extends JndiInjectionFlowConfig {
    CustomJndiInjectionFlowConfig(){ this = "CustomJndiInjectionFlowConfig"}
    override predicate isSource(DataFlow::Node source) {
        source instanceof LocalUserInput
    }
}

from DataFlow::PathNode source, DataFlow::PathNode sink, CustomJndiInjectionFlowConfig conf
where conf.hasFlowPath(source, sink)
select sink.getNode(), source, sink, "JNDI lookup might include name from $@.", source.getNode(),
  "this user input"

The new query now only considers local flow sources.
(This query is untested but should work)

[CaledoniaProject]

I understand cli arguments is not reachable remotely, just trying to create a simple case and learn CodeQL.

1. The query above didn't work.
2. How can I debug the problem? I'm not sure where the problem originates, i.e the "source" or the "sink" is not matched

[intrigus-lgtm]

1. I made a small mistake. To extend the existing JndiInjectionFlowConfig class we have to reuse its characteristic predicate (The JndiInjectionFlowConfig() { this = "JndiInjectionFlowConfig" } part).
   [The previous version had CustomJndiInjectionFlowConfig(){ this = "CustomJndiInjectionFlowConfig"} as the characteristic predicate. This says (simplified) that if we have an instance of type JndiInjectionFlowConfig and this = "CustomJndiInjectionFlowConfig" holds, we should consider this also as an instance of the type CustomJndiInjectionFlowConfig.
   The problem now is that the characteristic predicate of CustomJndiInjectionFlowConfig contradicts JndiInjectionFlowConfig!
   It can not hold that this = "CustomJndiInjectionFlowConfig" (for CustomJndiInjectionFlowConfig) and also in the super class this = "JndiInjectionFlowConfig" (for JndiInjectionFlowConfig).]

   The query below works on the Main.java file.

   java/ql/src/Security/CWE/CWE-074/JndiInjection.ql

   /**
    * @name JNDI lookup with user-controlled name
    * @description Performing a JNDI lookup with a user-controlled name can lead to the download of an untrusted
    *              object and to execution of arbitrary code.
    * @kind path-problem
    * @problem.severity error
    * @precision high
    * @id java/jndi-injection
    * @tags security
    *       external/cwe/cwe-074
    */
   
   import java
   import semmle.code.java.security.JndiInjectionQuery
   import DataFlow::PathGraph
   import semmle.code.java.dataflow.DataFlow
   
   private class CustomJndiInjectionFlowConfig extends JndiInjectionFlowConfig {
       CustomJndiInjectionFlowConfig(){this = "JndiInjectionFlowConfig"  } // <-- here is the change
       override predicate isSource(DataFlow::Node source) {
           source instanceof LocalUserInput
       }
   }
   
   from DataFlow::PathNode source, DataFlow::PathNode sink, CustomJndiInjectionFlowConfig conf
   where conf.hasFlowPath(source, sink)
   select sink.getNode(), source, sink, "JNDI lookup might include name from $@.", source.getNode(),
     "this user input"

   main/Main.java

   package main;
   
   import java.io.*;
   import javax.naming.*;
   
   public class Main
   {
   
       public static void main(String[] args) throws Exception {
           Context ctx = new InitialContext();
           Object object = ctx.lookup(args[0]);
       }
   }

2. If you want to debug data flow queries, I can recommend this article:
   https://codeql.github.com/docs/writing-codeql-queries/debugging-data-flow-queries-using-partial-flow/

[CaledoniaProject]

Thanks, I'll take a look.

The "$@" does not seem to be replaced by variables, is that a bug?

[CaledoniaProject]

@intrigus-lgtm This works for for simple context lookup, but not the log4j project I uploaded earlier: log4j-test.zip

It also uses LocalUserInput as source, but the query above didn't find anything:

package main;

import java.io.*;
import org.apache.logging.log4j.*;

public class Main
{
    public void test(String input) throws Exception
    {
        System.out.println(input);

        Logger logger = LogManager.getLogger(this.getClass());
        logger.error(input);
    }

    public static void main(String[] args) throws Exception
    {
        if (args.length == 0) {
            System.out.println("not enough arguments");
            return;
        }

        Main main = new Main();
        main.test(args[0]);
    }
}

[smowton]

Thanks, I'll take a look.

The "$@" does not seem to be replaced by variables, is that a bug?

Do you have @kind path-problem (and import DataFlow::PathGraph) in your query? Replacement happens in building the higher-level "alerts" produced by path-problem and problem queries -- by comparison your screenshot shows a raw #select result, suggesting either there was no @kind set or you explicitly navigated away from alerts to the raw #select output.

[intrigus-lgtm]

@intrigus-lgtm This works for for simple context lookup, but not the log4j project I uploaded earlier: log4j-test.zip

It also uses LocalUserInput as source, but the query above didn't find anything:

package main;

import java.io.*;
import org.apache.logging.log4j.*;

public class Main
{
    public void test(String input) throws Exception
    {
        System.out.println(input);

        Logger logger = LogManager.getLogger(this.getClass());
        logger.error(input);
    }

    public static void main(String[] args) throws Exception
    {
        if (args.length == 0) {
            System.out.println("not enough arguments");
            return;
        }

        Main main = new Main();
        main.test(args[0]);
    }
}

CodeQL creates a complete database of your project; for dependencies of your project (such as log4j in this case) it only extracts a limited amount of data. This extracted data is not sufficient to enable data flow on dependencies.

There are three possible solutions to this problem depending on the exact problem:

1. You build your dependencies from source when compiling your project. This should allow database to work also in dependencies, because it is compiled from source and so all necessary data should be available. This is the most general solution, but probably also the most complex solution.
   
   It may also be possible to create a normal database of your project and combine it with a database of the dependency. I never tried this (and I would not recommend sinking too much time into it), but it might be possible.
2. Problem: The sink is in the dependency.
   This is the case with log4j, the JNDI injections happens inside it. If you know which log4j methods will perform the JNDI injection, you can adjust the sinks and change them to the log4j methods. This is what the "official" log4j query does:
   https://github.com/github/codeql/blob/main/java/ql/src/experimental/Security/CWE/CWE-020/Log4jJndiInjection.ql#L186
3. Problem: Taint/data should flow through a dependency.
   If you have a function in a dependency like String transfer(String taintedInput) and know that a tainted argument will always taint the return value, you can define a summary to tell CodeQL about this additional way taint/data can flow.
   If you want to do this, have a look at this (experimental) API:
   https://github.com/github/codeql/blob/15c1ce722a4bcaa892e982c34ac28cd3d77e4a11/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll

[CaledoniaProject]

Understood, now I'm going to analyze log4j 2.11 directly.

• The source is: https://github.com/apache/logging-log4j2/blob/f72100df0decc9bda96b4d769822c4e48b2848fc/log4j-api/src/main/java/org/apache/logging/log4j/spi/AbstractLogger.java#L281
• The sink is: javax.naming.Context.lookup()

I first verified that lookup() method call exists in log4j

import java

from MethodAccess call, Method method
where method.hasName("lookup") and method.getDeclaringType().getASupertype().hasQualifiedName("javax.naming", "Context") and call.getMethod() = method
select call

Then I created a flow query from AbstractLogger.debug() to Context.lookup(), but no results is found:

import java
import semmle.code.java.security.JndiInjectionQuery
import DataFlow::PathGraph
import semmle.code.java.dataflow.DataFlow

private class CustomJndiInjectionFlowConfig extends JndiInjectionFlowConfig {
  CustomJndiInjectionFlowConfig() { this = "JndiInjectionFlowConfig" } // <-- here is the change

  override predicate isSource(DataFlow::Node source) {
    exists(Method method |
      method.hasName("debug") and
      method
        .getDeclaringType()
        .getAnAncestor()
        .hasQualifiedName("org.apache.logging.log4j.spi", "AbstractLogger") and
      source.asParameter() = method.getParameter(0)
    )
  }

  override predicate isSink(DataFlow::Node sink) {
    exists(MethodAccess call, Method method |
      method.hasName("lookup") and
      method.getDeclaringType().getASupertype().hasQualifiedName("javax.naming", "Context") and
      call.getMethod() = method and
      sink.asExpr() = call.getArgument(1)
    )
  }
}

from DataFlow::PathNode source, DataFlow::PathNode sink, CustomJndiInjectionFlowConfig conf
where conf.hasFlowPath(source, sink)
select sink.getNode(), source, sink, "JNDI lookup might include name from $@.", source.getNode(),
  "this user input"

What was wrong?

[intrigus-lgtm]

Did you try doing partial dataflow as described in the article I posted earlier? (https://codeql.github.com/docs/writing-codeql-queries/debugging-data-flow-queries-using-partial-flow/)
This should help you to find the position at which flow is getting lost.
Also quick eval your isSource and isSink predicates to see if they return expected results.