Skip to content
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.

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

fix(userspace/engine): avoid reading duplicate exception values #2200

Merged
merged 1 commit into from
Sep 12, 2022

Conversation

jasondellaluce
Copy link
Contributor

@jasondellaluce jasondellaluce commented Sep 12, 2022

What type of PR is this?

/kind bug

Any specific area of the project related to this PR?

/area engine

What this PR does / why we need it:

The recursive function that reads rule exception values does not clear out the value list, so consecutive list-type exception values may end up containing in duplicate items.

Which issue(s) this PR fixes:

Special notes for your reviewer:

The regression seems to be introduced in #2098

A minimum example to reproduce the issue is reported below:

- rule: Sample Rule
  desc: A sample rule
  output: Sample output (type=%evt.type)
  priority: ERROR
  condition: >
    evt.dir = <
  exceptions:
    - name: sample_exception
      fields: [evt.type, proc.name]
      comps: [in, in]
      values:
        - [[openat], [ls]]

This rule and its exceptions will be compiled into the following condition (note the rolled-over duplication of openat):

(evt.dir = < and not (evt.type in (openat) and proc.name in (openat, ls)))

With this fix, the rule is correctly compiled as:

(evt.dir = < and not (evt.type in (openat) and proc.name in (ls)))

Does this PR introduce a user-facing change?:

fix(userspace/engine): avoid reading duplicate exception values

Signed-off-by: Jason Dellaluce <jasondellaluce@gmail.com>
@jasondellaluce
Copy link
Contributor Author

/milestone 0.33.0

@poiana poiana added this to the 0.33.0 milestone Sep 12, 2022
@poiana poiana added the size/XS label Sep 12, 2022
Copy link
Contributor

@FedeDP FedeDP left a comment

Choose a reason for hiding this comment

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

/approve

@poiana
Copy link
Contributor

poiana commented Sep 12, 2022

LGTM label has been added.

Git tree hash: c4154b1b53ee51904e1208d495e94e2d110999cd

Copy link
Member

@Andreagit97 Andreagit97 left a comment

Choose a reason for hiding this comment

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

/approve

@poiana
Copy link
Contributor

poiana commented Sep 12, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP, jasondellaluce

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [Andreagit97,FedeDP,jasondellaluce]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@poiana poiana merged commit cf9baea into master Sep 12, 2022
@poiana poiana deleted the fix/exceptions-in-values branch September 12, 2022 13:53
@VadimZy
Copy link
Contributor

VadimZy commented Sep 12, 2022

Technically speaking, the implementation of decode_exception_info_entry is error-prone.

  • The return value is ignored at the root level,
  • a case where tmp.items.empty() is ignored (left to the clients)
  • node checks only at the root level, no validation in the tree branches and leafs
  • multiple copying of tmp objects, even with possible moving it will require a lot of allocations for the inner entry structures;
std::unique_ptr<rule_loader::rule_exception_info::entry> decode_exception_info_entry(
	const YAML::Node& node)
{
	using entry_t = rule_loader::rule_exception_info::entry;
	if (node.IsDefined())
	{
		if (node.IsScalar())
		{
			std::unique_ptr<entry_t> ret(new entry_t());
			
			ret->is_list = false;
			if (YAML::convert<string>::decode(node, ret->item))
			{
				return ret;
			}
			return {};
		}
		if (node.IsSequence())
		{
			std::unique_ptr<entry_t> ret(new entry_t());
			ret->is_list = true;
			for(const YAML::Node& v : node)
			{
				auto item = decode_exception_info_entry(v);
				if (item != nullptr)
				{
					ret->items.push_back(std::move(item));
				}
			}
			return ret->items.empty() ? std::unique_ptr<entry_t>() : std::move(ret);
		}
	}
	return {};
}

same for read_rule_exceptions function

rule_exception_info, rule_exception_info::entry should have their constructors with throwing validations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants