Answers & Feedback - GitHub Security Lab CTF 3

This page outlines general feedback put together by reviewers while assessing the submissions for the CTF that were entered before the contest period.

It also includes a model answer for question 3.1.

Step 1.0

Feedback

Some submissions identified assignments to properties of $.fn by using toString and regexpMatch on the LHS:

from Assignment assignment, ASTNode lhs
where // BAD
  assignment.getLhs() = lhs and
  lhs.toString().regexpMatch("^\$\\.fn\\.[a-zA-Z]+$")
select assignment.getRhs()

This is not a recommended approach as toString is strictly used for debugging, and should therefore not be relied on for semantics of a query. Some submissions also noted that some toString calls resulted in strings that were shortened ... in the middle.

A less error-prone way of solving finding such assignments is to do the following:

from VarAccess dollar, PropAccess fn, PropAccess write, Assignment assign
where // GOOD
  dollar.getName() = "$" and
  fn.accesses(dollar, "fn") and
  write.getBase() = fn and
  assign.getLhs() = write
select assign.getRhs()

Step 1.1

Feedback

Some submissions used getASuccessor*() for identifying flow from a function declaration to the property assignment:

from DataFlow::FunctionNode plugin, DataFlow::PropWrite write
where write = jquery().getAPropertyRead("fn").getAPropertyWrite(_) and
      write.getRhs() = plugin.getASuccessor*() // BAD
select plugin

While this works, it is generally better to avoid using .getASuccessor* as much as possible, and instead use methods like getAPropertySource instead:

from DataFlow::FunctionNode plugin
where jquery().getAPropertyRead("fn").getAPropertySource() = plugin // GOOD
select plugin

Step 1.3

Feedback

Some submissions identified the plugin options parameter and expected exactly that parameter to be the base of the property read that would correspond to a single option value:

from DataFlow::FunctionNode plugin, DataFlow::ParameterNode options, DataFlow::PropRead prop
where jquery().getAPropertyRead("fn").getAPropertySource() = plugin and
  plugin.getLastParameter() = options and
  prop.getBase() = options // BAD
select prop

This is problematic because the options above corresponds to the declaration of the parameter, and not a read of the parameter. Instead, it should be checked where the parameter flows:

from DataFlow::FunctionNode plugin, DataFlow::ParameterNode options, DataFlow::PropRead prop
where jquery().getAPropertyRead("fn").getAPropertySource() = plugin and
  plugin.getLastParameter() = options and
  options.getAPropertyRead() = prop // GOOD
select prop

Step 3.0

Feedback

Several submissions suggested an exploit like the one below, and some of those submissions observed that the plugin would throw an exception due to malformed CSS before reaching the exploitable call.

$('body').scrollspy({target: '<img src=x onerror=alert(1)>'});

Few submissions worked around this limitation with exploits like:

$("body").scrollspy({target: "<img src=x onerror='alert(1)'>//"});

Step 3.1

Feedback

Most submissions had almost the same answer here, but with some subtle differences. The typical wrong answer here is:

exists(ClassNode c, string n |
  pred = c.getAnInstanceReference().getAPropertyWrite(n) and // wrong
  succ = c.getAnInstanceReference().getAPropertyRead(n)
)

Note that pred in this implementation is the property write itself, and not the right-hand side of the assignment. The correct solution here is:

exists(ClassNode c, string n |
  pred = c.getAnInstanceReference().getAPropertyWrite(n).getRhs() and
  succ = c.getAnInstanceReference().getAPropertyRead(n)
)

A sub-optimal, but more precise, solution is to hard-code the class field name that we want flow through

exists(ClassNode c, string n |
  pred = c.getAnInstanceReference().getAPropertyWrite(n).getRhs() and
  succ = c.getAnInstanceReference().getAPropertyRead(n) and
  name = "options" // sub-optimal
)

Model Answer

/**
 * @name Cross-site scripting vulnerable plugin
 * @kind path-problem
 * @id js/xss-unsafe-plugin
 */

import javascript
import DataFlow::PathGraph

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

  override predicate isSource(DataFlow::Node source) {
    source = jquery()
          .getAPropertyRead("fn")
          .getAPropertySource()
          .(DataFlow::FunctionNode)
          .getLastParameter()
  }

  override predicate isSink(DataFlow::Node sink) {
    exists(JQuery::MethodCall call | call.interpretsArgumentAsHtml(sink))
  }

  override predicate isAdditionalTaintStep(DataFlow::Node src, DataFlow::Node sink) {
    exists(DataFlow::ClassNode cn, string p |
      cn.getAnInstanceReference().getAPropertyWrite(p).getRhs() = src and
      cn.getAnInstanceReference().getAPropertyRead(p) = sink
    )
  }
}

from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
where cfg.hasFlowPath(source, sink)
select sink.getNode(), source, sink, "Potential XSS vulnerability in plugin."

Step 4.1

Feedback

The answers varied a lot for this question. In general, the answers shared some common mistakes:

  • the use of toString for semantic reasoning, as also seen in 1.0.
  • sanitization as part of the Configuration::isSink predicate instead of Configuration::isSanitizerEdge. This meant that sanitization only would be applied at exactly the sink, so indirection through a variable could break the sanitization.
  • identification of HTML-like default values by matching with the exact HTML string in the bootstrap plugins. A better solution is to pattern match on the default value to check if it looks like some kind of HTML, for instance v.matches("%<%>%").
  • hard-wiring of the name "DEFAULTS" for identifying the object with default values. A better solution is to find objects that also flow to an extend call together with the options.

Step 4.2

Feedback

The answers varied a lot for this question. In general, the answers shared some common mistakes:

  • the SanitizerGuardNode::sanitizes implementation had a hard-coded outcome = true or dually a hard-coded outcome = true, meaning that the sanitizer would only work for either != or == checks. While it is slightly more complex, it is possible to have a single sanitizer implementation that supports both kinds of checks correctly.
  • the characteristic predicate of the SanitizerGuardNode only supported either === or ==, or dually !== or ==. Expr::EqualityTest covers all of these cases, and should be used instead of sole checks like this.asExpr() instanceof NEqExpr.

Step 4.3

Feedback

The answers varied a lot for this question. No answers were perfect, and it is even unlikely that a perfect, practical, solution exists. The answers generally fell into two categories:

  • find a property read at the sink report the property name of such a read.
  • find a property read on the path between the source and sink PathNodes and report the property name of such a read.

Both of those solutions generally contained the mistake, that if no property name was found, then no result would be reported. It is highly recommended that a fallback-message without a friendly option name is used in that case.