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

.sobelow-skips not picked up when running in (gitlab) CI. #147

Open
ottenkoop opened this issue Nov 14, 2023 · 7 comments
Open

.sobelow-skips not picked up when running in (gitlab) CI. #147

ottenkoop opened this issue Nov 14, 2023 · 7 comments

Comments

@ottenkoop
Copy link

ottenkoop commented Nov 14, 2023

Hi there,

We are currently trying to implement sobelow into our CI pipeline.
Followed the steps to create a .sobelow-skips file and push this to the repository.

But while running:
mix sobelow --skip --exit Low

It's not picking up the .sobelow-skips file. It returns all the issues as before adding the skips file.
Locally it's working correctly.

Couple of hunches we tried:

  • Check if the .sobelow-skips file file is available ✅ . => Is found and printed in the CI.
  • Specify explicitly the root folder with the -r /path/to ✅ => Didn't fix the issue.

Wonder what else we can try to solve this.

Hope you can help us out! 🙌

Cheers!

@houllette
Copy link
Collaborator

houllette commented Dec 7, 2023

This is very strange behavior indeed - especially if you've confirmed it's working locally but not in CI! May I ask how you're running Sobelow in your CI environment? Are you using the GitHub Action? Is it installed into the ephemeral environment or the Elixir app your testing?

EDIT: Just realized in the issue title you specific a GitLab environment - my question still largely remains the same, just ignore the GitHub Action bit 🙂 Does it have its own GitLab job to run or is it a part of a more general test suite job?

@realcorvus
Copy link
Contributor

I'm seeing this same issue too in Github actions.

@realcorvus
Copy link
Contributor

realcorvus commented Dec 19, 2023

image

I ran a custom fork of Sobelow in Github actions that printed out the md5 hashes of the skips, so Sobelow can read the file. Something about the environment is causing the skip logic to break.

@realcorvus
Copy link
Contributor

The reason this is happening:

lib/sobelow/finding.ex

  def fingerprint(%Sobelow.Finding{} = finding) do
   ...
    [finding.type, finding.vuln_source, filename, finding.vuln_line_no]
    |> :erlang.term_to_binary()
    |> :erlang.md5()
    |> Base.encode16()
  end

:erlang.term_to_binary() gives a different output depending on your Erlang version. The left is my local machine, right is Github action:

image

:jason is already a dependency, I vote to switch to JSON for serializing the finding list because you won't have to deal with this problem. Granted it will break old sobelow-skips files, but it's already sort of broken anyway. Maybe do a 1.0 release and warn people that it's a breaking change? There's also a PR open for changing the skip file format anyway, adding a comment with each hash is a good idea - #149

Fun fact, I first ran into the binary_to_term compatibility issue when researching RCE exploits, it shows up everywhere in Elixir security!

@sb8244
Copy link

sb8244 commented Dec 20, 2023

Oh that's a fun one.

One thought to avoid a breaking change but keep the idea of a fingerprint: calculate new fingerprints using :erlang.phash2, which is stable across different architecture + ERTS versions.

The old way of calculating a fingerprint could be checked against the skip list as well, so existing skip files don't break. The same logic applies to a JSON fingerprint as well.

@ottenkoop
Copy link
Author

ottenkoop commented Dec 20, 2023

That seems to have done the trick indeed @realcorvus . Generated the .sobelow-skips file in the CI and copied that one to include in our repo.

Thanks a lot for the effort @realcorvus @houllette 🙌

@houllette
Copy link
Collaborator

Someone also pointed out on the EEF Security Slack channel that we can specify a version for term_to_binary instead of having it pick a default, which should prevent breakage between OTP versions moving forward (docs).

It may require regeneration of previously established skip files still, so maybe that isn't ideal and probably won't hold up long term - but it could be a band-aid solution while we look to make a larger change proposed by @sb8244 in #149.

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

No branches or pull requests

4 participants