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

Major Refactor #8

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Conversation

yunginnanet
Copy link

@yunginnanet yunginnanet commented Jun 4, 2024

Code Refactoring and Enhancements

  • Refactor of CSV Handling:

    • Introduced dynamic CSV marshal functionality via reflection
  • Modularization and Code Cleanup:

    • Significant restructuring of sandfly-entropyscan.go for better readability and maintainability.
    • Improved error handling for missing /proc files
  • Build System:

    • Deprecated build.sh in favor of Makefile for a more standardized build process.
    • Updated .gitignore to exclude build artifacts and IDE files.
  • Improved Reporting:

    • Resolve procfs links so that the actual name of the bin is retrieved

New Features

  • JSON Output:

    • Introduced JSON output option, has parity with CSV output
  • Checksum Toggle:

    • Introduced the ability to enable and disable checksum calculation at will.
  • Concurrency:

    • Introduced concurrent checksum calculations to improve performance.

Workflows and Tests

  • Added Unit Tests:

    • Implemented comprehensive unit tests for new and existing functionalities.
    • Ensured coverage for checksum calculations and CSV parsing.
  • GitHub Actions:

    • Added CI workflow (.github/workflows/go.yml) for automated testing on push and pull requests.
    • Added CD workflow (.github/workflows/release.yml) for automated releases.

@yunginnanet yunginnanet marked this pull request as ready for review June 4, 2024 09:18
(cherry picked from commit c8b5451)
assure flags package doesn't fire off twice during testing

test the unit tests actual PID and marshal it to JSON, then CSV, verify parity of the marshaller functions
 - pass down any errors we receive during `IsElf` and checksum operations

 - add test case for errors passed down

 - clean up
  repetitive code

(cherry picked from commit 71d4fb5)
Signed-off-by: [email protected] <[email protected]>
@yunginnanet
Copy link
Author

yunginnanet commented Jun 11, 2024

this last commit to this branch, 5e13d28, addresses a rather troublesome scenario that I actually encountered in the wild on my own machine while testing.

Previously, if there was an error that occurred during much of the file operations, it would be silently dropped1.

In my case, I use a container named Conty. Even as root, if you try and read bytes from applications running in the container, it is not unexpected to receive a permission denied error.

While in my case, this was expected behavior, this is also behavior that has been observed as a result of (arguably low caliber) Linux malware evasion techniques. Before this last commit, these errors were silently ignored.

Old behavior (current behavior in sandflysecurity/sandfly-entropyscan):

for pid := 0; pid < len(pidPaths); pid++ {
// Only check elf files which should be all these will be anyway.
fileInfo, err := checkFilePath(pidPaths[pid], true, entropyMaxVal)
// anything that is not an error is a valid /proc/*/exe link we could see and process. We will analyze it.
if err == nil {
if fileInfo.entropy >= entropyMaxVal {
printResults(fileInfo, csvOutput, delimChar)
}
}
}

Footnotes

  1. https://github.com/sandflysecurity/sandfly-entropyscan/blob/874da6717b6b4aad837bb622021c127ebe154339/sandfly-entropyscan.go#L120-L129

@yunginnanet
Copy link
Author

I know this PR is a lot. If requested I will happily cherry-pick parts of it, and even make individual issues/PRs if desired.

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

Successfully merging this pull request may close these issues.

1 participant