Skip to content

Commit

Permalink
Class intervals: Intersect intervals when creating issues
Browse files Browse the repository at this point in the history 
Summary:
When creating an issue, there should be a non-empty intersection of class intervals between source frames and sink frames. If not, a flow cannot actually occur in practice and the issue would be a false positive.

Intersection between two frames is defined as:
* preserves_type_context = false on either/both frames (i.e. class interval is not applicable) **OR**
* class intervals intersect.

Also, any non-intersecting frames should not be reported as part of the issue.

Reviewed By: arthaud

Differential Revision: D48658545

fbshipit-source-id: 0fb5487500868b5b5a51b547ff3ce29fb8df1101
  • Loading branch information
@facebook-github-bot
Yuh Shin Ong authored and facebook-github-bot committed Aug 30, 2023
1 parent 93b7d5c commit efd010c
Show file tree
Hide file tree
Showing 8 changed files with 243 additions and 101 deletions.
10 changes: 8 additions & 2 deletions source/ForwardTaintTransfer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -508,8 +508,14 @@ void check_sources_sinks_flows(

auto sources_by_kind = sources.partition_by_kind();
auto sinks_by_kind = sinks.partition_by_kind();
for (const auto& [source_kind, source_taint] : sources_by_kind) {
for (const auto& [sink_kind, sink_taint] : sinks_by_kind) {
for (auto& [source_kind, source_taint] : sources_by_kind) {
for (auto& [sink_kind, sink_taint] : sinks_by_kind) {
source_taint.intersect_intervals_with(sink_taint);
sink_taint.intersect_intervals_with(source_taint);
if (source_taint.is_bottom() || sink_taint.is_bottom()) {
// Intervals do not intersect, flow is not possible.
continue;
}
// Check if this satisfies any rule. If so, create the issue.
const auto& rules = context->rules.rules(source_kind, sink_kind);
for (const auto* rule : rules) {
Expand Down
36 changes: 36 additions & 0 deletions source/Taint.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
* LICENSE file in the root directory of this source tree.
*/

#include <unordered_set>

#include <mariana-trench/Constants.h>
#include <mariana-trench/JsonValidation.h>
#include <mariana-trench/Taint.h>
Expand Down Expand Up @@ -265,6 +267,40 @@ std::unordered_map<const Kind*, Taint> Taint::partition_by_kind() const {
return partition_by_kind<const Kind*>([](const Kind* kind) { return kind; });
}

void Taint::intersect_intervals_with(const Taint& other) {
std::unordered_set<CallClassIntervalContext> other_intervals;
for (const auto& other_frame : other.frames_iterator()) {
const auto& other_frame_interval = other_frame.class_interval_context();
// All frames in `this` will intersect with a frame in `other` that does not
// preserve type context.
if (!other_frame_interval.preserves_type_context()) {
return;
}

other_intervals.insert(other_frame_interval);
}

// Keep only frames that intersect with some interval in `other`.
// Frames that do not preserve type context are considered to intersect with
// everything.
filter([&other_intervals](const Frame& frame) {
const auto& frame_interval = frame.class_interval_context();
if (!frame_interval.preserves_type_context()) {
return true;
}

bool intersects_with_some_other_frame = std::any_of(
other_intervals.begin(),
other_intervals.end(),
[&frame_interval](const auto& other_frame_interval) {
return !other_frame_interval.callee_interval()
.meet(frame_interval.callee_interval())
.is_bottom();
});
return intersects_with_some_other_frame;
});
}

FeatureMayAlwaysSet Taint::features_joined() const {
auto features = FeatureMayAlwaysSet::bottom();
for (const auto& callee_frames : set_) {
Expand Down
6 changes: 6 additions & 0 deletions source/Taint.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,12 @@ class Taint final : public sparta::AbstractDomain<Taint> {
return result;
}

/**
* Retain only intervals that intersect with `other`. This happens regardless
* of kind, i.e. intervals will be dropped even if kind is not the same.
*/
void intersect_intervals_with(const Taint& other);

/**
* Returns all features for this taint tree, joined as `FeatureMayAlwaysSet`.
*/
Expand Down
181 changes: 181 additions & 0 deletions source/tests/TaintTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1336,6 +1336,187 @@ TEST_F(TaintTest, PartitionByKindGeneric) {
}));
}

TEST_F(TaintTest, IntersectIntervals) {
auto context = test::make_empty_context();

Scope scope;
auto* method1 =
context.methods->create(redex::create_void_method(scope, "LOne;", "one"));
auto* method2 =
context.methods->create(redex::create_void_method(scope, "LTwo;", "two"));

auto initial_taint = Taint{
test::make_taint_config(
/* kind */ context.kind_factory->get("TestSource"),
test::FrameProperties{
.callee = method1,
.class_interval_context = CallClassIntervalContext(
ClassIntervals::Interval::finite(2, 6),
/* preserves_type_context*/ true),
.call_info = CallInfo::callsite(),
}),
test::make_taint_config(
/* kind */ context.kind_factory->get("TestSource"),
test::FrameProperties{
.callee = method2,
.class_interval_context = CallClassIntervalContext(
ClassIntervals::Interval::finite(4, 5),
/* preserves_type_context*/ false),
.call_info = CallInfo::callsite(),
})};

{
// Frames with preserves_type_context = false are always preserved even if
// they intersect with nothing.
auto taint = initial_taint;
taint.intersect_intervals_with(Taint::bottom());
EXPECT_EQ(
taint,
Taint{test::make_taint_config(
/* kind */ context.kind_factory->get("TestSource"),
test::FrameProperties{
.callee = method2,
.class_interval_context = CallClassIntervalContext(
ClassIntervals::Interval::finite(4, 5),
/* preserves_type_context*/ false),
.call_info = CallInfo::callsite(),
})});
}

{
// Drop frames with preserves_type_context=true that don't intersect with
// anything.
auto taint = initial_taint;
taint.intersect_intervals_with(Taint{test::make_taint_config(
/* kind */ context.kind_factory->get("TestSource"),
test::FrameProperties{
.callee = method2,
.class_interval_context = CallClassIntervalContext(
ClassIntervals::Interval::finite(10, 11),
/* preserves_type_context*/ true),
.call_info = CallInfo::callsite(),
})});
EXPECT_EQ(
taint,
Taint{test::make_taint_config(
/* kind */ context.kind_factory->get("TestSource"),
test::FrameProperties{
.callee = method2,
.class_interval_context = CallClassIntervalContext(
ClassIntervals::Interval::finite(4, 5),
/* preserves_type_context*/ false),
.call_info = CallInfo::callsite(),
})});
}

{
// Taint is never "expanded" as a result of the intersection.
auto taint = Taint::bottom();
taint.intersect_intervals_with(initial_taint);
EXPECT_EQ(taint, Taint::bottom());
}

{
// Taint intersecting with itself gives itself
auto taint = initial_taint;
taint.intersect_intervals_with(initial_taint);
EXPECT_EQ(taint, initial_taint);
}

{
// Produce bottom() if nothing intersects
auto taint = Taint{test::make_taint_config(
/* kind */ context.kind_factory->get("TestSource"),
test::FrameProperties{
.callee = method2,
.class_interval_context = CallClassIntervalContext(
ClassIntervals::Interval::finite(4, 5),
/* preserves_type_context*/ true),
.call_info = CallInfo::callsite(),
})};
taint.intersect_intervals_with(Taint{test::make_taint_config(
/* kind */ context.kind_factory->get("TestSource"),
test::FrameProperties{
.callee = method2,
.class_interval_context = CallClassIntervalContext(
ClassIntervals::Interval::finite(6, 7),
/* preserves_type_context*/ true),
.call_info = CallInfo::callsite(),
})});
EXPECT_TRUE(taint.is_bottom());
}

{
// If a frame is kept during intersection, all properties are kept the same
auto taint = initial_taint;
taint.intersect_intervals_with(Taint{test::make_taint_config(
/* kind */ context.kind_factory->get("TestSource"),
test::FrameProperties{
.callee = method2, // method1 in initial_taint
.class_interval_context = CallClassIntervalContext(
ClassIntervals::Interval::finite(
2, 3), // (2, 6) in initial_taint
/* preserves_type_context*/ true),
.call_info = CallInfo::callsite(),
})});
EXPECT_EQ(taint, initial_taint);
}

{
// Taint is preserved as long as it intersects with least one frame
auto taint = initial_taint;
taint.intersect_intervals_with(Taint{
test::make_taint_config(
/* kind */ context.kind_factory->get("TestSource"),
test::FrameProperties{
.callee = method1,
.class_interval_context = CallClassIntervalContext(
ClassIntervals::Interval::finite(2, 3),
/* preserves_type_context*/ true),
.call_info = CallInfo::callsite(),
}),
test::make_taint_config(
/* kind */ context.kind_factory->get("TestSource"),
test::FrameProperties{
.callee = method1,
.class_interval_context = CallClassIntervalContext(
ClassIntervals::Interval::finite(10, 11),
/* preserves_type_context*/ true),
.call_info = CallInfo::callsite(),
}),
});
EXPECT_EQ(taint, initial_taint);
}

{
// If other has at least one frame with preserves_type_context=false, all
// frames are retained (because everything intersects with it).
auto taint = initial_taint;
taint.intersect_intervals_with(Taint{
test::make_taint_config(
/* kind */ context.kind_factory->get("TestSource"),
test::FrameProperties{
.callee = method2,
.class_interval_context = CallClassIntervalContext(
ClassIntervals::Interval::top(),
/* preserves_type_context*/ false),
.call_info = CallInfo::callsite(),
}),
test::make_taint_config(
/* kind */ context.kind_factory->get("TestSource"),
test::FrameProperties{
.callee = method1,
.class_interval_context = CallClassIntervalContext(
ClassIntervals::Interval::finite(
10, 11), // intersects with nothing in initial_taint
/* preserves_type_context*/ true),
.call_info = CallInfo::callsite(),
}),
});
EXPECT_EQ(taint, initial_taint);
}
}

TEST_F(TaintTest, FeaturesJoined) {
auto context = test::make_empty_context();

Expand Down
4 changes: 2 additions & 2 deletions source/tests/integration/end-to-end/code/class_intervals/Basic.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@ abstract class Base {

abstract void derived1Sink(Object argument);

void falsePositive() {
void noIssue() {
// Derived1's source cannot flow into Derived2's sink since they are
// unrelated objects in the hierarchy. Class intervals are necessary
// to address this false positive.
// to address this or we will get a false positive.
Object o = this.potentialSource();
this.potentialSink(o);
}
Expand Down
2 changes: 1 addition & 1 deletion source/tests/integration/end-to-end/code/class_intervals/expected_call_graph.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@
"Ljava/lang/Object;.<init>:()V"
]
},
"Lcom/facebook/marianatrench/integrationtests/Basic$Base;.falsePositive:()V" :
"Lcom/facebook/marianatrench/integrationtests/Basic$Base;.noIssue:()V" :
{
"virtual" :
[
Expand Down
12 changes: 6 additions & 6 deletions source/tests/integration/end-to-end/code/class_intervals/expected_dependencies.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,11 @@
],
"Lcom/facebook/marianatrench/integrationtests/Basic$Base;.potentialSink:(Ljava/lang/Object;)V" :
[
"Lcom/facebook/marianatrench/integrationtests/Basic$Base;.falsePositive:()V"
"Lcom/facebook/marianatrench/integrationtests/Basic$Base;.noIssue:()V"
],
"Lcom/facebook/marianatrench/integrationtests/Basic$Base;.potentialSource:()Ljava/lang/Object;" :
[
"Lcom/facebook/marianatrench/integrationtests/Basic$Base;.falsePositive:()V"
"Lcom/facebook/marianatrench/integrationtests/Basic$Base;.noIssue:()V"
],
"Lcom/facebook/marianatrench/integrationtests/Basic$Derived1;.derived1Sink:(Ljava/lang/Object;)V" :
[
Expand All @@ -62,11 +62,11 @@
],
"Lcom/facebook/marianatrench/integrationtests/Basic$Derived1;.potentialSink:(Ljava/lang/Object;)V" :
[
"Lcom/facebook/marianatrench/integrationtests/Basic$Base;.falsePositive:()V"
"Lcom/facebook/marianatrench/integrationtests/Basic$Base;.noIssue:()V"
],
"Lcom/facebook/marianatrench/integrationtests/Basic$Derived1;.potentialSource:()Ljava/lang/Object;" :
[
"Lcom/facebook/marianatrench/integrationtests/Basic$Base;.falsePositive:()V"
"Lcom/facebook/marianatrench/integrationtests/Basic$Base;.noIssue:()V"
],
"Lcom/facebook/marianatrench/integrationtests/Basic$Derived2;.derived1Sink:(Ljava/lang/Object;)V" :
[
Expand All @@ -78,11 +78,11 @@
],
"Lcom/facebook/marianatrench/integrationtests/Basic$Derived2;.potentialSink:(Ljava/lang/Object;)V" :
[
"Lcom/facebook/marianatrench/integrationtests/Basic$Base;.falsePositive:()V"
"Lcom/facebook/marianatrench/integrationtests/Basic$Base;.noIssue:()V"
],
"Lcom/facebook/marianatrench/integrationtests/Basic$Derived2;.potentialSource:()Ljava/lang/Object;" :
[
"Lcom/facebook/marianatrench/integrationtests/Basic$Base;.falsePositive:()V"
"Lcom/facebook/marianatrench/integrationtests/Basic$Base;.noIssue:()V"
],
"Lcom/facebook/marianatrench/integrationtests/ClassIntervalDetails$Base;.<init>:(Lcom/facebook/marianatrench/integrationtests/ClassIntervalDetails;)V" :
[
Expand Down
Loading

0 comments on commit efd010c

Please sign in to comment.