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

File_integrety module shows inconsistent convention when the file path is the root. #24430

Closed
travisestill opened this issue Mar 9, 2021 · 4 comments · Fixed by #28354
Closed
Assignees

Comments

@travisestill
Copy link

travisestill commented Mar 9, 2021

Please include configurations and logs if available.

For confirmed bugs, please report:

  • Version: 7.11.1 and earlier (7.8.x)

  • Operating System: Verified on RHEL7.9 3.10.0-1160.11.1.el7.x86_64 and Ubuntu 20.04.2 LTS

  • Steps to Reproduce:

  1. Configure the file integrity module path either as - / or -
    eg:
- module: file_integrity
  paths:
  - /bin
  - /usr/bin
  - /sbin
  - /usr/sbin
  - /etc
  -

or

- module: file_integrity
  paths:
  - /bin
  - /usr/bin
  - /sbin
  - /usr/sbin
  - /etc
  - /
  1. Create a file in the root.

Expected:
The file.path field reflects the root in a consistent fashion that other directories are listed.
eg

    "file": {
...
      "path": "/",
...
    },

Actual:
The file.path shows the name of the file with a ./ or double-slash
eg:

    "file": {
...
      "path": "./test5.txt"
    },

or

    "file": {
...
      "path": "//test4.txt",
    },
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Mar 9, 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 Mar 9, 2021
@jamiehynds jamiehynds added the bug label Mar 24, 2021
@efd6 efd6 self-assigned this Oct 11, 2021
@efd6
Copy link
Contributor

efd6 commented Oct 11, 2021

This is due to an incorrect assumption about file path structures in the backing fsnotify package which is also present in upstream. The code currently assumes that all paths have a not path separator character to precede the suffix and so adds a path separator. This is obviously not true for root.

				name += "/" + strings.TrimRight(string(bytes[0:nameLen]), "\000")

Making this conditional on name being "/" does not alter the behaviour of file paths with a leading dot. The two approaches to that would be to use filepath.Join in fsnotify or to use filepath.Clean to fix up the ugly paths after they have gone through the configs path selection filter.

@efd6
Copy link
Contributor

efd6 commented Oct 11, 2021

@travisestill Can you provide more detail for the "./test5.txt" case? I would like to understand the configuration that generates that? Can I confirm that this happens when an empty paths array element is provided — i.e. your first case, -. If it is, that doesn't look like a configuration syntax that I would want to foster.

OK. I've figured out that case. The empty path config resolves to . due to filepath.Clean's semantics. In this case the directory of the file is not a part of the path. We can obtain the absolute path though and render that. Would this be the preferred behaviour?

@andrewkroh
Copy link
Member

If it is, that doesn't look like a configuration syntax that I would want to foster.

Regarding the empty paths element, I tend to agree with you. That configuration isn't particularly clear as to what it does so we should try steer users toward / and make that work properly. I could even see Auditbeat rejecting empty values at config validation.

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

Successfully merging a pull request may close this issue.

5 participants