From 7ee871f3ba5b0bd2652e73517ca94d8e716ca70a Mon Sep 17 00:00:00 2001 From: Mark Winterrowd Date: Fri, 7 Oct 2022 15:10:02 -0700 Subject: [PATCH] Slim down the SQL policy verifier and separate it from datalog_policy_verifier. In the early days of Raksha, we created the `datalog_policy_verifier`. This was before we understood that the Souffle language and the Souffle internal architecture meant that we would have to create a separate library per policy interface, and that having one Souffle library to serve all of our needs probably wasn't going to cut it. Others coming to the Raksha project saw the name and, quite reasonably, believed it was the generic Raksha analysis rather than a SQL-verifier-specific analysis. Attempts to use this library as a generic policy library made it bloated and tangled. This commit attempts to move us to a better state by separating out the SQL policy verifier into a separate `sql_policy_verifier_interface` and associated `sql_policy_verifier`. This allows slimming down that library and extracting it from the tangle. The `datalog_policy_verifier` is now not used for any production purpose, and we can clean it up at our leisure. Fixes #747 See #728 PiperOrigin-RevId: 479677692 --- src/analysis/souffle/BUILD | 11 ++++ .../souffle/sql_policy_verifier_interface.dl | 34 ++++++++++++ src/backends/policy_engine/BUILD | 14 +++++ .../catchall_policy_rule_policy.h | 54 +++++++++++++++++++ src/backends/policy_engine/souffle/BUILD | 34 +++++++++++- .../souffle/check_policy_compliance.cc | 6 +-- .../policy_engine/sql_policy_rule_policy.h | 6 +-- src/frontends/sql/BUILD | 2 +- 8 files changed, 153 insertions(+), 8 deletions(-) create mode 100644 src/analysis/souffle/sql_policy_verifier_interface.dl create mode 100644 src/backends/policy_engine/catchall_policy_rule_policy.h diff --git a/src/analysis/souffle/BUILD b/src/analysis/souffle/BUILD index 1f2e7f888..88e2e7960 100644 --- a/src/analysis/souffle/BUILD +++ b/src/analysis/souffle/BUILD @@ -188,6 +188,8 @@ souffle_cc_binary( additional_deps = ["logfunctors"], ) +# TODO(#728): This datalog_policy_verifier is very entangled and is doing too many things. We should +# untangle it and delete it. raksha_policy_verifier_library( name = "datalog_policy_verifier", additional_dl_files = [ @@ -200,6 +202,15 @@ raksha_policy_verifier_library( policy_verifier_interfaces = [":policy_verifier_interface.dl"], ) +# This is a policy verifier focused on the exfiltration of secret columns in the SQL verification +# engine specifically. +raksha_policy_verifier_library( + name = "sql_policy_verifier", + policies = ["//src/backends/policy_engine/souffle/testdata:empty_policy.auth"], + policy_verifier_interfaces = [":sql_policy_verifier_interface.dl"], +) + +# This is a policy verifier focused on the propagation of differential privacy parameter info. raksha_policy_verifier_library( name = "dp_policy_verifier", additional_dl_files = [ diff --git a/src/analysis/souffle/sql_policy_verifier_interface.dl b/src/analysis/souffle/sql_policy_verifier_interface.dl new file mode 100644 index 000000000..aaed655ca --- /dev/null +++ b/src/analysis/souffle/sql_policy_verifier_interface.dl @@ -0,0 +1,34 @@ +//----------------------------------------------------------------------------- +// Copyright 2022 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +//----------------------------------------------------------------------------- + +#ifndef SRC_ANALYSIS_SOUFFLE_SQL_POLICY_VERIFIER_INTERFACE +#define SRC_ANALYSIS_SOUFFLE_SQL_POLICY_VERIFIER_INTERFACE + +#include "src/analysis/souffle/sql_output.dl" +#include "src/analysis/souffle/tag_transforms.dl" +#include "src/analysis/souffle/taint.dl" + +// An interface used for running and getting results from the policy verifier for +// the specific purpose of the SQL verifier. +// Does not concern itself with authorization logic facts, considers only +// `Operation`s and `SqlPolicyRule`s. Returns as output the violatesPolicy +// relation, which indicates whether there were failures in a way that is easy +// to read across the Souffle C++ interface. +.input isOperation(delimiter=";") +.input isSqlPolicyRule(delimiter=";") +.output violatesPolicy(delimiter=";") + +#endif diff --git a/src/backends/policy_engine/BUILD b/src/backends/policy_engine/BUILD index ab0c38c93..429c4bb07 100644 --- a/src/backends/policy_engine/BUILD +++ b/src/backends/policy_engine/BUILD @@ -49,6 +49,20 @@ cc_library( ], ) +cc_library( + name = "catchall_policy_rule_policy", + hdrs = ["catchall_policy_rule_policy.h"], + visibility = [ + "//:__pkg__", + "//src:__subpackages__", + ] + frontend_packages, + deps = [ + ":policy", + "@com_google_absl//absl/status", + "@com_google_absl//absl/strings", + ], +) + cc_library( name = "auth_logic_policy", hdrs = ["auth_logic_policy.h"], diff --git a/src/backends/policy_engine/catchall_policy_rule_policy.h b/src/backends/policy_engine/catchall_policy_rule_policy.h new file mode 100644 index 000000000..2ff6f362a --- /dev/null +++ b/src/backends/policy_engine/catchall_policy_rule_policy.h @@ -0,0 +1,54 @@ +//----------------------------------------------------------------------------- +// Copyright 2022 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +//---------------------------------------------------------------------------- +#ifndef SRC_BACKENDS_POLICY_ENGINE_CATCHALL_POLICY_RULE_POLICY_H_ +#define SRC_BACKENDS_POLICY_ENGINE_CATCHALL_POLICY_RULE_POLICY_H_ + +#include +#include + +#include "absl/strings/string_view.h" +#include "src/backends/policy_engine/policy.h" + +namespace raksha::backends::policy_engine { + +// TODO(#728): We ended up in a dependency mess around the SqlPolicyRulePolicy, +// where a number of test frontends started using it that had nothing to do with +// SQL. This is a shim to slim down the SQL infrastructure and allow us to +// eventually eliminate the tests using this as a catchall policy. +class CatchallPolicyRulePolicy : public Policy { + public: + explicit CatchallPolicyRulePolicy(std::string is_sql_policy_rule_facts) + : is_sql_policy_rule_facts_(std::move(is_sql_policy_rule_facts)) {} + + std::string GetPolicyAnalysisCheckerName() const override { + return "datalog_policy_verifier_cxx"; + } + + std::optional GetPolicyFactName() const override { + return "isSqlPolicyRule"; + } + + std::optional GetPolicyString() const override { + return is_sql_policy_rule_facts_; + } + + private: + std::string is_sql_policy_rule_facts_; +}; + +} // namespace raksha::backends::policy_engine + +#endif diff --git a/src/backends/policy_engine/souffle/BUILD b/src/backends/policy_engine/souffle/BUILD index 0c7f86c10..a34bb1d19 100644 --- a/src/backends/policy_engine/souffle/BUILD +++ b/src/backends/policy_engine/souffle/BUILD @@ -84,9 +84,9 @@ cc_binary( deps = [ ":souffle_policy_checker", "//src/backends/policy_engine:auth_logic_policy", + "//src/backends/policy_engine:catchall_policy_rule_policy", "//src/backends/policy_engine:dp_parameter_policy", "//src/backends/policy_engine:policy", - "//src/backends/policy_engine:sql_policy_rule_policy", "//src/common/logging", "//src/ir:proto_to_ir", "//src/parser/ir:ir_parser", @@ -371,6 +371,8 @@ cc_test( ], ) +# Use this version of the `souffle_policy_checker` when binary size does not matter. Linked with +# every library that may use it. cc_library( name = "souffle_policy_checker", srcs = ["souffle_policy_checker.cc"], @@ -394,6 +396,7 @@ cc_library( ":utils", "//src/analysis/souffle:datalog_policy_verifier", "//src/analysis/souffle:dp_policy_verifier", + "//src/analysis/souffle:sql_policy_verifier", "//src/backends/policy_engine:policy", "//src/backends/policy_engine:policy_checker", "//src/backends/policy_engine/souffle/dp_testdata/delta_1:dp_policy_verifier_delta_1_with_auth_logic_lib", @@ -408,6 +411,35 @@ cc_library( ], ) +# This is a version of the `souffle_policy_checker` for use with the SQL verifier only. Links only +# with the SQL verifier library. +cc_library( + name = "souffle_policy_checker_sql", + srcs = ["souffle_policy_checker.cc"], + hdrs = ["souffle_policy_checker.h"], + copts = [ + "-fexceptions", + "-Iexternal/souffle/src/include/souffle", + ], + # Turn off header modules, as Google precompiled headers use + # -fno-exceptions, and combining a precompiled header with -fno-exceptions + # with a binary that uses -fexceptions makes Clang upset. + features = ["-use_header_modules"], + linkopts = ["-pthread"], + visibility = ["//src:__subpackages__"] + frontend_packages, + deps = [ + ":datalog_lowering_visitor", + ":utils", + "//src/analysis/souffle:sql_policy_verifier", + "//src/backends/policy_engine:policy", + "//src/backends/policy_engine:policy_checker", + "//src/common/utils:filesystem", + "//src/ir:module", + "@com_google_absl//absl/strings", + "@souffle//:souffle_include_lib", + ], +) + cc_test( name = "souffle_policy_checker_test", srcs = ["souffle_policy_checker_test.cc"], diff --git a/src/backends/policy_engine/souffle/check_policy_compliance.cc b/src/backends/policy_engine/souffle/check_policy_compliance.cc index 4c84c2bfb..3d3b832f1 100644 --- a/src/backends/policy_engine/souffle/check_policy_compliance.cc +++ b/src/backends/policy_engine/souffle/check_policy_compliance.cc @@ -29,7 +29,7 @@ #include "src/backends/policy_engine/dp_parameter_policy.h" #include "src/backends/policy_engine/policy.h" #include "src/backends/policy_engine/souffle/souffle_policy_checker.h" -#include "src/backends/policy_engine/sql_policy_rule_policy.h" +#include "src/backends/policy_engine/catchall_policy_rule_policy.h" #include "src/common/logging/logging.h" #include "src/ir/proto_to_ir.h" #include "src/parser/ir/ir_parser.h" @@ -120,7 +120,7 @@ absl::StatusOr GetIrGraphComponentsFromProtoPath( using raksha::backends::policy_engine::AuthLogicPolicy; using raksha::backends::policy_engine::DpParameterPolicy; using raksha::backends::policy_engine::SoufflePolicyChecker; -using raksha::backends::policy_engine::SqlPolicyRulePolicy; +using raksha::backends::policy_engine::CatchallPolicyRulePolicy; using raksha::ir::IrProgramParser; int main(int argc, char* argv[]) { @@ -167,7 +167,7 @@ int main(int argc, char* argv[]) { << sql_policy_rules.status(); return UnwrapExitCode(ReturnCode::ERROR); } - SqlPolicyRulePolicy policy(*sql_policy_rules); + CatchallPolicyRulePolicy policy(*sql_policy_rules); policyCheckSucceeded = SoufflePolicyChecker().IsModulePolicyCompliant( *components.value().ir_module, policy); } else if (absl::GetFlag(FLAGS_epsilon_dp_parameter).has_value() && diff --git a/src/backends/policy_engine/sql_policy_rule_policy.h b/src/backends/policy_engine/sql_policy_rule_policy.h index fba608980..70fce0049 100644 --- a/src/backends/policy_engine/sql_policy_rule_policy.h +++ b/src/backends/policy_engine/sql_policy_rule_policy.h @@ -24,15 +24,15 @@ namespace raksha::backends::policy_engine { -// Placeholder for the Policy datastructure. Right now, the policy is hard-coded -// in datalog directly. +// A form of policy for the SQL verifier, which communicates tag transforms on +// SQL expressions in its special rule format. class SqlPolicyRulePolicy : public Policy { public: explicit SqlPolicyRulePolicy(std::string is_sql_policy_rule_facts) : is_sql_policy_rule_facts_(std::move(is_sql_policy_rule_facts)) {} std::string GetPolicyAnalysisCheckerName() const override { - return "datalog_policy_verifier_cxx"; + return "sql_policy_verifier_cxx"; } std::optional GetPolicyFactName() const override { diff --git a/src/frontends/sql/BUILD b/src/frontends/sql/BUILD index 5ac09040e..a0ec035fd 100644 --- a/src/frontends/sql/BUILD +++ b/src/frontends/sql/BUILD @@ -99,7 +99,7 @@ cc_library( ":decode", ":sql_ir_cc_proto", "//src/backends/policy_engine:policy", - "//src/backends/policy_engine/souffle:souffle_policy_checker", + "//src/backends/policy_engine/souffle:souffle_policy_checker_sql", ], )