Back to GitHub.com
Home Bounties Research Advisories CodeQL Wall of Fame Get Involved Events

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:

Step 4.2

Feedback

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

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:

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.