From e87fbf513609885b62510df3452204c792538cdb Mon Sep 17 00:00:00 2001 From: Anwesh Tuladhar Date: Thu, 19 Sep 2024 03:45:12 -0700 Subject: [PATCH] Update comments + clean up for the exploitability origin changes Summary: As titiled, updates code and test comments, removes unused methods that was missed. Reviewed By: arthaud Differential Revision: D62978375 fbshipit-source-id: ce16b20db39b4112ba2c4f109b3a6bed66b2a863 --- source/Origin.cpp | 4 - source/Origin.h | 16 ++-- .../ExploitabilityRule.java | 3 - .../exploitability_rule/expected_output.json | 88 +++++++++---------- 4 files changed, 52 insertions(+), 59 deletions(-) diff --git a/source/Origin.cpp b/source/Origin.cpp index 1b410c1b..4aa91a64 100644 --- a/source/Origin.cpp +++ b/source/Origin.cpp @@ -118,10 +118,6 @@ std::optional ExploitabilityOrigin::to_model_validator_string() return std::nullopt; } -std::string ExploitabilityOrigin::issue_handle_callee() const { - return fmt::format("{}:{}", exploitability_root_->show(), callee_->str()); -} - Json::Value ExploitabilityOrigin::to_json() const { auto value = Json::Value(Json::objectValue); value["exploitability_root"] = exploitability_root_->to_json(); diff --git a/source/Origin.h b/source/Origin.h index 08d9c250..16067787 100644 --- a/source/Origin.h +++ b/source/Origin.h @@ -148,8 +148,14 @@ class StringOrigin final : public Origin { * Similar to how method/field/string origins are first added when creating * user-declared models (see Taint::add_origins_if_declaration()), * exploitability-origins are added when we first infer the source-as-transform - * sinks. It tracks the (caller method + sink callee) pair where - * the source-as-transform sink was materialized/originated. + * sinks. It tracks the (caller method + sink callee + position) tuple where + * the source-as-transform sink was materialized/originated. This helps us track + * the information related to the original source/sink flow and is used in the + * computation of issue grouping (and hence issue handles) when the + * exploitability rule is fulfilled. We create one issue per + * exploitability-origin and use it as the "callee" of the Issue, maintaining + * the issue grouping from the original source/sink flow. See + * exploitability_rules integration test for more information. * * From the SAPP UI perspective: Similar to how method/field/string origins * indicates the leaf frames of the source/sink traces, exploitability-origins @@ -183,12 +189,6 @@ class ExploitabilityOrigin final : public Origin { return callee_; } - const Position* position() const { - return position_; - } - - std::string issue_handle_callee() const; - private: const Method* exploitability_root_; const DexString* callee_; diff --git a/source/tests/integration/end-to-end/code/exploitability_rule/ExploitabilityRule.java b/source/tests/integration/end-to-end/code/exploitability_rule/ExploitabilityRule.java index 8b86bec1..7a6cdd41 100644 --- a/source/tests/integration/end-to-end/code/exploitability_rule/ExploitabilityRule.java +++ b/source/tests/integration/end-to-end/code/exploitability_rule/ExploitabilityRule.java @@ -65,9 +65,6 @@ protected void testMultiplePathsToExploitabilityRoot() { // This method is Exported _and_ has call-chain flows to the same exploitability-root via // multiple paths. // Expect: a single issue here. - // TODO: T201904683: Currently we find multiple issues but create the same issue handle. This is - // because we use the position of the callsite in this method in IssueSet, but that is ignored - // in the sapp parser when creating the handle. // Path 1: Direct path to issue. ExploitabilityRule.testExported(this); diff --git a/source/tests/integration/end-to-end/code/exploitability_rule/expected_output.json b/source/tests/integration/end-to-end/code/exploitability_rule/expected_output.json index f164c89f..4a37cd5e 100644 --- a/source/tests/integration/end-to-end/code/exploitability_rule/expected_output.json +++ b/source/tests/integration/end-to-end/code/exploitability_rule/expected_output.json @@ -992,7 +992,7 @@ "method" : "Lcom/facebook/marianatrench/integrationtests/ExploitabilityRule;.:()V", "position" : { - "line" : 94, + "line" : 91, "path" : "ExploitabilityRule.java" } } @@ -1000,7 +1000,7 @@ "method" : "Lcom/facebook/marianatrench/integrationtests/ExploitabilityRule;.dfaSink:(Ljava/lang/Object;)V", "position" : { - "line" : 95, + "line" : 92, "path" : "ExploitabilityRule.java" }, "sinks" : @@ -1036,7 +1036,7 @@ "method" : "Lcom/facebook/marianatrench/integrationtests/ExploitabilityRule;.hopToSink:(Ljava/lang/Object;)V", "position" : { - "line" : 98, + "line" : 95, "path" : "ExploitabilityRule.java" }, "sinks" : @@ -1052,7 +1052,7 @@ "port" : "Argument(0)", "position" : { - "line" : 99, + "line" : 96, "path" : "ExploitabilityRule.java" } }, @@ -1094,7 +1094,7 @@ "port" : "Argument(0)", "position" : { - "line" : 105, + "line" : 102, "path" : "ExploitabilityRule.java" } }, @@ -1115,7 +1115,7 @@ "port" : "Return", "position" : { - "line" : 105, + "line" : 102, "path" : "ExploitabilityRule.java" } }, @@ -1139,7 +1139,7 @@ "exploitability_root" : "Lcom/facebook/marianatrench/integrationtests/ExploitabilityRule;.testExported:(Landroid/app/Activity;)V", "position" : { - "line" : 105, + "line" : 102, "path" : "ExploitabilityRule.java" } } @@ -1161,7 +1161,7 @@ "method" : "Lcom/facebook/marianatrench/integrationtests/ExploitabilityRule;.testExported:(Landroid/app/Activity;)V", "position" : { - "line" : 104, + "line" : 101, "path" : "ExploitabilityRule.java" } } @@ -1179,7 +1179,7 @@ "port" : "Argument(0)", "position" : { - "line" : 117, + "line" : 114, "path" : "ExploitabilityRule.java" }, "resolves_to" : "Lcom/facebook/marianatrench/integrationtests/ExploitabilityRule;.hopToSink:(Ljava/lang/Object;)V" @@ -1202,7 +1202,7 @@ "port" : "Return", "position" : { - "line" : 117, + "line" : 114, "path" : "ExploitabilityRule.java" }, "resolves_to" : "Lcom/facebook/marianatrench/integrationtests/ExportedActivity;.getIntentHop1:()Landroid/content/Intent;" @@ -1227,7 +1227,7 @@ "exploitability_root" : "Lcom/facebook/marianatrench/integrationtests/ExploitabilityRule;.testMultihopExported:(Lcom/facebook/marianatrench/integrationtests/ExportedActivity;)V", "position" : { - "line" : 117, + "line" : 114, "path" : "ExploitabilityRule.java" } } @@ -1249,7 +1249,7 @@ "method" : "Lcom/facebook/marianatrench/integrationtests/ExploitabilityRule;.testMultihopExported:(Lcom/facebook/marianatrench/integrationtests/ExportedActivity;)V", "position" : { - "line" : 116, + "line" : 113, "path" : "ExploitabilityRule.java" } } @@ -1267,7 +1267,7 @@ "port" : "Argument(0)", "position" : { - "line" : 131, + "line" : 128, "path" : "ExploitabilityRule.java" } }, @@ -1288,7 +1288,7 @@ "port" : "Return", "position" : { - "line" : 131, + "line" : 128, "path" : "ExploitabilityRule.java" } }, @@ -1312,7 +1312,7 @@ "exploitability_root" : "Lcom/facebook/marianatrench/integrationtests/ExploitabilityRule;.testMultipleEffectSourcesDifferentDistance:(Landroid/app/Activity;)V", "position" : { - "line" : 131, + "line" : 128, "path" : "ExploitabilityRule.java" } } @@ -1334,7 +1334,7 @@ "method" : "Lcom/facebook/marianatrench/integrationtests/ExploitabilityRule;.testMultipleEffectSourcesDifferentDistance:(Landroid/app/Activity;)V", "position" : { - "line" : 130, + "line" : 127, "path" : "ExploitabilityRule.java" } } @@ -1352,7 +1352,7 @@ "port" : "Argument(0)", "position" : { - "line" : 124, + "line" : 121, "path" : "ExploitabilityRule.java" } }, @@ -1373,7 +1373,7 @@ "port" : "Return", "position" : { - "line" : 124, + "line" : 121, "path" : "ExploitabilityRule.java" } }, @@ -1397,7 +1397,7 @@ "exploitability_root" : "Lcom/facebook/marianatrench/integrationtests/ExploitabilityRule;.testMultipleEffectSourcesSameDistance:(Landroid/app/Activity;)V", "position" : { - "line" : 124, + "line" : 121, "path" : "ExploitabilityRule.java" } } @@ -1419,7 +1419,7 @@ "method" : "Lcom/facebook/marianatrench/integrationtests/ExploitabilityRule;.testMultipleEffectSourcesSameDistance:(Landroid/app/Activity;)V", "position" : { - "line" : 123, + "line" : 120, "path" : "ExploitabilityRule.java" } } @@ -1437,7 +1437,7 @@ "port" : "Argument(0)", "position" : { - "line" : 111, + "line" : 108, "path" : "ExploitabilityRule.java" } }, @@ -1458,7 +1458,7 @@ "port" : "Return", "position" : { - "line" : 111, + "line" : 108, "path" : "ExploitabilityRule.java" } }, @@ -1482,7 +1482,7 @@ "exploitability_root" : "Lcom/facebook/marianatrench/integrationtests/ExploitabilityRule;.testUnexported:(Landroid/app/Activity;)V", "position" : { - "line" : 111, + "line" : 108, "path" : "ExploitabilityRule.java" } } @@ -1504,7 +1504,7 @@ "method" : "Lcom/facebook/marianatrench/integrationtests/ExploitabilityRule;.testUnexported:(Landroid/app/Activity;)V", "position" : { - "line" : 110, + "line" : 107, "path" : "ExploitabilityRule.java" } } @@ -1883,7 +1883,7 @@ "exploitability_root" : "Lcom/facebook/marianatrench/integrationtests/ExploitabilityRule;.testExported:(Landroid/app/Activity;)V", "position" : { - "line" : 105, + "line" : 102, "path" : "ExploitabilityRule.java" } } @@ -1995,7 +1995,7 @@ "exploitability_root" : "Lcom/facebook/marianatrench/integrationtests/ExploitabilityRule;.testExported:(Landroid/app/Activity;)V", "position" : { - "line" : 105, + "line" : 102, "path" : "ExploitabilityRule.java" } }, @@ -2154,7 +2154,7 @@ "exploitability_root" : "Lcom/facebook/marianatrench/integrationtests/ExploitabilityRule;.testMultipleEffectSourcesSameDistance:(Landroid/app/Activity;)V", "position" : { - "line" : 124, + "line" : 121, "path" : "ExploitabilityRule.java" } }, @@ -2314,7 +2314,7 @@ "exploitability_root" : "Lcom/facebook/marianatrench/integrationtests/ExploitabilityRule;.testMultipleEffectSourcesDifferentDistance:(Landroid/app/Activity;)V", "position" : { - "line" : 131, + "line" : 128, "path" : "ExploitabilityRule.java" } }, @@ -2463,7 +2463,7 @@ "exploitability_root" : "Lcom/facebook/marianatrench/integrationtests/ExploitabilityRule;.testMultihopExported:(Lcom/facebook/marianatrench/integrationtests/ExportedActivity;)V", "position" : { - "line" : 117, + "line" : 114, "path" : "ExploitabilityRule.java" } }, @@ -2766,13 +2766,13 @@ "exploitability_root" : "Lcom/facebook/marianatrench/integrationtests/ExploitabilityRule;.testExported:(Landroid/app/Activity;)V", "position" : { - "line" : 105, + "line" : 102, "path" : "ExploitabilityRule.java" } }, "position" : { - "line" : 72, + "line" : 69, "path" : "ExploitabilityRule.java" }, "rule" : 1, @@ -2787,7 +2787,7 @@ "position" : { "end" : 13, - "line" : 76, + "line" : 73, "path" : "ExploitabilityRule.java", "start" : 4 }, @@ -2837,7 +2837,7 @@ "position" : { "end" : 34, - "line" : 73, + "line" : 70, "path" : "ExploitabilityRule.java", "start" : 23 }, @@ -2889,7 +2889,7 @@ "position" : { "end" : 0, - "line" : 72, + "line" : 69, "path" : "ExploitabilityRule.java", "start" : 0 } @@ -2918,7 +2918,7 @@ ], "position" : { - "line" : 72, + "line" : 69, "path" : "ExploitabilityRule.java" } } @@ -3037,7 +3037,7 @@ "method" : "Lcom/facebook/marianatrench/integrationtests/UnexportedActivity;.:()V", "position" : { - "line" : 79, + "line" : 76, "path" : "ExploitabilityRule.java" } } @@ -3088,7 +3088,7 @@ "exploitability_root" : "Lcom/facebook/marianatrench/integrationtests/ExploitabilityRule;.testUnexported:(Landroid/app/Activity;)V", "position" : { - "line" : 111, + "line" : 108, "path" : "ExploitabilityRule.java" } } @@ -3226,7 +3226,7 @@ "port" : "call-chain-exploitability", "position" : { - "line" : 86, + "line" : 83, "path" : "ExploitabilityRule.java" }, "resolves_to" : "Lcom/facebook/marianatrench/integrationtests/ExploitabilityRule;.testUnexported:(Landroid/app/Activity;)V" @@ -3260,7 +3260,7 @@ "exploitability_root" : "Lcom/facebook/marianatrench/integrationtests/ExploitabilityRule;.testUnexported:(Landroid/app/Activity;)V", "position" : { - "line" : 111, + "line" : 108, "path" : "ExploitabilityRule.java" } } @@ -3285,7 +3285,7 @@ "port" : "Return", "position" : { - "line" : 85, + "line" : 82, "path" : "ExploitabilityRule.java" } }, @@ -3311,7 +3311,7 @@ "local_positions" : [ { - "line" : 85 + "line" : 82 } ] } @@ -3321,7 +3321,7 @@ "method" : "Lcom/facebook/marianatrench/integrationtests/UnexportedActivity;.onCreate:()V", "position" : { - "line" : 84, + "line" : 81, "path" : "ExploitabilityRule.java" } } @@ -3329,7 +3329,7 @@ "method" : "Lcom/facebook/marianatrench/integrationtests/UnexportedActivity;.onStart:()V", "position" : { - "line" : 90, + "line" : 87, "path" : "ExploitabilityRule.java" }, "sinks" : @@ -3345,7 +3345,7 @@ "port" : "Argument(0)", "position" : { - "line" : 91, + "line" : 88, "path" : "ExploitabilityRule.java" } },