Open CaledoniaProject opened 2 years ago
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.
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.
@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
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" ] } }
@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]);
}
}
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
}
}
Did you mean String[] args
is not marked as source?
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" } }
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)
I understand cli arguments is not reachable remotely, just trying to create a simple case and learn CodeQL.
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.
Thanks, I'll take a look.
The "$@" does not seem to be replaced by variables, is that a bug?
@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]);
}
}
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 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:
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.qllUnderstood, now I'm going to analyze log4j 2.11 directly.
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?
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.
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:
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?