nccgroup / sobelow

Security-focused static analysis for the Phoenix Framework
Apache License 2.0
1.66k stars 92 forks source link

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

Open ottenkoop opened 9 months ago

ottenkoop commented 9 months ago

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:

Wonder what else we can try to solve this.

Hope you can help us out! 🙌

Cheers!

houllette commented 8 months ago

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 commented 8 months ago

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

realcorvus commented 8 months ago

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 commented 8 months ago

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 - https://github.com/nccgroup/sobelow/pull/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 commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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.