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

New path filtering logic excluding large number of unintended paths #2667

Closed
jpalermo opened this issue Feb 24, 2024 · 5 comments · Fixed by #2675
Closed

New path filtering logic excluding large number of unintended paths #2667

jpalermo opened this issue Feb 24, 2024 · 5 comments · Fixed by #2675
Assignees
Labels
bug Something isn't working

Comments

@jpalermo
Copy link
Contributor

What happened:
#2644 introduced new logic to prevent scanning of unixSystemRuntimePrefixes even if those paths are not at the root, for example remounted root file systems. The solution however prevents scanning any paths that include a /proc, /dev or /sys within them. For example:
folder/syslog-scan.sbom.json
source_code/system/files
source_code/dev/other_files
are all skipped after this change.

What you expected to happen:
Only /proc, /dev and /sys that are part of fs roots should be excluded, not anything including those strings.

Steps to reproduce the issue:
Attempt to scan an sbom file that starts with the string sys

Anything else we need to know?:

Environment:

  • Output of syft version: 0.105.0
  • OS (e.g: cat /etc/os-release or similar): Any
@jpalermo jpalermo added the bug Something isn't working label Feb 24, 2024
jpalermo added a commit to jpalermo/syft that referenced this issue Feb 24, 2024
…nchore#2644)"

This change caused the skipping of any paths including "/proc", "/dev" and "/sys" which
was not the intended change and excludes many things that should be scanned.

anchore#2667

This reverts commit a909e3c.
jpalermo added a commit to jpalermo/syft that referenced this issue Feb 24, 2024
…nchore#2644)"

This change caused the skipping of any paths including "/proc", "/dev" and "/sys" which
was not the intended change and excludes many things that should be scanned.

anchore#2667

This reverts commit a909e3c.

Signed-off-by: Joseph Palermo <joseph.palermo@broadcom.com>
@klakin-pivotal
Copy link

I wonder if a better way to handle this sort of exclusion would be to (when initializing/starting the scan) find filesystem paths that have devtmpfs, sysfs, and proc types and exclude those paths and their children, rather than assuming that any directory named sys or proc or dev MUST contain a kernel-managed pseudo-filesystem that's okay to exclude from scans.

@wagoodman
Copy link
Contributor

I like the suggestion to look for forbidden mounts instead of leaning on mount naming conventions. We can't revert this PR since we have had other users that are mounting these filesystems within nested directories. That doesn't explain away the reason for reverting this though... I think there are unintended consequences.

@jpalermo / @klakin-pivotal would either of you be interested in implementing the suggestion within the file resolver for finding forbidden filesystems first?

@jpalermo
Copy link
Contributor Author

It seems like the "correct fix" for the original problem is incredibly difficult, so somebody who wants it should work on it. An alternative is people who are scanning mounted images can exclude the paths they don't want scanned.

As far as not reverting, the current syft is in a very bad state because of the original PR. Large chunks of code bases are currently being excluded accidentally, meaning the generated sboms are incorrect.

@klakin-pivotal
Copy link

I'm not interested in implementing the proper zero-false-exclusions version of the "Don't scan kernel pseudo-filesystems because we know that there are no artifacts we care about in there" code. Conceptually it's not tough to do, but I've no knowledge of how the filesystem scanner code works.

Even a version of the code in the PR that just scans for directories named sys, proc, dev is inevitably gonna end up falsely excluding artifacts in someone's codebase.

@wagoodman wagoodman self-assigned this Feb 27, 2024
@wagoodman
Copy link
Contributor

No worries! I wanted to offer it up in case there was interest --I can take a stab at this later this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
3 participants