Skip to content

[Rust] weird behavior in dataflow when trying to select a specific node #19983

Description

@intrigus-lgtm

(Apologies for the vague issue title, but I just didn't know any better title...)

Description of the issue

(I don't think this is related to #19982 but linking anyway)

When running the "working base version" (see below for code), I successfully find flow from the argument data to remaining_len - 4 - tail_len..remaining_len - 4 just as I wanted:

Image

source change

The problem appears when I change the source predicate to this more general version (all results of read_region are assumed to return user-controlled data) where I've manually verified that the result of read_region is used in a call with a data argument:

predicate isSource(DataFlow::Node node) {
    exists(CallExpr ce, Function f | f.getName().getText() = "read_region" |
      node.asExpr().getExpr() = ce and ce.getStaticTarget() = f
    ) and
    node.getLocation().getStartLine() = 727
  }

This results in ZERO alerts/#select/nodes/subpaths/edges.

any() filtering in sink

So obviously, I tried to debug it and for that I changed the sink predicate to this:

  predicate isSink(DataFlow::Node node) {
    any()
  }

(Yes, yes, there is partial flow and stuff, but good old any works pretty well in simple cases :P)
This is exactly the flow I'm trying to find.
Image

RangeExpr filtering in sink

What happens if I change my sink predicate to check whether the node is a RangeExpr using node.asExpr().getExpr() instanceof RangeExpr? That should work, because earlier, sink.getNode().asExpr().getExpr().getAPrimaryQlClass() returned RangeExpr, right?

Well, we get ZERO alerts/.../... again.

getStartLine filterting in sink

Okay, then let's change the predicate to node.getLocation().getStartLine() = 20 because that should also work as we verified that the node of interested has its start line indeed in line 20.

This does give us four results:
but our RangeExpr is nowhere to be found!?

Image

two sections.rs files

There exist two sections.rs files:
cosmwasm-db-cf413c5ad6a58a87e0be894584f01506f3b2e0af^/src.zip/home/sg/dev/cosmwasm/packages/std/src/sections.rs
and
cosmwasm-db-cf413c5ad6a58a87e0be894584f01506f3b2e0af^/src.zip/home/sg/dev/cosmwasm/packages/vm/src/sections.rs
but I don't really see how that could be relevant, but highlighting anyway.

Summary

I'm not sure whether I did something dumb or whether I've run into a bug :)

Reproduction steps

git clone https://github.com/CosmWasm/cosmwasm.git
git checkout cf413c5ad6a58a87e0be894584f01506f3b2e0af^
codeql database create --language=rust "cosmwasm-db-cf413c5ad6a58a87e0be894584f01506f3b2e0af^" --overwrite

"Working base version"

/**
 * @name Empty block
 * @kind path-problem
 * @problem.severity warning
 * @id rust/example/empty-block
 */

import rust
import codeql.rust.dataflow.DataFlow
import codeql.rust.dataflow.TaintTracking

module MyConfig implements DataFlow::ConfigSig {
  predicate isSource(DataFlow::Node node) {
    node.asPat().getPat().(IdentPat).getName().getText() = "data"
  }

  predicate isSink(DataFlow::Node node) {
    exists(IndexExpr ie | node.asExpr().getExpr().(RangeExpr) = ie.getIndex())
  }

  predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ) {
    // If the start/end of a range is tainted, then the whole range is tainted.
    exists(RangeExpr re |
      pred.asExpr().getExpr() = [re.getStart(), re.getEnd()] and
      succ.asExpr().getExpr() = re
    )
    or
    // Propagate taint from `tainted` to the result of `tainted?`
    exists(TryExpr te |
      te.getExpr() = pred.asExpr().getExpr() and
      te = succ.asExpr().getExpr()
    )
    or
    // Propagate taint from `tainted` to `[tainted]`
    exists(ArrayListExpr ale |
      ale.getAnExpr() = pred.asExpr().getExpr() and
      ale = succ.asExpr().getExpr()
    )
    or
    // Propagate taint from `tainted` to `from_be_bytes(tainted)`
    exists(CallExpr ce, Function f |
      pred.asExpr().getExpr() = ce.getArg(0) and
      succ.asExpr().getExpr() = ce and
      ce.getStaticTarget() = f and
      f.getName().getText() = "from_be_bytes"
    )
    or
    // Propagate taint from `tainted` to `tainted as usize`
    exists(CastExpr ce |
      pred.asExpr().getExpr() = ce.getExpr() and
      succ.asExpr().getExpr() = ce and
      pred.getLocation().getFile().getAbsolutePath().matches("%section%")
    )
  }
}

module Flow = TaintTracking::Global<MyConfig>;

import Flow::PathGraph

from Flow::PathNode source, Flow::PathNode sink
where Flow::flowPath(source, sink)
select sink, source, sink,
  "PrimaryQlClass: " + sink.getNode().asExpr().getExpr().getAPrimaryQlClass() + "start: " +
    sink.getLocation().getStartLine()

Files

cosmwasm-db-cf413c5ad6a58a87e0be894584f01506f3b2e0af^.zip

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

questionFurther information is requested

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions