github / codeql

CodeQL: the libraries and queries that power security researchers around the world, as well as code scanning in GitHub Advanced Security
https://codeql.github.com
MIT License
7.67k stars 1.54k forks source link

Why can't SqlTainted.ql analyze header injection and cookie injection? java #3079

Closed niuzhi closed 4 years ago

niuzhi commented 4 years ago

@aschackmull Thanks. I checked the source and sink。 source: image Sink: image

Why can't I check it out? TaintTracking::Configuration code: image

aschackmull commented 4 years ago

It appears that we're missing a taint step through URLDecoder.decode. Thanks for flagging this. I've added a PR to fix this here: https://github.com/Semmle/ql/pull/3081. In the meantime you can add the following additional step:

override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
  exists(MethodAccess ma, Method m |
    ma.getMethod() = m and m.getDeclaringType().hasQualifiedName("java.net", "URLDecoder") and
    m.getName() = "decode" and
    node1.asExpr() = ma.getArgument(0) and
    node2.asExpr() = ma
  )
}

In general you can debug missing steps by overriding the explorationLimit() predicate on configurations:

/**
   * Gets the exploration limit for `hasPartialFlow` measured in approximate
   * number of interprocedural steps.
   */
  int explorationLimit() { none() }

  /**
   * Holds if there is a partial data flow path from `source` to `node`. The
   * approximate distance between `node` and the closest source is `dist` and
   * is restricted to be less than or equal to `explorationLimit()`. This
   * predicate completely disregards sink definitions.
   *
   * This predicate is intended for dataflow exploration and debugging and may
   * perform poorly if the number of sources is too big and/or the exploration
   * limit is set too high without using barriers.
   *
   * This predicate is disabled (has no results) by default. Override
   * `explorationLimit()` with a suitable number to enable this predicate.
   *
   * To use this in a `path-problem` query, import the module `PartialPathGraph`.
   */
  final predicate hasPartialFlow(PartialPathNode source, PartialPathNode node, int dist) {
    partialFlow(source, node, this) and
    dist = node.getSourceDistance()
  }
}
aschackmull commented 4 years ago

Its use is documented in the qldoc sections I quoted. Basically, starting from a query you're developing with a configuration you'd like to debug, you then add override in explorationLimit() { result = 5 } (or some other suitable number, depending on your use case). With that in place you can use hasPartialFlow instead of hasFlow to see all the places your sources reach.

niuzhi commented 4 years ago

@aschackmull thanks. I use explorationLimit function,why dist is zero? can you help me,thanks very much image

aschackmull commented 4 years ago

Qldoc for explorationLimit says

Gets the exploration limit for hasPartialFlow measured in approximate number of interprocedural steps.

and qldoc for hasPartialFlow says

The approximate distance between node and the closest source is dist and is restricted to be less than or equal to explorationLimit().

I guess it could be a bit clearer, but as it says, dist is the distance to a source measured in approximate number of interprocedural steps, i.e. if everything is in the same method the dist is 0.

niuzhi commented 4 years ago

@aschackmull thanks 1.TaintTrackingUtil.qll not existsorg.apache.commons.codec.binary.Base64 method taint step,please add。 image

aschackmull commented 4 years ago

At a guess we might be missing the queryForLong method call as an sql sink. Checking with a simple query to see whether you can find the expected sinks without requiring flow is a good way to debug something like this:

from QueryInjectionSink sink
select sink
niuzhi commented 4 years ago

@aschackmull I have solved it, thanks