new(userspace/engine): add a resolver to generate warnings from a filter AST

The first warnings we support involve the unsafe comparisons with <NA>, which were present
in the legacy regression tests for PSPs.

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>

userspace/engine/CMakeLists.txt

@@ -19,6 +19,7 @@ set(FALCO_ENGINE_SOURCE_FILES
     formats.cpp
     filter_macro_resolver.cpp
     filter_evttype_resolver.cpp
+    filter_warning_resolver.cpp
     rule_loader.cpp
     rule_reader.cpp
     stats_manager.cpp)

userspace/engine/filter_warning_resolver.cpp

@@ -0,0 +1,85 @@
+/*
+Copyright (C) 2022 The Falco Authors.
+
+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
+
+    http://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.
+*/
+
+#include <sinsp.h>
+#include "filter_warning_resolver.h"
+
+static const char* no_value = "<NA>";
+static const char* warn_unsafe_na_check = "unsafe-na-check";
+
+static inline bool is_equality_operator(const string& op)
+{
+	return op == "==" || op == "=" || op == "!="
+		|| op == "in" || op == "intersects" || op == "pmatch";
+}
+
+bool filter_warning_resolver::run(
+	libsinsp::filter::ast::expr* filter,
+	std::set<string>& warnings) const
+{
+	visitor v;
+	auto size = warnings.size();
+	v.m_is_equality_check = false;
+	v.m_warnings = &warnings;
+	filter->accept(&v);
+	return warnings.size() > size;
+}
+
+// todo(jasondellaluce): use an hard-coded map once we support more warnings
+bool filter_warning_resolver::format(
+	const std::string& code,
+	std::string& out) const
+{
+	if (code == warn_unsafe_na_check)
+	{
+		out = "comparing a field value with <NA> is unsafe and can lead to "
+			"unpredictable behavior of the rule condition. If you need to "
+			" check for the existence of a field, consider using the "
+			"'exists' operator instead.";
+		return true;
+	}
+	return false;
+}
+
+void filter_warning_resolver::visitor::visit(
+	libsinsp::filter::ast::binary_check_expr* e)
+{
+	if (is_equality_operator(e->op))
+	{
+		m_is_equality_check = true;
+		e->value->accept(this);
+		m_is_equality_check = false;
+	}
+}
+
+void filter_warning_resolver::visitor::visit(
+	libsinsp::filter::ast::value_expr* e)
+{
+	if (m_is_equality_check && e->value == no_value)
+	{
+		m_warnings->insert(warn_unsafe_na_check);
+	}
+}
+
+void filter_warning_resolver::visitor::visit(
+	libsinsp::filter::ast::list_expr* e)
+{
+	if (m_is_equality_check
+		&& std::find(e->values.begin(), e->values.end(), no_value) != e->values.end())
+	{
+		m_warnings->insert(warn_unsafe_na_check);
+	}
+}

userspace/engine/filter_warning_resolver.h

@@ -0,0 +1,84 @@
+/*
+Copyright (C) 2022 The Falco Authors.
+
+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
+
+    http://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.
+*/
+
+#pragma once
+
+#include <filter/parser.h>
+#include <string>
+#include <set>
+#include <memory>
+#include "falco_common.h"
+
+/*!
+	\brief Searches for bad practices in filter conditions and
+	generates warning messages
+*/
+class filter_warning_resolver
+{
+public:
+	/*!
+		\brief Visits a filter AST and substitutes macro references
+		according with all the definitions added through set_macro(),
+		by replacing the reference with a clone of the macro AST.
+		\param filter The filter AST to be visited
+		\param warnings Set of strings to be filled with warning codes. This
+		is not cleared up before the visit
+		\param blocking Filled-out with true if at least one warning is
+		found and at least one warning prevents the filter from being loaded
+		\return true if at least one warning is generated
+	*/
+	bool run(
+		libsinsp::filter::ast::expr* filter,
+		std::set<std::string>& warnings) const;
+	
+	/*!
+		\brief Given a warning code retrieved through run(), returns
+		a verbose message describing the problem of the warning.
+		\param code The warning code string
+		\param out The string to be filled-out with the warning message
+		\return true if the warning code is recognized, false otherwise
+	*/
+	bool format(const std::string& code, std::string& out) const;
+
+	/*!
+		\brief Given a warning code retrieved through run(), returns
+		a verbose message describing the problem of the warning.
+		\param code The warning code string
+		\return The warning message string
+		\throw falco_exception if the warning code is not recognized
+
+	*/
+	inline std::string format(const std::string& code) const
+	{
+		std::string v;
+		if (!format(code, v))
+		{
+			throw falco_exception("unrecognized warning code: " + code);
+		}
+		return v;
+	}
+
+private:
+	struct visitor : public libsinsp::filter::ast::base_expr_visitor
+	{
+		bool m_is_equality_check;
+		std::set<std::string>* m_warnings;
+
+		void visit(libsinsp::filter::ast::value_expr* e) override;
+		void visit(libsinsp::filter::ast::list_expr* e) override;
+		void visit(libsinsp::filter::ast::binary_check_expr* e) override;
+	};
+};

