Skip to content

Java: Add backward dataflow edges through modelled function invocations #5223

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Mar 5, 2021
Merged

Java: Add backward dataflow edges through modelled function invocations #5223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
40b0f68
Add backward dataflow edges through modelled function invocations.
smowton Mar 4, 2021
da0a7f3
Move existing value-preserving methods to use ValuePreservingCallable
smowton Feb 19, 2021
990bdc2
Move value-preserving callable class into FlowSteps
smowton Mar 5, 2021
45f3365
Apply suggestions from code review
smowton Mar 5, 2021
ca86925
Update java/ql/src/semmle/code/java/dataflow/FlowSteps.qll
smowton Mar 5, 2021
a37b98c
Value-preserving methods: handle generics in DataFlowUtil.qll
smowton Mar 5, 2021
eed357d
ValuePreservingCallable -> ValuePreservingMethod
smowton Mar 5, 2021
012058a
Apply review suggestions: use ArgumentNode.argumentOf, and change mor…
smowton Mar 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions java/ql/src/semmle/code/java/dataflow/FlowSteps.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,57 @@ private module Frameworks {
private import semmle.code.java.frameworks.ApacheHttp
}

/**
* A method that returns the exact value of one of its parameters or the qualifier.
*
* Extend this class and override `returnsValue` to add additional value-preserving steps through a
* method that should be added to the basic local flow step relation.
*
* These steps will be visible for all global data-flow purposes, as well as via
* `DataFlow::Node.getASuccessor` and other related functions exposing intraprocedural dataflow.
*/
abstract class ValuePreservingMethod extends Method {
/**
* Holds if this method returns precisely the value passed into argument `arg`.
* `arg` is a parameter index, or is -1 to indicate the qualifier.
*/
abstract predicate returnsValue(int arg);
}

/**
* A method that returns the exact value of its qualifier (e.g., `return this;`)
*
* Extend this class to add additional value-preserving steps from qualifier to return value through a
* method that should be added to the basic local flow step relation.
*
* These steps will be visible for all global data-flow purposes, as well as via
* `DataFlow::Node.getASuccessor` and other related functions exposing intraprocedural dataflow.
*/
abstract class FluentMethod extends ValuePreservingMethod {
override predicate returnsValue(int arg) { arg = -1 }
}

private class StandardLibraryValuePreservingMethod extends ValuePreservingMethod {
int returnsArgNo;

StandardLibraryValuePreservingMethod() {
this.getDeclaringType().hasQualifiedName("java.util", "Objects") and
(
this.hasName(["requireNonNull", "requireNonNullElseGet"]) and returnsArgNo = 0
or
this.hasName("requireNonNullElse") and returnsArgNo = [0 .. this.getNumberOfParameters() - 1]
or
this.hasName("toString") and returnsArgNo = 1
)
or
this.getDeclaringType().getASourceSupertype*().hasQualifiedName("java.util", "Stack") and
this.hasName("push") and
returnsArgNo = 0
}

override predicate returnsValue(int argNo) { argNo = returnsArgNo }
}

