Finding the Command Injection flow in Railsgoat

[MaxSchlueter]

Railsgoat is a delibarately vulnerable Ruby on Rails application and has a Command Injection vulnerability: https://github.com/OWASP/railsgoat/wiki/A1-Command-Injection

The built-in CommandInjectionQuery.ql didn't return any results so I set out to write my own query:

/**
 * @kind path-problem
 */

import ruby
import codeql.ruby.frameworks.ActionController
import codeql.ruby.Concepts
import codeql.ruby.dataflow.RemoteFlowSources
import codeql.ruby.TaintTracking

module CommandInjectionConfig implements DataFlow::ConfigSig {
  predicate isSource(DataFlow::Node source) { 
    source instanceof RemoteFlowSource 
   }

  predicate isSink(DataFlow::Node sink) {
    exists(SystemCommandExecution c | c.isShellInterpreted(sink))
  }
}

module CommandInjection = TaintTracking::Global<CommandInjectionConfig>;
import CommandInjection::PathGraph

from CommandInjection::PathNode source, CommandInjection::PathNode sink
where CommandInjection::flowPath(source, sink)
select sink.getNode(), source, sink, "Potential command injection"

which unsurprisingly didn't turn up any results either, so I tried debugging my dataflow query using partial flow:

int explorationLimit() { result = 10 }

module PartialFlow = CommandInjection::FlowExplorationFwd<explorationLimit/0>;

DataFlow::Node getSource() {
  exists(DataFlow::Node src, Ast::AstNode n |
    src instanceof RemoteFlowSource and
    n = src.asExpr().getExpr() and
    n.getFile().getBaseName() = "benefit_forms_controller.rb" and
    n.getLocation().getStartLine() = 19 and 
    result = src
    )
}

predicate adhocPartialFlow(Ast::Callable c, PartialFlow::PartialPathNode n, DataFlow::Node src, int dist) {
  exists(PartialFlow::PartialPathNode source |
    PartialFlow::partialFlow(source, n, dist) and
    src = source.getNode() and
    src = getSource() and
    c = n.getNode().asExpr().getExpr().getEnclosingCallable()
  )
}

which finds the flow to file in the constructed string argument to system with a distance of 3. It feels like this is similar to: #14092 Is it that the file variable is tainted, but the access to file.original_filename is not? I allowed implicit reads at the sink to my dataflow config like this:

   predicate allowImplicitRead(DataFlow::Node n, DataFlow::ContentSet c) {
    isSink(n)
   }

But that didn't find the flow either. How can I debug my query further?

[alexrford]

Hi, I agree with your assessment that the issue is likely a lack of taint flow from file to the result of file.original_filename.

A potential solution would be to add a flow summary stating that, in a call to original_filename, taint propagates from the call receiver to the call return value. For example:

private import codeql.ruby.dataflow.FlowSummary

private class OriginalFilenameSummary extends SimpleSummarizedCallable {
  OriginalFilenameSummary() { this = "original_filename" }

  override predicate propagatesFlow(string input, string output, boolean preservesValue) {
    input = "Argument[self]" and
    output = "ReturnValue" and
    preservesValue = false
  }
}

This summary fills in the gap by giving the dataflow analysis additional information about the functionality of original_filename, namely that it propagates taint flow from the receiver to the return value. This is necessary because we don't have access to the original_filename method in our CodeQL database (as it's part of ActiveSupport rather than railsgoat), so the dataflow analysis cannot automatically infer this fact.

[MaxSchlueter]

Thank you for your explanation! I just got there in the end by adding this to my dataflow config:

  predicate isAdditionalFlowStep(DataFlow::Node fromNode, DataFlow::Node toNode) {
    exists(Ast::MethodCall c | 
      c.getReceiver() = fromNode.asExpr().getExpr() and
      toNode.asExpr().getExpr() = c)
  }

... But your solution seems to be a lot more precise than this.