userspace/engine/rule_loader.cpp

@@ -15,9 +15,11 @@ limitations under the License.
 */
 
 #include "falco_engine.h"
+#include "falco_utils.h"
 #include "rule_loader.h"
 #include "filter_macro_resolver.h"
 #include "filter_evttype_resolver.h"
+#include "filter_warning_resolver.h"
 
 #define MAX_VISIBILITY		((uint32_t) -1)
 #define THROW(cond, err)    { if (cond) { throw falco_exception(err); } }
@@ -652,15 +654,20 @@ void rule_loader::compile_rule_infos(
 	indexed_vector<falco_rule>& out)
 {
 	string err, condition;
+	set<string> warn_codes;
+	filter_warning_resolver warn_resolver;
 	for (auto &r : m_rule_infos)
 	{
 		try
 		{
+			// skip the rule if below the minimum priority
 			if (r.priority > cfg.min_priority)
 			{
 				continue;
 			}
 
+			// build filter AST by parsing the condition, building exceptions,
+			// and resolving lists and macros
 			falco_rule rule;
 			condition = r.cond;
 			if (!r.exceptions.empty())
@@ -670,7 +677,20 @@ void rule_loader::compile_rule_infos(
 			}
 			auto ast = parse_condition(condition, lists);
 			resolve_macros(macros, ast, MAX_VISIBILITY, "");
-			
+
+			// check for warnings in the filtering condition
+			warn_codes.clear();
+			if (warn_resolver.run(ast.get(), warn_codes))
+			{
+				for (auto &w : warn_codes)
+				{
+					cfg.warnings.push_back(
+						"Rule " + r.name + ": warning (" + w + "):\n    "
+						+ falco::utils::wrap_text(warn_resolver.format(w), 4, 50));
+				}
+			}
+
+			// build rule output message
 			rule.output = r.output;
 			if (r.source == falco_common::syscall_source)
 			{
@@ -678,8 +698,8 @@ void rule_loader::compile_rule_infos(
 			}
 			THROW(!is_format_valid(cfg.engine, r.source, rule.output, err), 
 				"Invalid output format '" + rule.output + "': '" + err + "'");
-			
-			
+
+			// construct rule definition and compile it to a filter
 			rule.name = r.name;
 			rule.source = r.source;
 			rule.description = r.desc;
@@ -702,7 +722,8 @@ void rule_loader::compile_rule_infos(
 					throw falco_exception("Rule " + rule.name + ": error " + err);
 				}
 			}
-
+			
+			// popolate set of event types and emit an special warning
 			set<uint16_t> evttypes;
 			if(rule.source == falco_common::syscall_source)
 			{
@@ -713,8 +734,8 @@ void rule_loader::compile_rule_infos(
 				{
 					cfg.warnings.push_back(
 						"Rule " + rule.name + ": warning (no-evttype):\n" +
-						+ "		 matches too many evt.type values.\n"
-						+ "		 This has a significant performance penalty.");
+						+ "    matches too many evt.type values.\n"
+						+ "    This has a significant performance penalty.");
 				}
 			}
 			else if (rule.source == "k8s_audit")
@@ -728,6 +749,7 @@ void rule_loader::compile_rule_infos(
 				evttypes = { ppm_event_type::PPME_PLUGINEVENT_E };
 			}
 
+			// add rule and its filter in the engine
 			cfg.engine->add_filter(filter, rule.name, rule.source, evttypes, rule.tags);
 			cfg.engine->enable_rule(rule.name, r.enabled);
 		}