/**
* A unit class for adding additional taint steps.
*
Expand Down
28 changes: 6 additions & 22 deletions java/ql/src/semmle/code/java/dataflow/internal/DataFlowUtil.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ private import semmle.code.java.dataflow.SSA
private import semmle.code.java.dataflow.TypeFlow
private import semmle.code.java.controlflow.Guards
private import semmle.code.java.dataflow.ExternalFlow
private import semmle.code.java.dataflow.FlowSteps
import semmle.code.java.dataflow.InstanceAccess

cached
Expand Down Expand Up @@ -408,28 +409,11 @@ predicate simpleLocalFlowStep(Node node1, Node node2) {
or
summaryStep(node1, node2, "value")
or
exists(MethodAccess ma, Method m |
ma = node2.asExpr() and
m = ma.getMethod() and
m.getDeclaringType().hasQualifiedName("java.util", "Objects") and
(
m.hasName(["requireNonNull", "requireNonNullElseGet"]) and node1.asExpr() = ma.getArgument(0)
or
m.hasName("requireNonNullElse") and node1.asExpr() = ma.getAnArgument()
or
m.hasName("toString") and node1.asExpr() = ma.getArgument(1)
)
)
or
exists(MethodAccess ma, Method m |
ma = node2.asExpr() and
m = ma.getMethod() and
m.getDeclaringType()
.getSourceDeclaration()
.getASourceSupertype*()
.hasQualifiedName("java.util", "Stack") and
m.hasName("push") and
node1.asExpr() = ma.getArgument(0)
exists(MethodAccess ma, ValuePreservingMethod m, int argNo |
ma.getCallee().getSourceDeclaration() = m and m.returnsValue(argNo)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the DIL for this join? It's a triangular join involving an integer argument position and that has been known to have a tendency to go bad. (The DIL for this join should be inspected in a suitably realistic query context - i.e. not through quick-eval).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(It's the order of returnsValue, getCallee + getSourceDeclaration, and argumentOf that matters).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the context of an XSS.ql evaluation:

    {3} r43 = JOIN Expr::Call::getCallee_dispred#ff_10#join_rhs AS L WITH Member::Callable::getSourceDeclaration_dispred#bf AS R ON FIRST 1 OUTPUT L.<1>, 61, R.<1>
    {2} r44 = JOIN r43 WITH exprs ON FIRST 2 OUTPUT r43.<0>, r43.<2>
    {3} r45 = JOIN r44 WITH DataFlowUtil::TExprNode#ff@staged_ext AS R ON FIRST 1 OUTPUT r44.<0>, r44.<1>, R.<1>
    {4} r46 = JOIN r45 WITH DataFlowPrivate::ArgumentNode::argumentOf_dispred#fff_102#join_rhs AS R ON FIRST 1 OUTPUT r45.<1>, R.<2>, r45.<2>, R.<1>
    {2} r47 = JOIN r46 WITH FlowSteps::StandardLibraryValuePreservingMethod#class#ff AS R ON FIRST 2 OUTPUT r46.<3>, r46.<2>

Copy link
Contributor Author

@smowton smowton Mar 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So looks like at present, the fact that all models define returnsValue as -1 means any join on the integer is wholly optimised away?

The join against the class relation is joining on the Method and the argument number concurrently

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the last line is doing the integer join. So this is all good, the integer join is scheduled last, so the pipeline is bounded linearly by arguments.

Copy link
Contributor Author

@smowton smowton Mar 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively,

exists(/* FlowSteps::ValuePreservingMethod */ cached entity m, int argNo |
    exists(/* Expr::MethodAccess */ cached entity ma |
      exists(/* Member::Callable */ cached entity receiver#30 |
        Member::Callable::getSourceDeclaration_dispred#bf(receiver#30, m),
        Expr::Call::getCallee_dispred#ff(ma, receiver#30)
      ),
      exists(cached int arg1, /* @type */ cached dontcare entity _,
             /* @exprparent */ cached dontcare entity _1, cached dontcare int _2 |
        arg1 = 61, exprs(ma, arg1, _, _1, _2)
      ),
      DataFlowUtil::TExprNode(ma, node2),
      DataFlowPrivate::ArgumentNode::argumentOf_dispred#fff(node1, ma, argNo)
    ),
    FlowSteps::StandardLibraryValuePreservingMethod#class#ff(m, argNo)
  )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The member predicate returnsValue is the same as the characteristic predicate FlowSteps::StandardLibraryValuePreservingMethod#class#ff - note that this is binary. This is because the index is a field on the class.

|
node2.asExpr() = ma and
node1.(ArgumentNode).argumentOf(ma, argNo)
)
}

Expand Down
22 changes: 22 additions & 0 deletions java/ql/test/library-tests/dataflow/fluent-methods/Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,16 @@ public Test fluentNoop() {
return this;
}

public Test modelledFluentMethod() {
// A model in the accompanying .ql file will indicate that the qualifier flows to the return value.
return null;
}

public static Test modelledIdentity(Test t) {
// A model in the accompanying .ql file will indicate that the argument flows to the return value.
return null;
}

public Test indirectlyFluentNoop() {
return this.fluentNoop();
}
Expand Down Expand Up @@ -47,4 +57,16 @@ public static void test3() {
sink(t.get()); // $hasTaintFlow=y
}

public static void testModel1() {
Test t = new Test();
t.indirectlyFluentNoop().modelledFluentMethod().fluentSet(source()).fluentNoop();
sink(t.get()); // $hasTaintFlow=y
}

public static void testModel2() {
Test t = new Test();
Test.modelledIdentity(t).indirectlyFluentNoop().modelledFluentMethod().fluentSet(source()).fluentNoop();
sink(t.get()); // $hasTaintFlow=y
}

}
11 changes: 11 additions & 0 deletions java/ql/test/library-tests/dataflow/fluent-methods/flow.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import java
import semmle.code.java.dataflow.DataFlow
import semmle.code.java.dataflow.FlowSteps
import TestUtilities.InlineExpectationsTest

class Conf extends DataFlow::Configuration {
Expand All @@ -14,6 +15,16 @@ class Conf extends DataFlow::Configuration {
}
}

class Model extends FluentMethod {
Model() { this.getName() = "modelledFluentMethod" }
}

class IdentityModel extends ValuePreservingMethod {
IdentityModel() { this.getName() = "modelledIdentity" }

override predicate returnsValue(int arg) { arg = 0 }
}

class HasFlowTest extends InlineExpectationsTest {
HasFlowTest() { this = "HasFlowTest" }

Expand Down