CodeQL query to detect JNDI injections by ggolawski · Pull Request #3288 · github/codeql

ggolawski · 2020-04-17T19:53:30Z

This PR adds a query to detect JNDI injections. It flags the code where user-provided input is used in JNDI lookup:

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);
}

It also supports Spring's and Apache Shiro's JndiTemplate.

If the name being used to look up the data is controlled by the user, it can point to a
malicious server, which can return an arbitrary object. In the worst case, this can allow remote
code execution.

The tests and required qlpack.yml in src/experimental and test/experimental are also included.

pwntester · 2020-05-05T09:19:58Z

+
+  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");


should we have a sink for user-controlled URLs?

Makes sense. I'll add it.

pwntester · 2020-05-05T09:28:03Z

+
+/** The class `javax.naming.CompositeName`. */
+class TypeCompositeName extends Class {
+  TypeCompositeName() { this.hasQualifiedName("javax.naming", "CompositeName") }


Should account for javax.naming.Name interface instead? Not sure if CompositeName is the only one that can hold a JNDI inj payload or if it can be hold in a Name object at all (maybe only String params to lookup are valid for injection purposes)

Name interface has 3 implementations in JDK: CompositeName, CompoundName and LdapName. CompositeName and CompoundName can hold JNDI payload:

ctx.lookup(new CompositeName("rmi:\\/\\/127.0.0.1:12345/abc")); ctx.lookup(new CompoundName("rmi://127.0.0.1:12345/abc", new Properties()));

LdapName, most likely, can't.

I'll add support for CompoundName in the query, but if you think that handling all javax.naming.Name implementations is better, I can change it.

If LdapName cant, I think its enough with adding CompoundName, thanks!

pwntester · 2020-05-05T09:30:08Z

+ * method from Spring's `LdapTemplate`.
+ */
+predicate springLdapTemplateSinkMethod(Method m, int index) {
+  m.getDeclaringType() instanceof TypeSpringLdapTemplate and


Cant find TypeSpringLdapTempalte definition. Make sure it also accounts for LdapOperations

In older versions of the API, these clases were in org.springframework.ldap namespace (without core), see https://docs.spring.io/spring-ldap/docs/1.1.2/api/

TypeSpringLdapTemplate is defined in java/ql/src/semmle/code/java/frameworks/SpringLdap.qll:

class TypeSpringLdapTemplate extends Class { TypeSpringLdapTemplate() { this.hasQualifiedName("org.springframework.ldap.core", "LdapTemplate") } }

But it makes sense to account for any implementation of LdapOperations interface - I'll change it. I'll also add support for org.springframework.ldap package (without core).

pwntester · 2020-05-05T09:41:08Z

+ */
+predicate springLdapTemplateSinkMethod(Method m, int index) {
+  m.getDeclaringType() instanceof TypeSpringLdapTemplate and
+  (m.hasName("lookup") or m.hasName("lookupContext")) and


Please review if other methods may end up calling lookup operations. For example:

https://github.com/spring-projects/spring-ldap/blob/master/core/src/main/java/org/springframework/ldap/core/LdapTemplate.java#L1695

https://github.com/spring-projects/spring-ldap/blob/master/core/src/main/java/org/springframework/ldap/core/LdapTemplate.java#L718

Also the searchForObject methods (using RETURN_OBJ_FLAG) are vulnerable as well

Thanks for pointing this out. I'll add findByDn, rename, list, listBindings, unbind (if recursive is true).

It looks like all search and searchForObject methods where the last parameter is ContextMapper and there's no SearchControls parameter are vulnerable as well (in all these cases, search with RETURN_OBJ_FLAG is called eventually). I'll add them all.

ggolawski · 2020-05-07T21:23:37Z

@pwntester, I updated the query according to your comments (I hope that I didn't miss anything).
Please let me know if it's OK now. You can check the test file (java/ql/test/experimental/query-tests/security/CWE-074/JndiInjection.java) to see all the supported cases (methods with Bad in method name).

…table

aschackmull

Overall the QL code looks solid, but I've left a few inline comments.

….qll Co-authored-by: Anders Schack-Mulligen <aschackmull@users.noreply.github.com>

ggolawski · 2020-05-18T21:44:20Z

@aschackmull Thanks for reviewing the query. I've updated it according to your comments. Please check if everything is fine now.

CodeQL query to detect JNDI injections

af48bc3

ggolawski requested review from a team and felicitymay as code owners April 17, 2020 19:53

ggolawski mentioned this pull request Apr 17, 2020

CodeQL query to detect JNDI injections github/securitylab#66

Closed

1 task

RasmusWL added the Java label Apr 20, 2020

Remove qlpack.yml as these are not needed

0c75330

pwntester suggested changes May 1, 2020

View reviewed changes

Comment thread java/ql/src/experimental/Security/CWE/CWE-074/JndiInjectionLib.qll

Comment thread java/ql/src/experimental/Security/CWE/CWE-074/JndiInjectionLib.qll

Add Spring LDAP and JMXServiceURL related sinks

f893954

pwntester suggested changes May 5, 2020

View reviewed changes

Update according to the review comments

df9921f

ggolawski requested a review from a team as a code owner May 7, 2020 21:19

Fix the case where user-controlled input is passed as URL to env Hash…

afea933

…table

intrigus-lgtm reviewed May 8, 2020

View reviewed changes

Comment thread java/ql/src/experimental/Security/CWE/CWE-074/JndiInjectionLib.qll Outdated

Fix typos

a16295e

aschackmull requested changes May 18, 2020

View reviewed changes

ggolawski and others added 2 commits May 18, 2020 23:18

Update java/ql/src/experimental/Security/CWE/CWE-074/JndiInjectionLib…

0075d35

….qll Co-authored-by: Anders Schack-Mulligen <aschackmull@users.noreply.github.com>

Enhanced comments according to the review comment

73e736b

aschackmull approved these changes May 19, 2020

View reviewed changes

aschackmull merged commit c36e621 into github:master May 19, 2020

ggolawski deleted the jndi-injection branch June 3, 2020 18:06

atorralba mentioned this pull request May 20, 2021

Java: Promote JNDI Injection query from experimental #5931

Merged

Conversation

ggolawski commented Apr 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggolawski commented May 7, 2020

Uh oh!

Uh oh!

aschackmull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ggolawski commented May 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ggolawski commented Apr 17, 2020 •

edited

Loading