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

auditd: Store program arguments in process.args array #29601

Merged
merged 2 commits into from
Jan 10, 2022

Conversation

adriansr
Copy link
Contributor

@adriansr adriansr commented Dec 24, 2021

What does this PR do?

Changes Filebeat's auditd module to store program arguments (from an EXECVE call) in process.args (arg0 also in process.executable). Previously it was using fields arg0 to argN under auditd.log.

Why is it important?

This prevents too many fields being created. When a call contained more than 10.000 arguments, this lead to an ingest error and contributed to very large indices:

Could not index event to Elasticsearch. "status"=>400, "error"=>{"type"=>"illegal_argument_exception", "reason"=>"Limit of total fields [10000] has been exceeded"}}}}

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • [ ] I have made corresponding changes to the documentation
  • [ ] I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

This avoids a fields mapping explosion and indexing errors when execve
calls have thousands of arguments.
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Dec 24, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Dec 24, 2021
@adriansr adriansr removed the bug label Dec 24, 2021
@elasticmachine
Copy link
Collaborator

elasticmachine commented Dec 24, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-01-05T08:24:43.040+0000

  • Duration: 100 min 50 sec

  • Commit: 184c47d

Test stats 🧪

Test Results
Failed 0
Passed 9644
Skipped 1282
Total 10926

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

String fld = String.format("a%d", fmt);
def arg = ctx.auditd.log.remove(fld);
if (arg == null) break;
args.add(arg);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we retain the original argument order? At the moment we are retaining the args in lexical order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is caused by the test_modules.py sorting all array fields before comparison, as some array fields can be generated in a different order each run.

for key in objects[k].keys():
if isinstance(objects[k][key], list):
objects[k][key].sort(key=str)

I've added some configuration to the testing so that we can skip the sorting step for some fields, for now just process.args.

Copy link
Contributor

@efd6 efd6 left a comment

Choose a reason for hiding this comment

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

LGTM

@adriansr adriansr merged commit 03bf169 into elastic:master Jan 10, 2022
@adriansr adriansr deleted the fb_auditd_args branch January 10, 2022 08:54
mergify bot pushed a commit that referenced this pull request Jan 10, 2022
Changes Filebeat's auditd module to store program arguments
(from an EXECVE call) in process.args (arg0 also in process.executable).
Previously it was using fields arg0 to argN under auditd.log.

This prevents too many fields being created. When a call contained
more than 10.000 arguments, this lead to an ingest error and contributed to
very large indices:

> Could not index event to Elasticsearch: "status"=>400,
>   "error"=>{
>     "type"=>"illegal_argument_exception",
>     "reason"=>"Limit of total fields [10000] has been exceeded"}}

(cherry picked from commit 03bf169)
adriansr added a commit that referenced this pull request Jan 10, 2022
Changes Filebeat's auditd module to store program arguments
(from an EXECVE call) in process.args (arg0 also in process.executable).
Previously it was using fields arg0 to argN under auditd.log.

This prevents too many fields being created. When a call contained
more than 10.000 arguments, this lead to an ingest error and contributed to
very large indices:

> Could not index event to Elasticsearch: "status"=>400,
>   "error"=>{
>     "type"=>"illegal_argument_exception",
>     "reason"=>"Limit of total fields [10000] has been exceeded"}}

(cherry picked from commit 03bf169)

Co-authored-by: Adrian Serrano <[email protected]>
v1v added a commit to v1v/beats that referenced this pull request Jan 12, 2022
…b-for-macos

* upstream/master: (172 commits)
  [Elastic Agent] Fix issue with ensureServiceToken. (elastic#29800)
  [Winlogbeat] Add provider name to Security routing pipeline check (elastic#29781)
  Add summary to journeys which don't emit journey:end (early node subprocess exits) (elastic#29606)
  Prepare 8.0.0-rc1 changelog (elastic#29795) (elastic#29806)
  Change docker image from CentOS 7 to Ubuntu 20.04 (elastic#29681)
  libbeat/processors/add_process_metadata: implement a process cache eviction policy (elastic#29717)
  [Automation] Update elastic stack version to 8.1.0-7004acda for testing (elastic#29783)
  Missing changelog entry for elastic#29773 (elastic#29791)
  Add a readme for k8s autodiscover provider (elastic#28213)
  Remove overriding of index pattern on the Kubernetes overview dashboard (elastic#29676)
  jjbb: remove obsoleted branches (<7.16) (elastic#29707)
  Add k8s metadata in state_cronjob metricset (elastic#29572)
  ibmmq: Fix timestamp parsing (elastic#29773)
  Do not add date to index if `@meta.index` is set (elastic#29775)
  ci: uses aliases for the branches (elastic#29706)
  Filebeat tests: Restore `@timestamp` field validation (elastic#29772)
  Forward port 7.16.3 changelog to master (elastic#29777)
  auditd: Store program arguments in process.args array (elastic#29601)
  System/socket: Support kernel_clone() replacement for _do_fork() (elastic#29744)
  Do not mention removal if version is not specified in `cfgwarn` messages (elastic#29727)
  ...
adriansr added a commit to elastic/integrations that referenced this pull request Feb 23, 2022
Prevents the indices exceeding the 10,000 field limit due to an
arbitrarily large number of aNN fields.

This is a combination of the following Filebeat module fixes:
 - elastic/beats#29601
 - elastic/beats#30382

Updates version to 2.1.0
eyalkraft pushed a commit to build-security/integrations that referenced this pull request Mar 30, 2022
…2730)

Prevents the indices exceeding the 10,000 field limit due to an
arbitrarily large number of aNN fields.

This is a combination of the following Filebeat module fixes:
 - elastic/beats#29601
 - elastic/beats#30382

Updates version to 2.1.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v8.0.0 Automated backport with mergify backport-v8.1.0 Automated backport with mergify breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants