codeql icon indicating copy to clipboard operation
codeql copied to clipboard

ATM: undo unsound performance optimizations

Open esbena opened this issue 2 years ago • 2 comments

For an ordinary path-problem query, it is a requirement that at least one sink exists, otherwise there is nothing to alert on. Thus the optimization with checking isSink(_, this, _) pruning in Configuration::hasFlow is sound.

For programs without any relevant sinks (which is likely to be the case when ATM is used), the Configuration::hasFlow calls will not hold due to the above pruning step. The optimizations removed in this commit are thus unsound for programs without any relevant sinks.

Relevant pings: @henrymercer @adityasharad

Hopefully performance works out without this 🤞🏻🤞🏻.

Impact

This means that it now is possible to predict sinks in programs without any known sinks(!!!).

Testing

Testing this is presumably tricky at the moment due to the required presence of a mlmodel. So I include my own manual test case for completeness and posterity.

Example database sources

// danger_sink_call.js
danger(sink)
// flow_to_predicted_sink.ts
// carefully selected file content that presently predicts `req.params.id` to be a sink
import { Request } from "express";

module.exports = function retrieveBasket() {
  return (req: Request) => {
    models.Basket.findOne({
      where: { id: req.params.id }
    });
  };
};

Example query that produces no/some results without/with this PR.

import DataFlow::PathGraph
import experimental.adaptivethreatmodeling.TaintedPathATM
import experimental.adaptivethreatmodeling.EndpointFeatures
import experimental.adaptivethreatmodeling.EndpointScoring

from DataFlow::Node scored, EndpointType type, string basename, float scoreNum, string description
where
  description = type.getDescription() and
  ModelScoring::endpointScores(scored, type.getEncoding(), scoreNum) and
  scored.getFile().getBaseName() = basename and
  basename = ["danger_sink_call.js", "flow_to_predicted_sink.ts"]
select scored, basename, description, scoreNum

Confirming that hasFlow(...) works for the full ATM query without any known sinks

Run TaintedPathATM.ql.

esbena avatar Mar 16 '22 21:03 esbena