Add jOOQ methods as SQL Injection Sinks

[JLLeitschuh]

jOOQ is a Java SQL framework that dynamically generates your SQL queries from your database schema. It is designed to be completely type safe and, through this type safe nature, prevent SQL injection. However, there are methods annotated with org.jooq.PlainSQL that are intentionally unsafe and can still enable SQL injection.

https://www.jooq.org/doc/current/manual/sql-building/plain-sql/

[JLLeitschuh]

Related question that made this query easy and as simple as it is.
github/securitylab#139

[JLLeitschuh]

@aschackmull is this bountyable? If not, it's fine, but if it is, let me know and I'll submit an issue.

[aschackmull]

LGTM, but has acquired a conflict from the merge of #3926. Once this is resolved this looks good to merge.

[aschackmull]

@aschackmull is this bountyable? If not, it's fine, but if it is, let me know and I'll submit an issue.

I don't actually know. That's the domain of the security lab people - I'll pass on the question.

[anticomputer]

@aschackmull is this bountyable? If not, it's fine, but if it is, let me know and I'll submit an issue.

Hey, so from the seclab bounty perspective all we consider is that the query finds real world vulnerable code, the fact that the API enabling the vulnerability is insecure by design does not preclude it from bounty consideration as long as the query has TP results on at least 1 real project.

[JLLeitschuh]

Is there some way that I can test this change in isolation against the CodeQL dataset? I don't see any good way to extract this logic from the CodeQL core in order to run it in isolation. Thoughts?

[JLLeitschuh]

LGTM, but has acquired a conflict from the merge of #3926. Once this is resolved this looks good to merge.

Should be resolved now.

[intrigus-lgtm]

Is there some way that I can test this change in isolation against the CodeQL dataset? I don't see any good way to extract this logic from the CodeQL core in order to run it in isolation. Thoughts?

Shouldn't this be possible now that the classes are importable from queries on the query console?
I.e. add

private class JooqInjectionSink extends QueryInjectionSink {
...

and copy-paste the jOOQSqlMethod-related stuff?

[aschackmull]

The autoformat check failed for jOOQ.qll.

[JLLeitschuh]

What's the keycomo for the formatter? Any plan to automate something that pushed a format fix automatically as a GH action?

[intrigus-lgtm]

What's the keycomo for the formatter? Any plan to automate something that pushed a format fix automatically as a GH action?

Ctrl-Shift-I

[aschackmull]

It's Meta+Shift+F for me. In any case, it's available as "Format Document" in the right-click menu in VSCode. I don't think we have any immediate plans to automate this further for now.

[aschackmull]

@JLLeitschuh You can test the changes on a number of project with a query like this (I've run it on all projects for you): https://lgtm.com/query/2491162600877999399/
There doesn't appear to be any results, so maybe something's missing?

[JLLeitschuh]

@aschackmull thanks! I'm not certain what may be missing. Perhaps there legitimately aren't any projects with this vulnerability? I'll create a POC repository and check. Can you take another pass at my logic and see if you think I'm missing something?

[aschackmull]

Nothing stands out to me, so verifying that it works on a small POC repo sounds like a good plan.

[aschackmull]

Well the sink modelling appears to work - querying for just the sinks does have results: https://lgtm.com/query/3264670151438784148/
But there aren't a lot projects with results, so it's not surprising that the injection query didn't give new results.

[JLLeitschuh]

@aschackmull thanks for following up and checking on this. I just returned to work this week after being in & out a lot this summer trying to sell my mom's house. This was something I kept meaning to get back to but never had time. Thanks for pushing it over the finish line. 🎉

[JLLeitschuh]

Need to try this out sometime:

/**
 * @name Query built without neutralizing special characters
 * @description Building a SQL or Java Persistence query without escaping or otherwise neutralizing any special
 *              characters is vulnerable to insertion of malicious code.
 * @kind problem
 * @problem.severity error
 * @precision high
 * @id java/concatenated-sql-query
 * @tags security
 *       external/cwe/cwe-089
 *       external/cwe/cwe-564
 */

import java
import semmle.code.java.security.SqlUnescapedLib

import semmle.code.java.dataflow.DataFlow
import semmle.code.java.frameworks.jOOQ

/** A sink for database query language injection vulnerabilities. */
abstract class QueryInjectionSink extends DataFlow::Node { }

/** A sink for SQL injection vulnerabilities. */
private class SqlInjectionSink extends QueryInjectionSink {
  SqlInjectionSink() {
    exists(MethodAccess ma, Method m, int index |
      ma.getMethod() = m and
      if index = -1
      then this.asExpr() = ma.getQualifier()
      else ma.getArgument(index) = this.asExpr()
    |
      index = 0 and jOOQSqlMethod(m)
    )
  }
}

class UncontrolledStringBuilderSource extends DataFlow::ExprNode {
  UncontrolledStringBuilderSource() {
    exists(StringBuilderVar sbv |
      uncontrolledStringBuilderQuery(sbv, _) and
      this.getExpr() = sbv.getToStringCall()
    )
  }
}

class UncontrolledStringBuilderSourceFlowConfig extends TaintTracking::Configuration {
  UncontrolledStringBuilderSourceFlowConfig() {
    this = "SqlUnescaped::UncontrolledStringBuilderSourceFlowConfig"
  }

  override predicate isSource(DataFlow::Node src) { src instanceof UncontrolledStringBuilderSource }

  override predicate isSink(DataFlow::Node sink) { sink instanceof QueryInjectionSink }

  override predicate isSanitizer(DataFlow::Node node) {
    node.getType() instanceof PrimitiveType or node.getType() instanceof BoxedType
  }
}

from QueryInjectionSink query, Expr uncontrolled
where
  (
    builtFromUncontrolledConcat(query.asExpr(), uncontrolled)
    or
    exists(StringBuilderVar sbv, UncontrolledStringBuilderSourceFlowConfig conf |
      uncontrolledStringBuilderQuery(sbv, uncontrolled) and
      conf.hasFlow(DataFlow::exprNode(sbv.getToStringCall()), query)
    )
  )
select query, "Query might not neutralize special characters in $@.", uncontrolled,
  "this expression"